Resolve deferred review items from split tab groups#227
Conversation
Addresses the remaining deferred suggestions from the PR 224 review rounds: - closing a background tab no longer switches the group's active tab; a successor is picked only when the closed tab was active (VS Code behavior) - pending layout writes flush on beforeunload, so a split made just before quit is not lost to the 500ms debounce; the debounce timer now reuses flushLayoutPersist instead of duplicating it - secondary group strips get role=tablist to match their role=tab children (the primary strip already had one via PanelTabBar) - after an n-ary sibling insert into an existing split, the stored model syncs once from Allotment's live sizes (defaultSizes is mount-only, so on-screen and stored sizes diverged until the next sash drag) - add a vitest suite for the pure tree math in panelLayout.ts (44 tests: normalize, splitGroup, movePanel, reconcile, geometry, drop zones) and run frontend unit tests in the quality workflow
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec1cb5e0d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Flush a pending layout write before the window closes: the debounce would | ||
| // otherwise lose a split made just before quit. | ||
| useEffect(() => { | ||
| window.addEventListener('beforeunload', flushLayoutPersist); |
There was a problem hiding this comment.
When the user quits the app via the app-level quit flow, this beforeunload hook is bypassed by the existing shutdown path: main/src/index.ts prevents app.before-quit at line 1200 and later calls app.exit(0) at line 1419 instead of closing the BrowserWindow, so the renderer can be terminated without a beforeunload event. In that split-then-quit scenario the layout can still remain only in pendingLayoutRef; trigger the flush from the main quit flow or explicitly close the window after the pending write has completed.
Useful? React with 👍 / 👎.
1 participant
Description
Closes out every remaining deferred item from the #224 review rounds (the should-fixes went out in #225, the strip drop gap is #226):
group.activePanelId === panel.id.flushLayoutPersistinstead of duplicating its body.role="tablist"on secondary strips: theirrole="tab"children previously had no tablist container (the primary strip gets one from PanelTabBar).defaultSizesis mount-only, so dropping a tab as a new sibling in an existing split left on-screen sizes diverged from the stored model until the next sash drag.onChangenow keeps a per-split snapshot in a ref (no re-renders), and a child-count change syncs the model from it once. Sash behavior is unchanged (persist still happens on drag end only).Type of Change
Checklist
pnpm typecheckandpnpm lintlocallyCritical Areas Modified
Additional Notes
The remaining flagged items are deliberate tradeoffs, not fixed: Windows/Linux
Ctrl+\is the split hotkey so it no longer reaches the PTY as SIGQUIT there (macOS preserved in #225), and the first split / last un-split remounts the surviving group per the pixel-identical single-group rule.