fix: preserve raw mode across component re-renders (issue #843)#1198
fix: preserve raw mode across component re-renders (issue #843)#1198LifeJiggy wants to merge 2 commits into
Conversation
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for digging into #843 — the diagnosis looks plausible and it's a real, annoying issue, so I appreciate you taking it on. A few changes needed before this can land:
The current approach makes the useLayoutEffect cleanup an unconditional no-op (use-input.ts:58), which is a bit too blunt: it leaks raw mode on every unmount across every entrypoint, not just the MCP-async re-render race you're targeting. It also means setting options.isActive to false no longer disables raw mode, which is a behavior change beyond the stated scope. The "bridgeMain resets on exit" safety net only covers the bridge path (bridgeMain.ts:2754), not TUI/non-bridge exits, so terminals could be left in raw mode there.
Could you scope the fix to the actual race — e.g. a mount-counter guard or detecting the re-render churn — so legitimate teardown still restores the terminal? A regression test (you mention one is needed) plus passing build/smoke would make this an easy approve. Thanks again for the solid investigation here.
…P re-render churn (issue Gitlawb#843)
|
Good catch on the blunt no-op — you’re right it would leak raw mode on every intentional unmount and break the isActive: false contract outside the bridge. The new commit d45071c replaces it with a guard inside the cleanup handler: return () => { What this preserves:
Why it’s scoped: only the isActive transition gates the cleanup, so normal teardown (component leaving with isActive: false) works exactly as before, while the problematic true → true unmount/remount cycle becomes a no-op. bun run build + bun run smoke both pass. |
jatmn
left a comment
There was a problem hiding this comment.
Thanks for following up on the earlier review. The first revision's unconditional cleanup no-op is narrowed now, but I found one remaining issue below.
Findings
- [P1] Balance raw mode when an active input deactivates or unmounts
src/ink/hooks/use-input.ts:63
This cleanup closes over the render that enabled raw mode, sooptions.isActiveis stilltruefor both atrue -> falseupdate and a normal active unmount. That means the cleanup returns before callingsetRawMode(false), leavingApp.rawModeEnabledCountincremented. The next activation/remount increments it again, and later teardown only decrements once, so raw mode and the stdin listeners can remain enabled after the UI no longer has an activeuseInput. This still breaks theisActive: falsecontract called out in the earlier review and can leave non-bridge exits or temporary dialogs in raw mode. Please make the MCP rerender workaround preserve onesetRawMode(false)for every successfulsetRawMode(true)activation, and add a regression test for thetrue -> false/unmount path.
Summary
Impact
Testing
Notes