fix(web-ui): SplitPane a11y, logic bugs, and test coverage#518
Conversation
- Remove aria-hidden from divider; add role=separator + aria-orientation - Add type="button" to collapse buttons (prevent form submission) - Add keyboard resize via ArrowLeft/ArrowRight (WCAG 2.1.1) - Fix lastNonCollapsed corruption when cross-collapsing panes - Don't write 0/100 to localStorage on collapse (prevents reload drift) - Clamp readStorage to valid expanded range [min, 100-min] - Return null until isMobile resolves (prevent hydration flash) - Expand tests: 29 tests covering all 8 review findings
|
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:
WalkthroughTightened persisted split bounds and ignored collapsed sentinels (0/100); made mobile detection SSR-safe by deferring render until detection settles; centralized expanded-commit logic; added keyboard resizing and ARIA to the divider; tests refactored to use a matchMedia mock with a Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Divider as "Divider (UI)"
participant SplitPane as "SplitPane Component"
participant Storage as "localStorage"
participant MatchMedia as "window.matchMedia"
User->>Divider: drag / click / key press
Divider->>SplitPane: onMouseDown / onMouseMove / onMouseUp / onKeyDown / onClick
SplitPane->>SplitPane: compute nextPercent (clamp to min/max)
alt drag with movement
SplitPane->>SplitPane: set livePercent, lastNonCollapsed
SplitPane->>Storage: write expanded percent (clamped, not 0/100)
else click without movement
SplitPane->>SplitPane: do not commit or persist
end
Note over MatchMedia,SplitPane: mobile detection on mount
MatchMedia->>SplitPane: reports match -> isMobile (boolean)
SplitPane->>SplitPane: render stacked layout if mobile
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Review: SplitPane a11y, logic bugs, and test coverage (follow-up to #507)All 8 issues from the previous review are addressed. The test reorganization is a genuine improvement — nested 1.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@web-ui/src/components/sessions/SplitPane.tsx`:
- Around line 227-231: The separator currently always advertises
aria-valuemin={minPanePercent} and aria-valuemax={100 - minPanePercent}, which
is invalid when splitPct is collapsed (0 or 100); update the computed ARIA range
around the separator in SplitPane so it reflects the actual collapsed state: if
splitPct <= minPanePercent set aria-valuemin=0 and aria-valuemax=minPanePercent,
if splitPct >= 100 - minPanePercent set aria-valuemin=100 - minPanePercent and
aria-valuemax=100, otherwise keep aria-valuemin=minPanePercent and
aria-valuemax=100 - minPanePercent; keep aria-valuenow={splitPct} and retain
role="separator" and aria-orientation="vertical".
- Around line 137-152: Create a single helper (e.g., commitExpandedSplit) and
use it from both the keyboard handler and the drag mouse-up commit so both paths
update the same state; the helper should take the new percent (next), set
transitionEnabled.current = false, update livePercent.current and
lastNonCollapsed.current to next, clear collapsed flags via
setIsLeftCollapsed(false) and setIsRightCollapsed(false), call setSplitPct(next)
and persist with writeStorage(storageKey, next); then replace the inline commits
in onDividerKeyDown and the onMouseUp drag commit to call this helper.
🪄 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: 2fdf76c6-c351-4595-b164-992540df2634
📒 Files selected for processing (2)
web-ui/src/__tests__/components/sessions/SplitPane.test.tsxweb-ui/src/components/sessions/SplitPane.tsx
- Extract commitExpandedSplit helper shared by drag and keyboard paths - Fix aria-valuemin/aria-valuemax to reflect actual collapsed state range
Follow-up review (second commit)The second commit (
Reviewing the current state against my earlier comment: items 2–4 are addressed or already verified, but item 1 is still open. Outstanding: nested interactive children inside The divider is Everything else is solid: localStorage collapse suppression, |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web-ui/src/components/sessions/SplitPane.tsx (2)
128-147:⚠️ Potential issue | 🟠 MajorSkip the mouse-up commit when the divider never moved.
After a pane is collapsed,
livePercent.currentstill contains the old expanded width. BecauseonMouseUpalways commits it, a plain click on the divider reopens the pane even though no drag happened.🖱️ Suggested fix
const onMouseUp = () => { if (!isDragging.current) return; isDragging.current = false; + if (livePercent.current === splitPct) return; commitExpandedSplit(livePercent.current); }; @@ - }, [minPanePercent, commitExpandedSplit]); + }, [minPanePercent, splitPct, commitExpandedSplit]); const onDividerMouseDown = () => { isDragging.current = true; + livePercent.current = splitPct; transitionEnabled.current = false; if (leftPaneRef.current) leftPaneRef.current.style.transition = ''; if (rightPaneRef.current) rightPaneRef.current.style.transition = ''; };Also applies to: 140-140
🤖 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 128 - 147, The mouse-up commit should be skipped when the divider wasn't actually moved: add a ref like dragMoved/current or startPercent/current and initialize it in onDividerMouseDown (e.g., set dragMoved.current = false and record startPercent = livePercent.current); in the onMouseMove handler set dragMoved.current = true (or set it when Math.abs(livePercent.current - startPercent.current) > a small threshold) and in onMouseUp only call commitExpandedSplit(livePercent.current) if dragMoved.current is true; update references to isDragging.current, onDividerMouseDown, onMouseMove, and onMouseUp accordingly so a plain click does not reopen a collapsed pane.
237-238:⚠️ Potential issue | 🟡 MinorOnly widen the separator’s ARIA range for the true collapsed states.
<=/>=flips the announced range as soon as the pane hitsminPanePercentor100 - minPanePercent, even though ArrowLeft/ArrowRight are still clamped there. Key this offsplitPct === 0/splitPct === 100(or the collapsed flags) instead.♿ Suggested fix
- aria-valuemin={splitPct <= minPanePercent ? 0 : minPanePercent} - aria-valuemax={splitPct >= 100 - minPanePercent ? 100 : 100 - minPanePercent} + aria-valuemin={splitPct === 0 ? 0 : minPanePercent} + aria-valuemax={splitPct === 100 ? 100 : 100 - minPanePercent}🤖 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 237 - 238, The ARIA min/max logic currently uses <= / >= with splitPct which flips the announced range prematurely; update the aria-valuemin and aria-valuemax expressions to only widen the range when the pane is truly collapsed by checking splitPct === 0 and splitPct === 100 (or the existing collapsed flags if present) instead of using splitPct <= minPanePercent or >= 100 - minPanePercent; adjust the expressions around aria-valuemin, aria-valuemax (and any references to minPanePercent) so they return 0 when splitPct === 0 and 100 when splitPct === 100, otherwise use minPanePercent and 100 - minPanePercent as before.
🤖 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 151-157: The Arrow-key handler onDividerKeyDown incorrectly
reopens a pane when splitPct is at the collapsed sentinels (0 or 100); update
onDividerKeyDown to explicitly handle those sentinels: if splitPct === 0 and
e.key === 'ArrowLeft' return (no-op); if splitPct === 100 and e.key ===
'ArrowRight' return (no-op); otherwise compute delta as before and for inward
restore use commitExpandedSplit(minPanePercent) when left pane is collapsed
inward or commitExpandedSplit(100 - minPanePercent) when right pane is collapsed
inward; reference the existing symbols splitPct, minPanePercent,
commitExpandedSplit, clamp and the onDividerKeyDown function to locate where to
add these checks.
---
Duplicate comments:
In `@web-ui/src/components/sessions/SplitPane.tsx`:
- Around line 128-147: The mouse-up commit should be skipped when the divider
wasn't actually moved: add a ref like dragMoved/current or startPercent/current
and initialize it in onDividerMouseDown (e.g., set dragMoved.current = false and
record startPercent = livePercent.current); in the onMouseMove handler set
dragMoved.current = true (or set it when Math.abs(livePercent.current -
startPercent.current) > a small threshold) and in onMouseUp only call
commitExpandedSplit(livePercent.current) if dragMoved.current is true; update
references to isDragging.current, onDividerMouseDown, onMouseMove, and onMouseUp
accordingly so a plain click does not reopen a collapsed pane.
- Around line 237-238: The ARIA min/max logic currently uses <= / >= with
splitPct which flips the announced range prematurely; update the aria-valuemin
and aria-valuemax expressions to only widen the range when the pane is truly
collapsed by checking splitPct === 0 and splitPct === 100 (or the existing
collapsed flags if present) instead of using splitPct <= minPanePercent or >=
100 - minPanePercent; adjust the expressions around aria-valuemin, aria-valuemax
(and any references to minPanePercent) so they return 0 when splitPct === 0 and
100 when splitPct === 100, otherwise use minPanePercent and 100 - minPanePercent
as before.
🪄 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: a01f6a5f-0c21-41d4-bf7c-5bd145868947
📒 Files selected for processing (1)
web-ui/src/components/sessions/SplitPane.tsx
Third-pass reviewThe 1.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/src/components/sessions/SplitPane.tsx (1)
24-35:⚠️ Potential issue | 🟡 MinorClamp stored and fallback widths into the expanded range.
Line 30 drops every out-of-range stored value back to
fallback, and Line 57 then seeds state with that rawdefaultSplit. IfminPanePercentchanges or a caller passes an invaliddefaultSplit, the pane can reopen at the wrong width — or outside the expanded range entirely — while both collapsed flags still read asfalse.Suggested fix
function readStorage(key: string, fallback: number, min: number, max: number): number { + const normalizedFallback = clamp(fallback, min, max); try { const raw = localStorage.getItem(key); if (raw !== null) { - const parsed = parseFloat(raw); - // Only restore valid expanded positions; ignore collapsed values (0/100) - if (!isNaN(parsed) && parsed >= min && parsed <= max) return parsed; + const parsed = Number(raw); + if (Number.isFinite(parsed)) { + if (parsed === 0 || parsed === 100) return normalizedFallback; + return clamp(parsed, min, max); + } } } catch { // localStorage unavailable (SSR, private browsing, etc.) } - return fallback; + return normalizedFallback; }Also applies to: 56-58
🤖 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 stored or fallback split can exceed the allowed expanded range; update readStorage to clamp both the parsed stored value and the fallback into the provided min..max before returning (i.e., if parsed is valid return Math.min(max, Math.max(min, parsed)), and if using the fallback return the clamped fallback). Also clamp wherever the raw defaultSplit is used to seed state in the SplitPane component (the defaultSplit/state initialization) so that changes to minPanePercent or an invalid defaultSplit cannot open the pane outside the expanded range.
🧹 Nitpick comments (1)
web-ui/src/__tests__/components/sessions/SplitPane.test.tsx (1)
125-130: Add a stale-width clamp case here.This only proves the collapsed-sentinel path (
0/100). It still won't catch the reload-drift case where a stored10or90should reopen atminPanePercent/100 - minPanePercentafter the bounds change. A focused10 -> 15and90 -> 85assertion would cover the missing branch.🤖 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 125 - 130, Extend the test in SplitPane.test.tsx to also cover the stale-width clamp branch: set localStorageMock.setItem('split-pane-position', '10') and render via renderSplitPane with minPanePercent set to 15 (and defaultSplit as appropriate) and assert the left pane width becomes '15%'; repeat symmetrically with stored '90' and minPanePercent 15 to assert the left pane width becomes '85%'. Use the same test harness (renderSplitPane, screen.getByTestId('split-pane-left')) so these assertions exercise the reload-drift clamping code path in the SplitPane component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web-ui/src/components/sessions/SplitPane.tsx`:
- Around line 24-35: The stored or fallback split can exceed the allowed
expanded range; update readStorage to clamp both the parsed stored value and the
fallback into the provided min..max before returning (i.e., if parsed is valid
return Math.min(max, Math.max(min, parsed)), and if using the fallback return
the clamped fallback). Also clamp wherever the raw defaultSplit is used to seed
state in the SplitPane component (the defaultSplit/state initialization) so that
changes to minPanePercent or an invalid defaultSplit cannot open the pane
outside the expanded range.
---
Nitpick comments:
In `@web-ui/src/__tests__/components/sessions/SplitPane.test.tsx`:
- Around line 125-130: Extend the test in SplitPane.test.tsx to also cover the
stale-width clamp branch: set localStorageMock.setItem('split-pane-position',
'10') and render via renderSplitPane with minPanePercent set to 15 (and
defaultSplit as appropriate) and assert the left pane width becomes '15%';
repeat symmetrically with stored '90' and minPanePercent 15 to assert the left
pane width becomes '85%'. Use the same test harness (renderSplitPane,
screen.getByTestId('split-pane-left')) so these assertions exercise the
reload-drift clamping code path in the SplitPane component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c7ad836-e14f-428f-afd9-0fed0b6fb360
📒 Files selected for processing (2)
web-ui/src/__tests__/components/sessions/SplitPane.test.tsxweb-ui/src/components/sessions/SplitPane.tsx
- readStorage: clamp fallback and stored values into [min,max]; reject collapsed sentinels (0/100) explicitly via Number.isFinite path - onDividerMouseDown: reset livePercent to splitPct and dragMoved=false - onMouseUp: only call commitExpandedSplit when dragMoved — plain clicks on divider no longer reopen collapsed panes - ARIA range: use splitPct===0/===100 (not <=/>= minPanePercent) - Tests: stale-width clamp (10→15, 90→85), invalid defaultSplit clamp, plain-click no-op on collapsed pane (37 tests total)
Fourth-pass reviewBuilding on my prior third-pass comment — reviewing the new changes in this commit. Fixed: plain-click bug (item 3 from third pass)The
The corresponding test ( Still open from previous passesItem 1 (low risk): if (isLeftCollapsed || isRightCollapsed) {
setIsLeftCollapsed(false);
setIsRightCollapsed(false);
}Item 2 (informational): Structural concern (raised in first pass, still open): The focusable SummaryThe plain-click fix is the right approach and well-tested. The PR is in good shape for the scope it targets. The nested-interactive-inside-separator structure is the one remaining item worth resolving before merge if accessibility is a hard requirement. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web-ui/src/__tests__/components/sessions/SplitPane.test.tsx (1)
26-29: Keep the newMatchMediaHandleralias in the shared types module.This adds another project-defined type inside
web-ui/src. Move it toweb-ui/src/types/index.tsso the tests stay aligned with the repo’s type-location rule.As per coding guidelines, "TypeScript types must be defined in
web-ui/src/types/index.ts."🤖 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 26 - 29, The new MatchMediaHandler type is defined in the test file; move it into the shared types module (types/index.ts) so it follows the repo rule. Add and export the alias MatchMediaHandler from types/index.ts, remove the local declaration from SplitPane.test.tsx, and import MatchMediaHandler into the test; verify mockMatchMedia still uses the imported MatchMediaHandler and that mockMatchMedia remains unchanged.
🤖 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 152-156: The divider's onDividerMouseDown currently starts a drag
for any mousedown on the divider wrapper, which lets clicks on the collapse
button bubble into the drag path; change onDividerMouseDown to accept the
MouseEvent and return early when the event originated from the collapse button
(e.g. check event.target.closest('button') or a specific collapse-button
class/data attribute) or when event.button !== 0, so button clicks don't begin a
drag. Update the divider wrapper's onMouseDown binding to pass the event to
onDividerMouseDown and keep the existing behavior for real drags
(isDragging.current, dragMoved.current, livePercent.current,
transitionEnabled.current) unchanged.
- Around line 123-141: The mouse drag path currently clamps outward drags from a
fully collapsed pane into the valid range and then commits them on mouseup;
update onMouseMove and onMouseUp so drags that start at or remain at the extreme
edges are treated as no-ops: detect the starting edge collapse
(livePercent/current width at 0 or 100 or container edge hit) and if the current
mouse movement would move further outward (left from 0 or right from 100) do not
update livePercent, do not change leftPaneRef/rightPaneRef widths, and do not
set dragMoved; only set dragMoved and call
commitExpandedSplit(livePercent.current) when the pointer actually moves inward
enough to exit the collapsed state. Keep references to onMouseMove, onMouseUp,
isDragging, dragMoved, livePercent, commitExpandedSplit, clamp, minPanePercent,
containerRef, leftPaneRef and rightPaneRef when implementing and add regression
tests covering both left-collapsed and right-collapsed outward drags.
---
Nitpick comments:
In `@web-ui/src/__tests__/components/sessions/SplitPane.test.tsx`:
- Around line 26-29: The new MatchMediaHandler type is defined in the test file;
move it into the shared types module (types/index.ts) so it follows the repo
rule. Add and export the alias MatchMediaHandler from types/index.ts, remove the
local declaration from SplitPane.test.tsx, and import MatchMediaHandler into the
test; verify mockMatchMedia still uses the imported MatchMediaHandler and that
mockMatchMedia remains unchanged.
🪄 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: f0055c6d-b5f3-4281-bd84-6315b46f5523
📒 Files selected for processing (2)
web-ui/src/__tests__/components/sessions/SplitPane.test.tsxweb-ui/src/components/sessions/SplitPane.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fourth-pass reviewThe latest commit addresses the three open items from my third-pass review and CodeRabbit's duplicate comments:
Still open
The fix is structural: move the collapse buttons outside the separator element. A wrapping This is the one remaining issue I would want resolved before merging if AT support is a stated goal of this PR. Minor (non-blocking)Unbounded localStorage writes on keyboard hold — Everything else is in good shape. The 29-test suite, |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 122-141: The drag path reopens collapsed panes because onMouseMove
always sets dragMoved and livePercent even when the pointer moves further
outward; modify the logic in onMouseMove (and the mousedown handler that starts
dragging) to record the initial mouse X (e.g., dragStartXRef) when isDragging
becomes true and, if splitPct is 0 or 100, treat outward movement as no-op: only
set dragMoved.current = true and update livePercent.current when the pointer
moves inward toward the center beyond a small threshold (e.g., >4px) — keep
using containerRef to compute clamped percent and call
commitExpandedSplit(livePercent.current) on mouseup only when dragMoved.current
is true; leave other behaviors (isDragging, dragMoved reset,
leftPaneRef/rightPaneRef width updates) unchanged.
🪄 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: 4557baae-376c-4dfc-8a96-ac11de195d07
📒 Files selected for processing (1)
web-ui/src/components/sessions/SplitPane.tsx
Moves dragged from a fully-collapsed edge (splitPct 0 or 100) that keep the pointer at or beyond the same edge no longer set dragMoved or update pane widths; only inward movement that crosses back inside the container exits the collapsed state and triggers commitExpandedSplit. Adds four regression tests covering both left- and right-collapsed outward and inward drag paths.
Third-pass reviewFollowing up on my earlier reviews: the CodeRabbit actionable items from the second commit are verified fixed. All core logic (localStorage clamping, lastNonCollapsed guard, commitExpandedSplit shared path, ARIA value ranges) is correct. Still outstanding: nested interactive buttons inside role=separator WAI-ARIA 1.2 specifies that a focusable separator is a widget and must not contain interactive descendants. The current structure has role="separator" tabIndex={0} wrapping the two collapse buttons -- AT will either suppress the buttons or exhibit undefined navigation behavior. Suggested fix: make the resize handle and the collapse controls siblings rather than parent/child. Position the buttons absolutely over the divider area so the visual result is identical, but Tab/Shift-Tab reach the separator and the collapse buttons as independent focusable elements. Everything else in this PR is solid and ready to merge once this structural a11y issue is resolved. |
A focusable separator is a widget and must not contain interactive descendants. Wraps the resize handle and both collapse buttons in a sibling-level track div; role="separator" now has no children so AT reaches all three focusable elements independently. Visual output is unchanged — buttons remain absolutely positioned over the same pixel area. Updates the mobile-hidden test to target the track instead of the separator.
|
Follow-up Review - picking up from my previous comments. Issue 1 role=separator nesting: RESOLVED. The separator is now a self-closing div and the collapse buttons are proper siblings inside split-pane-divider-track. The inline WAI-ARIA 6.8 reference in the comment is a nice touch. Issue 2 Unbounded localStorage writes on keyboard repeat: STILL OPEN. onDividerKeyDown calls commitExpandedSplit (which calls writeStorage) on every keydown, including keyboard repeats. Holding an arrow key fires around 30 events per second, so 30 localStorage writes per second while resizing via keyboard. The fix from my previous review still applies: move the storage write to a keyup handler and remove writeStorage from commitExpandedSplit. Drag-end via mouse already persists from onMouseUp. Issue 3 isMobile null flash: ACKNOWLEDGED. Still the blank-until-detected approach. As noted previously, this is a minor UX tradeoff. No change needed unless a blank flash is observed in practice. Issue 4 aria-label on collapse buttons: CONFIRMED PRESENT. Tests assert the aria-labels and are passing, so the attributes are in the component even though they are not visible as added lines in the diff (pre-existing context). New observation - e.target !== e.currentTarget guard on separator mousedown: Now that the collapse buttons are siblings of the separator (not children), click events from those buttons cannot bubble to the separator mousedown handler anyway. The guard can never fire in practice. Harmless, but could be removed or replaced with a clarifying comment. Summary: The structural a11y fix (issue 1) is correct and the code is clearly better than before. The main open item before merging is issue 2 (keyboard storage debounce). |
1 participant
Follow-up to #507 — addresses all 8 issues from code review.
Fixes
aria-hiddenon divider hid collapse buttons from screen readersrole="separator"+aria-orientation="vertical"lastNonCollapsedcorrupted when cross-collapsing panessplitPctis in valid expanded rangeisMobilecaused hydration flashnulluntilmatchMediaeffect resolvesArrowLeft/ArrowRighton divider, 5% step, clampedreadStorageto valid rangematchMediachange event untestedTest plan
npm test)npm run build)Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests