Review 5075

PR

#5075 fix(SideNav): give collapse a single owner across collapsible and resizable by cixzhang (bucket: the maintainer)

HEAD REVIEWED

cdc12ee416b0d7b9d6456d02b4ba787b186fa0b4 <- every claim below was verified at this commit

VERSIONS

LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13

LANE: full WHY: Four runtime decisions, public API and behavior changes, persistence, and a visible endpoint require more than one decisive check.

PROBLEM

WHY 1: SideNav can restore a different collapse state from its resize owner, and a pointer gesture can retain the collapse value captured at pointerdown. WHY 2: One path makes the nav invisible after reload; the other repeats collapse notifications and ignores a drag back toward expanded. WHY 3: People lose navigation or cannot recover the panel, while builders cannot rely on collapse callbacks to describe real transitions.

USER-FACING PROBLEM: Collapse can leave a persistent SideNav invisible after reload or leave a direct resizable panel stuck after the user reverses a drag. PROBLEM SEVERITY: broken task — navigation or panel recovery can become unavailable through the UI.

Main reproduced a 0px nav with “Collapse sidebar”; this head restores a 48px rail with “Expand sidebar.”

VERDICT: clear

SOLUTION

SideNav and the resize behavior now share one collapse owner instead of initializing competing booleans. The resize hook retains expanded width separately from collapsed state, restores both from storage, and uses live state for imperative and drag transitions. SideNav normalizes its two public configurations into that owner and warns when both try to control it.

SOLUTION (4 decisions · 239 runtime additions of 1,123 additions)

1. Normalize collapsible and resizable into one owner, backed by the standard controlled/uncontrolled pair. [the #4790 root fix]
2. Persist {size, isCollapsed}, retain expanded size, and migrate number/zero entries. [the #4790 reload fix]
3. Warn when both props supply collapse state and let resizable win. [the stated builder-conflict problem]
4. Read live collapse state for imperative/drag callbacks and normalize notification counts. [the stated duplicate/stale-callback problems]

BURDEN: high — public surface, a storage migration, callback semantics, and 766 added test lines. BURDEN MATCH: proportionate — it removes a broken navigation state and two independently reproduced callback failures; all four decisions and their risks are stated.

VERDICT: clear

ARCHITECTURE

OWNER: useResizable owns resize/collapse lifecycle; SideNav adapts its two configs into that owner. TIER 1: useResizable, SideNavCollapseContext, and useDevWarning are reused. TIER 2: size ownership stays in useResizable; no new system. SEAMS: direct hook, SideNav with either/both props, persisted reload, out-of-tree collapse button, and pointerdown-captured ResizeHandle props. BEHAVIOR UNIT: useResizable — named public hook with focused tests for state, persistence, current-commit timing, and the held-pointer gesture.

Driven: main/head persisted reload 0px→48px; held-pointer collapse/return stayed collapsed on main and re-expanded to 240px here. The final head also passes the controlled current-commit timing repro and 28 focused hook tests.

VERDICT: clear

IMPACT

Existing persistent SideNav users recover the 48px rail instead of losing navigation; direct hook consumers get controlled collapse and retained width. Existing onCollapseChange consumers get one notification per real transition, and a reversed drag re-expands during the same gesture. Current controlled state is available immediately to imperative callers in the same commit.

VERDICT: clear

API

const region = useResizable({
  collapsible: true,
  isCollapsed,
  onCollapseChange: setIsCollapsed,
});

	change	public?	class	doc'd?	verdict
+	ResizableConfig.defaultIsCollapsed?: boolean = false	yes — Resizable subpath and SideNav.resizable	SideNavCollapsibleConfig, SideNavItem, ChatComposerDrawer	JSDoc + component docs	ok
+	ResizableConfig.isCollapsed?: boolean	yes	same controlled-state class	JSDoc + component docs	ok
+	ResizableConfig.onCollapseChange?: (isCollapsed: boolean) => void	yes	existing useResizable callback vocabulary	JSDoc + component docs	ok — explicitly settled in #5073
+	UseResizableSingleConfig.defaultIsCollapsed?: boolean	yes — Resizable subpath	same class	JSDoc + hook docs	ok
+	UseResizableSingleConfig.isCollapsed?: boolean	yes	same class	JSDoc + hook docs	ok
~	autoSaveId persists {size, isCollapsed} and migrates legacy numbers	yes — behavioral meaning	existing persistence contract	docs + tests	intentional

Ossification: controlled collapse is an established class, and the maintainer-authored #5073 explicitly chooses the existing useResizable.onCollapseChange vocabulary. New fields are optional; the class, owner, and spelling are already decided.

VERDICT: clear

THEMING

No theme target, token, CSS variable, StyleX declaration, style-driving prop, or class changes. The persisted, ordinary user-triggered, and resize-owned collapsed rails produced byte-identical 48×480 PNGs (924fce…eda2).

VERDICT: clear

BREAKING

• API — no existing call stops compiling; all fields are optional.
• Visual — yes, intentionally: the broken persisted state changes from a 0px invisible nav to the existing 48px collapsed rail; all three valid rail paths are byte-identical.
• Theme — no target, selector, variable, or override changes.
• Behaviour — yes, intentionally: duplicate collapse notifications are removed, resize() out of collapse reports false, and expand() is silent when already expanded. The PR body and patch changeset record these corrections; no codemod is needed.

VERDICT: note — intentional, documented behavior corrections on released core@0.5.0 surface.

PERFORMANCE & RESOURCES

Effect + deps	external system	why render/handler cannot do it	state/render cost	lifetime + cleanup	focused test
~ useEffect [size,isCollapsed,autoSaveId] useResizable.ts:303	localStorage	persistence is external	same render count and one storage write per changed state as main	effect-scoped; no listener/layout	persistence suite
+ useDevWarning SideNav.tsx:425	developer console	warning must run outside render; canonical hook	no state/render; one guarded warning per mount	no listener/layout	conflict-warning suite

The transient ref mirror now follows the landed render-ref pattern instead of adding a passive Effect. Profiler commits remain two across mount+collapse at N=1/3/20; no listener, observer, layout read, or dependency was added.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: the acceptance claim changes a visible persisted endpoint; the new story had no legacy-storage baseline.

frame	build/story	theme · mode · dir	viewport/media	semantic state	geometry/errors
before	origin/main production sources + PR story; exact two-file arm diff inspected	neutral · light · LTR	900×600@1; normal	legacy 0 reload	sensors pass; nav 0px, Collapse action
after	cdc12ee416b0…; same story	neutral · light · LTR	same	same seed	sensors pass; nav 48px, Expand action

Before artifact: PR 5075 before: persisted collapse reload
After artifact: PR 5075 after: persisted collapse reload

The only changed endpoint is the PR’s stated fix: “a persisted-collapsed nav renders the collapsed rail instead of an invisible zero-width one.” The persisted, ordinary user-triggered, and resize-owner handoff rails were recaptured at this head with receipts and are byte-identical (924fcecc27f30f09a9bd85a337f8729b6a1cce5d0bfc946289c6cb5b617aeda2).

VERDICT: clear

A11Y & I18N

Auto-covered: pr-a11y and pr-rtl were green on the immediately preceding functional head; the final two-line fix changes only ref timing and adds a unit test, while final CI reruns. Checked in Chromium: the nav has an accessible name and the collapsed control is named “Expand sidebar.” No shipped visible/AT string, direction logic, role, or ARIA attribute changed.

VERDICT: clear — no accessibility or i18n regression attributable to the diff.

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	clear
ARCHITECTURE	clear
IMPACT	clear
API	clear
THEMING	clear
BREAKING	note — intentional behavior corrections
PERFORMANCE	clear
VISUAL EVIDENCE	clear
A11Y & I18N	clear

PRIOR REVIEW: no submitted reviews or inline comments exist. The maintainer’s direction comment says to keep the single-owner solution and finish visual acceptance; this review preserves that direction and verifies the implementation.

JUDGEMENT NEEDED: none — #5073 records the maintainer’s API choice, and every remaining axis is mechanically settled.

GOAL: met — main reproduced a 0px unreachable nav; this head restores the 48px rail, all three collapse-owner paths are byte-identical, and the held-pointer drag re-expands to 240px.

DISPOSITION: no negative findings remain. The controlled current-commit defect found in the rewrite was fixed at this head; its regression test and all 28 focused hook tests pass.

ADVICE: omitted — no unresolved defect remains.

AUTHOR CAN PROCEED: yes — all review-owned acceptance criteria pass; only the rerunning repository checks remain.

WORST OUTCOME: none found → approve and merge

approve and merge

REVIEW

Thanks, this is good. The single-owner, persistence, held-pointer, and controlled-timing paths all check out.

INLINE (0-3, only if genuinely line-anchored)

None.

EVIDENCE I DID NOT SPEND

• A SideNavCollapsibleConfig-shaped variable with onCollapsedChange structurally type-checks under resizable and no-ops, but #5073 explicitly settles the new config on existing useResizable.onCollapseChange; this is misuse, not a finding.
• Keyboard collapse drops focus to <body> on both main and this head; it is pre-existing, not this PR’s debt.
• Contributor #4824 and #5118 remain open; #5075 contains and credits their persistence and live-state work, so the team-level solution takes precedence.

TIME

TIME total 44m setup 6m rules + worktree + fast install + one build + Storybook dev on port 6437 (warm main reused: yes) reading 11m body, prior comments, full diff, repo/wiki rules, competing PRs measuring 19m targeted tests, candidate-blocker repros, fixed-head reruns, main/head browser probes, three exact-head recaptures writing 8m draft + critic self-grade + rewrite + rewrite critique + final revision waste 3m wrong bare-repo worktree invocation, wrong initial PR search term, one self-clearing storage probe, and blocked image-upload attempts

WHAT I COULD NOT VERIFY

• WebKit/Firefox: unavailable on this managed Mac; no engine-specific code changed.
• Final full CI is still rerunning after the last push; targeted suites pass locally.

MERGE: auto

What changed before posting

The public comment uses the approved REVIEW text, then links this full record and appends the attribution marker.