feat(ui): add VS Code-style tabs for open pages#249
Conversation
Implements a tab system for managing multiple open pages: - Add useOpenTabsStore for persisted tab state management - Create TabBar and TabItem components with auto-hide behavior (only shows when 2+ tabs are open) - Support keyboard shortcuts: Ctrl+1-9 for tab switching, Ctrl+W to close, Ctrl+Tab to cycle - Add middle-click and Ctrl+click to open pages in background tabs - Integrate useTabSync hook to sync URL navigation with tabs - Place TabBar below TopBar as full-width accordion element The tab bar automatically hides when only one page is open, reducing UI noise while providing power-user functionality. https://claude.ai/code/session_01PWWeSnM1SQpd9Dp9LpDFmy
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a persistent tabbed UI: new TabBar/TabItem components, a Zustand open-tabs store (with tests), a useTabSync hook to sync URL ↔ tabs, layout integration (TabBar render and useTabSync invocation), middle-click background-tab handling, and a RecentsDropdown component. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PageTreeItem
participant Browser
participant CenterPanel
participant TabSync as useTabSync
participant Tabs as OpenTabsStore
participant TabBar
User->>PageTreeItem: middle-click or Cmd/Ctrl+click page link
PageTreeItem->>Tabs: openTabInBackground(pageId, driveId)
Tabs->>Tabs: insert tab after active (active unchanged)
User->>Browser: URL changes / direct navigation
Browser->>CenterPanel: route params updated
CenterPanel->>TabSync: invoke sync for (pageId, driveId)
TabSync->>Tabs: find existing tab?
alt tab exists
TabSync->>Tabs: setActiveTab(tabId)
else tab missing
TabSync->>Tabs: openTab(pageId, driveId, title, type) and setActive
end
Tabs->>TabBar: emit updated tabs state
User->>TabBar: keyboard shortcut (Cmd/Ctrl+1 or Cmd/Ctrl+Tab)
TabBar->>Tabs: setActiveTabByIndex / cycleTab
Tabs->>Browser: navigate to active tab route
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![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: 8437bdb39e
ℹ️ 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".
| // Handle middle-click on mousedown (for middle-click detection) | ||
| const handleMouseDown = useCallback((e: MouseEvent<HTMLAnchorElement>) => { | ||
| e.stopPropagation(); | ||
| if (e.button === 1) { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
Middle-click on a link is handled by the browser’s auxclick default action, so preventDefault() in the onMouseDown handler does not reliably stop the new-tab behavior. In browsers like Chrome, this will still open a new browser tab while also calling openTabInBackground, giving users duplicate tabs (one in-app, one in the browser). To suppress the native behavior for middle-clicks, handle onAuxClick/onMouseUp and call preventDefault() there instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 1537c46. Changed onMouseDown to onAuxClick in TabItem.tsx for proper middle-click handling. This is the standard event for middle-click cross-browser compatibility.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/web/src/components/layout/tabs/TabBar.tsx`:
- Around line 41-60: The handleClose logic prevents the "no tabs left" branch
from ever running because it only executes when tabs.length > 1; update
handleClose to always handle the isClosingActive case regardless of tabs.length
by computing remainingTabs = tabs.filter(t => t.id !== tabId) (or remainingCount
= tabs.length - 1) and then: if remainingTabs.length > 0 pick newActiveIndex =
Math.min(tabIndex, remainingTabs.length - 1) and router.push to
`/dashboard/${newActiveTab.driveId}/${newActiveTab.id}`, otherwise construct the
dashboard path using params.driveId and router.push(`/dashboard${driveId ?
`/${driveId}` : ''}`); keep the existing closeTab(tabId) call and reference the
handleClose, tabs, activeTabId, closeTab, router, and params.driveId symbols.
In `@apps/web/src/hooks/useTabSync.ts`:
- Around line 30-35: Reset the stale lastSyncedPageId ref when no page/drive is
selected so the subsequent navigation to the same page triggers a sync; inside
useTabSync where you check "if (!pageId || !driveId || isLoading) return" add
logic to set lastSyncedPageId.current = null (or undefined) when pageId or
driveId are falsy (but do not reset on isLoading) so the later guard "if
(lastSyncedPageId.current === pageId) return" won’t incorrectly block resync
when the user moved to dashboard/settings and back.
In `@apps/web/src/stores/useOpenTabsStore.ts`:
- Around line 207-223: The pinTab handler uses Array.sort on newTabs causing
non-stable reordering of equal items; to fix, capture original indices before
mutating (e.g., map tabs to objects with originalIndex), update the target item
via pinTab to set isPinned true, then sort by isPinned first and by
originalIndex as a tiebreaker so unpinned tabs preserve their original order,
and finally call set({ tabs: ... }) with the reordered list; update references
to tabs/newTabs/pinTab/isPinned/set accordingly.
🧹 Nitpick comments (2)
apps/web/src/components/layout/tabs/TabItem.tsx (2)
69-91: Consider handling auxclick for middle-click cross-browser compatibility.Using
onMouseDownwithbutton === 1check works butonAuxClickis the standard event for middle-click handling. Some browsers may not firemousedownfor middle button consistently.♻️ Suggested improvement
onClick={handleClick} - onMouseDown={handleMiddleClick} + onAuxClick={handleMiddleClick} onKeyDown={handleKeyDown}And update the handler:
const handleMiddleClick = useCallback((e: React.MouseEvent) => { - if (e.button === 1) { - e.preventDefault(); - onClose(tab.id); - } + e.preventDefault(); + onClose(tab.id); }, [tab.id, onClose]);
93-99: Avoid type assertion; validate or narrow the type instead.The
as TabPageTypecast on line 94 bypasses TypeScript's type checking. Iftab.typeis already typed asTabPageTypein theTabinterface (which it is), this cast is redundant. If there's a type mismatch, fixing the source is preferable to casting.♻️ Suggested fix
<PageTypeIcon - type={tab.type as TabPageType} + type={tab.type} className={cn(
Add 43 unit tests covering all store actions: - openTab, openTabInBackground (tab creation) - closeTab, closeOtherTabs, closeTabsToRight, closeAllTabs - setActiveTab, setActiveTabByIndex, cycleTab (navigation) - pinTab, unpinTab (pin functionality) - reorderTab, updateTabTitle - Selector helpers (selectTabCount, selectHasMultipleTabs, selectActiveTab) Also refactor TabPageType to use existing PageType enum from @pagespace/lib/client-safe instead of duplicating the type. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🔬 Code Review SummaryChanges ReviewedBranch: Assessment: ✅ APPROVED (after test additions)
Issues Addressed
Key Strengths
Security ReviewAll OWASP Top 10 categories reviewed - no vulnerabilities found. localStorage persistence is appropriate for UI state (no sensitive data). 🤖 Generated by Claude Code Review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/stores/useOpenTabsStore.ts`:
- Around line 50-92: When opening a new unpinned tab from a pinned active tab,
the code in openTab and openTabInBackground inserts the new tab immediately
after the active tab which can split the pinned group; change the insertIndex
calculation to, when active tab (found via activeTabId) isPinned, set
insertIndex to the index after the last pinned tab (e.g., find last index where
t.isPinned) so unpinned tabs are always inserted after all pinned tabs, then
proceed to splice and set tabs/activeTabId as before.
- Hide TabBar on screens < 1024px using useBreakpoint hook - Add RecentsDropdown component with clock icon for mobile navigation - Fix type error: use PageType import instead of TabPageType cast Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix handleClose to navigate to dashboard when last tab is closed - Fix useTabSync to reset lastSyncedPageId when navigating away - Fix pinTab to use stable sort preserving original order - Fix onAuxClick instead of onMouseDown for middle-click handling - Fix openTab/openTabInBackground to insert after all pinned tabs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/components/layout/tabs/TabItem.tsx`:
- Around line 71-139: The outer <button> created for the tab should not contain
the inner close <button>; change the outer element to a non-interactive
container with role="tab" (e.g., a <div role="tab">) and keep its accessibility
attributes (aria-selected, tabIndex) and event handlers (handleClick,
handleMiddleClick, handleKeyDown) attached to that container, while leaving the
inner close button (handleCloseClick, aria-label={`Close ${tab.title}`}) as a
native <button>; ensure you remove the outer button-specific props
(type="button") and maintain the same className logic (the cn(...) call),
PageTypeIcon, title rendering, dirty/shortcut/pin indicators, and focus/keyboard
behavior by preserving focus-visible styles on the container and inner button.
- Around line 48-51: The onAuxClick handler (handleMiddleClick) currently closes
the tab for any non-primary mouse button; update handleMiddleClick to first
check e.button === 1 (middle-click) and return early otherwise, then call
e.preventDefault() and onClose(tab.id) only for middle clicks; update the place
where the handler is attached (the onAuxClick prop usage) to use this guarded
handleMiddleClick so right-clicks no longer trigger tab close.
- Add e.button === 1 check to handleMiddleClick to only trigger on middle-click (not right-click) - Change outer tab button to div with role="tab" to avoid nested interactive elements (close button inside button is invalid HTML) - Added cursor-pointer class to maintain visual affordance Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2witstudios
force-pushed
the
claude/add-tabs-architecture-9gt5m
branch
from
8d9fedd to
19c7c78
Compare
Resolved conflict in PageTreeItem.tsx by keeping both: - openTabInBackground from tabs feature - isTouchDevice from master Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Implements a tab system for managing multiple open pages:
(only shows when 2+ tabs are open)
Ctrl+W to close, Ctrl+Tab to cycle
The tab bar automatically hides when only one page is open,
reducing UI noise while providing power-user functionality.
https://claude.ai/code/session_01PWWeSnM1SQpd9Dp9LpDFmy
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.