Repository navigation
Record chat tab opens and closes in main (dark, layout-owner build only) - #30019
Merged
Merged
Conversation
…yout-owner build only) A chat tab the window opens, closes, or adds and removes from a host listing of this machine's chats now reaches main as openAgentSessionTab, closeTabs or setTabProps under the window's ids, so the window's save no longer teaches main a chat tab. openAgentSessionTab takes the window's place, creation time and new split group, and the same open again changes nothing. Switch-off builds are unchanged.
…nd synced chat opens in tab order (dark) Only a window's openAgentSessionTab (main marks it drawn on layout.apply) takes the new rules: a repeat changes nothing, a taken id is refused, and a workspace with no group takes the window's. A windowless host's own saves mint a fresh group and keep a chat whose id another tab holds, as before. A host listing's new chats are recorded in tab order, so each open's anchor already reached main.
…ller names only a group it holds (dark) Drops the per-sender drawn flag. The windowless save passes its remembered active group only when the workspace still holds it, so it mints a fresh group as before. Refusing a taken chat tab id is gated on the build switch: shipped builds still save the duplicate id.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 15 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main change: recording chat-tab opens and closes in the main process for the layout-owner build. Description check The description is complete and aligned with the template. It explains the user impact, implementation, rationale, testing, scope, compatibility considerations, and checklist status. The linked issue … 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: Description check
-
Explanation
The description is complete and aligned with the template. It explains the user impact, implementation, rationale, testing, scope, compatibility considerations, and checklist status. The linked issue is marked N/A for an internal refactor step, which is a minor deviation from the template.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 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.
Comment @coderabbitai help to get the list of available commands.
9 tasks done
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ELI5
In the test-only build where the app's background process owns the tab layout, chat tabs now tell the background process when they open or close. Before, it only learned about them later, from the window's periodic save. Shipped builds don't change.
What Changed
Applies only with the layout-owner build switch on, which is off in every shipped build.
Why
Main is taking over the tab layout from the window. Until every change reaches main as a command, main learns some of them only from the window's saves, and the save-divergence detector logs each of those saves. Chat tabs were one of the remaining sources.
Why one rule can't serve both callers today. Main already uses this command for a host with no window open. In shipped builds, that path saves a chat tab even when another tab already holds its id, which leaves a duplicate id. A window-sent open must instead refuse a taken id. So refusing a taken id is gated on the build switch. The gate goes away when the switch ships on. The duplicate id is a minor shipped bug that turning the switch on removes, so there is no separate fix.
The windowless caller now passes its remembered active group only when the workspace still holds that group. Otherwise it gets a freshly made group, exactly as it does today.
Linked Issue
N/A (internal refactor step E2-5).
Visual Proof
N/A. Nothing changes with the switch off, and with it on, the tab bar looks the same; only who records the change differs.
Testing
Unit tests, all of which failed before the fix:
Switch-off unminified bundle diff: renderer call sites are gone. The recorder functions remain as dead exports because two callers sit in lazy-loaded files. The main-bundle changes give the same results as before on every switch-off path.
Save-divergence detector, in a switch-on app (layout oracle, 22 specs, plus a chat evidence spec):
The chat evidence spec, not committed, uses a fake Claude CLI. It creates two chats on this machine's host (as the CLI or a phone would), closes one from the window and one on the host, then opens a chat from the New tab menu and closes it. test(e2e): open and close structured chat tabs with an isolated Claude stand-in #30011 adds a durable chat e2e.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Agent skill upstream boundary
Notes
Folder workspaces take the local path. SSH hosts cannot run structured chats. Paired servers are skipped. The shipped build is unchanged.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)