Repository navigation
Manager: Improve UI stability on onboarding checklist - #36341
Conversation
Two guards, one per layer. useUniversalStore (manager and preview variants) notified React on every setState in the no-selector path, so a state replaced with a deeply equal copy re-rendered every consumer; the manager variant also overwrote its snapshot ref before notifying, defeating the referential-equality guard in getSnapshot. Both hooks now apply the same deep-equality comparison the selector path always had. Their API is unchanged. The checklist widget registered its transition keys with setItem on every items-array identity change, and setItem resets the transition state of a key it already tracks. Any store update or story index refresh therefore collapsed and faded back in every visible row. Keys are now registered once. The StableAcrossStateSyncs story pins the symptom: it replays a content-identical store sync and asserts that no row changes its class or loses its DOM node. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014gogULoeyEc5aKMdtgS8Mf
The universal store dedupe and the checklist transition guard move to #36341 so this diff carries only the sidebar rebuild. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014gogULoeyEc5aKMdtgS8Mf
ghengeveld
left a comment
There was a problem hiding this comment.
Code wise I'm fine with this, the approach seems sound.
However, looking at this story (wait for it to animate), it seems the transition is broken. It looks like it's already broken like that on next though, so it's not caused by this PR. Nevertheless we should address it. I verified that the issue exists not just in the story but in the actual sidebar as well.
I think it would help to have a story with some dynamic controls so we can play around with the checklist state easily.
When a task completes, its row exits while the next task's row enters. The exiting row never actually collapsed: ActionList items carry a 32px min-height that overrides block-size: 0, so for the length of the exit transition the list held four full-height rows and the widget grew by one row before snapping back. Collapsed items now drop their min-height as well, and min-height is part of the transitioned properties so both rows resize over the same duration. The entering row starts collapsed through @starting-style instead of react-transition-state's preEnter step, which lands the enter and the exit in the same commit and keeps the two heights complementary from the first frame. The StableHeightOnTaskCompletion story records the widget height on every DOM mutation and asserts it never grows across a completion. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014kbAkZeM9HcHMvLjPzK6oG
WalkthroughStore hooks now skip updates when compared states are deeply equal. Checklist transitions register each key once, and ActionList uses shared collapsed and expanded styles. New stories check row stability during state syncs and card height during task completion. ChangesChecklist state and transitions
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
🔇 Additional comments: 7
code/core/src/shared/universal-store/use-universal-store-manager.ts: 1
11-11: LGTM!Also applies to: 48-49
code/core/src/shared/universal-store/use-universal-store-preview.ts: 1
10-11: LGTM!Also applies to: 46-47
code/core/src/shared/universal-store/use-universal-store-manager.test.ts: 1
96-118: LGTM!code/core/src/components/components/ActionList/ActionList.tsx: 1
19-22: LGTM!Also applies to: 80-80, 92-96
code/core/src/manager/components/sidebar/ChecklistWidget.tsx: 1
64-76: 🎯 Functional CorrectnessStale keys do not accumulate for returning tasks in this implementation.
The
allItemspassed touseTransitionArraycontains all checklist items derived from the staticchecklistData.sections. Items never leave this array—they remain present with different computed state flags (completion status, availability, etc.). TherenderItemssubset changes which items are displayed, but item IDs always remain inallItems.The transition state keys are registered when items enter
allItems, which occurs only once per application session for each checklist ID. Since items do not leave and re-enterallItems, the scenario of a previously-removed key returning is not reachable.code/core/src/manager/components/sidebar/ChecklistWidget.stories.tsx: 2
174-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Module-level
cardHeightsis shared state across story renders.The array is cleared only inside
play. The decorator observer pushes heights during mount, so entries recorded beforeplayruns are cleared at Line 196. This is correct for the current single-story use. Reuse of the decorator, or two renders in one page such as docs mode, could mix measurements. Keeping the array in auseRefor in the story context would remove the risk.Also applies to: 196-196
144-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Fixed
wait()delays make these stories slow and timing-dependent.The stories wait 3000 ms twice and 600 ms once. A slow CI runner can fail them or give false passes. The 600 ms wait in
StableAcrossStateSyncsis the weakest. It only proves that no class change happens in that window. PreferwaitForconditions where possible. Otherwise, keep the delays tied to--transition-duration.Also applies to: 165-165, 194-194, 211-211
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
639e6830-096e-480e-a278-637162c07f0d
📒 Files selected for processing: 6
code/core/src/components/components/ActionList/ActionList.tsxcode/core/src/manager/components/sidebar/ChecklistWidget.stories.tsxcode/core/src/manager/components/sidebar/ChecklistWidget.tsxcode/core/src/shared/universal-store/use-universal-store-manager.test.tscode/core/src/shared/universal-store/use-universal-store-manager.tscode/core/src/shared/universal-store/use-universal-store-preview.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
|
The new equality check can leave the UI out of sync because I reproduced this with two stores starting at The same test passes against the base branch and fails against this PR. Could we compare the incoming value with the last snapshot exposed by the hook instead? The preview hook appears to need the same adjustment. |
|
Discussed with the team: we know that concurrent events can be lossy, but we cannot know if the manager or preview's event should be the one that's honoured without building a shared clock system. Considering the lifespan of UniversalStore, we've decided to not pay the price of building such a system to handle this edge case. |
What I did
When working on tree 2.0, my agent stumbled upon an instability in the checklist widget DOM. We are apparently refiring state syncs with mutable data even when the store state has not effectively changed.
This PR adds an equality check to state changes within the underlying store to keep the DOM more stable by not replacing DOM rows with themselves (which triggers Emotion changes).
The PR also fixes a CLS bug where an item that is checked off the list keeps taking 32px of height whilst being animated out, resulting in a temporary state where the checklist shows 4 rows and grows in height.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
cd code && yarn storybook:uiDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🤖 Generated with Claude Code
https://claude.ai/code/session_014kbAkZeM9HcHMvLjPzK6oG