Keep onKittyQueryResponse stable across rerender() calls - #1007
Conversation
`Ink.render()` passed `onKittyQueryResponse` as a new arrow function on every call. The prop is a dependency of `handleReadable`, which feeds `attachReadableListener` and `handleSetRawMode`, so every `rerender()` changed the `setRawMode` identity in `StdinContext`. Every `useInput` and `usePaste` then re-ran its raw-mode effect, and the count-hits-zero path reset the input parser, cancelled the pending-escape flush and detached the `readable` listener. A pending Escape was lost, a split escape sequence became a plain key, and a bracketed paste spanning two reads was delivered to `useInput` as `part2` + `[201~` even with `usePaste` mounted. Use a stable class property instead.
30061e2 to
1128794
Compare
|
The implementation looks right to me. The callback belongs to the long-lived Ink instance and reads the current detection resolver when called, so making it a stable instance property is the smallest fix. Handling an unstable parent callback inside App would need extra ref or effect plumbing and introduce stale-callback timing concerns. useEffectEvent is not a better fit here because it is intentionally unstable and restricted to Effect-owned calls. The new tests need one adjustment, though. Readable.push() plus a 2 ms sleep does not guarantee that the first chunk reached the parser before rerender(), so the buggy implementation can sometimes pass without exercising the bug. The sleep after rerender() can also cross the parser's 20 ms escape timeout under load, making the fixed implementation fail. I reproduced the latter locally. Please use the existing synchronous stdin helper to deliver the chunks. Fake timers are only needed for the pending Escape timeout. For the split sequence, emit A immediately after the synchronous rerender(), and the paste test can also stay fully synchronous. That should make the tests deterministc. |
Ink.render()passedonKittyQueryResponsetoAppas an inline arrow function, so it was a new function on every call, including every publicrerender(). InApp, that prop is a dependency ofhandleReadable, which feedsattachReadableListenerandhandleSetRawMode, so everyrerender()changed thesetRawModeidentity inStdinContext. Every mounteduseInput,usePasteanduseFocusthen re-ran its raw-mode effect (setRawMode(false)thensetRawMode(true)), and the count-hits-zero path calledclearInputState(): the input parser was reset, the pending-escape flush timer cancelled and thereadablelistener detached and re-attached.Anything buffered in the parser at that moment was lost:
ESC[thenA) arrived as the plain keyAwithshift: trueinstead ofupArrow;useInputaspart2and[201~, even withusePastemounted.In-tree
setStatere-renders do not trigger it, onlyrerender()does, and it happens whether or notkittyKeyboardis configured.Make the callback a stable class property. Side effect:
useStdinconsumers no longer re-render on everyrerender(), since the context value is stable again.Tests (
test/rerender-input.tsx): a pending Escape survivesrerender(), an escape sequence split across two reads survivesrerender(), a paste split across two reads survivesrerender(), andrerender()does not remove thereadablelistener. All four fail on master.Fixes #1009