feat(ui): Add EditorTabs component (Phase 1)#6
Merged
Merged
Conversation
Browser-style tab strip with single-row scrollable layout, keyboard navigation, and pinned + collapsible group segments. - Add 16 IDE tab semantic tokens to theme (all 4 theme modes + density) - Implement EditorTabs compound component with controlled/uncontrolled state - Tab segments: pinned, grouped (with collapsible group chips), ungrouped - Dirty dot / close button stacking in same fixed-size trailing slot - Vertical dividers that hide adjacent to active/hovered tabs - Scroll buttons + gradient shadows for overflow - Roving tabindex keyboard navigation (Arrow, Home/End, Delete) - Variant system (solid/soft/outline/ghost), color, and size props - Context menu story using library ContextMenu component - Fix ContextMenu separator visibility (use theme-aware border token) - 8 unit tests, 7 Storybook stories with full controls
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete EditorTabs feature (types, utils, context, hook, multiple components, styling, stories, and tests) and a small ContextMenu CSS tweak introducing a separator CSS variable with a color-mix fallback. Changes
sequenceDiagram
participant User as User
participant Tabs as EditorTabs
participant View as Viewport
participant Context as EditorTabsContext
participant Scroll as useTabScroll
participant DOM as Browser DOM
rect rgba(200,200,255,0.5)
User->>Tabs: click / right-click / keyboard nav
Tabs->>Context: call handlers (activate, open context menu, close)
Context-->>Tabs: provide activeId, handlers, scroll helpers
end
rect rgba(200,255,200,0.5)
User->>View: wheel / scroll gesture
View->>Scroll: handleWheel
Scroll->>DOM: adjust scrollLeft
Scroll->>Tabs: update scrollState
Tabs->>View: toggle ScrollButton / ScrollShadow visibility
end
rect rgba(255,220,200,0.5)
User->>Tabs: click close button
Tabs->>Context: onClose(tabId)
Context->>Tabs: update selection / emit callbacks
Tabs->>DOM: update tab elements
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/context-menu/ContextMenu.module.css`:
- Line 102: The separator background currently uses color-mix with the popup
border token (--_ov-border) which the ghost variant sets to transparent, causing
separators to disappear; introduce a dedicated separator token (e.g.
--popup-separator or --_separator-color), set a sensible default and a
non-transparent value for the ghost variant, then replace the background rule
using color-mix(in srgb, var(--_ov-border) 72%, transparent 28%) with a rule
that references the new token (and a fallback) so separators remain visible
across variants; update any relevant selectors for the ghost variant and the
place where --_ov-border was previously relied upon (the separator background
declaration and the ghost variant block).
In `@packages/base-ui/src/components/editor-tabs/EditorTabCloseButton.tsx`:
- Around line 5-9: The close button currently uses a generic accessible name;
update the EditorTabCloseButton component to accept the tab title (e.g., add a
tabTitle: string or title: string field to EditorTabCloseButtonProps) and use it
in the button's accessible label (e.g., set aria-label or accessible name to
include the tab title like "Close {tabTitle} tab"); update the button markup in
EditorTabCloseButton (including the other instance referenced at lines 19-20) to
use this prop instead of the generic label so each close button is uniquely
identifiable to assistive tech.
In `@packages/base-ui/src/components/editor-tabs/EditorTabGroupChip.tsx`:
- Around line 46-57: EditorTabGroupChip currently forces a white foreground
('#fff') when group.color is set, which can fail contrast for light backgrounds;
update the component (EditorTabGroupChip) to compute an accessible foreground
color based on group.color luminance (or prefer a supplied foreground in
TabGroupDescriptor if present) and use that computed/supplied value instead of
the hardcoded '#fff'. Implement a small utility to compute contrast (e.g.,
luminance or YIQ) and apply the resulting color to the inline style where
group.color is used, and remove the duplicated style branches by building a
single style object using the computed fg/bg values.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.module.css`:
- Around line 146-158: The .GroupChipCount rule uses a hardcoded background
rgb(255 255 255 / 0.25) which breaks theme switching; change the background to a
theme-aware CSS variable (e.g., var(--ov-chip-bg) or an existing overlay token)
so it responds to light/dark modes, and add a sensible fallback value for
compatibility; update EditorTabs.module.css to replace the background
declaration on .GroupChipCount and add/verify the corresponding token in your
theme tokens if it doesn't exist.
- Around line 256-260: The .TabTitle rule currently hardcodes max-width: 160px
which is inflexible; make it configurable by adding a CSS custom property (e.g.,
--_ov-tab-title-max-width) to the .Root selector and update .TabTitle to use
that property via var(--_ov-tab-title-max-width, 160px) so callers can override
the tab title max width without changing the CSS file; ensure the default
fallback remains 160px for backward compatibility.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx`:
- Around line 356-365: The clipboard write in the ContextMenu.Item click handler
can reject and is currently ignored; wrap the navigator.clipboard.writeText call
inside an async try/catch (or handle the returned Promise) in the onClick for
the ContextMenu.Item that calls getContextTab()/findTab(), and on failure log
the error (e.g., console.error) or surface a user notification in production
(toast/alert) so failures are not silent.
- Around line 242-251: The closeTab callback mixes two state setters (setTabs
and setActiveId) inside the same setTabs functional update which is confusing;
refactor closeTab to compute the new tabs array first (e.g., const next =
tabs.filter(t => t.id !== id) or derive it inside a functional updater), then
call setTabs(next) and separately update setActiveId based on the computed next
(if current === id and next.length > 0 setActiveId(next[0].id) else leave
current), or replace both states with a single reducer to update tabs and
activeId together; ensure you reference the existing closeTab, setTabs,
setActiveId and TabId symbols when making the change and add tabs to the
dependency array if you capture it directly.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.test.tsx`:
- Around line 7-13: The test suite replaces globalThis.ResizeObserver in the
beforeAll block but never restores it; modify the test to save the original
(e.g., const originalResizeObserver = globalThis.ResizeObserver) before
overriding in beforeAll and add an afterAll that restores
globalThis.ResizeObserver = originalResizeObserver so the stub only affects this
suite (reference the existing beforeAll and add a corresponding afterAll to undo
the change).
In `@packages/base-ui/src/components/editor-tabs/EditorTabScrollButton.tsx`:
- Around line 17-24: The button in EditorTabScrollButton is currently removed
from the accessibility tree via aria-hidden and tabIndex={-1}; remove those
attributes, add an accessible name (e.g., aria-label derived from the direction
prop like "Scroll tabs left" / "Scroll tabs right"), and use the visible prop to
disable the control when it is not usable (set disabled and aria-disabled="true"
when visible is false) while keeping it focusable and present in DOM; keep the
existing onClick/scrollTo logic and className usage so the change only affects
accessibility behavior.
In `@packages/base-ui/src/components/editor-tabs/EditorTabsSegment.tsx`:
- Line 1: The file currently imports forwardRef and ReactNode but uses
React.CSSProperties; add an explicit type import for CSSProperties from 'react'
(e.g., import type { CSSProperties } from 'react') and replace usages of
React.CSSProperties with the imported CSSProperties type to avoid relying on the
ambient React namespace; update the import line that currently contains
forwardRef and type ReactNode and change the type reference at the location
where React.CSSProperties is used (search for React.CSSProperties in
EditorTabsSegment/forwardRef-related code) to the new CSSProperties identifier.
In `@packages/base-ui/src/components/editor-tabs/EditorTabsViewport.tsx`:
- Around line 24-31: The onWheel handler in EditorTabsViewport currently calls
preventDefault() for vertical wheel events unconditionally, which hurts
accessibility; change this to be opt-in and only intercept when intent and
scrollability are clear: add a prop (e.g., enableHorizontalWheel = false) to the
EditorTabsViewport component and only call e.preventDefault() and modify
el.scrollLeft when that prop is true AND the element is actually scrollable
(check el.scrollWidth > el.clientWidth) and a clear intent is detected (e.g.,
the element is focused or hovered, or a modifier key like Shift is pressed).
Update the onWheel logic (the inline handler using viewportRef.current) to gate
behavior on those checks so default wheel behavior remains for assistive tech
unless explicitly enabled.
In `@packages/base-ui/src/components/editor-tabs/hooks/useTabScroll.ts`:
- Around line 44-59: The effect only listens to scroll and ResizeObserver on the
viewportRef element so canScrollLeft/canScrollRight can become stale when tabs
are added/removed; attach a MutationObserver to the viewport element (or its
tabs container) inside the same useEffect (where viewportRef.current and
updateScrollState are used) to watch childList/subtree changes and call
updateScrollState on mutations, and disconnect that MutationObserver in the
cleanup alongside removing the scroll listener and disconnecting the
ResizeObserver; keep references to viewportRef and updateScrollState (as already
used) to locate the change.
- Around line 35-41: handleWheel in useTabScroll currently calls
e.preventDefault() for mostly-vertical wheel events unconditionally, which
blocks page scrolling when the tab strip is already at a boundary; change the
logic to only call e.preventDefault() and modify el.scrollLeft when the wheel
will actually move the viewport: compute const maxScroll = el.scrollWidth -
el.clientWidth and if e.deltaY > 0 only act when el.scrollLeft < maxScroll, if
e.deltaY < 0 only act when el.scrollLeft > 0; otherwise do nothing so vertical
scrolling bubbles. Update the handleWheel callback (referencing viewportRef,
e.deltaY/e.deltaX and el.scrollLeft) to perform this edge check before
preventing default and changing scrollLeft.
- Line 1: The file's useTabScroll hook references React.WheelEvent but only
imports named hooks from 'react', causing a TS error; update the imports so the
WheelEvent type is available by adding a type import (e.g., import type {
WheelEvent } from 'react') or import the React namespace and change the handler
signature accordingly in useTabScroll (look for the onWheel handler and any
WheelEvent usages) so TypeScript can resolve the type.
In `@packages/base-ui/src/components/editor-tabs/utils/computeTabSegments.ts`:
- Around line 11-24: The loop in computeTabSegments builds group buckets but
uses groups?.some(...) for each tab causing O(n×m) behavior; pre-index group IDs
into a Set (e.g., const groupIdSet = new Set(groups?.map(g => g.id))) before the
for-loop and replace the groups?.some((g) => g.id === tab.groupId) check with
groupIdSet.has(tab.groupId), keeping the rest of the logic for pinned, groupMap,
and ungrouped unchanged (references: tabs, groups, groupMap, pinned, ungrouped).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30f03a24-2c8c-47e4-a6fe-be240cf88b97
📒 Files selected for processing (19)
packages/base-ui/src/components/context-menu/ContextMenu.module.csspackages/base-ui/src/components/editor-tabs/EditorTabCloseButton.tsxpackages/base-ui/src/components/editor-tabs/EditorTabGroupChip.tsxpackages/base-ui/src/components/editor-tabs/EditorTabItem.tsxpackages/base-ui/src/components/editor-tabs/EditorTabScrollButton.tsxpackages/base-ui/src/components/editor-tabs/EditorTabScrollShadow.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.test.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.tsxpackages/base-ui/src/components/editor-tabs/EditorTabsSegment.tsxpackages/base-ui/src/components/editor-tabs/EditorTabsViewport.tsxpackages/base-ui/src/components/editor-tabs/context/EditorTabsContext.tsxpackages/base-ui/src/components/editor-tabs/hooks/useTabScroll.tspackages/base-ui/src/components/editor-tabs/index.tspackages/base-ui/src/components/editor-tabs/types.tspackages/base-ui/src/components/editor-tabs/utils/computeTabSegments.tspackages/base-ui/src/components/index.tspackages/base-ui/src/theme/styles.css
- Fix ContextMenu separator visibility in ghost variant (dedicated fallback token) - Add unique accessible labels to close buttons (includes tab title) - Compute accessible foreground color for group chips based on luminance - Add optional `fg` field to TabGroupDescriptor for manual override - Replace hardcoded GroupChipCount background with theme-aware token - Make TabTitle max-width configurable via CSS custom property - Fix scroll buttons: proper aria-label, disabled state instead of aria-hidden - Fix onWheel to only preventDefault when viewport can actually scroll - Add MutationObserver to detect tab add/remove for scroll state updates - Pre-index group IDs with Set for O(n+m) segment computation - Restore globalThis.ResizeObserver in test afterAll - Use explicit WheelEvent type import in useTabScroll - Use explicit CSSProperties import in EditorTabsSegment - Handle clipboard writeText rejection in story
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/editor-tabs/EditorTabGroupChip.tsx`:
- Around line 8-19: The contrastFg function currently uses a YIQ cutoff and
returns white/black incorrectly for many colors; replace it with a
WCAG-compliant contrast check: parse the 6- or 3-digit hex in contrastFg
(keep/extend current regex to accept shorthand), convert sRGB channels to linear
values, compute relative luminance per WCAG, then compute contrast ratios of the
color against '#000' and '#fff' and return the hex (either '#000' or '#fff')
with the higher contrast ratio; keep the existing fallback to '#fff' for
unparseable inputs and ensure the function name contrastFg (used by
EditorTabGroupChip/EditorTabs) is updated with this logic.
- Around line 1-2: The file uses the type React.MouseEvent<HTMLElement> (e.g.,
in the event handler signature in EditorTabGroupChip) but doesn't import React
or MouseEvent; fix by adding an explicit type import from React — e.g., import
type { MouseEvent, ForwardedRef } from 'react' (or at minimum import type {
MouseEvent } from 'react') and update any handler signatures to use
MouseEvent<HTMLElement> so the TypeScript modern JSX transform resolves the type
correctly.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.module.css`:
- Around line 34-40: The Stylelint rule requires a blank line before the
`border-bottom-color` declaration inside the variant rule blocks; update each
variant block (e.g., the `.Root[data-ov-variant='solid']` rule and the analogous
blocks around the other variant sections) to insert a single blank line
immediately before `border-bottom-color` so the declaration is separated from
the preceding custom property declarations.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx`:
- Around line 257-279: The closeToRight and closeToLeft callbacks must guard
against a stale tab id: after computing idx with prev.findIndex in closeToRight
and closeToLeft, if idx === -1 simply return prev (no-op) instead of proceeding
to slice; this prevents clearing or truncating the tabs when the context tab no
longer exists—update the logic inside setTabs in both functions (the blocks in
closeToRight and closeToLeft that call prev.findIndex, compute next via slice,
and call setActiveId) to early-return prev when findIndex returns -1.
In `@packages/base-ui/src/components/editor-tabs/EditorTabsViewport.tsx`:
- Around line 24-34: The wheel interception is currently unconditional and can
interfere with assistive tech; add a boolean prop enableHorizontalWheel (default
false) to the EditorTabsViewport component and guard the onWheel handler with it
so horizontal-to-horizontal scrolling is only intercepted when
enableHorizontalWheel is true; update the component's props/type definition to
include enableHorizontalWheel, use EditorTabsViewport's viewportRef and existing
onWheel logic but early-return when enableHorizontalWheel is false, and document
the new prop/default in the component's prop comments or JS/TS docs.
In `@packages/base-ui/src/components/editor-tabs/hooks/useTabScroll.ts`:
- Around line 21-25: The updateScrollState callback currently calls
setScrollState(computeScrollState(el)) every tick, causing rerenders even when
canScrollLeft/canScrollRight haven't changed; change updateScrollState (which
reads viewportRef.current and calls computeScrollState) to compare the newly
computed booleans against the existing state before calling setScrollState—only
call setScrollState when canScrollLeft or canScrollRight differ from the current
scrollState (use the previous state via a ref or the functional setState form to
do the comparison) so ResizeObserver/MutationObserver and scroll handlers do not
trigger unnecessary React updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2410a4a2-86ee-4838-a093-b74593aa0745
📒 Files selected for processing (13)
packages/base-ui/src/components/context-menu/ContextMenu.module.csspackages/base-ui/src/components/editor-tabs/EditorTabCloseButton.tsxpackages/base-ui/src/components/editor-tabs/EditorTabGroupChip.tsxpackages/base-ui/src/components/editor-tabs/EditorTabItem.tsxpackages/base-ui/src/components/editor-tabs/EditorTabScrollButton.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.test.tsxpackages/base-ui/src/components/editor-tabs/EditorTabsSegment.tsxpackages/base-ui/src/components/editor-tabs/EditorTabsViewport.tsxpackages/base-ui/src/components/editor-tabs/hooks/useTabScroll.tspackages/base-ui/src/components/editor-tabs/types.tspackages/base-ui/src/components/editor-tabs/utils/computeTabSegments.ts
Comment on lines
+24
to
+34
| onWheel={(e) => { | ||
| const el = viewportRef.current; | ||
| if (!el) return; | ||
| if (Math.abs(e.deltaY) <= Math.abs(e.deltaX)) return; | ||
| const maxScroll = el.scrollWidth - el.clientWidth; | ||
| if (maxScroll <= 0) return; | ||
| if (e.deltaY > 0 && el.scrollLeft >= maxScroll) return; | ||
| if (e.deltaY < 0 && el.scrollLeft <= 0) return; | ||
| e.preventDefault(); | ||
| el.scrollLeft += e.deltaY; | ||
| }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improved wheel handling with boundary checks.
The wheel handler now includes boundary checks (lines 28-31) that prevent preventDefault() when already at scroll limits, which is an improvement. However, the previous review suggested making horizontal wheel interception opt-in via a prop for accessibility. The current behavior may still interfere with assistive technologies when the viewport is scrollable.
Consider adding an enableHorizontalWheel prop (default false) to make this behavior explicit, or document this interaction pattern for users who rely on standard trackpad gestures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/editor-tabs/EditorTabsViewport.tsx` around
lines 24 - 34, The wheel interception is currently unconditional and can
interfere with assistive tech; add a boolean prop enableHorizontalWheel (default
false) to the EditorTabsViewport component and guard the onWheel handler with it
so horizontal-to-horizontal scrolling is only intercepted when
enableHorizontalWheel is true; update the component's props/type definition to
include enableHorizontalWheel, use EditorTabsViewport's viewportRef and existing
onWheel logic but early-return when enableHorizontalWheel is false, and document
the new prop/default in the component's prop comments or JS/TS docs.
- Replace YIQ brightness with WCAG-compliant contrast ratio for group chip foreground color - Use explicit MouseEvent import instead of React.MouseEvent namespace - Add stylelint-required blank lines before border-bottom-color in variant blocks - Guard closeToRight/closeToLeft against stale tab IDs (idx === -1) - Use functional setState in useTabScroll to avoid unnecessary rerenders
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/editor-tabs/EditorTabGroupChip.tsx`:
- Around line 30-44: The contrastFg function currently assumes 3/6-digit hex and
returns '#fff' for anything else, causing non-hex colors (e.g., rgb(), hsl(),
var()) to incorrectly force white text; update contrastFg to return
null/undefined when the input isn't a parseable hex (keep existing hex parsing
and contrast logic intact), and change the caller that sets the CSS variable
--_ov-accent-fg (where TabGroupDescriptor.color is applied) to only set that
variable when contrastFg returns a non-null value so non-hex colors fall back to
the normal CSS text color instead of forcing '#fff'.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.module.css`:
- Around line 292-332: Add a visible keyboard focus style for the close button
so focus is obvious when it moves from the tab to .CloseButton: update the
.CloseButton selector to include a :focus and/or :focus-visible rule (e.g.
.CloseButton:focus-visible) that applies a visible ring/outline/box-shadow
matching the tab's focus ring (use the same radius var(--ov-primitive-radius-sm)
and the UI focus color/vars used elsewhere), remove any default outline if
needed, and ensure the focus ring sits above the tab content (adjust z-index if
required); target the .CloseButton symbol so keyboard users can see focus when
the .Tab:focus-visible ring is no longer applied.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx`:
- Around line 199-213: The controlled story renderers (e.g., ClosableRender)
keep passing a stale activeId after the last tab is removed; update the
onCloseTab handler (and the similar "Close All" handler in the other render
functions referenced) so that when remaining.length === 0 you clear the
controlled activeId state (call setActiveId(undefined) or null) instead of
leaving the previous id; locate the useState for activeId and the
onCloseTab/onCloseAll callbacks in the EditorTabs story render functions and add
the conditional that sets activeId to undefined when no tabs remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eef9e914-c8ba-4ee7-a78c-a0cd7776444b
📒 Files selected for processing (4)
packages/base-ui/src/components/editor-tabs/EditorTabGroupChip.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsxpackages/base-ui/src/components/editor-tabs/hooks/useTabScroll.ts
- Return null from contrastFg for non-hex colors (rgb/hsl/var) so --_ov-accent-fg falls back to normal CSS text color instead of '#fff' - Add :focus-visible ring on CloseButton for keyboard accessibility - Clear activeId when all tabs are removed in Closable and WithContextMenu stories
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.module.css`:
- Around line 170-171: Remove the redundant border-right declaration: locate the
CSS rule in EditorTabs.module.css that contains both "border: none" and
"border-right: none" and delete the "border-right: none" line so only "border:
none" remains (this keeps all borders reset while removing the unnecessary
duplicate).
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx`:
- Around line 380-422: The docs example references a non-existent helper
copyName; update the snippet to match the actual implementation by either (a)
adding a copyName helper that calls
navigator.clipboard.writeText(contextTabRef.current) and use it in the
ContextMenu.Item, or (b) replace the copyName call with the same inline
clipboard write used in the render (e.g.,
navigator.clipboard.writeText(contextTabRef.current)). Make the change near the
ContextMenu.Item that currently calls copyName so the EditorTabs docs reflect
the real onContextMenuTab/contextTabRef behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1740c30c-71e4-4fbc-abcc-35b34f012aa2
📒 Files selected for processing (3)
packages/base-ui/src/components/editor-tabs/EditorTabGroupChip.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx
Comment on lines
+380
to
+422
| parameters: { | ||
| docs: { | ||
| source: { | ||
| code: ` | ||
| <ContextMenu.Root> | ||
| <ContextMenu.Trigger> | ||
| <EditorTabs | ||
| tabs={tabs} | ||
| activeId={activeId} | ||
| onActiveChange={setActiveId} | ||
| onCloseTab={closeTab} | ||
| onContextMenuTab={(id) => { | ||
| contextTabRef.current = id; | ||
| }} | ||
| /> | ||
| </ContextMenu.Trigger> | ||
| <ContextMenu.Portal> | ||
| <ContextMenu.Positioner> | ||
| <ContextMenu.Popup> | ||
| <ContextMenu.Item onClick={() => closeTab(contextTabRef.current)}> | ||
| Close | ||
| </ContextMenu.Item> | ||
| <ContextMenu.Item onClick={() => closeOthers(contextTabRef.current)}> | ||
| Close Others | ||
| </ContextMenu.Item> | ||
| <ContextMenu.Item onClick={() => closeToRight(contextTabRef.current)}> | ||
| Close to the Right | ||
| </ContextMenu.Item> | ||
| <ContextMenu.Separator /> | ||
| <ContextMenu.Item onClick={() => togglePin(contextTabRef.current)}> | ||
| Pin Tab | ||
| </ContextMenu.Item> | ||
| <ContextMenu.Item onClick={() => copyName(contextTabRef.current)}> | ||
| Copy File Name | ||
| </ContextMenu.Item> | ||
| </ContextMenu.Popup> | ||
| </ContextMenu.Positioner> | ||
| </ContextMenu.Portal> | ||
| </ContextMenu.Root> | ||
| `, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Example code in docs slightly differs from actual implementation.
The docs.source.code block references a copyName function (line 412) that doesn't exist in the actual render function, which uses an inline clipboard write. This could confuse developers copying the example.
♻️ Align example code with implementation
- <ContextMenu.Item onClick={() => copyName(contextTabRef.current)}>
+ <ContextMenu.Item onClick={() => {
+ const tab = findTab(contextTabRef.current);
+ if (tab) navigator.clipboard.writeText(tab.title);
+ }}>
Copy File Name
</ContextMenu.Item>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx` around
lines 380 - 422, The docs example references a non-existent helper copyName;
update the snippet to match the actual implementation by either (a) adding a
copyName helper that calls navigator.clipboard.writeText(contextTabRef.current)
and use it in the ContextMenu.Item, or (b) replace the copyName call with the
same inline clipboard write used in the render (e.g.,
navigator.clipboard.writeText(contextTabRef.current)). Make the change near the
ContextMenu.Item that currently calls copyName so the EditorTabs docs reflect
the real onContextMenuTab/contextTabRef behavior.
- Remove redundant border-right: none (already covered by border: none) - Fix docs snippet referencing non-existent copyName helper
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.module.css`:
- Around line 183-186: Transitions on tab-related elements ignore users'
prefers-reduced-motion setting; add a media query for `@media`
(prefers-reduced-motion: reduce) that sets transition: none for the relevant
selectors (e.g., .Tab, .CloseButton, .ScrollButton, .ScrollShadow, .TabTrailing
.DirtyDot and .Tab::after) so those animations are disabled for users who
request reduced motion.
- Around line 141-147: The two empty CSS rules .Segment[data-segment='group']
and .Segment[data-collapsed] should be either removed or given a minimal
placeholder to satisfy linters; update the EditorTabs.module.css by deleting
those blocks if they’re not needed, or add a harmless declaration such as a
comment placeholder or a no-op property (e.g., an explicit /* placeholder */
comment or outline: none;) inside each rule so the linter recognizes them as
intentional hooks.
- Around line 338-361: Add a visible focus ring for keyboard users by adding
:focus-visible rules for .ScrollButton (and when applicable
.ScrollButton[data-visible]) similar to the CloseButton fix: define a clear
outline/box-shadow and ensure the focus style contrasts with
var(--_ov-group-header-bg) and var(--_ov-tab-fg), and do not rely on
opacity/pointer-events (only apply when [data-visible] is present so hidden
buttons remain non-interactive). Update the selectors
.ScrollButton:focus-visible and .ScrollButton[data-visible]:focus-visible to
include the focus styling so tabbing to scroll buttons shows a clear visual
indicator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c4c6973-a3d6-442e-b7d2-3a04fe3c5ddb
📒 Files selected for processing (2)
packages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx
- Add prefers-reduced-motion media query to disable transitions - Remove empty CSS rules for group/collapsed segments (replaced with comment) - Add :focus-visible ring on ScrollButton for keyboard accessibility
10 tasks
This was referenced Mar 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EditorTabscompound component — browser-style tab strip with single-row scrollable layout, keyboard navigation, and pinned + collapsible group segmentsdata-ov-*conventionsContextMenuseparator visibility — was using--ov-color-border-muted(invisible in dark mode), now uses theme-awarecolor-mixlike Card footer dividersWhat's included
ContextMenucomponent with close/pin/copy actionsTest plan
pnpm typecheckpassespnpm test— all 126 tests pass (8 new for EditorTabs)Summary by CodeRabbit
New Features
Style
Tests
Docs