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:
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.closeOtherTabs/closeTabsToRightpreviously wiped both nav stacks entirely, dropping the kept tab's back/forward history too. They now use a newremoveSnapshotTabshelper that strips only the closed tabs from the stacks (mirroring how single-tabcloseTabusesremoveSnapshotTab).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
pnpm --filter @oh-my-github/client typecheckpassesNote: the full client test suite was not run; verification covered typecheck, the locales test, and manual testing.