Repository navigation
Share chat option writes across desktop and phone - #36624
Conversation
Held launch refusals toast again while the transport is still closed, desktop notices follow only the chat's presentation scope so a fence or status change on the same chat still reports, and the phone banner no longer depends on the read record.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 18.18% 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 | |
The description is thorough and largely follows the repository template. It has a clear Problem and What Changes section, a How It Works section that explains the mechanism, and a Differences section … | Add an ELI5 section with a plain-language summary. Replace the blank "Fixes #" with a linked issue. Outside contributors need an actual issue, and maintainers should attach a pre-existing one. Add a Visual Proof section. Because the change … |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: sharing chat option writes between desktop and phone. 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 18.18% 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 11 functions across 15 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Description check
-
Explanation
The description is thorough and largely follows the repository template. It has a clear Problem and What Changes section, a How It Works section that explains the mechanism, and a Differences section that explains deviations from the common pattern. It also lists the tests that were added. Several template sections are missing or incomplete: the ELI5 section is absent, the Linked Issue section has no issue reference ("Fixes #" is blank), the Visual Proof section is absent (it is required for UI or behavior changes, and the description says behavior is unchanged but gives no N/A statement), the Testing checklist boxes are not checked and the description does not name the platforms tested, the AI Disclosure section is absent, the Review and Agent skill upstream boundary sections are absent, and the Notes and Checklist sections are absent. The description also does not state whether pnpm lint, typecheck, test, and build pass.
Resolution
Add an ELI5 section with a plain-language summary. Replace the blank "Fixes #" with a linked issue. Outside contributors need an actual issue, and maintainers should attach a pre-existing one. Add a Visual Proof section. Because the change is described as behavior-preserving but touches the desktop toast and phone banner paths, either attach before and after evidence or write
N/Awith a short reason. Mark the Testing checkboxes that apply and list the platforms actually tested (macOS, Linux, Windows, SSH). Add the AI Disclosure section, or leave it blank only if the author is a StablyAI team member. Complete the Agent skill upstream boundary checkbox. Add the Checklist items that apply and state the result of pnpm lint, typecheck, test, and build, or note that CI covers them. Remove the template comments once each section is filled in.
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.
|
Review loop status: done. Two review rounds, all findings fixed, CI green. The problem. When you changed a chat's model, effort, permissions or Fast setting, desktop and phone each ran their own copy of the write. Each copy also kept its own "a pick is pending" marker beside the shared record of in-flight picks, so the two could drift apart; on the phone the marker cleared when the latest pick finished even if an earlier pick was still in flight. This PR moves the write into one shared writer for both apps and works the phone's lock out from the shared in-flight record instead of storing it. What users get. Nothing should look or behave differently. Picks are accepted the same way, pickers lock and unlock at the same moments, and the same failure toast (desktop) or banner (phone) appears. The duplicated logic is gone, so future fixes land once for both apps. Fixed during review (33cf289):
Deferred (follow-ups, not user-visible):
What was verified.
Main was merged once before final CI (7d88dcd, no conflicts, no overlap with this PR's files); final CI on that head is green. For Brennan's review:
|
Problem. When you change a chat's model, effort, permissions or Fast setting, desktop and phone each ran their own copy of the write: checking the pick, sending it to the host, waiting for the answer, showing a failure, and refreshing. Each copy also kept its own "a pick is pending" marker beside the shared record of in-flight picks, so the two could disagree. On the phone, that separate marker was cleared when the latest pick finished even if an earlier pick was still in flight. #36582 already shared the reading side of these options; this shares the writing side, so both apps run one implementation.
What changes for users. Nothing should look or behave differently. The same picks are accepted, the same pickers lock and unlock at the same moments, and the same failure toast (desktop) or banner (phone) appears. This removes the duplicated logic behind those behaviors, so future fixes land once for both apps.
How it works. One shared writer, used by both apps, checks a pick, records it as in flight, sends it, and handles the host's answer. Nothing stores a separate "pending" marker any more. The phone works out whether its options sheet is locked from the latest pick it sent, whether that pick was made while the agent was starting or was a permission change, and whether that pick is still in flight. That keeps today's phone behavior: picks made during startup and permission picks never lock the sheet, and resetting or reconnecting clears the lock. Desktop keeps its per-row busy state, sends picks made while a new chat is still launching straight to the launch path, keeps its original connection and its extra setup steps for older paired servers, and still refreshes after every answer. Failure notices keep the same scope as before: desktop shows a toast while you are still on that chat (and always for a refused pick made during launch), the phone shows its banner for a refused pick, and an unconfirmed delivery stays silent on the phone. Model and effort values are still sent exactly as picked, whether or not they appear in the model list and whatever name the agent reports back. No saved data, wire fields, capabilities or RPCs change.
Differences from the common pattern:
Tests. The shared writer tests run picks for Claude and Codex chats against an empty model list. They cover a pick sent as typed, an older host that leaves the options out of its reply, refused and unconfirmed replies, startup ending or beginning while a pick is in flight, two quick picks finishing in either order, reset, replacement, detach, and failure-notice scope. Desktop hook tests, using the production presentation check, cover a refusal shown during launch, a refusal shown on the same chat after its runtime changes, and a refresh after a superseded answer. A phone hook test covers the latest pick finishing while an earlier one is still in flight. Phone suites run in CI.