fix theme picker live preview broken by react-compiler memoization#395
fix theme picker live preview broken by react-compiler memoization#395kevincodex1 merged 2 commits intoGitlawb:mainfrom
Conversation
… preview actually previews
There was a problem hiding this comment.
Pull request overview
This PR fixes the ThemePicker live preview not updating when navigating themes, by removing react-compiler-generated memo caches that were causing stale element reconciliation and by forcing the preview subtree to remount on theme changes.
Changes:
- Rewrote
ThemePickerto idiomatic React using explicituseMemo/useCallbackinstead of react-compiler_cmemo caches. - Added
key={theme}on the preview container to force a remount when the resolved theme changes. - Added local typing helpers for the demo diff preview (
DEMO_PATCH+StructuredDiffViewtyping).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
looks cool! just a question is it possible we add tests to components too so we have robust implementation for the UI and UX? |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head f4a638ae2f99c17e912a711fe06fa07298fc6087.
I didn't find a blocker on the current implementation.
What I verified on this head:
- reviewed the full
ThemePicker.tsxrewrite against currentmain - checked the keybinding/context changes:
useRegisterKeybindingContext('ThemePicker', true)is equivalent to the default active behavior in the hookSelect'sonCancelcontract is synchronous, so the newhandleCancelshape is compatible
- checked that
useTheme()is based on the active preview-resolved theme, sokey={theme}is tied to the live preview palette rather than only the persisted setting bun run build-> successbun run smoke-> success
Residual risk is mostly PR shape rather than a demonstrated bug: this is a much larger rewrite than the minimal key={theme} style fix you might expect for the reported issue, and there is still no automated coverage for the live preview regression itself. But I couldn't verify a current-head correctness problem from the rewrite.
|
Thanks for reviewing, i added ThemePicker.test.tsx with 8 unit tests covering:
Run: bun test src/components/ThemePicker.test.tsx → 8 pass
|
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head e4d6c8c5f2aada2b0895254855898314661b4181.
I still don't see a functional blocker on the current implementation.
What I verified on this head:
- the underlying
ThemePicker.tsxrewrite from the previous head is unchanged bun test src/components/ThemePicker.test.tsx-> 8 passbun run build-> successbun run smoke-> success
One caution on the new test file: it is not strong regression coverage for the actual bug. The test mostly exercises hand-written local callback snippets and option arrays, and it never imports or renders ThemePicker itself. Running it with coverage only reports coverage for ThemePicker.test.tsx, not ThemePicker.tsx, so I am not treating it as proof that the live preview regression is now covered.
That said, I still don't have a verified correctness blocker on the current head, so I think this is mergeable.
|
@Meetpatel006 thanks for this. Looks great and good to be merge. |
…itlawb#395) * fix: remove react-compiler memo cache, restore classical JSX so theme preview actually previews * added themepicker test
Summary
Impact
Testing
Video
Before
pwsh.-.openclaude.-.Visual.Studio.Code.2026-04-05.19-00-46.-.Trim.mp4
After
pwsh.-.openclaude.-.Visual.Studio.Code.2026-04-05.18-59-08.-.Trim.mp4
Notes