Skip to content

Share chat options read ownership across desktop and phone - #36582

Merged
brennanb2025 merged 9 commits into
mainfrom
brennanb2025/options-read-owner
Oct 31, 2026
Merged

brennanb2025 merged 9 commits into
mainfrom
brennanb2025/options-read-owner

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​371 $\color{#cf222e}{\Huge{\mathbf{−}}}$​22 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​349
Prod 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​387 $\color{#cf222e}{\Huge{\mathbf{−}}}$​353 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​34

Problem

Desktop and the phone app each load a native chat's options (models, effort, permission modes, image support, slash commands) from the host through their own copy of the same logic: send the request, check the answer is still current, publish it, and release the picks it confirmed. The two copies drifted. On the phone, a slow answer that was already out of date could still update image support and the permission evidence, and clear the new-chat bookkeeping, even though it was correctly refused for the model menu. Every fix to this logic had to be made twice.

What changes for users

Nothing visible in normal use. An out-of-date options answer on the phone now changes nothing, the same as on desktop. On an older host without per-chat permission modes, the read-only permission row now appears only from a current answer. Pickers, Send, first paint and the declared default effort display behave as before.

How it works

One shared options reader in src/shared/structured-agent-session-options-read-owner.ts now owns the whole read for both apps. It captures the picks that are already settled, sends the request, checks the answer against the current chat, host connection and context, and only then applies the options, support and phone bookkeeping together and releases the captured picks. Within one context, reads run one at a time with at most one queued behind. A new context (reconnect, agent restart, chat switch) starts its read at once, and the old answer is ignored. The last good answer stays on screen across a reconnect or a controller change. Desktop and phone keep thin adapters for what really differs: desktop's paired-host routing and first-frame seed, the phone's connection generation and legacy permission fallback, and each app's own support display.

No saved data, wire format, RPC or capability changes.

Differences from the common pattern

  • Intended: host and controller identity, desktop pairing checks, the phone's connection generation, and releasing picks only after a read that saw them is accepted all stay explicit. Hosts and clients update independently, a chat can re-pair to a host, and picks can settle while a read is in flight.
  • Intended: a new turn or host option update on the same chat queues one more read behind the one in flight rather than cancelling it. The host already serves a chat's options reads one at a time and there is no cancel request, so cancelling would not answer sooner.
  • Intended: on the phone, a model list that finishes while a read is in flight now discards that answer and reads again, matching desktop. An answer requested before the account's model list loaded can lack the account's models, so showing it first would briefly show the wrong menu.
  • Temporary: after an agent restart, the pickers stay locked until the new read lands, and a failed read isn't retried. Unchanged from main; tracked in STA-13092.
  • Temporary: on the phone, returning to a chat before its read lands shows default pickers and no slash commands, because one view keeps a single last answer. An identical answer also re-renders that chat once. Unchanged from main; tracked in STA-13093.

Not in this PR

The write side (marking a pick pending while it is sent, and settling it) is still written separately on desktop and phone. That is a different mechanism; the clean fix is to derive the pending pick from the selection controller instead of storing it, which changes both apps' views. It is left for its own PR.

Overlap

#31241 is still open and edits the same two desktop hooks. Whichever lands second must route its transport gating, target-scoped support, account-switch evidence and current-read-failure clearing through this shared reader, not restore a second publication path.

Verification

Shared, desktop and phone hook suites run in CI, including stale-answer rejection, same-chat retention, a new context reading at once, release of captured picks, Pi refresh ordering, declared-effort display and Send parity, the existing reconnect and stable-client lifetime suites, and N/10N transport and render bounds. QA classification: tests.

@brennanb2025
brennanb2025 marked this pull request as ready for review October 31, 2026 04:23
@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • Resume reviews
  • Trigger review

No actionable comments were generated in the recent review.

Recent 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: b3f1e311-df90-465e-865f-ca79020a0b82
Commits

Reviewing files that changed from the base of the PR and between 4f9681a and 563e499.

Files selected for processing: 2
  • mobile/src/session/use-mobile-structured-agent-options.lifetime.test.ts
  • mobile/src/session/use-mobile-structured-option-state.ts
Files skipped from review as they are similar to previous changes: 2
  • mobile/src/session/use-mobile-structured-agent-options.lifetime.test.ts
  • mobile/src/session/use-mobile-structured-option-state.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

A shared read owner now manages structured agent option state, support snapshots, catalog updates, and read lifecycles. The mobile and renderer hooks use the owner and validate reads against current context and authority. Tests cover stale and pending responses, coalesced reads, selection behavior, session status updates, and read startup. The renderer options hook also derives its runtime target from the environment ID for environment targets.

Merge Risk: Minimal · up to 563e4

The selected changes have no identified issue that needs resolution before merging. Normal checks can proceed.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check Warning The description clearly explains the problem, user impact, implementation, scope, temporary behavior, overlap, and verification. It does not follow the required template fully: it has no linked issue,… Add the required template sections or equivalent information. Include an actual linked issue under Linked Issue, or explain the maintainer exception. Add ## Visual Proof with N/A and a brief reason if there is no visual or interaction c…
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title "Share chat options read ownership across desktop and phone" matches the main change. The PR adds one shared options reader in src/shared/structured-agent-session-options-read-owner.ts and m…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "Share chat options read ownership across desktop and phone" matches the main change. The PR adds one shared options reader in src/shared/structured-agent-session-options-read-owner.ts and moves desktop and phone option loading onto it. The title is specific and short, and it names both apps the change affects.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 14 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Full details: Description check

Explanation

The description clearly explains the problem, user impact, implementation, scope, temporary behavior, overlap, and verification. It does not follow the required template fully: it has no linked issue, no Visual Proof section with the required N/A statement, and no explicit testing checklist or platform coverage.

Resolution

Add the required template sections or equivalent information. Include an actual linked issue under Linked Issue, or explain the maintainer exception. Add ## Visual Proof with N/A and a brief reason if there is no visual or interaction change. Add explicit testing steps, platform coverage, and checklist status.

Finishing Touches
Generate docstrings
  • 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.

Share

Reads were coalesced on one queue for the whole view, so after a reconnect, provider restart, phone path change or chat switch the current read waited behind the previous context's request, up to the transport timeout. Each attachment now has its own queue: reads still coalesce to one active and one trailing request within a context, while a new context reads immediately and the old context's reply stays inert. Restores the shared option refresh contract and the reopen test to their original expectations.
The shared read queue starts a read one microtask after it is scheduled, where the old desktop runner started it synchronously. The selection lifetime and context usage tests now wait for each read to start before asserting on it, keeping every original assertion, including one active and one trailing refresh. The paired server test compares transport targets by value, since the options view memoizes an equal target per environment.
Desktop and phone rebuilt the conversation, image and command support objects every render, and a host that omits commands produced a fresh empty list each time. Project them once per accepted answer.
Replacing the selection controller, as a desktop re-pair does, cleared the last accepted support, so slash commands, the goal row and the context meter disappeared until the next options answer. On the phone, returning to a chat before the other chat answered showed fallbacks. The support stays tagged with its session and every consumer already filters by session, so it is now kept, as before the shared owner.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1


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: 5c936035-94a9-4eeb-a0c9-e17acf7d4201
Commits

Reviewing files that changed from the base of the PR and between b20a096 and b964edf.

Files selected for processing: 9
  • mobile/src/session/use-mobile-structured-option-state.ts
  • src/renderer/src/components/native-chat/use-structured-agent-session-context-usage.test.tsx
  • src/renderer/src/components/native-chat/use-structured-agent-session-option-state.ts
  • src/renderer/src/components/native-chat/use-structured-agent-session-options.test.tsx
  • src/renderer/src/components/native-chat/use-structured-agent-session-paired-runtime.test.tsx
  • src/renderer/src/components/native-chat/use-structured-session-selection-reads.performance.test.tsx
  • src/renderer/src/lib/structured-agent-session-selection-lifetime.test.tsx
  • src/shared/structured-agent-session-options-read-owner.test.ts
  • src/shared/structured-agent-session-options-read-owner.ts
Files skipped from review as they are similar to previous changes: 1
  • src/renderer/src/components/native-chat/use-structured-session-selection-reads.performance.test.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Each new turn or host status change re-attached the phone's options reader, so a quick run of turns sent one parallel options request per turn. These changes now reread within the current context, as on desktop: one request runs and at most one waits behind it, and only the latest answer publishes. A status change that arrives while a read is in flight now waits for that read, which the transport bounds.
A refused pick is dropped rather than kept as a finished operation, and the path change replaced the options record, so no revalidation read is in flight when the status wake arrives. The wake therefore reads at once, as the original test asserted. The burst test's superseded answer now carries a real conversation command.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review loop status: #36582

The problem. Desktop and the phone app each loaded a native chat's options (models, effort, permission modes, image support, slash commands) through their own copy of the same request, check, publish and release logic. The copies drifted. On the phone, a slow answer that was already out of date could still update image support and the permission evidence, and clear the new-chat bookkeeping, even though the model menu correctly refused it. Each fix to this logic had to be made twice.

What changes for users. Nothing visible in normal use. An out-of-date options answer on the phone now changes nothing, the same as on desktop. On older hosts without per-chat permission modes, the read-only permission row appears only from a current answer. Both apps now share one options reader.

Fixed during review

  • After a reconnect, agent restart, phone network switch or phone chat switch, the new options read waited behind the old request still in flight, for up to 15 s on the phone and remote hosts. Pickers stayed locked and slash commands were missing until it finished. Four phone lifetime cases and one desktop lifetime case caught this in CI. A new context now reads at once, and the old answer is ignored.
  • On the phone, every turn or host status change started its own parallel read, so 10 quick turns sent 11 requests. These now share one running read and one queued behind it, as on desktop.
  • After a desktop re-pair, slash commands, the goal row and the context meter disappeared until the next answer arrived. The last answer is now kept, as on main.
  • Support values were rebuilt on every render, so hosts that omit the commands list produced a new list each time. They are now built once per answer.
  • Three older desktop test assertions checked timing or object identity that the shared reader changes, with no difference a user can see: a read now starts one microtask later, and the paired-host target is compared by value. They were updated with each original check kept. The phone lifetime suite is unchanged from main apart from one new test: a burst of 10 turns sends one read and queues one behind it, and only the newer answer is shown.

Deferred / follow-ups

  • Temporary, unchanged from main: after an agent restart the pickers stay locked until the new read lands, and a failed read is never retried. Tracked in STA-13092.
  • Temporary, unchanged from main: on the phone, returning to a chat before its read lands shows default pickers and no slash commands, because one view keeps a single last answer. An identical answer also re-renders that chat once. Tracked in STA-13093.
  • The write side (marking a pick pending and settling it) is still written separately on desktop and phone. The clean fix derives the pending pick from the selection controller instead of storing it. It is a different mechanism, so it is left for its own PR, as both the builder and the architecture review concluded.
  • The performance test drives the real shared reader at N and 10N, but not the real desktop and phone wrappers around it.
  • No test runs the real options hook through to the desktop goal display. That consumer's code is unchanged.

Verified

  • CI is green on the head, including the phone suites that can't run on the review Mac (the phone lifetime, permission-mode, declared-effort, first-frame and options suites).
  • 16 shared and desktop suites were run by name during review.
  • Let phone users choose the next message’s model during a structured reply #36451 (phone pickers usable during a reply) and Keep declared default effort visible for every chat agent #36455 (declared default effort display) were traced on main and on the branch, and their suites pass.
  • The whole mechanism was compared with the common pattern; every difference is labelled in the PR body.
  • QA type is tests. The PR changes how options are read and published, not process lifecycle, login, layout or interaction. No app run was needed and none was done.
  • Main was merged once (4f9681a), before the final fixes. It shares no files with this PR. Commits since that merge: 563e499 (phone rereads coalesced) and afac04c (test-only CI repair). CI is green on afac04c.

Not verified: no real app run (tests row). Open PR #31241 edits the same two desktop hooks, and whichever lands second integrates; the PR body lists what it must route through the shared reader.

For Brennan's review

@brennanb2025
brennanb2025 merged commit 84aed85 into main Oct 31, 2026
51 checks passed
@brennanb2025
brennanb2025 deleted the brennanb2025/options-read-owner branch October 31, 2026 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant