Skip to content

fix(ink): restore host prop updates in React 19 reconciler#589

Merged
kevincodex1 merged 1 commit into
Gitlawb:mainfrom
catgirl3d:fix/ink-host-prop-updates
Apr 11, 2026
Merged

fix(ink): restore host prop updates in React 19 reconciler#589
kevincodex1 merged 1 commit into
Gitlawb:mainfrom
catgirl3d:fix/ink-host-prop-updates

Conversation

@catgirl3d

Copy link
Copy Markdown
Contributor

It seems we finally found the real root cause. Problem wasn't in Box or Text, but in src/ink/reconciler.ts: after migrating to React 19 / react-reconciler@0.33 our commitUpdate was broken, so mounted ink-* nodes didn't update onKeyDown, tabIndex and textStyles in-place. That's why the previous fixes acted as workarounds via remounting/avoiding stale handlers. This is now fixed in the reconciler, low-level regression tests have been added, and the bug reproducibly disappears without those workarounds

Summary

  • Fix the real root cause of stale TUI menu handlers/highlights in the Ink reconciler.
  • Restore in-place host prop updates for ink-* nodes under React 19 / react-reconciler@0.33.
  • Add a regression test that fails when ink-box handlers/attributes or ink-text styles stop updating on rerender.

What we found

The menu bugs in /agents, /config, and related screens looked like focus or repaint issues, but the shared failure mode was deeper: already-mounted ink-* host nodes were not receiving updated props reliably.

Minimal raw host-node probes showed that before the fix:

  • ink-box kept stale onKeyDown handlers across rerenders
  • ink-box could retain stale tabIndex
  • ink-text could retain stale textStyles

The root cause was in src/ink/reconciler.ts: our commitUpdate implementation still expected the older prepareUpdate/updatePayload shape, while react-reconciler@0.33.0 mutation mode calls commitUpdate(instance, type, oldProps, newProps, fiber).

That mismatch broke in-place host prop updates and made remount-based workarounds in Box.tsx and Text.tsx appear necessary.

Implementation

  • Update src/ink/reconciler.ts so commitUpdate diffs oldProps and newProps directly in the React 19 mutation path instead of expecting an updatePayload.
  • Keep applying changed attributes, event handlers, textStyles, and layout styles through the normal host update path.
  • Add src/ink/reconciler.test.ts with low-level regression coverage for:
    • ink-box updating tabIndex and onKeyDown in place on rerender
    • ink-text updating textStyles in place on rerender

Notes

  • Earlier fixes in Box.tsx and Text.tsx were valid workarounds, but not the root fix
  • The reconciler change is the key behavioral fix; the new test is intended to prevent this exact API mismatch from slipping back in

Testing

  • bun test src/ink/reconciler.test.ts
  • bun test src/components/InteractiveMenuRegression.test.tsx src/components/ThemePicker.test.tsx
  • bun run build

before:
image

after:

bandicam.2026-04-11.01-51-25-796.mp4

React 19's react-reconciler@0.33 mutation path calls commitUpdate with
(instance, type, oldProps, newProps, fiber), but our Ink host config
still expected an updatePayload from prepareUpdate. That left mounted
ink-* nodes with stale onKeyDown, tabIndex, and textStyles, making menu
navigation and highlights appear stuck until remount.

Diff old/new props directly inside commitUpdate and add regression tests
covering in-place updates for ink-box handlers/attributes and ink-text
styles.

@kevincodex1 kevincodex1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kevincodex1

Copy link
Copy Markdown
Contributor

thank you for this @catgirl3d !

@gnanam1990 gnanam1990 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging deeper on this. I pulled the current head locally and this looks like the right fix direction to me.

What I verified:

  • the change now targets the actual shared root cause in src/ink/reconciler.ts instead of relying on remount-based workarounds in Box/Text
  • commitUpdate now restores in-place host prop updates for ink-* nodes on the React 19 / react-reconciler@0.33 path
  • the new low-level regression coverage in src/ink/reconciler.test.ts directly checks the important failure modes:
    • ink-box updates onKeyDown and tabIndex in place across rerenders
    • ink-text updates textStyles in place across rerenders
    • handler removal and layout-style updates also behave correctly
  • the behavior-level symptom checks also pass through the higher-level tests

Local verification on my side:

  • bun test ./src/ink/reconciler.test.ts ./src/components/InteractiveMenuRegression.test.tsx ./src/components/ThemePicker.test.tsx
  • bun run build

Both passed.

Maintainer summary: this is a stronger and more principled fix than the earlier workaround-style approaches in the same bug family, and I do not see a blocker on the current head.

@kevincodex1 kevincodex1 merged commit 6e94dd9 into Gitlawb:main Apr 11, 2026
1 check passed
Vasanthdev2004 pushed a commit that referenced this pull request Apr 12, 2026
Rebase on current main (includes #589 reconciler fix).

The React Compiler memo caches (_c) in useTheme() and usePreviewTheme()
use referential equality checks on destructured context values. These
caches can return stale references when the ThemeProvider's useMemo
recreates the context value object but the individual property
references (setThemeSetting, setPreviewTheme, etc.) compare equal —
the memo short-circuits and returns a cached tuple/object that still
holds the old closure captures.

This is a distinct bug from #589 (which fixed the ink reconciler's
commitUpdate path for host prop updates). #589 ensures that when
React _does_ re-render a component with new props, those props actually
reach the DOM node. But the memo wrappers here prevent React from
_even seeing_ the new context value in the first place — the hook
returns the stale cached result.

Removing the memo wrappers ensures useTheme() and usePreviewTheme()
always read the current context value, eliminating the stale-reference
path entirely.
euxaristia pushed a commit to euxaristia/openclaude that referenced this pull request Apr 13, 2026
React 19's react-reconciler@0.33 mutation path calls commitUpdate with
(instance, type, oldProps, newProps, fiber), but our Ink host config
still expected an updatePayload from prepareUpdate. That left mounted
ink-* nodes with stale onKeyDown, tabIndex, and textStyles, making menu
navigation and highlights appear stuck until remount.

Diff old/new props directly inside commitUpdate and add regression tests
covering in-place updates for ink-box handlers/attributes and ink-text
styles.
Franzferdinan51 pushed a commit to Franzferdinan51/DuckHive that referenced this pull request Apr 22, 2026
Rebase on current main (includes Gitlawb#589 reconciler fix).

The React Compiler memo caches (_c) in useTheme() and usePreviewTheme()
use referential equality checks on destructured context values. These
caches can return stale references when the ThemeProvider's useMemo
recreates the context value object but the individual property
references (setThemeSetting, setPreviewTheme, etc.) compare equal —
the memo short-circuits and returns a cached tuple/object that still
holds the old closure captures.

This is a distinct bug from Gitlawb#589 (which fixed the ink reconciler's
commitUpdate path for host prop updates). Gitlawb#589 ensures that when
React _does_ re-render a component with new props, those props actually
reach the DOM node. But the memo wrappers here prevent React from
_even seeing_ the new context value in the first place — the hook
returns the stale cached result.

Removing the memo wrappers ensures useTheme() and usePreviewTheme()
always read the current context value, eliminating the stale-reference
path entirely.
C1ph3r404 pushed a commit to C1ph3r404/openclaude that referenced this pull request Apr 29, 2026
React 19's react-reconciler@0.33 mutation path calls commitUpdate with
(instance, type, oldProps, newProps, fiber), but our Ink host config
still expected an updatePayload from prepareUpdate. That left mounted
ink-* nodes with stale onKeyDown, tabIndex, and textStyles, making menu
navigation and highlights appear stuck until remount.

Diff old/new props directly inside commitUpdate and add regression tests
covering in-place updates for ink-box handlers/attributes and ink-text
styles.
kevincodex1 pushed a commit that referenced this pull request May 5, 2026
* fix(theme): remove stale React Compiler memo wrappers from theme hooks

Rebase on current main (includes #589 reconciler fix).

The React Compiler memo caches (_c) in useTheme() and usePreviewTheme()
use referential equality checks on destructured context values. These
caches can return stale references when the ThemeProvider's useMemo
recreates the context value object but the individual property
references (setThemeSetting, setPreviewTheme, etc.) compare equal —
the memo short-circuits and returns a cached tuple/object that still
holds the old closure captures.

This is a distinct bug from #589 (which fixed the ink reconciler's
commitUpdate path for host prop updates). #589 ensures that when
React _does_ re-render a component with new props, those props actually
reach the DOM node. But the memo wrappers here prevent React from
_even seeing_ the new context value in the first place — the hook
returns the stale cached result.

Removing the memo wrappers ensures useTheme() and usePreviewTheme()
always read the current context value, eliminating the stale-reference
path entirely.

* test(theme): add regression tests for useTheme()/usePreviewTheme() stale-value bug

These tests verify that context hooks always return fresh values after
ThemeProvider re-renders, even when React Compiler memo caches are in play.

- useTheme() must reflect currentTheme changes immediately after
  setThemeSetting is called (not return a stale cached tuple).
- usePreviewTheme() must return functional actions after context
  re-renders (not stale closures from before the theme change).

On current main (with _c memo wrappers), these tests expose the bug:
the memo cache compares setThemeSetting by reference (stable across
renders via useMemo) and short-circuits, returning the old cached result
with stale currentTheme.

* fix(test): correct import paths for ThemeProvider.test.tsx

Fix relative paths for ink.js, KeybindingSetup, AppStateProvider,
useStdin mock, systemTheme mock, and config mock to account for
the test file being in src/components/design-system/ rather than
src/components/.

* fix(test): rewrite ThemeProvider tests using Ink renderer

Use Ink's createRoot instead of react-dom/client, matching the pattern
from ThemePicker.test.tsx. The tests now render through Ink's terminal
renderer and check frame output for theme values, which is the same
environment ThemeProvider actually runs in.

* fix(test): correct all relative import paths for design-system/ depth

- ink.js, KeybindingSetup, AppStateProvider: ../ → ../../
- StructuredDiff: same pattern as ThemePicker test adjusted for depth

---------

Co-authored-by: root <root@vm7508.lumadock.com>
hotmanxp pushed a commit to hotmanxp/openclaude that referenced this pull request May 6, 2026
…awb#534)

* fix(theme): remove stale React Compiler memo wrappers from theme hooks

Rebase on current main (includes Gitlawb#589 reconciler fix).

The React Compiler memo caches (_c) in useTheme() and usePreviewTheme()
use referential equality checks on destructured context values. These
caches can return stale references when the ThemeProvider's useMemo
recreates the context value object but the individual property
references (setThemeSetting, setPreviewTheme, etc.) compare equal —
the memo short-circuits and returns a cached tuple/object that still
holds the old closure captures.

This is a distinct bug from Gitlawb#589 (which fixed the ink reconciler's
commitUpdate path for host prop updates). Gitlawb#589 ensures that when
React _does_ re-render a component with new props, those props actually
reach the DOM node. But the memo wrappers here prevent React from
_even seeing_ the new context value in the first place — the hook
returns the stale cached result.

Removing the memo wrappers ensures useTheme() and usePreviewTheme()
always read the current context value, eliminating the stale-reference
path entirely.

* test(theme): add regression tests for useTheme()/usePreviewTheme() stale-value bug

These tests verify that context hooks always return fresh values after
ThemeProvider re-renders, even when React Compiler memo caches are in play.

- useTheme() must reflect currentTheme changes immediately after
  setThemeSetting is called (not return a stale cached tuple).
- usePreviewTheme() must return functional actions after context
  re-renders (not stale closures from before the theme change).

On current main (with _c memo wrappers), these tests expose the bug:
the memo cache compares setThemeSetting by reference (stable across
renders via useMemo) and short-circuits, returning the old cached result
with stale currentTheme.

* fix(test): correct import paths for ThemeProvider.test.tsx

Fix relative paths for ink.js, KeybindingSetup, AppStateProvider,
useStdin mock, systemTheme mock, and config mock to account for
the test file being in src/components/design-system/ rather than
src/components/.

* fix(test): rewrite ThemeProvider tests using Ink renderer

Use Ink's createRoot instead of react-dom/client, matching the pattern
from ThemePicker.test.tsx. The tests now render through Ink's terminal
renderer and check frame output for theme values, which is the same
environment ThemeProvider actually runs in.

* fix(test): correct all relative import paths for design-system/ depth

- ink.js, KeybindingSetup, AppStateProvider: ../ → ../../
- StructuredDiff: same pattern as ThemePicker test adjusted for depth

---------

Co-authored-by: root <root@vm7508.lumadock.com>
The-FOOL-00 pushed a commit to The-FOOL-00/openclaude that referenced this pull request May 24, 2026
React 19's react-reconciler@0.33 mutation path calls commitUpdate with
(instance, type, oldProps, newProps, fiber), but our Ink host config
still expected an updatePayload from prepareUpdate. That left mounted
ink-* nodes with stale onKeyDown, tabIndex, and textStyles, making menu
navigation and highlights appear stuck until remount.

Diff old/new props directly inside commitUpdate and add regression tests
covering in-place updates for ink-box handlers/attributes and ink-text
styles.
The-FOOL-00 pushed a commit to The-FOOL-00/openclaude that referenced this pull request May 24, 2026
…awb#534)

* fix(theme): remove stale React Compiler memo wrappers from theme hooks

Rebase on current main (includes Gitlawb#589 reconciler fix).

The React Compiler memo caches (_c) in useTheme() and usePreviewTheme()
use referential equality checks on destructured context values. These
caches can return stale references when the ThemeProvider's useMemo
recreates the context value object but the individual property
references (setThemeSetting, setPreviewTheme, etc.) compare equal —
the memo short-circuits and returns a cached tuple/object that still
holds the old closure captures.

This is a distinct bug from Gitlawb#589 (which fixed the ink reconciler's
commitUpdate path for host prop updates). Gitlawb#589 ensures that when
React _does_ re-render a component with new props, those props actually
reach the DOM node. But the memo wrappers here prevent React from
_even seeing_ the new context value in the first place — the hook
returns the stale cached result.

Removing the memo wrappers ensures useTheme() and usePreviewTheme()
always read the current context value, eliminating the stale-reference
path entirely.

* test(theme): add regression tests for useTheme()/usePreviewTheme() stale-value bug

These tests verify that context hooks always return fresh values after
ThemeProvider re-renders, even when React Compiler memo caches are in play.

- useTheme() must reflect currentTheme changes immediately after
  setThemeSetting is called (not return a stale cached tuple).
- usePreviewTheme() must return functional actions after context
  re-renders (not stale closures from before the theme change).

On current main (with _c memo wrappers), these tests expose the bug:
the memo cache compares setThemeSetting by reference (stable across
renders via useMemo) and short-circuits, returning the old cached result
with stale currentTheme.

* fix(test): correct import paths for ThemeProvider.test.tsx

Fix relative paths for ink.js, KeybindingSetup, AppStateProvider,
useStdin mock, systemTheme mock, and config mock to account for
the test file being in src/components/design-system/ rather than
src/components/.

* fix(test): rewrite ThemeProvider tests using Ink renderer

Use Ink's createRoot instead of react-dom/client, matching the pattern
from ThemePicker.test.tsx. The tests now render through Ink's terminal
renderer and check frame output for theme values, which is the same
environment ThemeProvider actually runs in.

* fix(test): correct all relative import paths for design-system/ depth

- ink.js, KeybindingSetup, AppStateProvider: ../ → ../../
- StructuredDiff: same pattern as ThemePicker test adjusted for depth

---------

Co-authored-by: root <root@vm7508.lumadock.com>
discopops pushed a commit to discopops/openclaude that referenced this pull request May 28, 2026
React 19's react-reconciler@0.33 mutation path calls commitUpdate with
(instance, type, oldProps, newProps, fiber), but our Ink host config
still expected an updatePayload from prepareUpdate. That left mounted
ink-* nodes with stale onKeyDown, tabIndex, and textStyles, making menu
navigation and highlights appear stuck until remount.

Diff old/new props directly inside commitUpdate and add regression tests
covering in-place updates for ink-box handlers/attributes and ink-text
styles.
discopops pushed a commit to discopops/openclaude that referenced this pull request May 28, 2026
…awb#534)

* fix(theme): remove stale React Compiler memo wrappers from theme hooks

Rebase on current main (includes Gitlawb#589 reconciler fix).

The React Compiler memo caches (_c) in useTheme() and usePreviewTheme()
use referential equality checks on destructured context values. These
caches can return stale references when the ThemeProvider's useMemo
recreates the context value object but the individual property
references (setThemeSetting, setPreviewTheme, etc.) compare equal —
the memo short-circuits and returns a cached tuple/object that still
holds the old closure captures.

This is a distinct bug from Gitlawb#589 (which fixed the ink reconciler's
commitUpdate path for host prop updates). Gitlawb#589 ensures that when
React _does_ re-render a component with new props, those props actually
reach the DOM node. But the memo wrappers here prevent React from
_even seeing_ the new context value in the first place — the hook
returns the stale cached result.

Removing the memo wrappers ensures useTheme() and usePreviewTheme()
always read the current context value, eliminating the stale-reference
path entirely.

* test(theme): add regression tests for useTheme()/usePreviewTheme() stale-value bug

These tests verify that context hooks always return fresh values after
ThemeProvider re-renders, even when React Compiler memo caches are in play.

- useTheme() must reflect currentTheme changes immediately after
  setThemeSetting is called (not return a stale cached tuple).
- usePreviewTheme() must return functional actions after context
  re-renders (not stale closures from before the theme change).

On current main (with _c memo wrappers), these tests expose the bug:
the memo cache compares setThemeSetting by reference (stable across
renders via useMemo) and short-circuits, returning the old cached result
with stale currentTheme.

* fix(test): correct import paths for ThemeProvider.test.tsx

Fix relative paths for ink.js, KeybindingSetup, AppStateProvider,
useStdin mock, systemTheme mock, and config mock to account for
the test file being in src/components/design-system/ rather than
src/components/.

* fix(test): rewrite ThemeProvider tests using Ink renderer

Use Ink's createRoot instead of react-dom/client, matching the pattern
from ThemePicker.test.tsx. The tests now render through Ink's terminal
renderer and check frame output for theme values, which is the same
environment ThemeProvider actually runs in.

* fix(test): correct all relative import paths for design-system/ depth

- ink.js, KeybindingSetup, AppStateProvider: ../ → ../../
- StructuredDiff: same pattern as ThemePicker test adjusted for depth

---------

Co-authored-by: root <root@vm7508.lumadock.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants