refactor(workspace): polish tab context menu#29
Conversation
- Right-clicking a tab no longer switches to it (prevent focus-driven activation on the tab trigger); the context menu still opens. - Batch closes (close others / close to the right) now preserve the surviving tabs' navigation history instead of wiping both stacks, mirroring single-tab close (removeSnapshotTabs). - 'Close all' is disabled when the workspace is already just the default inbox tab, avoiding a redundant no-op. - 'Close all' uses a distinct trash icon to stand out from the other items. Follow-up to ohmygit-hub#26 addressing review nits.
AidenNovak
left a comment
AidenNovak
left a comment
There was a problem hiding this comment.
The “Close tabs to the right” path is still clearing navigation history.
The PR description says both closeOtherTabs and closeTabsToRight now preserve the surviving tab's history, but the diff only changes closeOtherTabs. closeTabsToRight still sets backStack.value and forwardStack.value to empty arrays.
I reproduced this from the production Electron build at commit ecff9dc. I opened Inbox and Needs Review, switched between them so Back was enabled, kept Needs Review active, then right-clicked Inbox and chose “Close tabs to the right.” Inbox was left open, but both Back and Forward became disabled.
The build, typecheck, and existing test suite all pass (68 files, 459 tests), so the current tests do not cover this interaction.
Could you update closeTabsToRight so it removes only the closed tab URLs from the navigation stacks, and add a regression test for this flow?
Before closing:
Context menu on the background Inbox tab:
After closing tabs to the right:
…he right The PR description claimed both closeOtherTabs and closeTabsToRight kept the surviving tabs' navigation history, but only closeOtherTabs was actually changed; closeTabsToRight still cleared both stacks. Route it through removeSnapshotTabs as well, and add regression tests covering the reported repro.
2 participants
What
Follow-up to #26 addressing the review nits plus a right-click bug I hit while using the feature.
TabsTrigger@focusauto-activation doesn't fire.closeOtherTabsandcloseTabsToRightpreviously wiped both nav stacks entirely, dropping the kept tab's back/forward history too. Both now use a newremoveSnapshotTabshelper that strips only the closed tab URLs from the stacks (mirroring how single-tabcloseTabusesremoveSnapshotTab). Note: the initial push only fixedcloseOtherTabs, despite this description claiming both — good catch in review.closeTabsToRightis fixed in 1981c49 together with regression tests covering the reported repro.canCloseAllTabsguard so the item is disabled when the workspace is already just the default/inboxtab, andcloseAllTabsearly-returns in that case.XtoTrash2so it scans apart from the other close actions.Why
These are the three nits from the #26 review (kept-tab history reset, close-all no-op guard, icon variety) plus the right-click-activates-tab behavior, which felt wrong in daily use.
Testing
use-workspace-tabs.test.ts) that drive the composable through a memory-history router and reproduce the reviewed flow; both failed at thecanGoBackassertion before the fix and pass after.main(v0.0.13) into the branch; no conflicts.pnpm --filter @oh-my-github/client typecheckpasses.pnpm -r test: api 254 + client 244 tests).