fix(desktop): persist workbar collapse per session - #4694
Conversation
|
Thanks for addressing the collapse state leaking between sessions. Please rebase onto current main now that #4789 has merged, and update the conflicting Workbar E2E to use its new open/close menu. Keep the session-specific collapse preference and catalog-hydration guard, and verify A → B → A plus reload. Please also retain main's 340px panel minimum. #4692 touches the same controller, but I don't see a reason to make it a prerequisite. AI-assisted review with Codex. |
Keep right-panel visibility independent across sessions and preserve it across renderer restarts. Prune stored entries only after the session catalog is authoritative, and retire the ownerless global preference. Fixes apache#4693 Generated-by: OpenAI Codex
352fcc5 to
ee5dec6
Compare
|
Rebased onto current main after #4789 and force-pushed ee5dec6. I resolved the Workbar E2E against the new face menu: the regression opens a face from the launcher, opens another through the shared open/close menu, then verifies A → B → A and reload. Main's 340px minimum remains unchanged. Verification: 27/27 focused model/controller tests passed; the requested E2E passed; full build, typecheck, Biome, diff check, and Desktop architecture validation passed. In the 9-test Workbar E2E file, 8 passed and the existing narrow-layout test timed out before reaching a Workbar assertion, then passed when rerun alone. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head ee5dec66984afa985c1efc1fff5d2bd40b52b92c.
This change moves right-workbar visibility into a per-Session map while keeping topology and dimensions global. I found one P2: cleanup of a previous Session's final dynamic tab can overwrite the newly active Session's saved expanded state. The new navigation/reload regression passes, but it retains static tabs and does not exercise this cleanup path.
Validation passed for the focused controller/model suite (27/27), the targeted Electron journey (1/1), Desktop build and typecheck, renderer architecture (101/101), repository lint and format, changed-file Biome, ASF headers, diff checks, and a clean merge against current main (7370f07a94cf8a553bc9cf97f09048e9fdf25e14). GitHub exposes no hosted checks for this head.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } | ||
| } | ||
| return { ...state, panels, rightCollapsed, bottomOpen }; | ||
| return withRightCollapsed({ ...state, panels, bottomOpen }, rightCollapsed); |
There was a problem hiding this comment.
[P2] Preserve the newly active Session's visibility during stale-tab cleanup
After activate-session binds Session B, the layout effects in use-workbar-controller.ts:522-578 remove Terminal and Side Chat tabs owned by Session A. If that stale tab is the last right-side tab, the close branch above sets rightCollapsed = true, and this call records that value under the current owner, B. On this exact head I reproduced the production controller path with B saved expanded, A owning the sole Terminal, then switching to B: cleanup removed A's tab and changed B from expanded to collapsed. The visibility effect then persists the overwritten map. Please keep cleanup closes from mutating the newly active Session's visibility (or target the old owner explicitly), and add a regression for the dynamic-only A → previously-expanded B path.
There was a problem hiding this comment.
Fixed in ac3b3a7. Stale Terminal and Side Chat cleanup now dispatches a topology-only remove-stale transition, so removing A's final right-side dynamic tab cannot record a collapse under active Session B. User-triggered closes still collapse an emptied right panel, and bottom-panel cleanup keeps its previous behavior. Added the requested production-controller regression: B is saved expanded, A opens the sole Terminal, then switching to B removes A's tab while B remains expanded. Verification: focused model/controller suite 28/28; A → B → A plus reload Electron journey 1/1; Desktop main/renderer builds, typecheck, renderer architecture 101/101, repository lint/format, ASF headers, Biome, and diff checks pass.
Remove session-owned Terminal and Side Chat tabs without recording an empty-panel collapse under the newly active session. Keep normal user closes and bottom-panel cleanup behavior unchanged. Generated-by: OpenAI Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head ac3b3a79c7c763f0a92cac52bd7ca37143a72499. I did not find a remaining correctness issue in this head.
The follow-up fixes the previously reported cross-Session visibility overwrite. Stale Terminal cleanup now uses the visibility-preserving path at use-workbar-controller.ts:539-546, stale Side Chat cleanup does the same at use-workbar-controller.ts:559-575, and workbar-layout.ts:249-276 removes those tabs without recording an empty right panel as collapsed for the newly active Session. Ordinary user closes still use the normal close action, while stale bottom-panel cleanup retains the prior behavior of closing an empty bottom panel.
Validation completed:
- Clean install and full
build:testpassed. - Full Desktop test suite: 2218 passed, 0 failed.
- Focused Workbar controller/model suite: 28 passed, including the dynamic-only stale Terminal regression.
- The A → B → A plus reload Electron journey passed on both the PR head and a synthetic merge with current
main(411512bd9cb698f668eccbae35adf41e71d7cf68). - Desktop build and typecheck, renderer architecture (101/101), repository lint/format, ASF headers, and diff checks passed.
- GitHub currently exposes no hosted checks for this head.
I did not run the Electron journey on native macOS or Windows.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at ac3b3a7 against main (a5a99a633), +293/−41 over 8 files, clean merge. CI had not run at this head (approval gate); I approved the run and test is green.
The problem is real and the decision was made where it should be: on main the right workbar's collapse bit is one global localStorage key (workbar-layout.ts:93-97), so a new Session inherits whatever the last one left, and #4693 (liuxiaocs7) asked for per-Session with either persistence answer acceptable as long as the superseded key goes. The PR takes persistence, retires the v1 key, and does it the way the issue suggested: the state lives in the layout reducer (workbar-layout.ts:203-215, activate-session / retain-sessions) rather than a parallel map with sync effects. I followed the production path: selectAuthoritativeSessionIds returning undefined before hydration reuses the store's existing revision semantics and both consumers already handle it; sessions:list is unpaginated and retain-sessions keeps the active Session, so pruning cannot drop a live preference; the render-phase activate-session dispatch is a self-reducer update React re-renders synchronously; remove-stale no longer writes the right collapse bit, which is what closes the earlier P2 from the automated pass. Ablation: dropping remove-stale reopens that P2, dropping the hydration guard wipes preferences on a cold start, so both stay. Tests render the real controller, the real catalog controller and the full shell; the A → B → A + reload case is in the e2e.
P3:
closeWorkbarTab(use-workbar-layout-state.ts:149-153, returned at:239) lost its last caller when the side-chat cleanup moved tocloseWorkbarTabs. Delete it.- Retiring the v1 key is an inline
try { localStorage.removeItem } catch {}atworkbar-layout.ts:177, bypassingbrowser-storage.tsand running on every right-visibility write. Add asafeLocalStorageRemovethere and call it once. - The before/after pair is Linux on the left and macOS on the right, one theme, hosted on your fork's raw URL. Same platform, light and dark, uploaded with
gh pr edit --attachso the images survive branch deletion.
Manual check before merge, light and dark: open A, expand, create B (collapsed), back to A (expanded), reload, both preserved; delete A, its preference gone.
Evidence boundary: static read; no build, no test run, no Storybook.
AI-assisted review: drafted with Maka; I verified the dead callback, the inline removeItem and the reducer path myself.
#4694 moved the right workbar's collapse bit into a per-Session map after #4877 wrote the shell story against the old `rightCollapsed` field, and it added a sixth Electron test to `session-workbar.spec.ts` without recording it in `e2e-budget.json`. Both guards fail on `main` since; this branch carries them so its CI can be read. Generated-by: Claude Code
… shut The right workbar's collapse toggle sat 24px in from the plate's edge on macOS, where the platform draws nothing on the right: the titlebar strip and the workbar's own bar each spelled out `--space-6 + overlay-right-width`. Fold that into one `--maka-titlebar-gutter-right` token with the bar's own `--space-2` inset, so the toggle keeps the same x in both bands and only Windows caption buttons push it inward. The column also snapped between `display: none` and its width, unlike the sidebar. On a wide window the frame and the overlay panel that share its grid area now animate width and margin, with `visibility` gated by the same duration. Their children hold the open width and hang off the right edge (`justify-content: end` on the lone grid track; an auto margin would resolve to 0 against negative free space), so the face and the toggle keep their resting x and the box's left edge sweeps over them, the way the sidebar reveals its own. Below 991px the column stacks under the conversation at full width, so there collapsing still removes the row, as it does for the bottom placement at every width. A modal obscuring the shell sets `hidden` on the frame instead of collapsing it, so a dialog does not play the ease. The panels stay mounted while hidden; a CDP trace of the 280ms ease costs ~95ms of main thread over 33 frames with no drops at 120Hz. `min-width`/`max-width` on the column go: `workbar-layout.ts` already clamps the value, and a floor would stop the collapse short of 0. Dead rules go with them: the narrow block's `max-width: none`, the panel's `[hidden]` override that `base.css` already covers with `!important`, and the side-chat panel's flex column, which the panel's own grid track now stretches. The shell story that holds the toggle to one x carries the motion contract too, since it is the one story that mounts both bands: it asserts the 8px gutter, then samples frames around collapse and restore for a running width transition, the panel tracking the frame, and the face and toggle never moving. It writes the simulated caption width on the document root, where the gutter token reads it, and passes `motionEnabled` so the shell frame does not pin the fixture attribute that turns transitions off. A narrow-viewport story pins the stacked layout. Two hunks catch `main` up to #4694, which moved the collapse bit into a per-Session map after the shell story was written against `rightCollapsed`, and added a sixth Electron test without recording it in `e2e-budget.json`. Generated-by: Claude Code
#4694 replaced WorkbarLayoutState.rightCollapsed with activeSessionId + collapsedBySession; #4877 landed a story that builds the old shape. Both were green on their own bases, so main's typecheck fails on stories/app-shell.stories.tsx. Build the story on the new state and read the collapse bit through isSessionWorkbarCollapsed, as the controller does. Generated-by: Claude Code
…4895) `check:e2e-budget` and the Storybook typecheck both failed on `main` after #4694 merged: that PR added a sixth test to `session-workbar.spec.ts` without updating `e2e-budget.json`, and replaced `WorkbarLayoutState.rightCollapsed` with `activeSessionId` + `collapsedBySession` while #4877 had just landed `stories/app-shell.stories.tsx` building the old shape. Its CI ran on a base that predated both, so neither showed until merge (reported on #4894). The budget entry now records 6 tests and names the Electron-owned mechanism (per-Session collapse persisted across a renderer reload). The story builds the new layout state and reads the collapse bit through `isSessionWorkbarCollapsed`, as the controller does. No behavior change; no migration. Refs #4694, #4761, #4877, #4894 Generated-by: Claude Code
3 participants
Summary
Fixes #4693
Opening the right workbar in conversation A currently opens it for a new conversation B. Store collapse state by session in the layout reducer so B starts collapsed and returning to A restores its preference, including after renderer reload. Bind the owner before commit so an open requested during activation is not overwritten.
This chooses persistent per-session visibility, while keeping panel topology and dimensions global. The old global
maka-session-workbar-collapsed-v1preference has no session owner and is retired in favor of v2; sessions default to collapsed once on upgrade. Please review that persistence/topology split and migration behavior against the decision requested in the issue.Prune deleted entries only after the catalog has a committed snapshot. The catalog selector distinguishes “not loaded” from “confirmed empty,” preventing startup from deleting saved preferences.
Verification
b0255edcb35588b2f24efd386447d9a494c1e395on Linux ARM64: B's panel is visible when it should be hidden.npm run lint,npm run format:check,npm run build,npm run typecheck, both Desktop/UIknipchecks, Desktop architecture validation (including 101 checker tests), andgit diff --checkpassed.Before: unchanged Linux ARM64 baseline, where B inherits A's expanded panel. The fixture image lacks CJK fonts, affecting labels only.
After: fixed macOS ARM64 build, where B stays collapsed after navigating back and reloading.
AI use
Tool(s) and scope: OpenAI Codex assisted with repository/issue investigation, implementation, regression tests, verification, and this PR description. The commit includes
Generated-by: OpenAI Codex. Human review remains required.Checklist
Does this PR entail a change in behavior?