feat(ui): Add layout utility components#19
Merged
Conversation
|
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 three new layout components to packages/base-ui: AppShell (compound layout with header/sidebar/content/footer and collapsible sidebar), DockLayout (hierarchical, resizable tabbed layout with nested splits), and ResizableSplitPane (two-pane draggable/keyboard-resizable splitter). Includes CSS modules, Storybook stories, tests, and barrel exports. Changes
Sequence DiagramsequenceDiagram
actor User
participant App as AppShell / DockLayout / ResizableSplitPane
participant Tree as LayoutTree
participant State as ComponentState
participant DOM as DOM/Renderer
User->>App: interact (drag divider / click tab / toggle sidebar / keyboard)
App->>Tree: compute layout changes (sizes, activeTab, node updates)
Tree-->>App: updated layout model
App->>State: set state / emit onLayoutChange
State->>DOM: apply CSS vars & re-render panes/tabs/dividers
DOM-->>User: visual update & accessibility focus
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)
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: 8
🤖 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/app-shell/AppShell.stories.tsx`:
- Around line 72-85: The button in the AppShell story toggling the sidebar (uses
collapsed state and setCollapsed) lacks an explicit type and will default to
submit inside forms; update the button element to include type="button" to
prevent accidental form submission when this component is rendered inside a form
context.
In `@packages/base-ui/src/components/dock-layout/DockLayout.tsx`:
- Around line 398-429: The handler handleDividerDrag currently uses
containerRef.current to compute containerSize but must use the actual split
element being resized; find the split DOM element corresponding to splitId (e.g.
via the split node returned by findSplitByIndex or a data attribute/class on the
rendered split) and compute containerSize from that element's
offsetWidth/offsetHeight depending on direction, fall back to containerRef if
not found, then use that size in the deltaFraction calculation (retain the
direction check, minFraction logic, and updateSizesByPath/newLayout flow).
- Around line 161-170: The divider currently exposes role="separator" but only
handles pointer events (handlePointerDown, handlePointerMove, handlePointerUp)
so keyboard and assistive users can't operate it; either remove the interactive
semantics or implement the full splitter pattern: keep role="separator" and add
tabIndex (e.g., tabindex=0), onKeyDown handler on the same element to handle
ArrowLeft/Right/Up/Down and Home/End to increment/decrement the position, expose
aria-valuenow, aria-valuemin and aria-valuemax (derived from the DockLayout's
size/constraints) and update them as resizing occurs, and ensure focus styles
and proper keyboard focus management in the DockLayout component so keyboard
users can resize equivalently to pointer users.
- Around line 183-203: handleKeyDown updates the active tab via onTabClick but
doesn't move DOM focus, so keyboard navigation stops; after calling
onTabClick(tabs[nextIndex].id) focus the newly activated tab element (e.g., find
it by its id tabs[nextIndex].id or data-tab-id) — call focus in a microtask/next
paint (requestAnimationFrame or setTimeout 0) so the DOM has rerendered, e.g.
schedule document.getElementById(tabs[nextIndex].id)?.focus() after onTabClick;
apply the same focus-fix to the other arrow-key handler block referenced around
lines 223-232.
- Around line 314-317: The children are currently keyed by index (key={i}) which
breaks identity when removeTabFromTree() mutates node.children; change the key
to a stable identifier from the node itself (e.g., key={node.id ?? node.key ??
node.uid}) instead of index in the node.children.map that renders
DockNodeRenderer, and if nodes lack a stable id add one when nodes are created
(assign a uuid or stable key in the node factory) or expose a helper like
getStableNodeKey(node) to guarantee stable keys across removals.
In `@packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx`:
- Around line 77-80: Clamp the incoming defaultSize prop before the first render
and use that clamped value to initialize size and sizeRef so the pane never
starts out-of-bounds; specifically, compute an initialSize = clamp(defaultSize,
minSize, maxSize) (handle undefined maxSize) and replace any direct uses of
defaultSize when initializing the size state and sizeRef inside the
ResizableSplitPane component (where size and sizeRef are created) so both start
within [minSize, maxSize].
- Around line 99-108: The clamp callback currently only bounds value by minSize
and an optional maxSize; when maxSize is undefined we must derive an
effectiveMax from the root container's measured dimension (width for horizontal,
height for vertical) so keyboard resizing and aria semantics have an upper
bound. Update the clamp function (and the other similar closures at lines
referenced) to compute effectiveMax = maxSize ?? measuredRootSize and clamp
against effectiveMax, and expose that same effectiveMax to the separator's ARIA
by returning/using it for aria-valuemax (replacing any null/undefined). Locate
and change the clamp/useCallback references and the code that sets aria-valuemax
to reuse this computed effectiveMax so the value and the aria max are
consistent.
- Around line 194-203: The setRefs callback is using deprecated
React.MutableRefObject casts; change rootRef to be a RefObject<HTMLDivElement |
null> (e.g. useRef<HTMLDivElement | null>(null)) and update setRefs to avoid
casts: assign rootRef.current = node, then handle the forwarded ref parameter
(named ref) by calling it if typeof ref === 'function', otherwise if ref is an
object with a 'current' property (e.g. typeof ref === 'object' && ref !== null)
set (ref as RefObject<HTMLDivElement | null>).current = node; remove all
React.MutableRefObject casts and ensure types use RefObject/Ref<HTMLDivElement>
throughout (referencing setRefs, rootRef, and the forwarded ref).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd213cba-fe7e-4464-9cce-078a47d58785
📒 Files selected for processing (17)
docs/COMPONENT_STATUS.mdpackages/base-ui/src/components/app-shell/AppShell.module.csspackages/base-ui/src/components/app-shell/AppShell.stories.tsxpackages/base-ui/src/components/app-shell/AppShell.test.tsxpackages/base-ui/src/components/app-shell/AppShell.tsxpackages/base-ui/src/components/app-shell/index.tspackages/base-ui/src/components/dock-layout/DockLayout.module.csspackages/base-ui/src/components/dock-layout/DockLayout.stories.tsxpackages/base-ui/src/components/dock-layout/DockLayout.test.tsxpackages/base-ui/src/components/dock-layout/DockLayout.tsxpackages/base-ui/src/components/dock-layout/index.tspackages/base-ui/src/components/index.tspackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.module.csspackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.stories.tsxpackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.test.tsxpackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsxpackages/base-ui/src/components/resizable-split-pane/index.ts
Comment on lines
+99
to
+108
| const clamp = useCallback( | ||
| (value: number) => { | ||
| let clamped = Math.max(value, minSize); | ||
| if (maxSize != null) { | ||
| clamped = Math.min(clamped, maxSize); | ||
| } | ||
| return clamped; | ||
| }, | ||
| [minSize, maxSize], | ||
| ); |
There was a problem hiding this comment.
Derive an effective max when maxSize is omitted.
In the default configuration, keyboard resize can grow the first pane without an upper bound. That can consume the available container space, and the focusable separator exposes aria-valuenow without a matching aria-valuemax, so its range semantics are incomplete for assistive tech.
Clamp against the root’s measured width/height when maxSize is unset, and reuse that same computed value for aria-valuemax.
Also applies to: 163-190, 220-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx`
around lines 99 - 108, The clamp callback currently only bounds value by minSize
and an optional maxSize; when maxSize is undefined we must derive an
effectiveMax from the root container's measured dimension (width for horizontal,
height for vertical) so keyboard resizing and aria semantics have an upper
bound. Update the clamp function (and the other similar closures at lines
referenced) to compute effectiveMax = maxSize ?? measuredRootSize and clamp
against effectiveMax, and expose that same effectiveMax to the separator's ARIA
by returning/using it for aria-valuemax (replacing any null/undefined). Locate
and change the clamp/useCallback references and the code that sets aria-valuemax
to reuse this computed effectiveMax so the value and the aria max are
consistent.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/base-ui/src/components/dock-layout/DockLayout.tsx (1)
351-354:⚠️ Potential issue | 🟡 MinorSplit key still includes array index, potentially causing unnecessary remounts.
The key
split-${i}-${getSplitId(child)}includes the array indexi. IfgetSplitId(child)is already unique (which it should be, as it's based on descendant leaf IDs), the index prefix is redundant and reintroduces the instability issue flagged in a previous review—when a sibling is removed, remaining nodes' indices shift, changing their keys.🐛 Suggested fix
{node.children.map((child, i) => ( <DockNodeRenderer - key={child.type === 'leaf' ? child.id : `split-${i}-${getSplitId(child)}`} + key={child.type === 'leaf' ? child.id : getSplitId(child)} node={child}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/dock-layout/DockLayout.tsx` around lines 351 - 354, The key for split nodes includes the array index `i`, which can cause remounts when siblings are removed; update the map in DockLayout where node.children.map renders DockNodeRenderer so that the key for non-leaf children uses only the stable identifier returned by getSplitId(child) (e.g., `split-${getSplitId(child)}`) instead of `split-${i}-${getSplitId(child)}`; keep the existing leaf key logic (child.id) unchanged and ensure getSplitId(child) remains the unique stable id for split nodes.
🤖 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/dock-layout/DockLayout.stories.tsx`:
- Around line 137-140: The story definitions (SplitView and IDELayout) set
args.layout but their render functions ignore args and use internal useState
with hardcoded layouts, so Storybook controls won't work; update each story's
render to accept the story args parameter and initialize the component/state
from args.layout (e.g., change render: () => <SplitViewDemo /> to render: (args)
=> <SplitViewDemo initialLayout={args.layout} /> or inside the story component
use useState(args.layout) instead of the hardcoded layout), or alternatively
remove the args property if you prefer not to support controls—refer to the
SplitView and IDELayout story objects and the components using useState to
locate where to apply this change.
In `@packages/base-ui/src/components/dock-layout/DockLayout.test.tsx`:
- Around line 220-234: The test 'navigates tabs with arrow keys' in DockLayout
is relying on synchronous updates by using fireEvent.keyDown on tab1/tab2;
replace these with async user interactions using userEvent from
`@testing-library/user-event` (import userEvent), e.g. await userEvent.keyboard
while the tab element (tab1) is focused to send ArrowRight, then await
screen.findByText('Content 2') to wait for the state update, and similarly await
userEvent.keyboard on the now-active tab (tab2) and await
screen.findByText('Content 3'); update references to tab1/tab2 and keep
singleLeaf and the test name the same.
In `@packages/base-ui/src/components/dock-layout/DockLayout.tsx`:
- Around line 461-465: The code accesses newSizes[dividerIndex] and
newSizes[dividerIndex + 1] which are typed as number | undefined under
noUncheckedIndexedAccess; add explicit checks or assertions before mutating
them: ensure newSizes.length > dividerIndex + 1 (or check typeof
newSizes[dividerIndex] === 'number' && typeof newSizes[dividerIndex + 1] ===
'number') and then perform the Math.max updates, or use non-null assertions
(newSizes[dividerIndex]!) if you prefer an assertion-style fix; reference
newSizes, existingSizes, dividerIndex, deltaFraction and minFraction when making
the change so TypeScript no longer complains.
- Around line 184-198: The Divider currently hardcodes aria-valuenow=50; change
this so DockSplitContainer computes the actual split percentage from its sizes
array (e.g., sizes[index] normalized to 0–100) and passes it as a prop (e.g.,
currentPercent) into DockDivider/DockDivider's Divider element; then replace the
hardcoded aria-valuenow with that prop (ensure it is a number between
aria-valuemin/aria-valuemax 0 and 100) and update any prop typings for
DockDivider and usage in handleKeyDown/handlePointer* if they rely on the value.
In `@packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx`:
- Around line 127-159: The pointer capture can be lost without handlePointerUp
running, leaving dragging.current true and data-ov-dragging stuck; add an
onLostPointerCapture handler to the drag Handle element that calls the existing
handlePointerUp (the same cleanup used for pointerup), so that when pointer
capture is released unexpectedly the code clears dragging.current and removes
the data-ov-dragging attribute from rootRef; update the Handle render to include
onLostPointerCapture={handlePointerUp} (ensuring handlePointerUp remains the
cleanup function referenced).
---
Duplicate comments:
In `@packages/base-ui/src/components/dock-layout/DockLayout.tsx`:
- Around line 351-354: The key for split nodes includes the array index `i`,
which can cause remounts when siblings are removed; update the map in DockLayout
where node.children.map renders DockNodeRenderer so that the key for non-leaf
children uses only the stable identifier returned by getSplitId(child) (e.g.,
`split-${getSplitId(child)}`) instead of `split-${i}-${getSplitId(child)}`; keep
the existing leaf key logic (child.id) unchanged and ensure getSplitId(child)
remains the unique stable id for split nodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3dd9fc9f-88be-4d34-9427-967fdf6a46bb
📒 Files selected for processing (6)
packages/base-ui/src/components/app-shell/AppShell.stories.tsxpackages/base-ui/src/components/dock-layout/DockLayout.stories.tsxpackages/base-ui/src/components/dock-layout/DockLayout.test.tsxpackages/base-ui/src/components/dock-layout/DockLayout.tsxpackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.stories.tsxpackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx
Comment on lines
+137
to
+140
| export const SplitView: Story = { | ||
| args: { layout: splitLayout }, | ||
| render: () => <SplitViewDemo />, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Story args are unused in the render function.
SplitView and IDELayout stories define args: { layout: ... } but their render functions ignore these args and use their own useState with predefined layouts. This means Storybook's controls won't affect these stories.
Either remove the args property or use args.layout as the initial state.
♻️ Option: Remove unused args
export const SplitView: Story = {
- args: { layout: splitLayout },
render: () => <SplitViewDemo />,
};♻️ Option: Use args for initial state
-function SplitViewDemo() {
+function SplitViewDemo({ initialLayout }: { initialLayout: DockNode }) {
- const [layout, setLayout] = useState<DockNode>(splitLayout);
+ const [layout, setLayout] = useState<DockNode>(initialLayout);
// ...
}
export const SplitView: Story = {
args: { layout: splitLayout },
- render: () => <SplitViewDemo />,
+ render: (args) => <SplitViewDemo initialLayout={args.layout} />,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const SplitView: Story = { | |
| args: { layout: splitLayout }, | |
| render: () => <SplitViewDemo />, | |
| }; | |
| export const SplitView: Story = { | |
| render: () => <SplitViewDemo />, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/dock-layout/DockLayout.stories.tsx` around
lines 137 - 140, The story definitions (SplitView and IDELayout) set args.layout
but their render functions ignore args and use internal useState with hardcoded
layouts, so Storybook controls won't work; update each story's render to accept
the story args parameter and initialize the component/state from args.layout
(e.g., change render: () => <SplitViewDemo /> to render: (args) =>
<SplitViewDemo initialLayout={args.layout} /> or inside the story component use
useState(args.layout) instead of the hardcoded layout), or alternatively remove
the args property if you prefer not to support controls—refer to the SplitView
and IDELayout story objects and the components using useState to locate where to
apply this change.
…, DockLayout) Add three layout utility components for IDE-style application layouts: - ResizableSplitPane: Draggable split pane with horizontal/vertical modes, min/max constraints, keyboard resize, and double-click reset - AppShell: Application shell with header, sidebar, content, and footer slots using CSS Grid, with collapsible sidebar animation - DockLayout: Recursive dockable panel system with split views, tabbed groups, resizable dividers, and tab close/switch interactions All components use forwardRef, CSS Modules with semantic tokens, data attributes for styling, and include comprehensive tests and Storybook stories.
…compliance ResizableSplitPane: - Direct DOM manipulation during drag for 60fps (bypass React reconciliation) - Add aria-valuenow/min/max on separator for screen readers - Fix data-ov-dragging attribute lifecycle (set on pointerdown, clear on pointerup) - Remove useEffect that overrode user resize when defaultSize unchanged - Remove redundant CSS classes (FirstPane/SecondPane duplicated Pane) - Add user-select: none during drag to prevent text selection AppShell: - Use semantic HTML elements (header, aside, main, footer) - Animate sidebar collapse via grid-template-columns transition (not width) - Apply reduced-motion override on Root grid transition DockLayout: - Fix split identity: use structural ID instead of referential identity for updateSizesInTree (fixes resize after first interaction) - Direction-aware sizing in divider drag (offsetHeight for vertical splits) - Guard setPointerCapture for jsdom compatibility - Add keyboard tab navigation (ArrowLeft/Right, Home/End) with wrap-around - Add aria-controls on active tab linking to panel, aria-labelledby on panel - Replace primitive tokens with semantic tokens in CSS - Replace hardcoded font-size with token variable - Properly handle empty tabs array (fallback to empty string) - Merge refs correctly with callback ref pattern
- DockDivider: add tabIndex, keyboard handler, aria-valuenow/min/max - DockTabBar: move DOM focus to newly activated tab on arrow keys - DockSplitContainer: use stable keys instead of index-based keys - DockLayout divider drag: measure actual split element dimensions - ResizableSplitPane: clamp defaultSize before initializing state - Remove deprecated React.MutableRefObject casts - AppShell story: add type="button" to toggle button - Fix TypeScript strict array access patterns
- DockLayout stories: move data declarations before meta, remove unused args from stateful demo stories (SplitView, IDELayout, SinglePanel) - DockLayout test: use userEvent for arrow key navigation test - DockDivider: compute aria-valuenow from actual split percentages - DockLayout handleDividerDrag: guard newSizes indexed access - DockSplitContainer: remove index from split node keys - ResizableSplitPane: add onLostPointerCapture to clean up drag state
- Replace raw <button> with Button component in AppShell story - Add LuPanelLeftOpen/LuPanelLeftClose icons to sidebar toggle - Replace × unicode with LuX icon for DockLayout tab close button
joshuapare
force-pushed
the
feat/layout-utilities
branch
from
b4d0e30 to
47b286d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx (1)
103-112:⚠️ Potential issue | 🟠 MajorDerive a measured max when
maxSizeis omitted.
clamp()is still open-ended in the default configuration. That lets the first pane grow past the container, makesEndunusable, and leaves the focusable separator withoutaria-valuemax. Please clamp against the root's measured width/height and reuse that same effective max everywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx` around lines 103 - 112, The clamp function currently only respects maxSize when provided, leaving the pane unbounded; compute a single effectiveMax inside ResizableSplitPane by measuring the root container (using the existing rootRef or measurement hook) and setting effectiveMax = maxSize != null ? maxSize : measuredRootSize (or measuredRootSize - minSize if appropriate), then use that effectiveMax everywhere: replace references to maxSize in clamp, the separator's aria-valuemax, and any "End"/jump-to-end handler so the first pane cannot grow beyond the container; keep minSize as before and memoize/effect the measured value so all consumers reuse the same derived max.
🤖 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/app-shell/AppShell.tsx`:
- Around line 68-71: The formatSize function currently returns numbers as px and
passes strings through unvalidated; update the implementation and docs for
formatSize to either validate string inputs or explicitly document expected
formats: add a JSDoc block above function formatSize stating accepted string
units/values (e.g., "10px", "1.5rem", "%", "auto", etc.) and describing that
invalid or non-parseable strings will fallback; optionally implement a simple
validation step inside formatSize that checks strings against a small
whitelist/regex of common CSS units (px|rem|em|%|vh|vw|auto) and returns the
fallback when the string fails validation, referencing the formatSize function
name for location.
In `@packages/base-ui/src/components/dock-layout/DockLayout.test.tsx`:
- Around line 122-130: The test currently clicks the already-active "Editor" tab
so it only exercises a no-op; update the test in DockLayout.test.tsx to use a
layout with a multi-tab leaf (instead of using horizontalSplit) and click an
inactive tab to trigger a real state transition and fire onLayoutChange;
specifically, create or reuse a layout object that has the same leaf containing
tabs ["Editor", "Console"] (or similar), render <DockLayout
layout={thatMultiTabLayout} onLayoutChange={onChange} />, then
fireEvent.click(screen.getByRole('tab', { name: 'Console' })) and assert
expect(onChange).toHaveBeenCalled(); reference the DockLayout component, the
onLayoutChange prop and the horizontalSplit variable when locating where to
substitute the multi-tab layout.
In `@packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx`:
- Around line 16-29: The ResizableSplitPane exposes an internal focusable
separator (role="separator"/Handle) but the props type ResizableSplitPaneProps
does not allow callers to pass an accessible name; add a prop like handleLabel?:
string (or handleAriaLabel/handleAriaLabelledBy pair) to ResizableSplitPaneProps
and thread it through the component into the rendered Handle element so the
Handle receives aria-label (or aria-labelledby) from the prop; update any places
that construct or forward props for Handle to pass the new prop (e.g., where
Handle is rendered inside ResizableSplitPane and any relevant prop spread
logic).
- Around line 92-95: The component currently only initializes size and sizeRef
from clampedInitial, so when minSize/maxSize change after mount the current
value can fall outside the new bounds; add a useEffect that runs when minSize or
maxSize (and any other bounds-related props used to compute clampedInitial)
change, re-computes the clamped value (using the same clamp logic that produced
clampedInitial), assigns it to sizeRef.current and calls setSize(clamped) if the
clamped value differs from the current size, ensuring aria-valuenow and the
visual pane stay within the updated bounds.
- Around line 244-252: ResizableSplitPane currently exposes static members Pane
and Handle via ResizableSplitPane.Pane/Handle but ResizableSplitPaneRoot always
injects its own internal <Pane> and <Handle>, so externally provided
ResizableSplitPane.Pane/Handle children are ignored; fix by either removing the
exported statics from the public surface or modify ResizableSplitPaneRoot to
compose children explicitly: detect and render provided Pane and Handle children
(or accept a children render structure) instead of unconditionally creating
internal Pane/Handle, ensuring the root uses the exported Pane and Handle
components (referencing ResizableSplitPaneRoot, Pane, Handle, and
ResizableSplitPane.Pane/.Handle) so consumers’ components participate in the
layout.
- Around line 167-193: The key handler handleKeyDown conflates "key recognized"
with "size changed" so Home/End can fall through when size already at min/max;
update handleKeyDown to first detect whether the pressed key is one the splitter
handles (ArrowLeft/Right, ArrowUp/Down, Home, End — using direction,
KEYBOARD_STEP, minSize, maxSize, size) and if so call e.preventDefault()
immediately to consume the event, then compute delta, compute newSize via
clamp(size + delta), and only call setSize/applySize when the size actually
changes; ensure End is considered "handled" even when maxSize is null? (only
consume when End branch is reachable in current logic — keep existing maxSize
check).
---
Duplicate comments:
In `@packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx`:
- Around line 103-112: The clamp function currently only respects maxSize when
provided, leaving the pane unbounded; compute a single effectiveMax inside
ResizableSplitPane by measuring the root container (using the existing rootRef
or measurement hook) and setting effectiveMax = maxSize != null ? maxSize :
measuredRootSize (or measuredRootSize - minSize if appropriate), then use that
effectiveMax everywhere: replace references to maxSize in clamp, the separator's
aria-valuemax, and any "End"/jump-to-end handler so the first pane cannot grow
beyond the container; keep minSize as before and memoize/effect the measured
value so all consumers reuse the same derived max.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 497cac6a-7e57-41f9-852b-a1b92310aa4c
📒 Files selected for processing (17)
docs/COMPONENT_STATUS.mdpackages/base-ui/src/components/app-shell/AppShell.module.csspackages/base-ui/src/components/app-shell/AppShell.stories.tsxpackages/base-ui/src/components/app-shell/AppShell.test.tsxpackages/base-ui/src/components/app-shell/AppShell.tsxpackages/base-ui/src/components/app-shell/index.tspackages/base-ui/src/components/dock-layout/DockLayout.module.csspackages/base-ui/src/components/dock-layout/DockLayout.stories.tsxpackages/base-ui/src/components/dock-layout/DockLayout.test.tsxpackages/base-ui/src/components/dock-layout/DockLayout.tsxpackages/base-ui/src/components/dock-layout/index.tspackages/base-ui/src/components/index.tspackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.module.csspackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.stories.tsxpackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.test.tsxpackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsxpackages/base-ui/src/components/resizable-split-pane/index.ts
- AppShell formatSize: add JSDoc and CSS unit validation with fallback - DockLayout test: fix no-op tab click test to use inactive tab - ResizableSplitPane: add handleLabel prop for accessible separator name - ResizableSplitPane: add useEffect to re-clamp size when bounds change - ResizableSplitPane: fix handleKeyDown to preventDefault on all handled keys (Home/End) even when delta is zero - ResizableSplitPane: remove compound Pane/Handle statics from public API since they are internal-only
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx (1)
107-116:⚠️ Potential issue | 🟠 MajorDerive an effective max from the rendered container.
When
maxSizeis omitted,clamp()becomes one-sided andaria-valuemaxstays unset. That lets pointer/keyboard resize push pane 1 past the available split size and leaves the separator with incomplete range semantics.💡 Proposed fix
+ const getEffectiveMax = useCallback(() => { + if (maxSize != null) return maxSize; + const el = rootRef.current; + if (!el) return undefined; + return direction === 'horizontal' ? el.clientWidth : el.clientHeight; + }, [direction, maxSize]); + const clamp = useCallback( (value: number) => { let clamped = Math.max(value, minSize); - if (maxSize != null) { - clamped = Math.min(clamped, maxSize); + const effectiveMax = getEffectiveMax(); + if (effectiveMax != null) { + clamped = Math.min(clamped, effectiveMax); } return clamped; }, - [minSize, maxSize], + [getEffectiveMax, minSize], ); ... - } else if (e.key === 'End' && maxSize != null) { - delta = maxSize - size; + } else { + const effectiveMax = getEffectiveMax(); + if (e.key === 'End' && effectiveMax != null) { + delta = effectiveMax - size; + handled = true; + } - handled = true; } ... - aria-valuemax={maxSize} + aria-valuemax={getEffectiveMax()}Based on learnings:
packages/base-uiis not server-rendered, so browser-only measurement is safe here.Also applies to: 206-208, 250-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx` around lines 107 - 116, The clamp() function must treat a missing maxSize as a browser-measured effective max so the separator has a proper aria-valuemax and clamp is two-sided: add a container ref and a derivedEffectiveMax state in ResizableSplitPane, measure the container's clientWidth/clientHeight (depending on orientation) on mount and on resize (ResizeObserver or window resize) and compute effectiveMax = containerSize - otherPaneMinSize (or containerSize - minSize of the opposite pane), then update clamp to use (maxSize ?? derivedEffectiveMax) and include derivedEffectiveMax in clamp's dependency array; also set aria-valuemax to (maxSize ?? derivedEffectiveMax). Apply the same change pattern to the other similar clamp/aria-valuemax occurrences referenced in the diff (lines around 206-208 and 250-255).
🤖 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/app-shell/AppShell.tsx`:
- Around line 101-115: AppShell currently only changes CSS width (mergedStyle)
so the aside remains in the accessibility tree; pass the collapsed state down
and remove the subtree from AT/tab order: in AppShell (where mergedStyle is
built) pass a prop like collapsed={sidebarCollapsed} (or isCollapsed) into
AppShell.Sidebar, then update AppShell.Sidebar to apply aria-hidden={collapsed}
and make the subtree non-focusable when collapsed (use inert if supported or
programmatically set tabIndex=-1 on its interactive elements / wrapper) so the
sidebar is removed from the accessibility tree and tab order when
sidebarCollapsed is true.
- Around line 80-83: formatSize currently converts numeric values like NaN,
Infinity, or negatives into invalid CSS (e.g., "NaNpx" or "-1px"); update the
function so that when value is a number it only returns `${value}px` if
Number.isFinite(value) and value >= 0, otherwise return the fallback; keep the
existing string branch using CSS_SIZE_RE to validate string sizes (and return
fallback for invalid strings). Ensure the change is made in the formatSize
function referenced here and preserve the CSS_SIZE_RE check for string inputs.
In `@packages/base-ui/src/components/dock-layout/DockLayout.test.tsx`:
- Around line 122-133: The test 'fires onLayoutChange on tab click' duplicates
the earlier 'switches active tab on click' and should be removed or broadened:
either delete this redundant test and keep the existing one, or modify this case
(in DockLayout.test.tsx) to exercise a different onLayoutChange path (for
example, assert that clicking the already-active tab does not call
onLayoutChange, or test onLayoutChange behavior when layout prop is a
multi-leaf/tree structure). Update references to the DockLayout component and
the onLayoutChange mock (onChange) accordingly so the revised test name and
assertions reflect the new coverage.
- Around line 224-266: The keyboard-navigation tests for DockLayout ('navigates
tabs with arrow keys', 'wraps tab navigation with arrow keys', 'navigates to
first/last tab with Home/End') assert content/ callbacks but not that focus
actually moved; add explicit toHaveFocus() assertions after each Arrow/Home/End
action and ensure the DOM actually updates by rendering an uncontrolled/stateful
test harness that updates the layout on onLayoutChange (or use a small
TestWrapper component with useState that passes layout and onLayoutChange into
DockLayout) so the newly active tab receives focus; replace uses of fireEvent
where appropriate with userEvent.keyboard for async focus transitions and assert
e.g. the new tab element has focus after each navigation.
In `@packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx`:
- Around line 133-141: applySize currently updates the DOM and sizeRef but not
the React state that renders aria-valuenow, so assistive tech sees stale values
during dragging; inside applySize, also update the size state (call
setSize(newSize)) or ensure the element's aria-valuenow is rendered from
sizeRef.current instead of size, and apply the same change to the analogous
update points (the other applySize-like handlers referenced around lines 161-167
and 250-255) so aria-valuenow stays synchronized while dragging; reference
applySize, sizeRef, setSize (or the aria render location) and
onResizeRef.current when making the fix.
---
Duplicate comments:
In `@packages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx`:
- Around line 107-116: The clamp() function must treat a missing maxSize as a
browser-measured effective max so the separator has a proper aria-valuemax and
clamp is two-sided: add a container ref and a derivedEffectiveMax state in
ResizableSplitPane, measure the container's clientWidth/clientHeight (depending
on orientation) on mount and on resize (ResizeObserver or window resize) and
compute effectiveMax = containerSize - otherPaneMinSize (or containerSize -
minSize of the opposite pane), then update clamp to use (maxSize ??
derivedEffectiveMax) and include derivedEffectiveMax in clamp's dependency
array; also set aria-valuemax to (maxSize ?? derivedEffectiveMax). Apply the
same change pattern to the other similar clamp/aria-valuemax occurrences
referenced in the diff (lines around 206-208 and 250-255).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed308944-6cfc-44ef-aae5-257aa094e180
📒 Files selected for processing (4)
packages/base-ui/src/components/app-shell/AppShell.tsxpackages/base-ui/src/components/dock-layout/DockLayout.test.tsxpackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsxpackages/base-ui/src/components/resizable-split-pane/index.ts
- AppShell: sidebar gets aria-hidden and inert when collapsed, removing it from the accessibility tree and tab order via context - AppShell: formatSize rejects NaN, Infinity, and negative numbers - DockLayout test: replace duplicate tab-click test with active-tab no-op assertion - DockLayout test: keyboard nav tests now assert focus moves to the newly activated tab using userEvent and toHaveFocus() - ResizableSplitPane: applySize updates aria-valuenow on the Handle element directly during drag so assistive tech sees live values
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/app-shell/AppShell.tsx`:
- Around line 58-60: Remove the obsolete `@ts-expect-error` above the inert prop
in AppShell.tsx and use the same boolean-or-undefined pattern as aria-hidden:
stop suppressing types and set the inert prop to sidebarCollapsed or undefined
(so it passes a boolean/undefined per React's types) instead of the current
string/undefined pattern; update the inert attribute next to aria-hidden in the
AppShell component to remove the suppression and supply a boolean-compatible
value.
In `@packages/base-ui/src/components/dock-layout/DockLayout.test.tsx`:
- Around line 122-134: Rename the test to reflect actual behavior (e.g., change
the it description to "keeps activeTab unchanged when clicking the
already-active tab"), and make the mock usage explicit: assert that the mock
onChange was called with expect(onChange).toHaveBeenCalled() before accessing
onChange.mock.calls[0][0], then assert the layout's activeTab remains 'tab-1';
update references to DockLayout and the onLayoutChange callback accordingly and
avoid directly indexing mock.calls without the prior assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c0051ca-1d20-4626-be12-c36086dd65a4
📒 Files selected for processing (3)
packages/base-ui/src/components/app-shell/AppShell.tsxpackages/base-ui/src/components/dock-layout/DockLayout.test.tsxpackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsx
- AppShell: remove obsolete @ts-expect-error on inert prop, use boolean-or-undefined pattern matching aria-hidden - DockLayout test: rename test to 'keeps activeTab unchanged when clicking the already-active tab', assert onChange was called before accessing mock.calls
This was referenced Mar 12, 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
All components use forwardRef, CSS Modules with semantic tokens, data attributes for styling, reduced motion support, and proper ARIA attributes (aria-valuenow/min/max on separators, aria-controls/aria-labelledby on tabs/panels).
Test plan
pnpm validate)pnpm storybook)Summary by CodeRabbit
New Features
Documentation
Tests