Conversation
Reading a diff is the point of the compare view, and the 280px component list sits next to it the whole time even once the reader has picked what they want to look at. There was no way to get that space back. `CompareSidebar` now collapses to a 36px rail. The chevron in its header toggles it, and the whole rail is a hit target on the way back out. The state is uncontrolled by default and persisted, so every surface that renders the sidebar picks the behaviour up without wiring, and a reader who collapses it finds it collapsed next time. `collapsed` / `onCollapsedChange` are there for a host that wants to drive it instead; a controlled sidebar never writes a preference its owner did not ask for. Two details worth naming: - `usePersistedToggle` reads storage in an effect, not during render. Seeding `useState` from `localStorage` makes the first client render disagree with the server's, which React 18 treats as a hydration failure. Callers get `hydrated` so they can hold back a transition until the stored value has landed — which is what stops a left-collapsed sidebar from visibly sliding shut on every load. - the component list stays mounted while collapsed and is hidden in CSS, so a collapse/expand round trip does not throw away every expanded file tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoAdd a persistent collapsible compare sidebar
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Sidebar slides when its scope changes
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
The correction ran in a passive effect, so a sidebar the reader had left collapsed rendered once at full width and only snapped shut after the browser had already painted that frame. Moving it to a layout effect makes it land in the same frame. Still not a render-time read: initialising state from `localStorage` would make the first client render disagree with the server's, which is a hydration failure. Rendering the default and correcting it before paint keeps both renders identical and costs the reader nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit aa7000f |
…erly Two defects Qodo caught in review. The restored width and the transition class landed in the same commit, so the browser saw a width change on a newly-transitionable element and animated 280px → 36px on every reload — the exact "slides shut on load" the layout effect was meant to prevent. Transitions now turn on a frame later, once the restored width is already in place, so only real toggles animate. `usePersistedToggle` also only replaced its value when the new key held a valid boolean, so moving a mounted caller to a scope with nothing stored left the previous scope's preference on screen. It now tracks which key produced the current value and falls back to the default for any other. Both are covered by specs that fail without the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment on lines
+282
to
+283
| if (!ready || animated) return undefined; | ||
| const frame = requestAnimationFrame(() => setAnimated(true)); |
There was a problem hiding this comment.
1. Sidebar slides when its scope changes 🐞 Bug ≡ Correctness
useAnimateAfter leaves animated true when hydrated returns to false, so changing collapseStorageKey does not disarm the width transition before the hook restores the new key. After the first scope has armed animation, switching a mounted sidebar to a scope whose value differs makes the 280px/36px restoration visibly slide even though persisted-state hydration is intended to happen without a transition.
Agent Prompt
## Issue description
The animation remains armed when persistence hydration restarts for a different storage key, causing the restored width to transition visibly.
## Fix Focus Areas
- components/ui/component-compare/component-compare/compare-sidebar.tsx[57-58]
- components/ui/component-compare/component-compare/compare-sidebar.tsx[278-287]
- components/ui/component-compare/component-compare/compare-sidebar.spec.tsx[79-87]
## Recommended Fix
Tie the armed animation state to the current storage key or hydration cycle so a key change synchronously disables transitions, then re-enable them on a later animation frame after the new stored value has committed. Extend the key-change test to verify that the animation class is absent during restoration and only appears afterward.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 392c525 |
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.
What
CompareSidebarcan now collapse to a 36px rail, and remembers whether the reader left it that way.Reading a diff is the point of the compare view, but the 280px component list sits next to it the whole time — including after the reader has already picked what they want to look at. There was no way to get that space back.
API
Uncontrolled and persisted by default, so every surface that renders the sidebar (today: lane compare) picks the behaviour up with no wiring:
For a host that wants to drive it instead:
A controlled sidebar never writes a preference its owner did not ask to persist.
Two details worth naming
usePersistedTogglereads storage in an effect, not during render. SeedinguseStatefromlocalStoragemakes the first client render disagree with the server's, which React 18 treats as a hydration failure. The hook applies the stored value on the first commit and returnshydrated, which the sidebar uses to hold back its width transition — that is what stops a left-collapsed sidebar from visibly sliding shut on every page load. Storage failures (private mode, quota) degrade to an in-memory toggle rather than breaking the surface. The hook is exported; it is not sidebar-specific.The component list stays mounted while collapsed and is hidden in CSS, so a collapse/expand round trip does not throw away every expanded file tree.
Testing
Six specs in
compare-sidebar.spec.tsxcovering the default, the toggle, the persisted round trip, key scoping, the mounted-while-collapsed guarantee, and the controlled path (including that it does not write storage).oxlint --deny-warningsclean on the component.🤖 Generated with Claude Code