fix(mobile): drop the dead /model row and re-park the caret on draft switch - #7523

Open
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers
Open

fix(mobile): drop the dead /model row and re-park the caret on draft switch#7523
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers

Conversation

@kirill-markin

@kirill-markinkirill-markin commented Aug 19, 2026

Copy link
Copy Markdown

What this PR is now

#8587 landed the draft composer trigger menus, so the original 775-line version of this PR is obsolete. I have rebased onto main and cut it down to the two behaviors @juliusmarminge named when reopening it:

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation.

One file, +10 / −4, both edits in apps/mobile/src/features/threads/use-composer-command-menu.ts.

1. /model is a dead row on the new-task draft screen

allBuiltIn let /model bypass the capability gate, so it rendered on both surfaces. Selecting it on a draft does nothing useful: onSelect has no model branch, so it inserts the literal text /model , detectComposerTrigger then returns a slash-model trigger this hook builds no items for, and the menu just closes. The draft screen already has a working model control in the same toolbar ("Choose model"), so the row is redundant as well as inert.

The row is now gated on hasThread. ThreadComposer passes hasThread: true and is unchanged.

One consequence worth naming: with plan mode off, NewTaskDraftScreen passes no onUpdateInteractionMode, so the built-in list is now empty. Typing / mid-line on such a draft — where provider commands are position-gated to column 0 — can produce no menu at all, whereas before it always produced the inert /model row. I think no menu beats a row that does nothing, but say the word if you would rather keep something there.

2. The caret is re-parked a render too late

The ownerKey reset ran in a useEffect, which commits after the render that already computed trigger from the previous draft's caret offset. Switching drafts could therefore flash the popover open over a draft the user never typed a trigger into. It now adjusts during render, so that pass never reaches the screen. The ref is assigned before setSelection, so the restarted render cannot loop — the same shape already used in HomeScreen.tsx and ThreadNavigationSidebar.tsx.

No path search was ever dispatched in that window — useComposerPathSearch debounces its target by 200 ms — so this is a rendering fix only.

Deliberately out of scope

  • Making /model work on mobile. Nothing in the repo consumes the slash-model trigger kind that packages/shared/src/composerTrigger.ts produces; only apps/web's ChatComposer handles the model command, by opening its own picker. So the row is inert in ThreadComposer too, not just on drafts. Pre-existing and untouched here — it needs a real mobile model picker wired into the menu. Happy to take that as its own PR.
  • The items.length > 0 guard that hides ComposerCommandPopover's loading and empty states at both call sites. Relaxing it is a deliberate UX change to the most-used surface, so it stays its own PR.
  • New tests.apps/mobile has no hook-render harness — no renderHook, no testing-library — and use-composer-command-menu.test.ts covers only the pure composerSelectionAtEnd. Adding that harness, or extracting the built-in filter into a new module to assert one boolean, is more machinery than this change earns. Say the word if you would rather have it.

Verification

CI on this PR is the gate; I ran no local checks. Statically: hasThread was already in the items memo's dependency list, ThreadComposer.tsx passes hasThread: true so its menu is byte-for-byte unchanged, and the clamp effect on draftMessage.length is a no-op after the re-park. Happy to add an emulator pass with before/after images if you want them for this one.

Model: Claude Opus 5, harness: Claude Code.

@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1c202d-e188-4e9d-b193-9184b876d2e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added the size:XL 500-999 changed lines (additions + deletions). label Aug 19, 2026
@@ -806,6 +843,11 @@ export function NewTaskDraftScreen(props: {
!isImportingShare &&
!flow.submitting &&
!(flow.workspaceMode === "worktree" && !flow.selectedBranchName);
// A share transfer locks the editor, so its rows must not be tappable.
const showComposerTriggerMenu =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumthreads/NewTaskDraftScreen.tsx:847

When an @ search is loading or returns no matches, the trigger menu disappears without showing ComposerCommandPopover, so users receive no loading or empty-results feedback. The items.length > 0 guard makes the popover's isLoading and empty-state rendering unreachable; render it whenever a trigger is active.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/NewTaskDraftScreen.tsx around line 847:
When an `@` search is loading or returns no matches, the trigger menu disappears without showing `ComposerCommandPopover`, so users receive no loading or empty-results feedback. The `items.length > 0` guard makes the popover's `isLoading` and empty-state rendering unreachable; render it whenever a trigger is active.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this against origin/main: the gate is pre-existing, not introduced here.

ThreadComposer.tsx:725 on main already reads composerTrigger && composerMenuItems.length > 0, so ComposerCommandPopover's empty-state branch — and the emptyText / "Searching files…" copy at ComposerCommandPopover.tsx:89-91,177 — was already unreachable on mobile before this PR. This patch keeps the same gate on both call sites so the two surfaces behave identically, which is what the "thread composer keeps its previous behavior exactly" constraint requires.

Relaxing it is a real improvement, but it is a deliberate UX change to both surfaces (a popover that appears on a bare @ and shows a spinner), so it belongs in its own PR rather than riding along here. Happy to send that follow-up if you want it — the one-line change is the items.length > 0 half of the condition at both call sites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

@macroscopeapp

macroscopeappBot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a small, localized mobile bug fix: it synchronizes caret reset on composer switches and removes a nonfunctional /model row from unsent drafts while preserving existing thread behavior. A separate unresolved Medium finding concerns pre-existing missing loading/empty-state feedback for @ searches and should remain visible as a merge risk.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@kirill-markin

Copy link
Copy Markdown
Author

Status, so this does not read as abandoned — the blocking finding is answered in the review thread: #7523 (comment)

Short version: the items.length > 0 guard that hides the popover's loading and empty state is not introduced here. ThreadComposer.tsx:725 already has it on main, so ComposerCommandPopover's emptyText branch (ComposerCommandPopover.tsx:89-91,177) was unreachable on mobile before this PR. Keeping the identical guard on both call sites is what makes the new surface behave exactly like the thread composer, which is the constraint this PR set for itself. Relaxing it is a deliberate UX change to both surfaces — a bare @ would start opening a popover with a spinner on the most-used surface — so I would rather send it as its own PR than bundle it here. Say the word and I will.

Everything else is green: Cursor Bugbot, Macroscope Effect Service Conventions, and the label checks. The Vercel marketing deploy fails with "Authorization required to deploy", which is the standard restriction on fork PRs rather than anything in this branch.

I also verified this locally instead of leaning on CI: 20/20 unit tests for the extracted logic, clean tsc --noEmit for apps/mobile, and a run on an Android emulator against a disposable backend where $ lists provider skills, @ lists project files, and tapping a row inserts the mention. Before/after images and a breakdown of the size:XL label are in the description.

@juliusmarminge

juliusmarminge commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation. My initial closure missed that remaining scope.

…switch
pingdotgg#8587 landed the draft composer trigger menus. Two details it left out.
`/model` bypassed the built-in capability gate, so it rendered on the
new-task draft screen, where selecting it only inserts the literal text
`/model `: `onSelect` has no `model` branch, and the `slash-model` trigger
that follows builds no items. It is now gated on `hasThread`, so the thread
composer is unchanged and the draft screen, which already carries its own
model control, loses the dead row.
The `ownerKey` caret reset ran in an effect, one commit too late, so
switching drafts could flash the trigger popover open over a draft the user
never typed a trigger into. It now adjusts during render, with the ref
assigned before `setSelection` so the restarted render cannot loop.
Model: Claude Opus 5, harness: Claude Code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kirill-markin
kirill-markinforce-pushed the fix/mobile-new-task-composer-triggers branch from ce5d27c to 6c68b1cCompareSeptember 2, 2026 07:15
@kirill-markinkirill-markin changed the title fix(mobile): support /, $ and @ in the new-task composerfix(mobile): drop the dead /model row and re-park the caret on draft switchSep 2, 2026
@github-actionsgithub-actionsBot added size:S 10-29 changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Sep 2, 2026
@kirill-markin

Copy link
Copy Markdown
Author

@juliusmarminge — rebased onto main and cut this down to exactly the two behaviors you named in the reopen. Force-pushed ce5d27c6c68b1c; the previous SHA is still in the timeline above if you want to compare.

The 775-line extraction is gone — #8587 owns that logic now, and re-landing my version of it would just be churn. What is left is one file, +10 / −4:

  • /model gated on hasThread, so it disappears from the new-task draft screen and ThreadComposer is untouched;
  • the ownerKey caret reset moved from a useEffect into the render body, so a draft switch can no longer flash the popover open for a frame.

Description rewritten to match. Two things in there worth your eye: with plan mode off, typing / mid-line on a draft can now produce no menu at all rather than the inert /model row, which I think is the right trade but is your call; and I skipped tests because apps/mobile has no hook-render harness — happy to add one if you would rather have the coverage.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kirill-markin@juliusmarminge
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(mobile): drop the dead /model row and re-park the caret on draft switch - #7523

Open
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers
Open

fix(mobile): drop the dead /model row and re-park the caret on draft switch#7523
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers

Conversation

@kirill-markin

@kirill-markinkirill-markin commented Aug 19, 2026

Copy link
Copy Markdown

What this PR is now

#8587 landed the draft composer trigger menus, so the original 775-line version of this PR is obsolete. I have rebased onto main and cut it down to the two behaviors @juliusmarminge named when reopening it:

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation.

One file, +10 / −4, both edits in apps/mobile/src/features/threads/use-composer-command-menu.ts.

1. /model is a dead row on the new-task draft screen

allBuiltIn let /model bypass the capability gate, so it rendered on both surfaces. Selecting it on a draft does nothing useful: onSelect has no model branch, so it inserts the literal text /model , detectComposerTrigger then returns a slash-model trigger this hook builds no items for, and the menu just closes. The draft screen already has a working model control in the same toolbar ("Choose model"), so the row is redundant as well as inert.

The row is now gated on hasThread. ThreadComposer passes hasThread: true and is unchanged.

One consequence worth naming: with plan mode off, NewTaskDraftScreen passes no onUpdateInteractionMode, so the built-in list is now empty. Typing / mid-line on such a draft — where provider commands are position-gated to column 0 — can produce no menu at all, whereas before it always produced the inert /model row. I think no menu beats a row that does nothing, but say the word if you would rather keep something there.

2. The caret is re-parked a render too late

The ownerKey reset ran in a useEffect, which commits after the render that already computed trigger from the previous draft's caret offset. Switching drafts could therefore flash the popover open over a draft the user never typed a trigger into. It now adjusts during render, so that pass never reaches the screen. The ref is assigned before setSelection, so the restarted render cannot loop — the same shape already used in HomeScreen.tsx and ThreadNavigationSidebar.tsx.

No path search was ever dispatched in that window — useComposerPathSearch debounces its target by 200 ms — so this is a rendering fix only.

Deliberately out of scope

  • Making /model work on mobile. Nothing in the repo consumes the slash-model trigger kind that packages/shared/src/composerTrigger.ts produces; only apps/web's ChatComposer handles the model command, by opening its own picker. So the row is inert in ThreadComposer too, not just on drafts. Pre-existing and untouched here — it needs a real mobile model picker wired into the menu. Happy to take that as its own PR.
  • The items.length > 0 guard that hides ComposerCommandPopover's loading and empty states at both call sites. Relaxing it is a deliberate UX change to the most-used surface, so it stays its own PR.
  • New tests.apps/mobile has no hook-render harness — no renderHook, no testing-library — and use-composer-command-menu.test.ts covers only the pure composerSelectionAtEnd. Adding that harness, or extracting the built-in filter into a new module to assert one boolean, is more machinery than this change earns. Say the word if you would rather have it.

Verification

CI on this PR is the gate; I ran no local checks. Statically: hasThread was already in the items memo's dependency list, ThreadComposer.tsx passes hasThread: true so its menu is byte-for-byte unchanged, and the clamp effect on draftMessage.length is a no-op after the re-park. Happy to add an emulator pass with before/after images if you want them for this one.

Model: Claude Opus 5, harness: Claude Code.

@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1c202d-e188-4e9d-b193-9184b876d2e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added the size:XL 500-999 changed lines (additions + deletions). label Aug 19, 2026
@@ -806,6 +843,11 @@ export function NewTaskDraftScreen(props: {
!isImportingShare &&
!flow.submitting &&
!(flow.workspaceMode === "worktree" && !flow.selectedBranchName);
// A share transfer locks the editor, so its rows must not be tappable.
const showComposerTriggerMenu =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumthreads/NewTaskDraftScreen.tsx:847

When an @ search is loading or returns no matches, the trigger menu disappears without showing ComposerCommandPopover, so users receive no loading or empty-results feedback. The items.length > 0 guard makes the popover's isLoading and empty-state rendering unreachable; render it whenever a trigger is active.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/NewTaskDraftScreen.tsx around line 847:
When an `@` search is loading or returns no matches, the trigger menu disappears without showing `ComposerCommandPopover`, so users receive no loading or empty-results feedback. The `items.length > 0` guard makes the popover's `isLoading` and empty-state rendering unreachable; render it whenever a trigger is active.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this against origin/main: the gate is pre-existing, not introduced here.

ThreadComposer.tsx:725 on main already reads composerTrigger && composerMenuItems.length > 0, so ComposerCommandPopover's empty-state branch — and the emptyText / "Searching files…" copy at ComposerCommandPopover.tsx:89-91,177 — was already unreachable on mobile before this PR. This patch keeps the same gate on both call sites so the two surfaces behave identically, which is what the "thread composer keeps its previous behavior exactly" constraint requires.

Relaxing it is a real improvement, but it is a deliberate UX change to both surfaces (a popover that appears on a bare @ and shows a spinner), so it belongs in its own PR rather than riding along here. Happy to send that follow-up if you want it — the one-line change is the items.length > 0 half of the condition at both call sites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

@macroscopeapp

macroscopeappBot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a small, localized mobile bug fix: it synchronizes caret reset on composer switches and removes a nonfunctional /model row from unsent drafts while preserving existing thread behavior. A separate unresolved Medium finding concerns pre-existing missing loading/empty-state feedback for @ searches and should remain visible as a merge risk.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@kirill-markin

Copy link
Copy Markdown
Author

Status, so this does not read as abandoned — the blocking finding is answered in the review thread: #7523 (comment)

Short version: the items.length > 0 guard that hides the popover's loading and empty state is not introduced here. ThreadComposer.tsx:725 already has it on main, so ComposerCommandPopover's emptyText branch (ComposerCommandPopover.tsx:89-91,177) was unreachable on mobile before this PR. Keeping the identical guard on both call sites is what makes the new surface behave exactly like the thread composer, which is the constraint this PR set for itself. Relaxing it is a deliberate UX change to both surfaces — a bare @ would start opening a popover with a spinner on the most-used surface — so I would rather send it as its own PR than bundle it here. Say the word and I will.

Everything else is green: Cursor Bugbot, Macroscope Effect Service Conventions, and the label checks. The Vercel marketing deploy fails with "Authorization required to deploy", which is the standard restriction on fork PRs rather than anything in this branch.

I also verified this locally instead of leaning on CI: 20/20 unit tests for the extracted logic, clean tsc --noEmit for apps/mobile, and a run on an Android emulator against a disposable backend where $ lists provider skills, @ lists project files, and tapping a row inserts the mention. Before/after images and a breakdown of the size:XL label are in the description.

@juliusmarminge

juliusmarminge commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation. My initial closure missed that remaining scope.

…switch
pingdotgg#8587 landed the draft composer trigger menus. Two details it left out.
`/model` bypassed the built-in capability gate, so it rendered on the
new-task draft screen, where selecting it only inserts the literal text
`/model `: `onSelect` has no `model` branch, and the `slash-model` trigger
that follows builds no items. It is now gated on `hasThread`, so the thread
composer is unchanged and the draft screen, which already carries its own
model control, loses the dead row.
The `ownerKey` caret reset ran in an effect, one commit too late, so
switching drafts could flash the trigger popover open over a draft the user
never typed a trigger into. It now adjusts during render, with the ref
assigned before `setSelection` so the restarted render cannot loop.
Model: Claude Opus 5, harness: Claude Code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kirill-markin
kirill-markinforce-pushed the fix/mobile-new-task-composer-triggers branch from ce5d27c to 6c68b1cCompareSeptember 2, 2026 07:15
@kirill-markinkirill-markin changed the title fix(mobile): support /, $ and @ in the new-task composerfix(mobile): drop the dead /model row and re-park the caret on draft switchSep 2, 2026
@github-actionsgithub-actionsBot added size:S 10-29 changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Sep 2, 2026
@kirill-markin

Copy link
Copy Markdown
Author

@juliusmarminge — rebased onto main and cut this down to exactly the two behaviors you named in the reopen. Force-pushed ce5d27c6c68b1c; the previous SHA is still in the timeline above if you want to compare.

The 775-line extraction is gone — #8587 owns that logic now, and re-landing my version of it would just be churn. What is left is one file, +10 / −4:

  • /model gated on hasThread, so it disappears from the new-task draft screen and ThreadComposer is untouched;
  • the ownerKey caret reset moved from a useEffect into the render body, so a draft switch can no longer flash the popover open for a frame.

Description rewritten to match. Two things in there worth your eye: with plan mode off, typing / mid-line on a draft can now produce no menu at all rather than the inert /model row, which I think is the right trade but is your call; and I skipped tests because apps/mobile has no hook-render harness — happy to add one if you would rather have the coverage.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kirill-markin@juliusmarminge
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(mobile): drop the dead /model row and re-park the caret on draft switch - #7523

Open
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers
Open

fix(mobile): drop the dead /model row and re-park the caret on draft switch#7523
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers

Conversation

@kirill-markin

@kirill-markinkirill-markin commented Aug 19, 2026

Copy link
Copy Markdown

What this PR is now

#8587 landed the draft composer trigger menus, so the original 775-line version of this PR is obsolete. I have rebased onto main and cut it down to the two behaviors @juliusmarminge named when reopening it:

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation.

One file, +10 / −4, both edits in apps/mobile/src/features/threads/use-composer-command-menu.ts.

1. /model is a dead row on the new-task draft screen

allBuiltIn let /model bypass the capability gate, so it rendered on both surfaces. Selecting it on a draft does nothing useful: onSelect has no model branch, so it inserts the literal text /model , detectComposerTrigger then returns a slash-model trigger this hook builds no items for, and the menu just closes. The draft screen already has a working model control in the same toolbar ("Choose model"), so the row is redundant as well as inert.

The row is now gated on hasThread. ThreadComposer passes hasThread: true and is unchanged.

One consequence worth naming: with plan mode off, NewTaskDraftScreen passes no onUpdateInteractionMode, so the built-in list is now empty. Typing / mid-line on such a draft — where provider commands are position-gated to column 0 — can produce no menu at all, whereas before it always produced the inert /model row. I think no menu beats a row that does nothing, but say the word if you would rather keep something there.

2. The caret is re-parked a render too late

The ownerKey reset ran in a useEffect, which commits after the render that already computed trigger from the previous draft's caret offset. Switching drafts could therefore flash the popover open over a draft the user never typed a trigger into. It now adjusts during render, so that pass never reaches the screen. The ref is assigned before setSelection, so the restarted render cannot loop — the same shape already used in HomeScreen.tsx and ThreadNavigationSidebar.tsx.

No path search was ever dispatched in that window — useComposerPathSearch debounces its target by 200 ms — so this is a rendering fix only.

Deliberately out of scope

  • Making /model work on mobile. Nothing in the repo consumes the slash-model trigger kind that packages/shared/src/composerTrigger.ts produces; only apps/web's ChatComposer handles the model command, by opening its own picker. So the row is inert in ThreadComposer too, not just on drafts. Pre-existing and untouched here — it needs a real mobile model picker wired into the menu. Happy to take that as its own PR.
  • The items.length > 0 guard that hides ComposerCommandPopover's loading and empty states at both call sites. Relaxing it is a deliberate UX change to the most-used surface, so it stays its own PR.
  • New tests.apps/mobile has no hook-render harness — no renderHook, no testing-library — and use-composer-command-menu.test.ts covers only the pure composerSelectionAtEnd. Adding that harness, or extracting the built-in filter into a new module to assert one boolean, is more machinery than this change earns. Say the word if you would rather have it.

Verification

CI on this PR is the gate; I ran no local checks. Statically: hasThread was already in the items memo's dependency list, ThreadComposer.tsx passes hasThread: true so its menu is byte-for-byte unchanged, and the clamp effect on draftMessage.length is a no-op after the re-park. Happy to add an emulator pass with before/after images if you want them for this one.

Model: Claude Opus 5, harness: Claude Code.

@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1c202d-e188-4e9d-b193-9184b876d2e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added the size:XL 500-999 changed lines (additions + deletions). label Aug 19, 2026
@@ -806,6 +843,11 @@ export function NewTaskDraftScreen(props: {
!isImportingShare &&
!flow.submitting &&
!(flow.workspaceMode === "worktree" && !flow.selectedBranchName);
// A share transfer locks the editor, so its rows must not be tappable.
const showComposerTriggerMenu =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumthreads/NewTaskDraftScreen.tsx:847

When an @ search is loading or returns no matches, the trigger menu disappears without showing ComposerCommandPopover, so users receive no loading or empty-results feedback. The items.length > 0 guard makes the popover's isLoading and empty-state rendering unreachable; render it whenever a trigger is active.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/NewTaskDraftScreen.tsx around line 847:
When an `@` search is loading or returns no matches, the trigger menu disappears without showing `ComposerCommandPopover`, so users receive no loading or empty-results feedback. The `items.length > 0` guard makes the popover's `isLoading` and empty-state rendering unreachable; render it whenever a trigger is active.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this against origin/main: the gate is pre-existing, not introduced here.

ThreadComposer.tsx:725 on main already reads composerTrigger && composerMenuItems.length > 0, so ComposerCommandPopover's empty-state branch — and the emptyText / "Searching files…" copy at ComposerCommandPopover.tsx:89-91,177 — was already unreachable on mobile before this PR. This patch keeps the same gate on both call sites so the two surfaces behave identically, which is what the "thread composer keeps its previous behavior exactly" constraint requires.

Relaxing it is a real improvement, but it is a deliberate UX change to both surfaces (a popover that appears on a bare @ and shows a spinner), so it belongs in its own PR rather than riding along here. Happy to send that follow-up if you want it — the one-line change is the items.length > 0 half of the condition at both call sites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

@macroscopeapp

macroscopeappBot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a small, localized mobile bug fix: it synchronizes caret reset on composer switches and removes a nonfunctional /model row from unsent drafts while preserving existing thread behavior. A separate unresolved Medium finding concerns pre-existing missing loading/empty-state feedback for @ searches and should remain visible as a merge risk.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@kirill-markin

Copy link
Copy Markdown
Author

Status, so this does not read as abandoned — the blocking finding is answered in the review thread: #7523 (comment)

Short version: the items.length > 0 guard that hides the popover's loading and empty state is not introduced here. ThreadComposer.tsx:725 already has it on main, so ComposerCommandPopover's emptyText branch (ComposerCommandPopover.tsx:89-91,177) was unreachable on mobile before this PR. Keeping the identical guard on both call sites is what makes the new surface behave exactly like the thread composer, which is the constraint this PR set for itself. Relaxing it is a deliberate UX change to both surfaces — a bare @ would start opening a popover with a spinner on the most-used surface — so I would rather send it as its own PR than bundle it here. Say the word and I will.

Everything else is green: Cursor Bugbot, Macroscope Effect Service Conventions, and the label checks. The Vercel marketing deploy fails with "Authorization required to deploy", which is the standard restriction on fork PRs rather than anything in this branch.

I also verified this locally instead of leaning on CI: 20/20 unit tests for the extracted logic, clean tsc --noEmit for apps/mobile, and a run on an Android emulator against a disposable backend where $ lists provider skills, @ lists project files, and tapping a row inserts the mention. Before/after images and a breakdown of the size:XL label are in the description.

@juliusmarminge

juliusmarminge commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation. My initial closure missed that remaining scope.

…switch
pingdotgg#8587 landed the draft composer trigger menus. Two details it left out.
`/model` bypassed the built-in capability gate, so it rendered on the
new-task draft screen, where selecting it only inserts the literal text
`/model `: `onSelect` has no `model` branch, and the `slash-model` trigger
that follows builds no items. It is now gated on `hasThread`, so the thread
composer is unchanged and the draft screen, which already carries its own
model control, loses the dead row.
The `ownerKey` caret reset ran in an effect, one commit too late, so
switching drafts could flash the trigger popover open over a draft the user
never typed a trigger into. It now adjusts during render, with the ref
assigned before `setSelection` so the restarted render cannot loop.
Model: Claude Opus 5, harness: Claude Code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kirill-markin
kirill-markinforce-pushed the fix/mobile-new-task-composer-triggers branch from ce5d27c to 6c68b1cCompareSeptember 2, 2026 07:15
@kirill-markinkirill-markin changed the title fix(mobile): support /, $ and @ in the new-task composerfix(mobile): drop the dead /model row and re-park the caret on draft switchSep 2, 2026
@github-actionsgithub-actionsBot added size:S 10-29 changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Sep 2, 2026
@kirill-markin

Copy link
Copy Markdown
Author

@juliusmarminge — rebased onto main and cut this down to exactly the two behaviors you named in the reopen. Force-pushed ce5d27c6c68b1c; the previous SHA is still in the timeline above if you want to compare.

The 775-line extraction is gone — #8587 owns that logic now, and re-landing my version of it would just be churn. What is left is one file, +10 / −4:

  • /model gated on hasThread, so it disappears from the new-task draft screen and ThreadComposer is untouched;
  • the ownerKey caret reset moved from a useEffect into the render body, so a draft switch can no longer flash the popover open for a frame.

Description rewritten to match. Two things in there worth your eye: with plan mode off, typing / mid-line on a draft can now produce no menu at all rather than the inert /model row, which I think is the right trade but is your call; and I skipped tests because apps/mobile has no hook-render harness — happy to add one if you would rather have the coverage.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kirill-markin@juliusmarminge
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(mobile): drop the dead /model row and re-park the caret on draft switch - #7523

Open
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers
Open

fix(mobile): drop the dead /model row and re-park the caret on draft switch#7523
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers

Conversation

@kirill-markin

@kirill-markinkirill-markin commented Aug 19, 2026

Copy link
Copy Markdown

What this PR is now

#8587 landed the draft composer trigger menus, so the original 775-line version of this PR is obsolete. I have rebased onto main and cut it down to the two behaviors @juliusmarminge named when reopening it:

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation.

One file, +10 / −4, both edits in apps/mobile/src/features/threads/use-composer-command-menu.ts.

1. /model is a dead row on the new-task draft screen

allBuiltIn let /model bypass the capability gate, so it rendered on both surfaces. Selecting it on a draft does nothing useful: onSelect has no model branch, so it inserts the literal text /model , detectComposerTrigger then returns a slash-model trigger this hook builds no items for, and the menu just closes. The draft screen already has a working model control in the same toolbar ("Choose model"), so the row is redundant as well as inert.

The row is now gated on hasThread. ThreadComposer passes hasThread: true and is unchanged.

One consequence worth naming: with plan mode off, NewTaskDraftScreen passes no onUpdateInteractionMode, so the built-in list is now empty. Typing / mid-line on such a draft — where provider commands are position-gated to column 0 — can produce no menu at all, whereas before it always produced the inert /model row. I think no menu beats a row that does nothing, but say the word if you would rather keep something there.

2. The caret is re-parked a render too late

The ownerKey reset ran in a useEffect, which commits after the render that already computed trigger from the previous draft's caret offset. Switching drafts could therefore flash the popover open over a draft the user never typed a trigger into. It now adjusts during render, so that pass never reaches the screen. The ref is assigned before setSelection, so the restarted render cannot loop — the same shape already used in HomeScreen.tsx and ThreadNavigationSidebar.tsx.

No path search was ever dispatched in that window — useComposerPathSearch debounces its target by 200 ms — so this is a rendering fix only.

Deliberately out of scope

  • Making /model work on mobile. Nothing in the repo consumes the slash-model trigger kind that packages/shared/src/composerTrigger.ts produces; only apps/web's ChatComposer handles the model command, by opening its own picker. So the row is inert in ThreadComposer too, not just on drafts. Pre-existing and untouched here — it needs a real mobile model picker wired into the menu. Happy to take that as its own PR.
  • The items.length > 0 guard that hides ComposerCommandPopover's loading and empty states at both call sites. Relaxing it is a deliberate UX change to the most-used surface, so it stays its own PR.
  • New tests.apps/mobile has no hook-render harness — no renderHook, no testing-library — and use-composer-command-menu.test.ts covers only the pure composerSelectionAtEnd. Adding that harness, or extracting the built-in filter into a new module to assert one boolean, is more machinery than this change earns. Say the word if you would rather have it.

Verification

CI on this PR is the gate; I ran no local checks. Statically: hasThread was already in the items memo's dependency list, ThreadComposer.tsx passes hasThread: true so its menu is byte-for-byte unchanged, and the clamp effect on draftMessage.length is a no-op after the re-park. Happy to add an emulator pass with before/after images if you want them for this one.

Model: Claude Opus 5, harness: Claude Code.

@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1c202d-e188-4e9d-b193-9184b876d2e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added the size:XL 500-999 changed lines (additions + deletions). label Aug 19, 2026
@@ -806,6 +843,11 @@ export function NewTaskDraftScreen(props: {
!isImportingShare &&
!flow.submitting &&
!(flow.workspaceMode === "worktree" && !flow.selectedBranchName);
// A share transfer locks the editor, so its rows must not be tappable.
const showComposerTriggerMenu =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumthreads/NewTaskDraftScreen.tsx:847

When an @ search is loading or returns no matches, the trigger menu disappears without showing ComposerCommandPopover, so users receive no loading or empty-results feedback. The items.length > 0 guard makes the popover's isLoading and empty-state rendering unreachable; render it whenever a trigger is active.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/NewTaskDraftScreen.tsx around line 847:
When an `@` search is loading or returns no matches, the trigger menu disappears without showing `ComposerCommandPopover`, so users receive no loading or empty-results feedback. The `items.length > 0` guard makes the popover's `isLoading` and empty-state rendering unreachable; render it whenever a trigger is active.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this against origin/main: the gate is pre-existing, not introduced here.

ThreadComposer.tsx:725 on main already reads composerTrigger && composerMenuItems.length > 0, so ComposerCommandPopover's empty-state branch — and the emptyText / "Searching files…" copy at ComposerCommandPopover.tsx:89-91,177 — was already unreachable on mobile before this PR. This patch keeps the same gate on both call sites so the two surfaces behave identically, which is what the "thread composer keeps its previous behavior exactly" constraint requires.

Relaxing it is a real improvement, but it is a deliberate UX change to both surfaces (a popover that appears on a bare @ and shows a spinner), so it belongs in its own PR rather than riding along here. Happy to send that follow-up if you want it — the one-line change is the items.length > 0 half of the condition at both call sites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

@macroscopeapp

macroscopeappBot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a small, localized mobile bug fix: it synchronizes caret reset on composer switches and removes a nonfunctional /model row from unsent drafts while preserving existing thread behavior. A separate unresolved Medium finding concerns pre-existing missing loading/empty-state feedback for @ searches and should remain visible as a merge risk.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@kirill-markin

Copy link
Copy Markdown
Author

Status, so this does not read as abandoned — the blocking finding is answered in the review thread: #7523 (comment)

Short version: the items.length > 0 guard that hides the popover's loading and empty state is not introduced here. ThreadComposer.tsx:725 already has it on main, so ComposerCommandPopover's emptyText branch (ComposerCommandPopover.tsx:89-91,177) was unreachable on mobile before this PR. Keeping the identical guard on both call sites is what makes the new surface behave exactly like the thread composer, which is the constraint this PR set for itself. Relaxing it is a deliberate UX change to both surfaces — a bare @ would start opening a popover with a spinner on the most-used surface — so I would rather send it as its own PR than bundle it here. Say the word and I will.

Everything else is green: Cursor Bugbot, Macroscope Effect Service Conventions, and the label checks. The Vercel marketing deploy fails with "Authorization required to deploy", which is the standard restriction on fork PRs rather than anything in this branch.

I also verified this locally instead of leaning on CI: 20/20 unit tests for the extracted logic, clean tsc --noEmit for apps/mobile, and a run on an Android emulator against a disposable backend where $ lists provider skills, @ lists project files, and tapping a row inserts the mention. Before/after images and a breakdown of the size:XL label are in the description.

@juliusmarminge

juliusmarminge commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation. My initial closure missed that remaining scope.

…switch
pingdotgg#8587 landed the draft composer trigger menus. Two details it left out.
`/model` bypassed the built-in capability gate, so it rendered on the
new-task draft screen, where selecting it only inserts the literal text
`/model `: `onSelect` has no `model` branch, and the `slash-model` trigger
that follows builds no items. It is now gated on `hasThread`, so the thread
composer is unchanged and the draft screen, which already carries its own
model control, loses the dead row.
The `ownerKey` caret reset ran in an effect, one commit too late, so
switching drafts could flash the trigger popover open over a draft the user
never typed a trigger into. It now adjusts during render, with the ref
assigned before `setSelection` so the restarted render cannot loop.
Model: Claude Opus 5, harness: Claude Code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kirill-markin
kirill-markinforce-pushed the fix/mobile-new-task-composer-triggers branch from ce5d27c to 6c68b1cCompareSeptember 2, 2026 07:15
@kirill-markinkirill-markin changed the title fix(mobile): support /, $ and @ in the new-task composerfix(mobile): drop the dead /model row and re-park the caret on draft switchSep 2, 2026
@github-actionsgithub-actionsBot added size:S 10-29 changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Sep 2, 2026
@kirill-markin

Copy link
Copy Markdown
Author

@juliusmarminge — rebased onto main and cut this down to exactly the two behaviors you named in the reopen. Force-pushed ce5d27c6c68b1c; the previous SHA is still in the timeline above if you want to compare.

The 775-line extraction is gone — #8587 owns that logic now, and re-landing my version of it would just be churn. What is left is one file, +10 / −4:

  • /model gated on hasThread, so it disappears from the new-task draft screen and ThreadComposer is untouched;
  • the ownerKey caret reset moved from a useEffect into the render body, so a draft switch can no longer flash the popover open for a frame.

Description rewritten to match. Two things in there worth your eye: with plan mode off, typing / mid-line on a draft can now produce no menu at all rather than the inert /model row, which I think is the right trade but is your call; and I skipped tests because apps/mobile has no hook-render harness — happy to add one if you would rather have the coverage.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kirill-markin@juliusmarminge
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(mobile): drop the dead /model row and re-park the caret on draft switch - #7523

Open
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers
Open

fix(mobile): drop the dead /model row and re-park the caret on draft switch#7523
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers

Conversation

@kirill-markin

@kirill-markinkirill-markin commented Aug 19, 2026

Copy link
Copy Markdown

What this PR is now

#8587 landed the draft composer trigger menus, so the original 775-line version of this PR is obsolete. I have rebased onto main and cut it down to the two behaviors @juliusmarminge named when reopening it:

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation.

One file, +10 / −4, both edits in apps/mobile/src/features/threads/use-composer-command-menu.ts.

1. /model is a dead row on the new-task draft screen

allBuiltIn let /model bypass the capability gate, so it rendered on both surfaces. Selecting it on a draft does nothing useful: onSelect has no model branch, so it inserts the literal text /model , detectComposerTrigger then returns a slash-model trigger this hook builds no items for, and the menu just closes. The draft screen already has a working model control in the same toolbar ("Choose model"), so the row is redundant as well as inert.

The row is now gated on hasThread. ThreadComposer passes hasThread: true and is unchanged.

One consequence worth naming: with plan mode off, NewTaskDraftScreen passes no onUpdateInteractionMode, so the built-in list is now empty. Typing / mid-line on such a draft — where provider commands are position-gated to column 0 — can produce no menu at all, whereas before it always produced the inert /model row. I think no menu beats a row that does nothing, but say the word if you would rather keep something there.

2. The caret is re-parked a render too late

The ownerKey reset ran in a useEffect, which commits after the render that already computed trigger from the previous draft's caret offset. Switching drafts could therefore flash the popover open over a draft the user never typed a trigger into. It now adjusts during render, so that pass never reaches the screen. The ref is assigned before setSelection, so the restarted render cannot loop — the same shape already used in HomeScreen.tsx and ThreadNavigationSidebar.tsx.

No path search was ever dispatched in that window — useComposerPathSearch debounces its target by 200 ms — so this is a rendering fix only.

Deliberately out of scope

  • Making /model work on mobile. Nothing in the repo consumes the slash-model trigger kind that packages/shared/src/composerTrigger.ts produces; only apps/web's ChatComposer handles the model command, by opening its own picker. So the row is inert in ThreadComposer too, not just on drafts. Pre-existing and untouched here — it needs a real mobile model picker wired into the menu. Happy to take that as its own PR.
  • The items.length > 0 guard that hides ComposerCommandPopover's loading and empty states at both call sites. Relaxing it is a deliberate UX change to the most-used surface, so it stays its own PR.
  • New tests.apps/mobile has no hook-render harness — no renderHook, no testing-library — and use-composer-command-menu.test.ts covers only the pure composerSelectionAtEnd. Adding that harness, or extracting the built-in filter into a new module to assert one boolean, is more machinery than this change earns. Say the word if you would rather have it.

Verification

CI on this PR is the gate; I ran no local checks. Statically: hasThread was already in the items memo's dependency list, ThreadComposer.tsx passes hasThread: true so its menu is byte-for-byte unchanged, and the clamp effect on draftMessage.length is a no-op after the re-park. Happy to add an emulator pass with before/after images if you want them for this one.

Model: Claude Opus 5, harness: Claude Code.

@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1c202d-e188-4e9d-b193-9184b876d2e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added the size:XL 500-999 changed lines (additions + deletions). label Aug 19, 2026
@@ -806,6 +843,11 @@ export function NewTaskDraftScreen(props: {
!isImportingShare &&
!flow.submitting &&
!(flow.workspaceMode === "worktree" && !flow.selectedBranchName);
// A share transfer locks the editor, so its rows must not be tappable.
const showComposerTriggerMenu =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumthreads/NewTaskDraftScreen.tsx:847

When an @ search is loading or returns no matches, the trigger menu disappears without showing ComposerCommandPopover, so users receive no loading or empty-results feedback. The items.length > 0 guard makes the popover's isLoading and empty-state rendering unreachable; render it whenever a trigger is active.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/NewTaskDraftScreen.tsx around line 847:
When an `@` search is loading or returns no matches, the trigger menu disappears without showing `ComposerCommandPopover`, so users receive no loading or empty-results feedback. The `items.length > 0` guard makes the popover's `isLoading` and empty-state rendering unreachable; render it whenever a trigger is active.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this against origin/main: the gate is pre-existing, not introduced here.

ThreadComposer.tsx:725 on main already reads composerTrigger && composerMenuItems.length > 0, so ComposerCommandPopover's empty-state branch — and the emptyText / "Searching files…" copy at ComposerCommandPopover.tsx:89-91,177 — was already unreachable on mobile before this PR. This patch keeps the same gate on both call sites so the two surfaces behave identically, which is what the "thread composer keeps its previous behavior exactly" constraint requires.

Relaxing it is a real improvement, but it is a deliberate UX change to both surfaces (a popover that appears on a bare @ and shows a spinner), so it belongs in its own PR rather than riding along here. Happy to send that follow-up if you want it — the one-line change is the items.length > 0 half of the condition at both call sites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

@macroscopeapp

macroscopeappBot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a small, localized mobile bug fix: it synchronizes caret reset on composer switches and removes a nonfunctional /model row from unsent drafts while preserving existing thread behavior. A separate unresolved Medium finding concerns pre-existing missing loading/empty-state feedback for @ searches and should remain visible as a merge risk.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@kirill-markin

Copy link
Copy Markdown
Author

Status, so this does not read as abandoned — the blocking finding is answered in the review thread: #7523 (comment)

Short version: the items.length > 0 guard that hides the popover's loading and empty state is not introduced here. ThreadComposer.tsx:725 already has it on main, so ComposerCommandPopover's emptyText branch (ComposerCommandPopover.tsx:89-91,177) was unreachable on mobile before this PR. Keeping the identical guard on both call sites is what makes the new surface behave exactly like the thread composer, which is the constraint this PR set for itself. Relaxing it is a deliberate UX change to both surfaces — a bare @ would start opening a popover with a spinner on the most-used surface — so I would rather send it as its own PR than bundle it here. Say the word and I will.

Everything else is green: Cursor Bugbot, Macroscope Effect Service Conventions, and the label checks. The Vercel marketing deploy fails with "Authorization required to deploy", which is the standard restriction on fork PRs rather than anything in this branch.

I also verified this locally instead of leaning on CI: 20/20 unit tests for the extracted logic, clean tsc --noEmit for apps/mobile, and a run on an Android emulator against a disposable backend where $ lists provider skills, @ lists project files, and tapping a row inserts the mention. Before/after images and a breakdown of the size:XL label are in the description.

@juliusmarminge

juliusmarminge commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation. My initial closure missed that remaining scope.

…switch
pingdotgg#8587 landed the draft composer trigger menus. Two details it left out.
`/model` bypassed the built-in capability gate, so it rendered on the
new-task draft screen, where selecting it only inserts the literal text
`/model `: `onSelect` has no `model` branch, and the `slash-model` trigger
that follows builds no items. It is now gated on `hasThread`, so the thread
composer is unchanged and the draft screen, which already carries its own
model control, loses the dead row.
The `ownerKey` caret reset ran in an effect, one commit too late, so
switching drafts could flash the trigger popover open over a draft the user
never typed a trigger into. It now adjusts during render, with the ref
assigned before `setSelection` so the restarted render cannot loop.
Model: Claude Opus 5, harness: Claude Code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kirill-markin
kirill-markinforce-pushed the fix/mobile-new-task-composer-triggers branch from ce5d27c to 6c68b1cCompareSeptember 2, 2026 07:15
@kirill-markinkirill-markin changed the title fix(mobile): support /, $ and @ in the new-task composerfix(mobile): drop the dead /model row and re-park the caret on draft switchSep 2, 2026
@github-actionsgithub-actionsBot added size:S 10-29 changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Sep 2, 2026
@kirill-markin

Copy link
Copy Markdown
Author

@juliusmarminge — rebased onto main and cut this down to exactly the two behaviors you named in the reopen. Force-pushed ce5d27c6c68b1c; the previous SHA is still in the timeline above if you want to compare.

The 775-line extraction is gone — #8587 owns that logic now, and re-landing my version of it would just be churn. What is left is one file, +10 / −4:

  • /model gated on hasThread, so it disappears from the new-task draft screen and ThreadComposer is untouched;
  • the ownerKey caret reset moved from a useEffect into the render body, so a draft switch can no longer flash the popover open for a frame.

Description rewritten to match. Two things in there worth your eye: with plan mode off, typing / mid-line on a draft can now produce no menu at all rather than the inert /model row, which I think is the right trade but is your call; and I skipped tests because apps/mobile has no hook-render harness — happy to add one if you would rather have the coverage.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kirill-markin@juliusmarminge
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(mobile): drop the dead /model row and re-park the caret on draft switch - #7523

Open
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers
Open

fix(mobile): drop the dead /model row and re-park the caret on draft switch#7523
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers

Conversation

@kirill-markin

@kirill-markinkirill-markin commented Aug 19, 2026

Copy link
Copy Markdown

What this PR is now

#8587 landed the draft composer trigger menus, so the original 775-line version of this PR is obsolete. I have rebased onto main and cut it down to the two behaviors @juliusmarminge named when reopening it:

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation.

One file, +10 / −4, both edits in apps/mobile/src/features/threads/use-composer-command-menu.ts.

1. /model is a dead row on the new-task draft screen

allBuiltIn let /model bypass the capability gate, so it rendered on both surfaces. Selecting it on a draft does nothing useful: onSelect has no model branch, so it inserts the literal text /model , detectComposerTrigger then returns a slash-model trigger this hook builds no items for, and the menu just closes. The draft screen already has a working model control in the same toolbar ("Choose model"), so the row is redundant as well as inert.

The row is now gated on hasThread. ThreadComposer passes hasThread: true and is unchanged.

One consequence worth naming: with plan mode off, NewTaskDraftScreen passes no onUpdateInteractionMode, so the built-in list is now empty. Typing / mid-line on such a draft — where provider commands are position-gated to column 0 — can produce no menu at all, whereas before it always produced the inert /model row. I think no menu beats a row that does nothing, but say the word if you would rather keep something there.

2. The caret is re-parked a render too late

The ownerKey reset ran in a useEffect, which commits after the render that already computed trigger from the previous draft's caret offset. Switching drafts could therefore flash the popover open over a draft the user never typed a trigger into. It now adjusts during render, so that pass never reaches the screen. The ref is assigned before setSelection, so the restarted render cannot loop — the same shape already used in HomeScreen.tsx and ThreadNavigationSidebar.tsx.

No path search was ever dispatched in that window — useComposerPathSearch debounces its target by 200 ms — so this is a rendering fix only.

Deliberately out of scope

  • Making /model work on mobile. Nothing in the repo consumes the slash-model trigger kind that packages/shared/src/composerTrigger.ts produces; only apps/web's ChatComposer handles the model command, by opening its own picker. So the row is inert in ThreadComposer too, not just on drafts. Pre-existing and untouched here — it needs a real mobile model picker wired into the menu. Happy to take that as its own PR.
  • The items.length > 0 guard that hides ComposerCommandPopover's loading and empty states at both call sites. Relaxing it is a deliberate UX change to the most-used surface, so it stays its own PR.
  • New tests.apps/mobile has no hook-render harness — no renderHook, no testing-library — and use-composer-command-menu.test.ts covers only the pure composerSelectionAtEnd. Adding that harness, or extracting the built-in filter into a new module to assert one boolean, is more machinery than this change earns. Say the word if you would rather have it.

Verification

CI on this PR is the gate; I ran no local checks. Statically: hasThread was already in the items memo's dependency list, ThreadComposer.tsx passes hasThread: true so its menu is byte-for-byte unchanged, and the clamp effect on draftMessage.length is a no-op after the re-park. Happy to add an emulator pass with before/after images if you want them for this one.

Model: Claude Opus 5, harness: Claude Code.

@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1c202d-e188-4e9d-b193-9184b876d2e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added the size:XL 500-999 changed lines (additions + deletions). label Aug 19, 2026
@@ -806,6 +843,11 @@ export function NewTaskDraftScreen(props: {
!isImportingShare &&
!flow.submitting &&
!(flow.workspaceMode === "worktree" && !flow.selectedBranchName);
// A share transfer locks the editor, so its rows must not be tappable.
const showComposerTriggerMenu =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumthreads/NewTaskDraftScreen.tsx:847

When an @ search is loading or returns no matches, the trigger menu disappears without showing ComposerCommandPopover, so users receive no loading or empty-results feedback. The items.length > 0 guard makes the popover's isLoading and empty-state rendering unreachable; render it whenever a trigger is active.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/NewTaskDraftScreen.tsx around line 847:
When an `@` search is loading or returns no matches, the trigger menu disappears without showing `ComposerCommandPopover`, so users receive no loading or empty-results feedback. The `items.length > 0` guard makes the popover's `isLoading` and empty-state rendering unreachable; render it whenever a trigger is active.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this against origin/main: the gate is pre-existing, not introduced here.

ThreadComposer.tsx:725 on main already reads composerTrigger && composerMenuItems.length > 0, so ComposerCommandPopover's empty-state branch — and the emptyText / "Searching files…" copy at ComposerCommandPopover.tsx:89-91,177 — was already unreachable on mobile before this PR. This patch keeps the same gate on both call sites so the two surfaces behave identically, which is what the "thread composer keeps its previous behavior exactly" constraint requires.

Relaxing it is a real improvement, but it is a deliberate UX change to both surfaces (a popover that appears on a bare @ and shows a spinner), so it belongs in its own PR rather than riding along here. Happy to send that follow-up if you want it — the one-line change is the items.length > 0 half of the condition at both call sites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

@macroscopeapp

macroscopeappBot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a small, localized mobile bug fix: it synchronizes caret reset on composer switches and removes a nonfunctional /model row from unsent drafts while preserving existing thread behavior. A separate unresolved Medium finding concerns pre-existing missing loading/empty-state feedback for @ searches and should remain visible as a merge risk.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@kirill-markin

Copy link
Copy Markdown
Author

Status, so this does not read as abandoned — the blocking finding is answered in the review thread: #7523 (comment)

Short version: the items.length > 0 guard that hides the popover's loading and empty state is not introduced here. ThreadComposer.tsx:725 already has it on main, so ComposerCommandPopover's emptyText branch (ComposerCommandPopover.tsx:89-91,177) was unreachable on mobile before this PR. Keeping the identical guard on both call sites is what makes the new surface behave exactly like the thread composer, which is the constraint this PR set for itself. Relaxing it is a deliberate UX change to both surfaces — a bare @ would start opening a popover with a spinner on the most-used surface — so I would rather send it as its own PR than bundle it here. Say the word and I will.

Everything else is green: Cursor Bugbot, Macroscope Effect Service Conventions, and the label checks. The Vercel marketing deploy fails with "Authorization required to deploy", which is the standard restriction on fork PRs rather than anything in this branch.

I also verified this locally instead of leaning on CI: 20/20 unit tests for the extracted logic, clean tsc --noEmit for apps/mobile, and a run on an Android emulator against a disposable backend where $ lists provider skills, @ lists project files, and tapping a row inserts the mention. Before/after images and a breakdown of the size:XL label are in the description.

@juliusmarminge

juliusmarminge commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation. My initial closure missed that remaining scope.

…switch
pingdotgg#8587 landed the draft composer trigger menus. Two details it left out.
`/model` bypassed the built-in capability gate, so it rendered on the
new-task draft screen, where selecting it only inserts the literal text
`/model `: `onSelect` has no `model` branch, and the `slash-model` trigger
that follows builds no items. It is now gated on `hasThread`, so the thread
composer is unchanged and the draft screen, which already carries its own
model control, loses the dead row.
The `ownerKey` caret reset ran in an effect, one commit too late, so
switching drafts could flash the trigger popover open over a draft the user
never typed a trigger into. It now adjusts during render, with the ref
assigned before `setSelection` so the restarted render cannot loop.
Model: Claude Opus 5, harness: Claude Code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kirill-markin
kirill-markinforce-pushed the fix/mobile-new-task-composer-triggers branch from ce5d27c to 6c68b1cCompareSeptember 2, 2026 07:15
@kirill-markinkirill-markin changed the title fix(mobile): support /, $ and @ in the new-task composerfix(mobile): drop the dead /model row and re-park the caret on draft switchSep 2, 2026
@github-actionsgithub-actionsBot added size:S 10-29 changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Sep 2, 2026
@kirill-markin

Copy link
Copy Markdown
Author

@juliusmarminge — rebased onto main and cut this down to exactly the two behaviors you named in the reopen. Force-pushed ce5d27c6c68b1c; the previous SHA is still in the timeline above if you want to compare.

The 775-line extraction is gone — #8587 owns that logic now, and re-landing my version of it would just be churn. What is left is one file, +10 / −4:

  • /model gated on hasThread, so it disappears from the new-task draft screen and ThreadComposer is untouched;
  • the ownerKey caret reset moved from a useEffect into the render body, so a draft switch can no longer flash the popover open for a frame.

Description rewritten to match. Two things in there worth your eye: with plan mode off, typing / mid-line on a draft can now produce no menu at all rather than the inert /model row, which I think is the right trade but is your call; and I skipped tests because apps/mobile has no hook-render harness — happy to add one if you would rather have the coverage.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kirill-markin@juliusmarminge
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(mobile): drop the dead /model row and re-park the caret on draft switch - #7523

Open
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers
Open

fix(mobile): drop the dead /model row and re-park the caret on draft switch#7523
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers

Conversation

@kirill-markin

@kirill-markinkirill-markin commented Aug 19, 2026

Copy link
Copy Markdown

What this PR is now

#8587 landed the draft composer trigger menus, so the original 775-line version of this PR is obsolete. I have rebased onto main and cut it down to the two behaviors @juliusmarminge named when reopening it:

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation.

One file, +10 / −4, both edits in apps/mobile/src/features/threads/use-composer-command-menu.ts.

1. /model is a dead row on the new-task draft screen

allBuiltIn let /model bypass the capability gate, so it rendered on both surfaces. Selecting it on a draft does nothing useful: onSelect has no model branch, so it inserts the literal text /model , detectComposerTrigger then returns a slash-model trigger this hook builds no items for, and the menu just closes. The draft screen already has a working model control in the same toolbar ("Choose model"), so the row is redundant as well as inert.

The row is now gated on hasThread. ThreadComposer passes hasThread: true and is unchanged.

One consequence worth naming: with plan mode off, NewTaskDraftScreen passes no onUpdateInteractionMode, so the built-in list is now empty. Typing / mid-line on such a draft — where provider commands are position-gated to column 0 — can produce no menu at all, whereas before it always produced the inert /model row. I think no menu beats a row that does nothing, but say the word if you would rather keep something there.

2. The caret is re-parked a render too late

The ownerKey reset ran in a useEffect, which commits after the render that already computed trigger from the previous draft's caret offset. Switching drafts could therefore flash the popover open over a draft the user never typed a trigger into. It now adjusts during render, so that pass never reaches the screen. The ref is assigned before setSelection, so the restarted render cannot loop — the same shape already used in HomeScreen.tsx and ThreadNavigationSidebar.tsx.

No path search was ever dispatched in that window — useComposerPathSearch debounces its target by 200 ms — so this is a rendering fix only.

Deliberately out of scope

  • Making /model work on mobile. Nothing in the repo consumes the slash-model trigger kind that packages/shared/src/composerTrigger.ts produces; only apps/web's ChatComposer handles the model command, by opening its own picker. So the row is inert in ThreadComposer too, not just on drafts. Pre-existing and untouched here — it needs a real mobile model picker wired into the menu. Happy to take that as its own PR.
  • The items.length > 0 guard that hides ComposerCommandPopover's loading and empty states at both call sites. Relaxing it is a deliberate UX change to the most-used surface, so it stays its own PR.
  • New tests.apps/mobile has no hook-render harness — no renderHook, no testing-library — and use-composer-command-menu.test.ts covers only the pure composerSelectionAtEnd. Adding that harness, or extracting the built-in filter into a new module to assert one boolean, is more machinery than this change earns. Say the word if you would rather have it.

Verification

CI on this PR is the gate; I ran no local checks. Statically: hasThread was already in the items memo's dependency list, ThreadComposer.tsx passes hasThread: true so its menu is byte-for-byte unchanged, and the clamp effect on draftMessage.length is a no-op after the re-park. Happy to add an emulator pass with before/after images if you want them for this one.

Model: Claude Opus 5, harness: Claude Code.

@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1c202d-e188-4e9d-b193-9184b876d2e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added the size:XL 500-999 changed lines (additions + deletions). label Aug 19, 2026
@@ -806,6 +843,11 @@ export function NewTaskDraftScreen(props: {
!isImportingShare &&
!flow.submitting &&
!(flow.workspaceMode === "worktree" && !flow.selectedBranchName);
// A share transfer locks the editor, so its rows must not be tappable.
const showComposerTriggerMenu =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumthreads/NewTaskDraftScreen.tsx:847

When an @ search is loading or returns no matches, the trigger menu disappears without showing ComposerCommandPopover, so users receive no loading or empty-results feedback. The items.length > 0 guard makes the popover's isLoading and empty-state rendering unreachable; render it whenever a trigger is active.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/NewTaskDraftScreen.tsx around line 847:
When an `@` search is loading or returns no matches, the trigger menu disappears without showing `ComposerCommandPopover`, so users receive no loading or empty-results feedback. The `items.length > 0` guard makes the popover's `isLoading` and empty-state rendering unreachable; render it whenever a trigger is active.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this against origin/main: the gate is pre-existing, not introduced here.

ThreadComposer.tsx:725 on main already reads composerTrigger && composerMenuItems.length > 0, so ComposerCommandPopover's empty-state branch — and the emptyText / "Searching files…" copy at ComposerCommandPopover.tsx:89-91,177 — was already unreachable on mobile before this PR. This patch keeps the same gate on both call sites so the two surfaces behave identically, which is what the "thread composer keeps its previous behavior exactly" constraint requires.

Relaxing it is a real improvement, but it is a deliberate UX change to both surfaces (a popover that appears on a bare @ and shows a spinner), so it belongs in its own PR rather than riding along here. Happy to send that follow-up if you want it — the one-line change is the items.length > 0 half of the condition at both call sites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

@macroscopeapp

macroscopeappBot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a small, localized mobile bug fix: it synchronizes caret reset on composer switches and removes a nonfunctional /model row from unsent drafts while preserving existing thread behavior. A separate unresolved Medium finding concerns pre-existing missing loading/empty-state feedback for @ searches and should remain visible as a merge risk.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@kirill-markin

Copy link
Copy Markdown
Author

Status, so this does not read as abandoned — the blocking finding is answered in the review thread: #7523 (comment)

Short version: the items.length > 0 guard that hides the popover's loading and empty state is not introduced here. ThreadComposer.tsx:725 already has it on main, so ComposerCommandPopover's emptyText branch (ComposerCommandPopover.tsx:89-91,177) was unreachable on mobile before this PR. Keeping the identical guard on both call sites is what makes the new surface behave exactly like the thread composer, which is the constraint this PR set for itself. Relaxing it is a deliberate UX change to both surfaces — a bare @ would start opening a popover with a spinner on the most-used surface — so I would rather send it as its own PR than bundle it here. Say the word and I will.

Everything else is green: Cursor Bugbot, Macroscope Effect Service Conventions, and the label checks. The Vercel marketing deploy fails with "Authorization required to deploy", which is the standard restriction on fork PRs rather than anything in this branch.

I also verified this locally instead of leaning on CI: 20/20 unit tests for the extracted logic, clean tsc --noEmit for apps/mobile, and a run on an Android emulator against a disposable backend where $ lists provider skills, @ lists project files, and tapping a row inserts the mention. Before/after images and a breakdown of the size:XL label are in the description.

@juliusmarminge

juliusmarminge commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation. My initial closure missed that remaining scope.

…switch
pingdotgg#8587 landed the draft composer trigger menus. Two details it left out.
`/model` bypassed the built-in capability gate, so it rendered on the
new-task draft screen, where selecting it only inserts the literal text
`/model `: `onSelect` has no `model` branch, and the `slash-model` trigger
that follows builds no items. It is now gated on `hasThread`, so the thread
composer is unchanged and the draft screen, which already carries its own
model control, loses the dead row.
The `ownerKey` caret reset ran in an effect, one commit too late, so
switching drafts could flash the trigger popover open over a draft the user
never typed a trigger into. It now adjusts during render, with the ref
assigned before `setSelection` so the restarted render cannot loop.
Model: Claude Opus 5, harness: Claude Code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kirill-markin
kirill-markinforce-pushed the fix/mobile-new-task-composer-triggers branch from ce5d27c to 6c68b1cCompareSeptember 2, 2026 07:15
@kirill-markinkirill-markin changed the title fix(mobile): support /, $ and @ in the new-task composerfix(mobile): drop the dead /model row and re-park the caret on draft switchSep 2, 2026
@github-actionsgithub-actionsBot added size:S 10-29 changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Sep 2, 2026
@kirill-markin

Copy link
Copy Markdown
Author

@juliusmarminge — rebased onto main and cut this down to exactly the two behaviors you named in the reopen. Force-pushed ce5d27c6c68b1c; the previous SHA is still in the timeline above if you want to compare.

The 775-line extraction is gone — #8587 owns that logic now, and re-landing my version of it would just be churn. What is left is one file, +10 / −4:

  • /model gated on hasThread, so it disappears from the new-task draft screen and ThreadComposer is untouched;
  • the ownerKey caret reset moved from a useEffect into the render body, so a draft switch can no longer flash the popover open for a frame.

Description rewritten to match. Two things in there worth your eye: with plan mode off, typing / mid-line on a draft can now produce no menu at all rather than the inert /model row, which I think is the right trade but is your call; and I skipped tests because apps/mobile has no hook-render harness — happy to add one if you would rather have the coverage.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kirill-markin@juliusmarminge
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(mobile): drop the dead /model row and re-park the caret on draft switch - #7523

Open
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers
Open

fix(mobile): drop the dead /model row and re-park the caret on draft switch#7523
kirill-markin wants to merge 1 commit into
pingdotgg:mainfrom
kirill-markin:fix/mobile-new-task-composer-triggers

Conversation

@kirill-markin

@kirill-markinkirill-markin commented Aug 19, 2026

Copy link
Copy Markdown

What this PR is now

#8587 landed the draft composer trigger menus, so the original 775-line version of this PR is obsolete. I have rebased onto main and cut it down to the two behaviors @juliusmarminge named when reopening it:

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation.

One file, +10 / −4, both edits in apps/mobile/src/features/threads/use-composer-command-menu.ts.

1. /model is a dead row on the new-task draft screen

allBuiltIn let /model bypass the capability gate, so it rendered on both surfaces. Selecting it on a draft does nothing useful: onSelect has no model branch, so it inserts the literal text /model , detectComposerTrigger then returns a slash-model trigger this hook builds no items for, and the menu just closes. The draft screen already has a working model control in the same toolbar ("Choose model"), so the row is redundant as well as inert.

The row is now gated on hasThread. ThreadComposer passes hasThread: true and is unchanged.

One consequence worth naming: with plan mode off, NewTaskDraftScreen passes no onUpdateInteractionMode, so the built-in list is now empty. Typing / mid-line on such a draft — where provider commands are position-gated to column 0 — can produce no menu at all, whereas before it always produced the inert /model row. I think no menu beats a row that does nothing, but say the word if you would rather keep something there.

2. The caret is re-parked a render too late

The ownerKey reset ran in a useEffect, which commits after the render that already computed trigger from the previous draft's caret offset. Switching drafts could therefore flash the popover open over a draft the user never typed a trigger into. It now adjusts during render, so that pass never reaches the screen. The ref is assigned before setSelection, so the restarted render cannot loop — the same shape already used in HomeScreen.tsx and ThreadNavigationSidebar.tsx.

No path search was ever dispatched in that window — useComposerPathSearch debounces its target by 200 ms — so this is a rendering fix only.

Deliberately out of scope

  • Making /model work on mobile. Nothing in the repo consumes the slash-model trigger kind that packages/shared/src/composerTrigger.ts produces; only apps/web's ChatComposer handles the model command, by opening its own picker. So the row is inert in ThreadComposer too, not just on drafts. Pre-existing and untouched here — it needs a real mobile model picker wired into the menu. Happy to take that as its own PR.
  • The items.length > 0 guard that hides ComposerCommandPopover's loading and empty states at both call sites. Relaxing it is a deliberate UX change to the most-used surface, so it stays its own PR.
  • New tests.apps/mobile has no hook-render harness — no renderHook, no testing-library — and use-composer-command-menu.test.ts covers only the pure composerSelectionAtEnd. Adding that harness, or extracting the built-in filter into a new module to assert one boolean, is more machinery than this change earns. Say the word if you would rather have it.

Verification

CI on this PR is the gate; I ran no local checks. Statically: hasThread was already in the items memo's dependency list, ThreadComposer.tsx passes hasThread: true so its menu is byte-for-byte unchanged, and the clamp effect on draftMessage.length is a no-op after the re-park. Happy to add an emulator pass with before/after images if you want them for this one.

Model: Claude Opus 5, harness: Claude Code.

@coderabbitai

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1c202d-e188-4e9d-b193-9184b876d2e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added the size:XL 500-999 changed lines (additions + deletions). label Aug 19, 2026
@@ -806,6 +843,11 @@ export function NewTaskDraftScreen(props: {
!isImportingShare &&
!flow.submitting &&
!(flow.workspaceMode === "worktree" && !flow.selectedBranchName);
// A share transfer locks the editor, so its rows must not be tappable.
const showComposerTriggerMenu =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumthreads/NewTaskDraftScreen.tsx:847

When an @ search is loading or returns no matches, the trigger menu disappears without showing ComposerCommandPopover, so users receive no loading or empty-results feedback. The items.length > 0 guard makes the popover's isLoading and empty-state rendering unreachable; render it whenever a trigger is active.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/NewTaskDraftScreen.tsx around line 847:
When an `@` search is loading or returns no matches, the trigger menu disappears without showing `ComposerCommandPopover`, so users receive no loading or empty-results feedback. The `items.length > 0` guard makes the popover's `isLoading` and empty-state rendering unreachable; render it whenever a trigger is active.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this against origin/main: the gate is pre-existing, not introduced here.

ThreadComposer.tsx:725 on main already reads composerTrigger && composerMenuItems.length > 0, so ComposerCommandPopover's empty-state branch — and the emptyText / "Searching files…" copy at ComposerCommandPopover.tsx:89-91,177 — was already unreachable on mobile before this PR. This patch keeps the same gate on both call sites so the two surfaces behave identically, which is what the "thread composer keeps its previous behavior exactly" constraint requires.

Relaxing it is a real improvement, but it is a deliberate UX change to both surfaces (a popover that appears on a bare @ and shows a spinner), so it belongs in its own PR rather than riding along here. Happy to send that follow-up if you want it — the one-line change is the items.length > 0 half of the condition at both call sites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

@macroscopeapp

macroscopeappBot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a small, localized mobile bug fix: it synchronizes caret reset on composer switches and removes a nonfunctional /model row from unsent drafts while preserving existing thread behavior. A separate unresolved Medium finding concerns pre-existing missing loading/empty-state feedback for @ searches and should remain visible as a merge risk.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@kirill-markin

Copy link
Copy Markdown
Author

Status, so this does not read as abandoned — the blocking finding is answered in the review thread: #7523 (comment)

Short version: the items.length > 0 guard that hides the popover's loading and empty state is not introduced here. ThreadComposer.tsx:725 already has it on main, so ComposerCommandPopover's emptyText branch (ComposerCommandPopover.tsx:89-91,177) was unreachable on mobile before this PR. Keeping the identical guard on both call sites is what makes the new surface behave exactly like the thread composer, which is the constraint this PR set for itself. Relaxing it is a deliberate UX change to both surfaces — a bare @ would start opening a popover with a spinner on the most-used surface — so I would rather send it as its own PR than bundle it here. Say the word and I will.

Everything else is green: Cursor Bugbot, Macroscope Effect Service Conventions, and the label checks. The Vercel marketing deploy fails with "Authorization required to deploy", which is the standard restriction on fork PRs rather than anything in this branch.

I also verified this locally instead of leaning on CI: 20/20 unit tests for the extracted logic, clean tsc --noEmit for apps/mobile, and a run on an Android emulator against a disposable backend where $ lists provider skills, @ lists project files, and tapping a row inserts the mention. Before/after images and a breakdown of the size:XL label are in the description.

@juliusmarminge

juliusmarminge commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reopening: #8587 added the draft menus, but this PR also suppresses the nonfunctional /model row on drafts and resets selection synchronously when drafts change. Those details are not in the merged implementation. My initial closure missed that remaining scope.

…switch
pingdotgg#8587 landed the draft composer trigger menus. Two details it left out.
`/model` bypassed the built-in capability gate, so it rendered on the
new-task draft screen, where selecting it only inserts the literal text
`/model `: `onSelect` has no `model` branch, and the `slash-model` trigger
that follows builds no items. It is now gated on `hasThread`, so the thread
composer is unchanged and the draft screen, which already carries its own
model control, loses the dead row.
The `ownerKey` caret reset ran in an effect, one commit too late, so
switching drafts could flash the trigger popover open over a draft the user
never typed a trigger into. It now adjusts during render, with the ref
assigned before `setSelection` so the restarted render cannot loop.
Model: Claude Opus 5, harness: Claude Code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kirill-markin
kirill-markinforce-pushed the fix/mobile-new-task-composer-triggers branch from ce5d27c to 6c68b1cCompareSeptember 2, 2026 07:15
@kirill-markinkirill-markin changed the title fix(mobile): support /, $ and @ in the new-task composerfix(mobile): drop the dead /model row and re-park the caret on draft switchSep 2, 2026
@github-actionsgithub-actionsBot added size:S 10-29 changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Sep 2, 2026
@kirill-markin

Copy link
Copy Markdown
Author

@juliusmarminge — rebased onto main and cut this down to exactly the two behaviors you named in the reopen. Force-pushed ce5d27c6c68b1c; the previous SHA is still in the timeline above if you want to compare.

The 775-line extraction is gone — #8587 owns that logic now, and re-landing my version of it would just be churn. What is left is one file, +10 / −4:

  • /model gated on hasThread, so it disappears from the new-task draft screen and ThreadComposer is untouched;
  • the ownerKey caret reset moved from a useEffect into the render body, so a draft switch can no longer flash the popover open for a frame.

Description rewritten to match. Two things in there worth your eye: with plan mode off, typing / mid-line on a draft can now produce no menu at all rather than the inert /model row, which I think is the right trade but is your call; and I skipped tests because apps/mobile has no hook-render harness — happy to add one if you would rather have the coverage.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S10-29 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@kirill-markin@juliusmarminge