feat(web-ui): SplitPane layout component — draggable, collapsible, persistent#517
Conversation
…rsistent (#507) - Draggable divider updates pane widths in real-time via direct DOM mutation during mousemove (no setState jank), flush to state on mouseup - Collapse/expand buttons on each pane edge (ArrowLeft01Icon/ArrowRight01Icon) - Split position persists to localStorage; restored on mount - minPanePercent enforced (default 15%) during drag - Smooth CSS transition on collapse/expand, disabled during drag - Mobile (<768px): vertical stack layout, divider hidden - 15 tests covering all acceptance criteria - Adds ArrowLeft01Icon to @hugeicons/react mock
WalkthroughThis PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant User as User/Mouse
participant Component as SplitPane Component
participant DOM as DOM/Window
participant Storage as localStorage
User->>Component: Click divider & drag mousedown
Component->>DOM: Start tracking mousemove
loop During Drag
User->>DOM: Move mouse (mousemove)
DOM->>Component: Update divider position (ref)
Component->>DOM: Mutate left/right pane widths (inline style)
end
User->>DOM: Release mouse (mouseup)
DOM->>Component: Calculate final split %
Component->>Component: Update React state (split %)
Component->>Storage: Persist split % to localStorage
Component->>DOM: Apply smooth CSS transition (complete)
alt User Clicks Collapse Left
User->>Component: Click left collapse button
Component->>Component: Set left width to 0
Component->>Storage: Persist new split to localStorage
Component->>Component: Remember pre-collapsed split for restore
else User Clicks Expand Left
User->>Component: Click left expand button
Component->>Component: Restore left width from remembered split
Component->>Storage: Persist restored split to localStorage
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Review: SplitPane component Clean implementation overall. The direct DOM mutation during drag is the right call for jank-free resize, and the test coverage is solid. A few issues worth addressing before merge. Bug: Collapsed state not restored from localStorage When a pane is collapsed, 0 or 100 is written to localStorage. On next mount, splitPct initializes to that value, but isLeftCollapsed and isRightCollapsed both default to false. The result: the pane renders at 0% width with collapse buttons pointing the wrong direction — the user sees a fully-hidden pane but clicking the button would try to collapse it again rather than expand it. Fix options: (a) Do not write 0/100 to storage during collapse — only write on drag end, persisting lastNonCollapsed instead. Collapse state is ephemeral React state that resets on mount anyway. (b) Or persist separate isLeftCollapsed/isRightCollapsed keys alongside the position. Accessibility: aria-hidden buries the collapse buttons The divider div has aria-hidden="true", which removes the entire subtree from the accessibility tree — including the collapse-left and collapse-right buttons. Those buttons have aria-label attributes that become unreachable to assistive technology. Fix: Remove aria-hidden from the container (or apply it only to the decorative drag strip, not the full divider). The container has no implicit ARIA role and the buttons are self-describing via aria-label, so the annotation is not needed there. Minor: no touch support on tablet-width devices The drag logic uses mousemove/mouseup only. Tablets at >= 768px see the desktop layout with a divider they cannot drag via touch. Adding touchmove/touchend listeners using e.touches[0].clientX would cover this. Low priority, but worth a follow-up issue if tablet support matters. Nitpick: React.ReactNode without namespace import The file uses named imports from react but references React.ReactNode in the interface. This works if React is globally available in tsconfig, but prefer importing type ReactNode directly and using it in the interface. What is working well:
The localStorage restore bug is the main blocker. The aria-hidden issue is a real accessibility defect. Both are straightforward fixes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
web-ui/src/__tests__/components/sessions/SplitPane.test.tsx (2)
168-173: Strengthen the mobile assertion to check actual inline width behavior.This test currently only validates
flex-col. It should also assert pane widths are not left inline-set in mobile mode.Suggested test addition
it('does not apply inline width styles on mobile', () => { mockMatchMedia(false); // mobile render(<SplitPane left={<div>L</div>} right={<div>R</div>} />); const container = screen.getByTestId('split-pane-container'); + const leftPane = screen.getByTestId('split-pane-left'); + const rightPane = screen.getByTestId('split-pane-right'); expect(container.className).toContain('flex-col'); + expect(leftPane.style.width).toBe(''); + expect(rightPane.style.width).toBe(''); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/__tests__/components/sessions/SplitPane.test.tsx` around lines 168 - 173, Update the mobile test in SplitPane.test.tsx to also assert that the left/right panes do not have inline width styles: after calling mockMatchMedia(false) and rendering <SplitPane ... />, locate the left and right pane elements (e.g., via screen.getByText('L') / 'R' or container.querySelector for the pane elements) and assert their style.width is empty/undefined or that their style attribute does not contain 'width'; keep the existing container.className check and use the same helpers mockMatchMedia, render, SplitPane, and screen.getByTestId to find elements.
134-166: Add a regression test for alternating collapse controls.Please add a case for
collapse-right -> collapse-left -> expand-leftto ensure split width and collapse labels/icons stay synchronized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/__tests__/components/sessions/SplitPane.test.tsx` around lines 134 - 166, Add a new test in SplitPane.test.tsx that renders <SplitPane left={<div/>} right={<div/>} defaultSplit={45} />, then perform the sequence: click getByTestId('collapse-right') to collapse the right pane and assert getByTestId('split-pane-right') has width '0%'; click getByTestId('collapse-left') to collapse the left pane and assert getByTestId('split-pane-left') has width '0%' and right pane now '100%'; click getByTestId('collapse-left') again to expand the left pane and assert getByTestId('split-pane-left') has width '45%' and getByTestId('split-pane-right') has width '55%'; include assertions on the collapse buttons' test ids to ensure state remains synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/components/sessions/SplitPane.tsx`:
- Around line 24-35: The persisted/default split value read by readStorage and
used to initialize splitPct can be outside a valid range (e.g., -20 or 180)
causing invalid pane widths; fix by clamping the value to a safe range before
use (for example between 5 and 95) — either normalize inside readStorage(key,
fallback) (return clamp(parsed, min, max)) or immediately after reading when
setting splitPct so that any parsed or fallback value is constrained; reference
readStorage and splitPct when making the change.
- Around line 209-210: The collapse control buttons in SplitPane (the JSX button
elements with data-testid="collapse-left" and the corresponding collapse-right
control around lines where collapse is rendered) lack an explicit type and thus
act as submit buttons inside forms; update both button elements to include
type="button" (i.e., set the button JSX attribute type="button" on the elements
referenced by data-testid="collapse-left" and the collapse-right counterpart in
the SplitPane component) so they no longer trigger form submissions.
- Around line 206-207: The divider container in SplitPane.tsx currently sets
aria-hidden="true", which hides the entire subtree including the interactive
collapse/expand buttons; remove the aria-hidden attribute from that container so
the buttons are exposed to assistive tech, and verify the collapse/expand
controls (the button elements inside the divider) have appropriate
roles/aria-labels or aria-expanded attributes to remain accessible (check the
SplitPane divider element and the collapse/expand button components referenced
in this file).
- Around line 140-170: The collapseLeft and collapseRight handlers can leave
isLeftCollapsed/isRightCollapsed out of sync with splitPct; update both boolean
flags based on the target split percentage whenever you change splitPct or
restore from lastNonCollapsed: use the restore/splitPct value to compute
setIsLeftCollapsed(restore === 0) and setIsRightCollapsed(restore === 100) (and
do the same when setting splitPct to 0 or 100 inside
collapseLeft/collapseRight), and continue to persist via writeStorage and update
lastNonCollapsed as currently done; modify collapseLeft and collapseRight to
always set both flags in this deterministic way.
- Around line 9-16: The SplitPaneProps type is declared inside the SplitPane
component file; move it into the shared types module and export it so the
component imports the centralized type. Create or add the interface
SplitPaneProps (matching the existing properties: left, right, defaultSplit,
minPanePercent, storageKey, className) to the shared types file and export it,
then update the SplitPane component to import SplitPaneProps from that module
instead of defining it locally (ensure the exported name matches and update any
usages inside the SplitPane component).
---
Nitpick comments:
In `@web-ui/src/__tests__/components/sessions/SplitPane.test.tsx`:
- Around line 168-173: Update the mobile test in SplitPane.test.tsx to also
assert that the left/right panes do not have inline width styles: after calling
mockMatchMedia(false) and rendering <SplitPane ... />, locate the left and right
pane elements (e.g., via screen.getByText('L') / 'R' or container.querySelector
for the pane elements) and assert their style.width is empty/undefined or that
their style attribute does not contain 'width'; keep the existing
container.className check and use the same helpers mockMatchMedia, render,
SplitPane, and screen.getByTestId to find elements.
- Around line 134-166: Add a new test in SplitPane.test.tsx that renders
<SplitPane left={<div/>} right={<div/>} defaultSplit={45} />, then perform the
sequence: click getByTestId('collapse-right') to collapse the right pane and
assert getByTestId('split-pane-right') has width '0%'; click
getByTestId('collapse-left') to collapse the left pane and assert
getByTestId('split-pane-left') has width '0%' and right pane now '100%'; click
getByTestId('collapse-left') again to expand the left pane and assert
getByTestId('split-pane-left') has width '45%' and
getByTestId('split-pane-right') has width '55%'; include assertions on the
collapse buttons' test ids to ensure state remains synchronized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c088b44-8ba8-4af3-8508-427ee1190dd4
📒 Files selected for processing (3)
web-ui/__mocks__/@hugeicons/react.jsweb-ui/src/__tests__/components/sessions/SplitPane.test.tsxweb-ui/src/components/sessions/SplitPane.tsx
| interface SplitPaneProps { | ||
| left: React.ReactNode; | ||
| right: React.ReactNode; | ||
| defaultSplit?: number; | ||
| minPanePercent?: number; | ||
| storageKey?: string; | ||
| className?: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move SplitPaneProps to the shared types module.
Line 9 defines SplitPaneProps inside the component file, which violates the repository type-location rule.
Proposed refactor
- interface SplitPaneProps {
- left: React.ReactNode;
- right: React.ReactNode;
- defaultSplit?: number;
- minPanePercent?: number;
- storageKey?: string;
- className?: string;
- }
+import type { SplitPaneProps } from '@/types';// web-ui/src/types/index.ts
export interface SplitPaneProps {
left: React.ReactNode;
right: React.ReactNode;
defaultSplit?: number;
minPanePercent?: number;
storageKey?: string;
className?: string;
}As per coding guidelines: "TypeScript types must be defined in web-ui/src/types/index.ts and API client functionality in web-ui/src/lib/api.ts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/sessions/SplitPane.tsx` around lines 9 - 16, The
SplitPaneProps type is declared inside the SplitPane component file; move it
into the shared types module and export it so the component imports the
centralized type. Create or add the interface SplitPaneProps (matching the
existing properties: left, right, defaultSplit, minPanePercent, storageKey,
className) to the shared types file and export it, then update the SplitPane
component to import SplitPaneProps from that module instead of defining it
locally (ensure the exported name matches and update any usages inside the
SplitPane component).
| function readStorage(key: string, fallback: number): number { | ||
| try { | ||
| const raw = localStorage.getItem(key); | ||
| if (raw !== null) { | ||
| const parsed = parseFloat(raw); | ||
| if (!isNaN(parsed)) return parsed; | ||
| } | ||
| } catch { | ||
| // localStorage unavailable (SSR, private browsing, etc.) | ||
| } | ||
| return fallback; | ||
| } |
There was a problem hiding this comment.
Normalize persisted/default split to a safe range before use.
At Line 55, splitPct can initialize from an out-of-range localStorage/default value (e.g., -20 or 180), which yields invalid pane widths.
Suggested fix
function readStorage(key: string, fallback: number): number {
try {
const raw = localStorage.getItem(key);
if (raw !== null) {
const parsed = parseFloat(raw);
- if (!isNaN(parsed)) return parsed;
+ if (Number.isFinite(parsed)) return clamp(parsed, 0, 100);
}
} catch {
// localStorage unavailable (SSR, private browsing, etc.)
}
- return fallback;
+ return clamp(fallback, 0, 100);
}Also applies to: 55-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/sessions/SplitPane.tsx` around lines 24 - 35, The
persisted/default split value read by readStorage and used to initialize
splitPct can be outside a valid range (e.g., -20 or 180) causing invalid pane
widths; fix by clamping the value to a safe range before use (for example
between 5 and 95) — either normalize inside readStorage(key, fallback) (return
clamp(parsed, min, max)) or immediately after reading when setting splitPct so
that any parsed or fallback value is constrained; reference readStorage and
splitPct when making the change.
| const collapseLeft = () => { | ||
| transitionEnabled.current = true; | ||
| if (isLeftCollapsed) { | ||
| const restore = lastNonCollapsed.current; | ||
| setIsLeftCollapsed(false); | ||
| setSplitPct(restore); | ||
| writeStorage(storageKey, restore); | ||
| } else { | ||
| lastNonCollapsed.current = splitPct; | ||
| setIsLeftCollapsed(true); | ||
| setIsRightCollapsed(false); | ||
| setSplitPct(0); | ||
| writeStorage(storageKey, 0); | ||
| } | ||
| }; | ||
|
|
||
| const collapseRight = () => { | ||
| transitionEnabled.current = true; | ||
| if (isRightCollapsed) { | ||
| const restore = lastNonCollapsed.current; | ||
| setIsRightCollapsed(false); | ||
| setSplitPct(restore); | ||
| writeStorage(storageKey, restore); | ||
| } else { | ||
| lastNonCollapsed.current = splitPct; | ||
| setIsRightCollapsed(true); | ||
| setIsLeftCollapsed(false); | ||
| setSplitPct(100); | ||
| writeStorage(storageKey, 100); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Collapse flags can drift from actual pane state in cross-toggle flows.
The boolean flags can become inconsistent with splitPct (e.g., collapse right → collapse left → expand left restores 100 while isRightCollapsed remains false), causing wrong icon/aria state.
Suggested fix
const collapseLeft = () => {
transitionEnabled.current = true;
if (isLeftCollapsed) {
const restore = lastNonCollapsed.current;
setIsLeftCollapsed(false);
+ setIsRightCollapsed(restore === 100);
setSplitPct(restore);
writeStorage(storageKey, restore);
} else {
lastNonCollapsed.current = splitPct;
setIsLeftCollapsed(true);
setIsRightCollapsed(false);
setSplitPct(0);
writeStorage(storageKey, 0);
}
};
const collapseRight = () => {
transitionEnabled.current = true;
if (isRightCollapsed) {
const restore = lastNonCollapsed.current;
+ setIsLeftCollapsed(restore === 0);
setIsRightCollapsed(false);
setSplitPct(restore);
writeStorage(storageKey, restore);📝 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.
| const collapseLeft = () => { | |
| transitionEnabled.current = true; | |
| if (isLeftCollapsed) { | |
| const restore = lastNonCollapsed.current; | |
| setIsLeftCollapsed(false); | |
| setSplitPct(restore); | |
| writeStorage(storageKey, restore); | |
| } else { | |
| lastNonCollapsed.current = splitPct; | |
| setIsLeftCollapsed(true); | |
| setIsRightCollapsed(false); | |
| setSplitPct(0); | |
| writeStorage(storageKey, 0); | |
| } | |
| }; | |
| const collapseRight = () => { | |
| transitionEnabled.current = true; | |
| if (isRightCollapsed) { | |
| const restore = lastNonCollapsed.current; | |
| setIsRightCollapsed(false); | |
| setSplitPct(restore); | |
| writeStorage(storageKey, restore); | |
| } else { | |
| lastNonCollapsed.current = splitPct; | |
| setIsRightCollapsed(true); | |
| setIsLeftCollapsed(false); | |
| setSplitPct(100); | |
| writeStorage(storageKey, 100); | |
| } | |
| }; | |
| const collapseLeft = () => { | |
| transitionEnabled.current = true; | |
| if (isLeftCollapsed) { | |
| const restore = lastNonCollapsed.current; | |
| setIsLeftCollapsed(false); | |
| setIsRightCollapsed(restore === 100); | |
| setSplitPct(restore); | |
| writeStorage(storageKey, restore); | |
| } else { | |
| lastNonCollapsed.current = splitPct; | |
| setIsLeftCollapsed(true); | |
| setIsRightCollapsed(false); | |
| setSplitPct(0); | |
| writeStorage(storageKey, 0); | |
| } | |
| }; | |
| const collapseRight = () => { | |
| transitionEnabled.current = true; | |
| if (isRightCollapsed) { | |
| const restore = lastNonCollapsed.current; | |
| setIsLeftCollapsed(restore === 0); | |
| setIsRightCollapsed(false); | |
| setSplitPct(restore); | |
| writeStorage(storageKey, restore); | |
| } else { | |
| lastNonCollapsed.current = splitPct; | |
| setIsRightCollapsed(true); | |
| setIsLeftCollapsed(false); | |
| setSplitPct(100); | |
| writeStorage(storageKey, 100); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/sessions/SplitPane.tsx` around lines 140 - 170, The
collapseLeft and collapseRight handlers can leave
isLeftCollapsed/isRightCollapsed out of sync with splitPct; update both boolean
flags based on the target split percentage whenever you change splitPct or
restore from lastNonCollapsed: use the restore/splitPct value to compute
setIsLeftCollapsed(restore === 0) and setIsRightCollapsed(restore === 100) (and
do the same when setting splitPct to 0 or 100 inside
collapseLeft/collapseRight), and continue to persist via writeStorage and update
lastNonCollapsed as currently done; modify collapseLeft and collapseRight to
always set both flags in this deterministic way.
| aria-hidden="true" | ||
| > |
There was a problem hiding this comment.
Remove aria-hidden from the divider container with interactive controls.
Line 206 hides the entire subtree from assistive tech, including the two collapse/expand buttons. That breaks screen-reader access to these controls.
Suggested fix
- aria-hidden="true"
+ role="separator"
+ aria-orientation="vertical"📝 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.
| aria-hidden="true" | |
| > | |
| role="separator" | |
| aria-orientation="vertical" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/sessions/SplitPane.tsx` around lines 206 - 207, The
divider container in SplitPane.tsx currently sets aria-hidden="true", which
hides the entire subtree including the interactive collapse/expand buttons;
remove the aria-hidden attribute from that container so the buttons are exposed
to assistive tech, and verify the collapse/expand controls (the button elements
inside the divider) have appropriate roles/aria-labels or aria-expanded
attributes to remain accessible (check the SplitPane divider element and the
collapse/expand button components referenced in this file).
| <button | ||
| data-testid="collapse-left" |
There was a problem hiding this comment.
Set explicit type="button" on collapse controls.
Without type, these default to submit buttons and can trigger unintended form submits when SplitPane is rendered inside a form.
Suggested fix
<button
+ type="button"
data-testid="collapse-left"
onClick={collapseLeft} <button
+ type="button"
data-testid="collapse-right"
onClick={collapseRight}Also applies to: 229-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/sessions/SplitPane.tsx` around lines 209 - 210, The
collapse control buttons in SplitPane (the JSX button elements with
data-testid="collapse-left" and the corresponding collapse-right control around
lines where collapse is rendered) lack an explicit type and thus act as submit
buttons inside forms; update both button elements to include type="button"
(i.e., set the button JSX attribute type="button" on the elements referenced by
data-testid="collapse-left" and the collapse-right counterpart in the SplitPane
component) so they no longer trigger form submissions.
|
Follow-up Review Following up on my initial review - no code changes have landed yet, so the issues I flagged remain open. Summarizing current status and adding observations from CodeRabbitAI's pass. Still Open from First Review localStorage collapse bug (blocker): When the left pane is collapsed, aria-hidden on divider container (accessibility defect): The collapse/expand buttons live inside the CodeRabbit Findings Worth Acting On Missing Clamping Moving Unchanged Positives The direct DOM mutation during drag, the SSR-safe storage helpers, the
|
1 participant
Closes #507
Summary
SplitPanecomponent atweb-ui/src/components/sessions/SplitPane.tsxlocalStorage, restored on mountminPanePercentenforced during drag (default 15%)< 768px): vertical stack, divider hidden; chat 60%, terminal 40%Test plan
npm run build)ArrowLeft01Iconadded to@hugeicons/reactJest mockSummary by CodeRabbit
Release Notes
New Features
Tests