Repository navigation
fix(native-chat): make Claude /resume reachable from Orca Mobile (STA-4617) - #16684
brennanb2025 wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds agent-picker metadata to Claude and Codex 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed, focused, and covers the change, rationale, linked issue, testing, compatibility considerations, and checklist. The visual proof section explains why screenshots or videos were not provided, although it does not use the template's exact N/A wording.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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. Comment |
…-4617) Claude's slash catalog had no `resume` entry, so mobile's `/` menu never offered it and a typed `/resume` classified as an unknown token. Even once dispatched, Claude answers it with its own TUI picker, which the chat view cannot render — so the command read as doing nothing at all. Add `resume` to the Claude catalog, mark the picker-opening commands (Claude and Codex `resume`) as such, and have an accepted mobile command send bring the terminal view forward through the existing agent-picker seam.
…bile reveal Three fixes from review of the first commit: - Desktop advertised `/resume` in its `/` menu but never revealed the TUI the picker draws in, so the new entry led to the same dead end it removes on mobile. The chat view — the only layer that knows it is covering the terminal — now owns that decision, through a hook that also keeps the local `Ran /x` marker. It covers both desktop dispatch paths and agents with no session-option catalog behind them. - The mobile reveal toggled the view from a render-snapshot guard while the toggle itself reads live state. A send resolving after the user switched to the terminal would flip them back into chat, hiding the picker and pinning the tab there. Added an idempotent `showTabTerminalView` and used it. - An ack-lost send returned before the reveal. Those usually did land, so the picker was open and invisible on exactly the flaky links this bug came from.
ba0f648 to
71a68c5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Revealing the terminal synchronously destroyed the send that triggered it. Switching views unmounts the chat subtree in the same commit, and the send lifecycle's unmount cleanup cancels every tracked handle: Claude's delayed Enter never fired and `onCancelUnsubmitted` wiped `/resume` off the TUI line, while Codex — typed key-by-key — lost it mid-word. That last one was a regression, since Codex `/resume` was already in the catalog. The same commit also discarded the composer's own draft clear, so the command reappeared in the box on the way back. The send handle now rides along with the dispatch report, and the reveal waits on it. The local `Ran /x` marker still lands immediately — it is feedback for the user's own action, not for the writes.
483655d to
7bdd4f9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mobile/src/session/use-mobile-native-chat-controller.test.ts (1)
796-798: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the new test comments to the required context. The test names already describe the behavior.
As per coding guidelines: "
**/*.{ts,tsx,js,jsx}: DO NOT: be verbose, explain the obvious, walk through the code."Also applies to: 829-830, 859-860, 884-885
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e92c278-2ec3-48d2-90a2-3552bde4fe8e
📥 Commits
Reviewing files that changed from the base of the PR and between 81f89a7 and 483655da032a4695c3d23674a48d3bc7cff16fc4.
📒 Files selected for processing (20)
mobile/src/session/MobileNativeChatComposer.test.tsmobile/src/session/mobile-native-chat-send-classification.test.tsmobile/src/session/mobile-native-chat-send-classification.tsmobile/src/session/use-mobile-native-chat-controller.test.tsmobile/src/session/use-mobile-native-chat-controller.tsmobile/src/session/use-mobile-native-chat-message-send.test.tsmobile/src/session/use-mobile-native-chat-message-send.tsmobile/src/session/use-mobile-session-view-mode.test.tsmobile/src/session/use-mobile-session-view-mode.tssrc/renderer/src/components/native-chat/NativeChatComposer.test.tsxsrc/renderer/src/components/native-chat/NativeChatComposer.tsxsrc/renderer/src/components/native-chat/NativeChatView.tsxsrc/renderer/src/components/native-chat/native-chat-composer-types.tssrc/renderer/src/components/native-chat/use-native-chat-picker-command-dispatch.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-picker-command-dispatch.tssrc/renderer/src/components/native-chat/use-native-chat-session-option-command.tssrc/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.tssrc/shared/native-chat-slash-commands.test.tssrc/shared/native-chat-slash-commands.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- mobile/src/session/use-mobile-session-view-mode.test.ts
- src/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.ts
- mobile/src/session/MobileNativeChatComposer.test.ts
- mobile/src/session/mobile-native-chat-send-classification.test.ts
- src/shared/native-chat-slash-commands.test.ts
- src/renderer/src/components/native-chat/NativeChatComposer.test.tsx
- mobile/src/session/use-mobile-native-chat-message-send.ts
- mobile/src/session/use-mobile-session-view-mode.ts
- mobile/src/session/mobile-native-chat-send-classification.ts
- mobile/src/session/use-mobile-native-chat-message-send.test.ts
- src/renderer/src/components/native-chat/NativeChatView.tsx
- mobile/src/session/use-mobile-native-chat-controller.ts
- src/shared/native-chat-slash-commands.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
…ggle `switchNativeChatToTerminal` routed through `toggleNativeChatForLeaf`, whose closure captures `effectiveChatViewMode` and `chatLeafId`. Now that the reveal waits on the send, it fires from a callback captured before that wait — so a user who reached the terminal themselves in the meantime (or a leaf that took its own exit route) was flipped back into chat with a null chat leaf, hiding the picker the reveal existed to show. Same hazard the mobile half was hardened against, and the same remedy: set the view, never flip it. The function no longer reads either value, so there is no stale state left to be wrong about. Also hand the session-option dispatch its own settle signal. Its report fires while the model-switch observer is still being awaited, so a future catalog entry marked `opensAgentPicker` would have unmounted the composer mid-dispatch and aborted the confirmation.
Open review findings (not fixed in this PR)A fourth adversarial review pass on 1. MEDIUM — a send queued behind the revealing one is destroyed (introduced here). The reveal waits for 2. LOW — the reveal also fires for a cancelled send. 3. LOW — telemetry regression. 4. LOW — the skill-collision case is worse on mobile than the trade-off noted above. A user-authored Claude skill named 5. LOW / scope — other Codex commands have the same dead end. Given #1 is a regression this PR introduces, I would not merge as-is — #1 and #2 want fixing first, and #3 is a one-liner. |
Independent review verification — CHANGES REQUESTEDReviewed exact head P2 — revealing
|
QA evidence — exact head
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6a8ed82-6130-4f2a-ad15-320b57daa862
📥 Commits
Reviewing files that changed from the base of the PR and between 483655da032a4695c3d23674a48d3bc7cff16fc4 and 03a704a.
📒 Files selected for processing (7)
src/renderer/src/components/native-chat/NativeChatComposer.test.tsxsrc/renderer/src/components/native-chat/NativeChatComposer.tsxsrc/renderer/src/components/native-chat/native-chat-runtime-send.tssrc/renderer/src/components/native-chat/native-chat-send-settlement.tssrc/renderer/src/components/native-chat/use-native-chat-session-option-command.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-session-option-command.tssrc/renderer/src/components/terminal-pane/TerminalPane.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
Review verification — exact head
|
QA / evidence — exact head
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/native-chat/NativeChatComposer.test.tsx (1)
42-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMirror the cancellation contract in the test mock.
The production helper forwards
cancelled, but this mock drops that argument. The mocked send handle also lackscancelled, so these tests cannot detect a cancellation-propagation regression.Proposed mock update
- sendHandle: { cancel: vi.fn(), settleAfterMs: 500, settled: Promise.resolve() }, + sendHandle: { + cancel: vi.fn(), + cancelled: vi.fn(() => false), + settleAfterMs: 500, + settled: Promise.resolve() + }, notifyNativeChatSlashCommand: ( - onSlashCommand: ((command: string, settled?: Promise<void>) => void) | undefined, + onSlashCommand: + | ((command: string, settled?: Promise<void>, cancelled?: () => boolean) => void) + | undefined, command: string, - settled?: Promise<void> - ) => onSlashCommand?.(command, settled) + settled?: Promise<void>, + cancelled?: () => boolean + ) => onSlashCommand?.(command, settled, cancelled)Also applies to: 88-93
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: da50646a-d1e2-4b7a-b660-b0865d535d5f
📒 Files selected for processing (10)
src/renderer/src/components/native-chat/NativeChatComposer.test.tsxsrc/renderer/src/components/native-chat/NativeChatComposer.tsxsrc/renderer/src/components/native-chat/native-chat-composer-types.tssrc/renderer/src/components/native-chat/native-chat-pty-send-queue.tssrc/renderer/src/components/native-chat/native-chat-runtime-send.tssrc/renderer/src/components/native-chat/native-chat-send-settlement.tssrc/renderer/src/components/native-chat/use-native-chat-picker-command-dispatch.tssrc/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.tssrc/renderer/src/components/terminal-pane/TerminalPane.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
|
Exact-head review verification — Fixed the remaining in-scope defects: CI classification: the prior |
|
Exact-head QA — Electron was exercised through Playwright CDP only against the repaired working tree (the app build label reported its checked-out base
Mobile iOS QA used
|
Review verification — exact head
|
QA / evidence — exact head
|
# Conflicts: # src/renderer/src/components/native-chat/NativeChatComposer.test.tsx
ELI5
On the phone, Claude's
/resumewas missing from the/menu, so there was no way to pick an earlier conversation. And even when you typed it out, Claude answers/resumeby drawing its own session picker in the terminal — which the chat view is covering — so the command looked like it did nothing. Now/resumeis in the menu, and using it reveals the terminal where the picker actually is. Desktop had the same dead end, so it gets the same reveal.User-facing before / after
/command menu in a Claude chat and look for/resume/resumeis listed alongside the other Claude commands/resumeyourself and send it/resumefrom the native chat viewWhen it bites
Anyone trying to return to an earlier Claude conversation — on Orca Mobile it was impossible, and on
the desktop it looked like the command did nothing.
What Changed
Shared catalog
src/shared/native-chat-slash-commands.ts: addresumeto the Claude catalog (claude+openclaude), and mark the commands whose answer is the agent's own interactive picker (resumefor Claude and Codex) withopensAgentPicker. NewnativeChatSlashCommandOpensAgentPicker(draft, commands)matches the leading token only, exactly likeclassifyNativeChatSend.Mobile
mobile-native-chat-send-classification.ts:mobileNativeChatSendOpensAgentPicker(agent, text), resolving the agent's verified catalog.use-mobile-native-chat-message-send.ts: optionalonAgentPicker, fired for a control send whose command opens a picker — onacceptedand onunknown. An ack-lost send usually did land (the hook's own comment says so, from a cellular-relay report), so the picker is open and invisible on exactly the links this bug came from. Revealing is self-correcting: the user sees the picker or their untouched prompt. Session-option dispatches (recordControlSend: false) keep their own decision.use-mobile-session-view-mode.ts: new idempotentshowTabTerminalView, factored out oftoggleTabChatViewso both derive "currently chat" from the same live ref. An absolute set that already holds returns without re-persisting.use-mobile-native-chat-controller.ts: the existinghandleAgentPicker(the seam/modelalready uses) now callsshowTabTerminalViewand is passed to the send hook.Desktop
use-native-chat-slash-command-dispatched.ts(new): the chat view's handler for a verified slash command that just went out — the localRan /xmarker, plus the reveal.NativeChatViewuses it in place of its inlineonSlashCommand.switchNativeChatToTerminalis now an absolute set (setTabViewMode(tabId, 'terminal')), not a pass throughtoggleNativeChatForLeaf. Once the reveal waits on the send, it fires from a callback captured before that wait, and the toggle's closure holds the oldeffectiveChatViewMode/chatLeafId— so a user who reached the terminal themselves meanwhile (or a leaf that took its own exit route) was flipped back into chat with a null chat leaf, hiding the picker. Same hazard the mobile half was hardened against, same remedy. It no longer reads either value, so there is no stale state left to be wrong about — the hazard is gone by construction rather than guarded, which is why this one has no test of its own while mobile'sshowTabTerminalViewdoes.opensAgentPickerwould have unmounted the composer mid-dispatch and aborted the confirmation.onSlashCommandnow carries the send handle'ssettledpromise, and the reveal waits on it. Revealing synchronously destroys the send it is revealing: the view switch unmounts the chat subtree in the same commit, anduseNativeChatSendLifecycle's unmount cleanup cancels every tracked handle — Claude's delayed Enter never fires andonCancelUnsubmittedCtrl+U's/resumeoff the TUI line, while Codex (typed key-by-key) loses it mid-word. That second one would have been a regression onmain, where Codex/resumeis already in the catalog. The same commit also discarded the composer's ownsetDraft(''), so the command reappeared in the box. The marker still lands immediately — it is feedback for the user's action, not for the writes.Why
The Claude catalog is the single source both the
/autocomplete menu and send classification read, so an omittedresumeis literally "resume unavailable" on a phone: the menu never lists it and a typed/resumeclassifies asunknown-token.Discoverability alone is not enough, and would have made things worse on desktop — advertising a command that leads to an invisible picker is the dead end this PR exists to remove. The reveal belongs to the view, because only the view knows the chat surface is covering the TUI the picker draws in. Mobile already had that mechanism (
onAgentPicker→ terminal view) built for Codex/model; desktop had the same seam (onSwitchToTerminal) wired only into session options. Both now route picker-opening slash commands through it rather than growing a second mechanism. Putting the desktop half ononSlashCommandcovers both of its dispatch paths (typed send and/menu) and works for agents likeopenclaudethat have slash commands but no session-option catalog.showTabTerminalViewexists because the reveal fires after an async send: a toggle guarded by a render snapshot could flip a user who had already switched to the terminal back into chat — hiding the picker and persisting that override.Linked Issue
STA-4617 (GitHub #15067).
Scope note: this PR covers the
/resumehalf of that ticket only. The transcript-sync half — a Claude session on an SSH host showing an empty chat on mobile — is fixed by #15672 (STA-3846), which reroutesnativeChat.readSession/subscribeto read on the execution host; that change is not duplicated here.Visual Proof
No new chrome.
/resumerenders as an ordinary row in the existing/suggestion list, and the reveal is the same transition the long-press "switch to terminal view" action already performs. Proven at render level instead:MobileNativeChatComposer.test.tsmounts the real composer withagent: 'claude'and/res, and asserts the rendered menu contains/resumeand its description row.Mobile emulator QA was not run: exercising this end to end needs a paired host with a live Claude session (the native-chat pairing rig), which is out of proportion to the change. Every seam it touches is covered by failing-first tests below.
Testing
Non-vacuity, measured on this HEAD. All five fix points were neutered at once —
resumeremoved from the Claude catalog, the mobile reveal short-circuited, the mobile reveal reverted to a toggle,showTabTerminalView's already-holds guard deleted, and the desktop reveal short-circuited. 17 tests failed:src/session/: 9 failed / 1,220 passedsrc/shared/native-chat-slash-commands+src/renderer/.../native-chat/: 8 failed / 784 passedRestoring all five makes every one pass. The temporary mutation is not in this branch.
The settle deferral has its own arm: reverting the reveal to a synchronous call and dropping the handle hand-off from both desktop dispatch paths fails 4 tests (
holds the reveal until the send settles, the two composer hand-off tests, and the picker-dispatch hand-off test).Suites run on this HEAD:
src/shared/+src/renderer/src/components/native-chat/+src/renderer/src/components/terminal-pane/: 1,033 files / 10,637 passed (4 files, 66 tests skipped)cd mobile && npx vitest run, no path filter): 467 files / 3,768 passed / 3 skippedpnpm run typecheck(node + cli + web) — cleancd mobile && npx tsc --noEmit— cleanoxlinton the changed trees — clean;oxfmt --writeappliednode config/scripts/check-changed-code-quality.mjs— 0 findings (native, type-aware, React Doctor) across 15 changed filespnpm run check:max-lines-ratchet— OK, no new bypasses (nomax-linessuppression added; the desktop reveal was extracted to its own module rather than growingNativeChatComposer.tsxpast its cap)git diff --check— clean;mobile/pnpm-lock.yamlunchanged (patchedDependenciesstill present)Rebase note
Rebased onto
mainafter the first CI run: the branch had forked 273 commits earlier, and CI testsrefs/pull/N/merge. The merged tree had since moved the mobile chat send's pre-body input clear into its own real transport write, which the new controller-testdescribedid not stub — its sends came backrejectedand the reveal assertions failed in CI while passing locally on the stale base. Fixed by giving thatdescribethe same accepted-terminal.sendclient stub the file's other blocks already use. The conflict inuse-mobile-native-chat-message-send.tswas resolved by derivingrevealsAgentPickernext to theclassificationmain now computes earlier.static analysisalso fails on the rebased head, atVerify localization catalog— a pre-existingmainbreak, not this branch. Evidence: the branch touches no i18n file (git diff --name-only <merge-base>..HEAD -- src/renderer/src/i18n/is empty), the failure reproduces locally on this branch's merged tree and names onlycomponents.skills.*keys from #16682, and two unrelated PRs from another author fail the identical step (nwparker/minidump-module-cap-minimal,nwparker/image-viewer-definite-box-minimal).Job-level CI outcome on head
7bdd4f91d3(the tree before the last commit): Mobile Checks green, and PR Checks 44 success / 1 skipped / 2 failures — the two beingstatic analysisabove andverify, which is only its aggregate. All 32 test shards (node 24 and 26), typecheck, Git compatibility, xterm patch sync, shell contracts, package, package (windows), and e2e passed.Two failures on that first (stale-base) run are not from this branch and reproduce on
main:static analysis→verify:localization-catalogonja.json(from #16682's skills i18n; this branch touches no i18n file), andpackage (windows)→ anEPERMtemp-dirrminconfig/scripts/rebuild-native-deps.test.mjs.AI Disclosure
Internal contributor.
Review
Remote wire compatibility
No wire change. The catalog and the picker predicate are client-side data and pure functions; no RPC params, stream frames, or host-published payloads are touched. A mixed-version pair is unaffected — an older client keeps the old menu, and the send itself is the same
terminal.sendit always was.Execution host / SSH / WSL / Windows
/resumeis delivered by the same send path as any other chat send, so the execution host still owns the write and the agent's picker draws on the host that runs the agent. Nothing here reads a transcript, resolves a path, or infers process state, so there is no new host-ownership or path-separator surface. Theunknown-outcome reveal is deliberately aligned with the existing rule that loss of contact is not evidence the write failed.Folder workspaces
The change keys off the tab's agent and its chat/terminal view mode, never off a worktree or git identity, so folder workspaces behave identically.
Known trade-off
Adding
resumeto the Claude catalog means a user-authored Claude skill namedresumeis now noise-filtered like any built-in instead of being re-surfaced as a user bubble bysurfaceSkillInvocationUserTurns. That is inherent to catalog membership and already true of the entries beside it (init,review,compactare at least as plausible as skill names); it is not a new mechanism. Not addressed here — separating built-in from skill envelopes is its own change.Agent / provider contracts
The picker flag is per-command catalog data, so Grok (no verified catalog) and any agent without a marked command are unaffected; the predicate returns false for an empty catalog, a null agent, an unknown token, and a leading-whitespace draft.
openclaudeis covered on both platforms even though it has no session-option catalog. Codex/modelkeeps its existing session-option path — the two reveals cannot double-fire today, since no command is both a marked slash command and a session-option picker command, and the mobile reveal is now idempotent regardless.Agent skill upstream boundary
Checklist
N/Awith reason