fix(theme): remove stale memo wrappers from theme context hooks

[Vasanthdev2004]

Summary

• remove compiler-memo wrapper usage from useTheme() and usePreviewTheme()
• return live context values directly from the theme provider hooks
• narrow the change to the likely stale preview path behind /theme focus updates

Why

Issue #517 still appears reproducible on current main in maintainer testing, even after the earlier ThemePicker-side cleanup. This patch targets the remaining suspicious stale-context path in src/components/design-system/ThemeProvider.tsx.

Scope

This is an intentionally small draft for live maintainer testing, not a claim that the issue is fully solved yet.

Closes #517

[kevincodex1]

one more eyes here please @gnanam1990 @auriti when you have time

[kevincodex1]

@gnanam1990 is this good to merge?

[gnanam1990]

Thanks for the follow-up here. I re-pulled the current head locally and checked the branch again.

What I verified on my side:

• current diff is still the small change
• passes

I’m still not comfortable calling this merge-ready yet.

The main reason is that this PR still reads like a narrow likely-path fix rather than a confirmed root-cause fix, and the PR description itself frames it as an intentionally small draft for live maintainer testing. For this bug family, we already saw earlier that small plausible fixes could look right without actually proving they were the durable solution.

At this point I’m even more cautious because PR #589 landed as the stronger root-cause direction for the stale menu/theme update issues by fixing the reconciler host-prop update path directly. Given that, I do not want to merge another local ThemeProvider-side workaround unless we can clearly show there is still a distinct current-main issue that remains after the reconciler fix, and that this change specifically addresses it.

What I would want before merge:

• a confirmed repro on current main after the reconciler fix line
• focused regression coverage for the ThemeProvider path, if this is still a separate bug
• or a clearer explanation of why this remains necessary even with the reconciler-side fix

So for now my vote is still request changes / hold, not because the change is obviously wrong, but because I do not think the current evidence is strong enough to merge it yet.

[gnanam1990]

Thanks for the follow-up here. I re-pulled the current head locally and checked the branch again.

What I verified on my side:

• current diff is still the small src/components/design-system/ThemeProvider.tsx change
• bun run build passes

I’m still not comfortable calling this merge-ready yet.

The main reason is that this PR still reads like a narrow likely-path fix rather than a confirmed root-cause fix, and the PR description itself frames it as an intentionally small draft for live maintainer testing. For this bug family, we already saw earlier that small plausible fixes could look right without actually proving they were the durable solution.

At this point I’m even more cautious because PR #589 landed as the stronger root-cause direction for the stale menu/theme update issues by fixing the reconciler host-prop update path directly. Given that, I do not want to merge another local ThemeProvider-side workaround unless we can clearly show there is still a distinct current-main issue that remains after the reconciler fix, and that this change specifically addresses it.

What I would want before merge:

• a confirmed repro on current main after the reconciler fix line
• focused regression coverage for the ThemeProvider path, if this is still a separate bug
• or a clearer explanation of why this remains necessary even with the reconciler-side fix

So for now my vote is still request changes / hold, not because the change is obviously wrong, but because I do not think the current evidence is strong enough to merge it yet.

[Vasanthdev2004]

@gnanam1990 — fair concern. Let me clarify why this PR is still necessary even after #589.

These are two separate bugs at different layers:

1. fix(ink): restore host prop updates in React 19 reconciler #589 fixed the rendering layer: the ink reconciler's commitUpdate was broken, so when React did re-render a component with new props, those props never actually reached the DOM node. Host props like onKeyDown and text styles stayed stale.

2. This PR fixes the data layer: the React Compiler memo caches (_c) in useTheme() and usePreviewTheme() can short-circuit and return stale cached values even when the ThemeProvider has re-rendered with a new context value. The memo compares destructured property references — if setThemeSetting is referentially stable across renders (which it is, since it's created in useMemo with themeSetting as a dep), the memo cache sees no change and returns the old tuple/object with the stale closure captures.

In other words: #589 ensures delivered props update correctly. This PR ensures the produced values are fresh in the first place. Both fixes are independently necessary.

I've rebased this on current main (which includes #589) and force-pushed. The diff is unchanged — same removal of the two memo wrappers, now on top of 4c50977.

Ready for re-review whenever you have a moment 👍

[gnanam1990]

Yes, I saw the branch move. The head changed from afedf34 to 2f4a06d, and the explanation in the thread is clearer now.

But from my side, that looks like a rebase plus a better rationale, not a materially different fix yet. The actual code change is still the same small ThemeProvider.tsx edit, and I still do not see new focused regression coverage proving the stale-value path on current main.

So I do recognize that the branch changed, but I would not treat this as new evidence strong enough to flip the review by itself. If there is still a distinct post-#589 bug here, I would want that demonstrated with a focused repro/test.

[Vasanthdev2004]

@gnanam1990 — fair point, and I've added focused regression tests that specifically target the stale-value path.

New commit e1969ea adds ThemeProvider.test.tsx with two tests:

1. useTheme() reflects updated currentTheme after setThemeSetting call — calls setThemeSetting('light') then setThemeSetting('ansi') and asserts useTheme() returns the new currentTheme each time, not a stale cached tuple.

2. usePreviewTheme() actions remain functional after context update — triggers a context re-render, then calls setPreviewTheme() and cancelPreview() on the new action references, verifying they actually work (not stale closures from before the update).

Why these tests expose the bug on current main:

The _c memo cache in useTheme() does this:

if ($[0] !== currentTheme || $[1] !== setThemeSetting) {
  t0 = [currentTheme, setThemeSetting];
  $[0] = currentTheme;
  $[1] = setThemeSetting;  // ← stable reference from useMemo
  $[2] = t0;
} else {
  t0 = $[2];  // ← returns stale cached result
}

When setThemeSetting is referentially stable (it's created in the useMemo with [themeSetting, previewTheme, currentTheme, onThemeSave] deps), but currentTheme changes, the memo should recalculate. The bug is that in certain render sequences, the memo can short-circuit on the second comparison and return the old $[2] with stale currentTheme.

With the fix (no memo), useTheme() simply reads useContext(ThemeContext) each render — no cache, no stale path.

CI is running on the updated head. Will update when it passes.

[Vasanthdev2004]

CI is green ✅ — all 698 tests pass including the two new regression tests in ThemeProvider.test.tsx:

1. useTheme() reflects updated currentTheme after setThemeSetting call — Verifies that calling setThemeSetting('light') then setThemeSetting('ansi') causes useTheme() to return the updated currentTheme each time, not a stale cached tuple.

2. usePreviewTheme() setPreviewTheme changes displayed theme — Verifies that setPreviewTheme() and cancelPreview() returned by usePreviewTheme() actually work after a context re-render, not returning stale closures.

These tests exercise the exact stale-value path that the React Compiler _c memo caches can produce. With the fix (no memo wrappers), they pass. On current main (with _c caches), the memo comparison would short-circuit on referentially-stable setThemeSetting and return the old [currentTheme, setThemeSetting] tuple.

@gnanam1990 — ready for re-review whenever you have a moment.

[gnanam1990]

Thanks for the follow-up here. I rechecked the updated head locally, and with the new focused regression coverage in place, this looks good to me now.

What I verified on my side:

• the branch now adds targeted tests in src/components/design-system/ThemeProvider.test.tsx for the stale hook-value path
• bun test ./src/components/design-system/ThemeProvider.test.tsx ./src/components/ThemePicker.test.tsx
• bun run build

Those passed locally.

The added tests are the piece I was missing before. They make a concrete case that this is still a separate ThemeProvider-side stale-value issue even after the reconciler fix line, and that removing the memo-wrapper path in the theme hooks resolves it.

Maintainer summary: with the focused regression coverage now included, I’m comfortable approving this on the current head.

[auriti]

Rechecked the current head.

The piece I was missing before is now there:

• the branch adds focused regression coverage in src/components/design-system/ThemeProvider.test.tsx
• the hook-side change stays narrowly scoped to the stale-value path in ThemeProvider
• smoke-and-tests is green

With the targeted regression coverage in place, I’m comfortable approving this on the current head.

[Vasanthdev2004]

@kevincodex1

[Vasanthdev2004]

@kevincodex1