Skip to content

fix(ink): resolve stale TUI state via separate Box and Text fixes#575

Closed
catgirl3d wants to merge 3 commits into
Gitlawb:mainfrom
catgirl3d:focus-fix-2
Closed

fix(ink): resolve stale TUI state via separate Box and Text fixes#575
catgirl3d wants to merge 3 commits into
Gitlawb:mainfrom
catgirl3d:focus-fix-2

Conversation

@catgirl3d

@catgirl3d catgirl3d commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

This PR resolves two distinct but overlapping shared issues that caused stale state in interactive menus such as /config and /agents.

Summary

A previous workaround in Box remounted the entire interactive subtree and happened to mask two separate defects at once. This change removes that coarse remount behavior and replaces it with:

  • a principled shared fix for stale retained onKeyDown handlers in Box
  • a narrower shared repaint workaround for stale style-only updates in Text

What we found

We investigated narrower shared layers such as useSelectNavigation and list/highlight rendering, but the failing paths did not go through them consistently.

Tracing showed that in affected screens like /agents:

  • component state did change after the first arrow key press
  • the component-level handleKeyDown callback identity also changed
  • but the retained ink-box host kept invoking the original onKeyDown handler

That means the main “moves one step then gets stuck” bug was not caused by useSelectNavigation, but by a stale retained host handler.

Separately, style-only updates on retained <ink-text> nodes could leave stale visual highlighting behind even after state was already correct.

Implementation

1. Shared handler fix (src/ink/components/Box.tsx)

Box now passes stable onKeyDown / onKeyDownCapture wrapper callbacks to the host element. Those wrappers read the latest handler from a ref.

This avoids stale retained host handlers without forcing subtree remounts and without relying on handler identity changes.

2. Shared text repaint workaround (src/ink/components/Text.tsx)

Text now derives a React key from serialized textStyles and remounts <ink-text> when style-only text changes occur.

This is an intentional shared workaround for stale retained text repainting. It fixes the visual focus/highlight lag without reintroducing the coarse subtree remount behavior from the earlier Box workaround.

Why this approach

  • addresses the stale handler problem at the shared host boundary
  • keeps the text repaint fix narrowly scoped to style-only text updates
  • avoids scattering per-screen remount/focus hacks across the codebase

Notes

  • the Box change is the principled fix for the stale keydown-handler issue
  • the Text change is still a shared workaround for retained text repainting, not a claimed final renderer root-cause fix

Testing

  • bun run build
  • bun run smoke
  • focused tests:
    • verified /agents no longer gets stuck after the first arrow-key move
    • verified /config updates both keyboard behavior and visual highlight correctly
    • verified the old Box subtree-remount workaround is no longer needed

before:
_260410031839

after:

bandicam.2026-04-10.20-59-13-972.mp4

kevincodex1
kevincodex1 previously approved these changes Apr 10, 2026

@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.

Looks good to me! Thank you so much

@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 @catgirl3d — I pulled up the screenshot and I can see exactly what you're describing. The chevron has moved to "Rewind code (checkpoints)" and the value on the right matches, but the red highlight box is stuck on "Thinking mode" from the previous focus position. That's a real bug and I'm glad you caught it.

A few things I'd like to sort out before we merge though:

1. Title/description don't match the code

The PR title and description both say this remounts <ink-text> when textStyles change by deriving a key from the serialized style object. But the actual diff modifies src/ink/components/Box.tsx to remount <ink-box> when the onKeyDown function identity changes, using a WeakMap<Function, number> to assign IDs per handler reference. Could you update the description to reflect what the code actually does? I want to be sure I'm reviewing the right mechanism.

2. The mechanism relies on a coincidence, not a principle

Walking through the logic: your code only generates a new key when onKeyDown gets a new function reference. That works in the current codebase because the handlers closing over the focus/selection state get recreated whenever that state updates. But it's incidental — not something the code contract guarantees:

  • If a caller ever wraps their handler in useCallback (normally good React hygiene), the handler identity becomes stable and your fix silently stops working
  • If a screen legitimately needs to repaint without the handler identity changing (e.g. an upstream state change that doesn't flow through onKeyDown), the fix does nothing
  • The fix is stronger than it needs to be — it remounts the entire <ink-box> subtree, including children that didn't need to repaint

Could you look at whether the root cause is actually in useSelectNavigation or the highlight rendering path? The symptom is specifically that a highlight box fails to repaint when focus state changes — which sounds more like a missed dependency in a useMemo or a stale render of the highlight overlay than an Ink renderer issue.

3. Overlap with parallel PRs

Two other open PRs target the same class of stale-focus bugs:

  • #562 (anandh8x, already approved) — fixes /theme preview by routing defaultFocusValue through initialFocusValue in useSelectState instead of the controlled focusValue path that was hijacking navigation
  • #534 (Vasanthdev2004) — speculatively removes React Compiler memo wrappers from theme context hooks

Could you pull #562 first and check whether the /config and /agents highlight trails still reproduce on that branch? If #562 alone resolves them, this PR becomes unnecessary. If not, comparing what still breaks after #562 would narrow down the real root cause significantly.

4. No tests

A behavioral change to Box.tsx — a primitive used by essentially every interactive screen — should have at least one regression test. I understand Ink testing is awkward, but something like a render-count assertion or a repro of the stale-highlight bug would protect this from future regressions.


Happy to pair on the investigation if it'd help. The bug is worth fixing — I just want to make sure we fix the right thing rather than pinning a workaround into a shared primitive.

@catgirl3d

catgirl3d commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @gnanam1990! Yes... relying on function identity is definitely a bit of a hack, and I'll get that PR description updated to match the code

I checked #562, #534 and it doesn't solve the problem that we fixed in this commit. Will try dig into useSelectNavigation for a more solid fix and make sure to add a regression test

Work around stale focus/selection highlights by remounting auto-focused tabbable ink-box hosts when their [`onKeyDown`](src/ink/components/Box.tsx:47) handler identity changes. The workaround is localized to [`BoxInner()`](src/ink/components/Box.tsx:63) in [`src/ink/components/Box.tsx`](src/ink/components/Box.tsx).
@catgirl3d catgirl3d changed the title fix(ink): remount ink-text on text style changes fix(ink): resolve stale TUI state via separate Box and Text fixes Apr 10, 2026
@catgirl3d catgirl3d marked this pull request as draft April 10, 2026 16:59
Prevent retained ink-box hosts from using stale onKeyDown callbacks in
interactive menus, and remount ink-text on textStyles changes so focus
and selection highlights repaint correctly.
Add an interactive regression test for AgentsList that reproduces the
“moves once, then gets stuck” behavior in TUI menus.

The test verifies that keyboard navigation still works after the first
arrow-key move, and was structured with polling-based frame waits plus
try/finally cleanup so it remains stable in CI and does not leak test
resources on failure
@catgirl3d

catgirl3d commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

i dug deeper in search of the cause

The evidence pointed away from useSelectNavigation() and list highlight state for the “moves one step, then gets stuck” bug. On /agents, component state changed after the first arrow key press, and the component-level handleKeyDown callback changed too - but the retained ink-box host kept invoking the original handler

So the old Box workaround was helping because it forced the host to rebind its keyboard handler, not because the navigation state itself was wrong

This patch replaces that coarse remount behavior with stable delegating handlers in src/ink/components/Box.tsx, so the host always calls the latest callback without remounting the whole subtree

There was also a separate retained-text repaint issue, so src/ink/components/Text.tsx now remounts on textStyles changes as a scoped workaround for stale focus/highlight rendering

So this ended up being two shared-layer issues with two different fixes:

  • Box.tsx: principled fix for stale retained key handlers
  • Text.tsx: scoped workaround for stale retained text repaint

also updated the PR description + added a focused regression test InteractiveMenuRegression.test.tsx that reproduces the /agents failure mode, fails without the Box fix, and passes with it

Please take a look @gnanam1990

@catgirl3d catgirl3d marked this pull request as ready for review April 10, 2026 17:44
@catgirl3d catgirl3d requested a review from gnanam1990 April 10, 2026 18:01
@catgirl3d

Copy link
Copy Markdown
Contributor Author

I found the real cause of the bug I'll make a new PR with a targeted fix

@catgirl3d catgirl3d closed this Apr 10, 2026
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