You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(mobile): run terminal chat model and effort pickers on desktop's option surface - #33976
Stacked on #33970: review the commits after 04224b7.
ELI5
The model and effort picker in a phone terminal chat now runs on desktop's code. Grok chats get pickers too, picks are saved as the next launch's defaults, and the labels match desktop ("Med", "XHigh").
What Changed
Option labels (native-chat [P2] Duplicated "checking for updates" #35). Before: the phone showed the catalog's raw labels and fell back to "Options". Now: labels come from src/shared/native-chat-session-option-labels.ts, an English table plus a say callback desktop wraps with translate. The pill shows Med / XHigh / Min like desktop, and the effort sheet title is "Effort".
Terminal option pickers (Fix first worktree card top border cutoff #15). Before: the phone built pickers only for Claude, Codex and OMP and re-implemented option apply around a tracked previousModelId. Now: the desktop PTY option surface moved to src/shared, along with its appliers, option cache and dispatch types. That covers createNativeChatPtySessionOptions, createSessionOptionAppliers and resolveEffectiveNativeChatModelId. The phone builds one surface per tab from them, so Grok and any agent with a catalog get pickers. The renderer cache path stays as a re-export so its 16 importers don't change.
Launch defaults (Fix TypeScript error and linting issues in TabBar #34). Before: terminal-chat picks on the phone were never saved. Now: they're saved as the host's launch defaults through the existing nativeChatSessionOptionsWrite RPC, as desktop does with persistSelection / adoptModelAsLaunchDefault.
The OMP live model-switch gate comes from the shared surface; the mobile copy is deleted.
The OMP discovery mapping moved to src/shared/native-chat-discovered-catalog-models.ts. That keeps isDefault and contextWindowTokens, and authoritative discovery retires a persisted model it no longer lists.
The structured option stamp moved into structured-agent-session-option-state.ts.
Why
Desktop logic wins in the mobile/desktop parity audit. The phone's own apply path missed Grok, couldn't set effort before a model report, and never saved picks. Running the desktop surface removes that copy.
Linked Issue
Part of the mobile/desktop parity audit
Visual Proof
No device captures for this fleet. The change adopts desktop's existing behavior: native-chat-pty-session-options.ts, native-chat-session-option-apply.ts and native-chat-session-option-labels.ts.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Desktop option tests moved with the shared modules. Mobile tests that pinned the old apply path are rewritten. New mobile tests cover per-tab surfaces, agent changes and retired surfaces, and the worker confirmed they fail without their fixes. Focused root option tests and mobile src/session pass.
AI Disclosure
Review
Codex review, 3 rounds, all findings fixed:
Grok lost its effort picker after discovery.
A pick that landed after leaving and returning to a tab was lost.
Picker completion could switch the wrong tab.
An unconfirmed Claude switch could be saved as a launch default.
A late pick could be saved to another host's defaults.
An agent change didn't reset the tab.
A retired surface's late write could overwrite the new agent's cache.
An OMP capability flip could bring back an older selection.
Agent skill upstream boundary
Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.
Notes
Mobile behavior removed (desktop wins):
A terminal pick whose write or reply never came back is no longer committed.
The "Options" / raw-label placeholder in the pill is gone when no values are known.
A hook-reported OMP model that authoritative discovery doesn't list is dropped, as on desktop.
The per-tab option record cap went from 32 to the shared 128.
Phone-only gaps kept:
The phone keeps applying Claude hook-model reports. Desktop reads the terminal screen instead, which the phone can't do.
A Claude model switch is not saved as a launch default from the phone, because desktop confirms it on the terminal screen. Effort picks are saved.
The phone fetches model lists only for OMP and Grok. Doing it for Claude and Codex would mean a CLI probe on the host on every visit.
Desktop: no visible change. The effort label's translation default is now the literal 'Effort', which every locale already has.
Compatibility: no new wire fields. Discovery and settings-write failures are swallowed, so old hosts behave as before. The discovery reply schema accepts replies with or without catalogOrigin.
Checklist
This PR is small and focused
I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
Before/after screenshots or videos attached for UI changes, or N/A with reason
Self-reviewed for correctness, security, and performance
Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)
The pull request moves pending-send reconciliation and session-option logic into shared native-chat modules and updates mobile and renderer integrations. It adds shared helpers for message copying, tool presentation, output sizing, availability, and other UI calculations. Mobile pending sends now use transcript boundaries and timestamps. The change also updates model discovery and settings persistence, removes superseded mobile and renderer modules and tests, and updates related test fixtures and runtime strings.
Priority: Normal
Merge Risk: Moderate · up to aaa32
Merge is blocked by the static-analysis check. The previously identified agent-selection, photo-echo, and notice-copy concerns also remain open; resolve or explicitly accept them before merging.
Pre-merge checks: 3 · 2
Failed checks: 2 warnings
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 26.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check
The description covers the implementation, rationale, testing scope, review findings, compatibility notes, and checklist. It does not provide the required linked issue in the Fixes # section; “Part …
Add the actual issue reference or link under Fixes #. If no visual or interaction change requires captures, write N/A and explain why. State whether CI covers pnpm lint, pnpm typecheck, pnpm test, and pnpm build, or provide thei…
Passed checks: 3 passed
Check name
Status
Explanation
Title check
The title clearly and concisely describes moving the mobile terminal chat model and effort pickers onto the desktop option surface.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 26.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 80 functions across 66 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Description check
Explanation
The description covers the implementation, rationale, testing scope, review findings, compatibility notes, and checklist. It does not provide the required linked issue in the Fixes # section; “Part of the mobile/desktop parity audit” is not an issue reference or link. The visual proof section also does not use the required N/A wording, and the unchecked full validation commands do not state whether CI will cover them.
Resolution
Add the actual issue reference or link under Fixes #. If no visual or interaction change requires captures, write N/A and explain why. State whether CI covers pnpm lint, pnpm typecheck, pnpm test, and pnpm build, or provide their results.
Finishing Touches: 💡 2
📝 Generate docstrings: 💡
Commit to this branch
Create a new PR
🛠️ Fix failing CI checks: 💡
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
⚠️Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
Consider caching model discovery per host and agent.
Each run of this effect sends a git.discoverCommitMessageModels RPC. That RPC runs a CLI probe on the host. The effect re-runs when client, enabled, scope, or worktreeId change. These changes happen on reconnect, on a tab switch that toggles enabled, and on a switch between structured and PTY chat. Each re-run starts a new probe. The result state does not outlive the hook instance.
The comment in use-mobile-native-chat-session-option-controller.ts says mobile avoids per-visit CLI probes. This effect does not guarantee that.
Keep a module-level map keyed by [hostId, agent]. Store the in-flight promise. Evict the entry if the promise rejects or resolves to null, so a failed probe can retry. This matches the once-per-host behavior of ensureNativeChatModelEnrichment on desktop.
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 0fa84292-b754-469d-9ad5-297b19ee16ca
Commits
Reviewing files that changed from the base of the PR and between 887b781 and 054f998.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Functional Correctness · Quick win
Apply the shared send-time fallback when the image-preview boundary is null or not resolved.
The comment says that this filter uses the same boundary rule as the shared pending code. It does not do that in two cases:
Null boundary:foldedBoundaryIndex returns -1 when afterMessageId is null, so every candidate row is allowed. The shared messagesAfterPendingBoundary keeps every row only when afterEmptyTranscriptSessionId is set. Otherwise it filters rows by send time.
Unresolved boundary:foldedBoundaryIndex returns undefined when the boundary row is paged out. Line 179 then allows every candidate row. The shared rule falls back to send time in this case too.
How it can happen:
The user sends a photo while the transcript is still loading. captureSendOrigin records afterMessageId: null with no empty-transcript claim.
Older history then arrives, and the transcript becomes settled.
For an image-only send, any older user row with enough image refs matches line 174. nativeChatPendingOccurrence(entry, 0) then selects the first such row.
What the user sees: the phone preview attaches to an old history message. retireLandedMobileNativeChatPending then removes the echo, because a mobile item's content key is 'empty' when it has only images. A captioned photo is affected the same way when older history has the same caption.
Fix:
Add afterMessageTimestamp and afterEmptyTranscriptSessionId to PendingImagePreviewEcho.
Remove the -1 and undefined shortcuts.
If the boundary row is not found, filter candidates with the same timestamp rule that messageIsAfterPendingTimestamp uses.
isAfterPendingSendTime should reuse the shared timestamp predicate. Export that predicate from native-chat-pending-sends.ts so the two rules cannot drift apart.
The reason will be displayed to describe this comment to others. Learn more.
Minor · Functional Correctness · Quick win
Preserve displayed host-status text when copying a notice.
If a system message has presentation: 'history-item-too-large' and fallback wire text, MobileNativeChatNoticeRow displays AGENT_SESSION_HOST_STATUS_COPY but this helper copies the fallback text. The previous mobile converter mapped host-status presentations before copying. Restore that mapping in the shared copy path so the clipboard and Select text actions match the notice on screen. (raw.githubusercontent.com)
As per coding guidelines, “Clients and servers update independently; preserve compatibility across mixed versions.”
Files skipped from review as they are similar to previous changes: 1
src/renderer/src/i18n/en-runtime-required.json
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Major · Maintainability & Code Quality · Quick win
Merge the duplicate imports from ./native-chat/input/native-chat-session-option-snapshot.
The file imports this module twice. Line 20 imports buildNativeChatSessionOptionSnapshot. Lines 21-25 import resolveEffectiveNativeChatModelId, withTrackedNativeChatModel, and NativeChatSessionOptionMode. The static analysis and typecheck job fails on this with "Modules should not be imported multiple times in the same file". The job runs oxlint with --deny-warnings, so this blocks CI. Combine both imports into one statement.
🧹 Proposed fix
--- "a/src/shared/native-chat-pty-session-options.ts"+++ "b/src/shared/native-chat-pty-session-options.ts"@@ -17,12 +17,12 @@
writeNativeChatSessionOptionCache
} from './native-chat-session-option-cache'
import { createSessionOptionAppliers } from './native-chat-session-option-apply'
-import { buildNativeChatSessionOptionSnapshot } from './native-chat/input/native-chat-session-option-snapshot'
import {
+ buildNativeChatSessionOptionSnapshot,
resolveEffectiveNativeChatModelId,
withTrackedNativeChatModel,
type NativeChatSessionOptionMode
} from './native-chat/input/native-chat-session-option-snapshot'
import type { NativeChatSessionOptionDispatchCommand } from './native-chat-session-option-dispatch'
type PersistSelection = (args: {
Committable suggestion
IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
GitHub Actions: PR 33976 | source 4414fee | workflow 4414fee | unit full / 16_static analysis and typecheck.txt
[warning] 20-20: oxlint: Modules should not be imported multiple times in the same file.
GitHub Actions: PR 33976 | source 4414fee | workflow 4414fee | unit full / static analysis and typecheck
[error] 20-20: Oxlint reported: Modules should not be imported multiple times in the same file. The command node node_modules/oxlint/bin/oxlint --config config/oxlint-code-quality-native-plugins.json src config tests mobile --deny-warnings failed with exit code 1 because warnings are denied.
…hed ones
Uses desktop's rule: rows before the send no longer retire an echo, a send that
never lands no longer lets later glued sends retire, and an unmatched echo sits
below later turns.
Mobile's options pill now shortens effort levels (Med, XHigh, Min) and names
choices and options the way desktop does, from one English table that desktop
translates.
…p surface
The terminal-chat option surface, its appliers, the option cache and the
dispatch types move to src/shared, and the phone builds its pickers from them.
Grok and every other agent with a catalog now get model and effort pickers on
the phone, picks are saved as the host's launch defaults, the OMP live switch
gate and discovery mapping come from one copy, and authoritative discovery
retires persisted models it no longer lists. The option stamp moves to shared.
The phone keeps one option surface per chat, so a pick that lands after leaving
and returning shows there, and a picker command only switches the chat it came
from. Discovered models merge with the seed catalog as on desktop, so Grok keeps
its effort picker. A Claude model switch the phone cannot confirm is never
saved as a launch default.
A tab that runs another agent drops its kept option surfaces, report markers
and cached record, so a fresh session shows its own model. A pick that lands
after the phone moved to another chat is not saved as a launch default.
A write that lands after its tab started another agent no longer publishes the
old agent's record, and a changed OMP switch capability rebuilds the chat's
surface from the cache instead of reviving an older one.
Desktop translation pieces use literal English defaults so the localization gate can read them, the selected-choice lookup is shared once, and the phone keeps one option surface per tab with the agent change detected from the cached record.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Stacked on #33970: review the commits after 04224b7.
ELI5
The model and effort picker in a phone terminal chat now runs on desktop's code. Grok chats get pickers too, picks are saved as the next launch's defaults, and the labels match desktop ("Med", "XHigh").
What Changed
src/shared/native-chat-session-option-labels.ts, an English table plus asaycallback desktop wraps withtranslate. The pill shows Med / XHigh / Min like desktop, and the effort sheet title is "Effort".previousModelId. Now: the desktop PTY option surface moved tosrc/shared, along with its appliers, option cache and dispatch types. That coverscreateNativeChatPtySessionOptions,createSessionOptionAppliersandresolveEffectiveNativeChatModelId. The phone builds one surface per tab from them, so Grok and any agent with a catalog get pickers. The renderer cache path stays as a re-export so its 16 importers don't change.nativeChatSessionOptionsWriteRPC, as desktop does withpersistSelection/adoptModelAsLaunchDefault.src/shared/native-chat-discovered-catalog-models.ts. That keepsisDefaultandcontextWindowTokens, and authoritative discovery retires a persisted model it no longer lists.structured-agent-session-option-state.ts.Why
Desktop logic wins in the mobile/desktop parity audit. The phone's own apply path missed Grok, couldn't set effort before a model report, and never saved picks. Running the desktop surface removes that copy.
Linked Issue
Part of the mobile/desktop parity audit
Visual Proof
No device captures for this fleet. The change adopts desktop's existing behavior:
native-chat-pty-session-options.ts,native-chat-session-option-apply.tsandnative-chat-session-option-labels.ts.Testing
Desktop option tests moved with the shared modules. Mobile tests that pinned the old apply path are rewritten. New mobile tests cover per-tab surfaces, agent changes and retired surfaces, and the worker confirmed they fail without their fixes. Focused root option tests and mobile
src/sessionpass.AI Disclosure
Review
Codex review, 3 rounds, all findings fixed:
Agent skill upstream boundary
Notes
Mobile behavior removed (desktop wins):
Phone-only gaps kept:
Desktop: no visible change. The effort label's translation default is now the literal 'Effort', which every locale already has.
Compatibility: no new wire fields. Discovery and settings-write failures are swallowed, so old hosts behave as before. The discovery reply schema accepts replies with or without
catalogOrigin.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)