Skip to content

feat(tui): upgrade Ink to v8 with minimum input compatibility fixes - #1869

Open
DenizOkcu wants to merge 2 commits into
Nano-Collective:mainfrom
DenizOkcu:review/ink-v8-core
Open

DenizOkcu wants to merge 2 commits into
Nano-Collective:mainfrom
DenizOkcu:review/ink-v8-core

Conversation

@DenizOkcu

@DenizOkcu DenizOkcu commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Why

Split from #1570 following @addyCooks’ request for smaller PRs. This is scope (a) only.

Changes

Validation

Lockfile YAML and whitespace checks pass. Local input-checkout typecheck, build, and 60 focused tests passed; separate branch CI is still required. Real VS Code Shift/Alt+Enter smoke test: not run; reproduction steps are in docs/features/terminal-compatibility.md.

FIFO/paste routing and new Ink APIs are submitted separately; this PR should merge first.

Split PRs

@github-actions

Copy link
Copy Markdown
Contributor

nc-review: needs work — 1 blocking, 1 important, 5 nits

@DenizOkcu — there is a blocking item below.

This upgrades Ink 6.8 -> 8.0 (and the React floor to 19.3), ports the throttled es-toolkit import patch forward, and adds a stdin proxy stage that rewrites xterm's modifyOtherKeys modified-Enter sequence (ESC [27;<m>;13~) into the kitty CSI-u form Ink parses natively. The rewriter itself is well tested and the patch-carried-over approach is sound, but the PR does not build or typecheck on its own branch: a modified spec imports createTerminalInputFilter from @/utils/terminal-input, a module that does not exist at base and is not added by this PR. Separately, the exported flush() method is documented as required to preserve stdin ordering but is never called from cli.tsx, so the reordering hazard it exists to prevent is live. Not ready; the missing-module dependency is a hard blocker on merging this before #1871.

🔴 blocking · correctness · source/components/text-input-newline.spec.tsx:3

This spec imports a module that does not exist:

import {createTerminalInputFilter} from '@/utils/terminal-input';

source/utils/terminal-input.ts is not in this PR's changed-file list, and source/utils/ at base contains only terminal-keypress.ts, terminal-paste.ts, terminal-mouse.ts and terminal-file-link.ts. Searching the base tree for createTerminalInputFilter returns nothing. The helper belongs to the sibling FIFO/paste branch (#1871), which the description explicitly says is to merge after this one ("FIFO/paste routing (draft, next)", "this PR should merge first").

So merged as-is, pnpm run test:types and pnpm run test:ava source/components/text-input-newline.spec.tsx both fail on an unresolved import — the branch as described cannot be green on its own, and the PR description's claim of "60 focused tests passed" was evidently measured with the sibling branch checked out. Either inline a local filter helper in the spec, or drop the createInputProxy tests until #1871 lands and rely on terminal-keypress.spec.ts for the rewriter coverage (which is self-contained and does pass).

🟠 important · correctness · source/cli.tsx:945

flush() is added to the rewriter and documented for exactly the case that forwardInput creates, but nothing ever calls it. From the new JSDoc:

Emit any held-back partial sequence now, unrewritten … Callers use this to preserve stream order when something else must be emitted immediately: a bracketed paste that arrives after a held-back tail must not be reordered ahead of it.

In the modified forwardInput, extractPastes(text) emits every paste payload to pasteEvents synchronously, before rewriteEnter.push(result.clean) runs. If chunk N ends in a held-back tail (ESC, ESC [, ESC [27;, …) and chunk N+1 contains a bracketed paste, the paste is published first and the older held-back bytes are flushed 20ms later — the payload lands in the input ahead of keystrokes the user typed before it.

This is the FIFO/ordering problem #1871 is meant to solve, but the rewriter introduced here is what makes the window exist, and it ships a flush() that nothing calls. Either call rewriteEnter.flush() before emitting to pasteEvents, or drop flush() from this PR and leave the whole ordering question to #1871 rather than shipping the half of it that opens the hole.

⚪ nit · correctness · source/utils/terminal-keypress.ts:62

Every chunk whose last ESC begins a tail matching isPartial is delayed by ENTER_SEQUENCE_TIMEOUT_MS (20ms) before its tail reaches Ink. XTERM_ENTER_PREFIX.startsWith(tail) matches a bare ESC, so a plain Escape keypress is now buffered for 20ms on every keypress path, and so is the prefix of every arrow key that happens to be split. 20ms is below the human perception threshold for a key press so this is not user-visible in practice, but it does add latency to the most latency-sensitive input the app handles, and it applies unconditionally rather than only when an incomplete 27;…;13~ is genuinely in flight. Worth a comment noting the measured cost, or narrowing the held-back set.

⚪ nit · design · source/components/text-input.tsx:354

The new early return:

if (key.eventType === 'release') {
    return;
}

is defended by a comment about the kitty keyboard protocol reporting releases, but text-input.tsx two hundred lines below states that nanocoder does not enable kittyKeyboard, and there is no kittyKeyboard/reportEventTypes option anywhere in source/. So this branch is unreachable today and nothing tests it. It is harmless as forward-looking defence, but the comment reads as if it fixes a live double-handling bug, which could mislead the next reader. Either drop it or reword it as dormant-until-kitty like the existing comment at the Backspace branch.

⚪ nit · design · source/storage/storage-app.tsx:33

const stdout = stream as NodeJS.WriteStream; is a bare assertion over Ink 8's WritableStream-typed useStdout() result. The comment on useViewport above still says "Ink's stdout, not process.stdout, is the surface used by the renderer", which is fine, but nothing records why the cast is needed or what it would take to drop it (Ink exposing columns/rows on its stdout type). Since this file is the only consumer of that field, a one-line note on the cast would save the next reader from re-deriving it.

⚪ nit · scope

Beyond the Ink bump, the diff also adds a snapToCharBoundary(nextValue, nextCursorOffset) clamp in text-input.tsx and rewrites the newline-key.spec.ts Alt+Enter case from "ESC+CR (Option+Enter on macOS)" to "Alt+Enter in the kitty CSI-u encoding". The cursor clamp is a real behaviour change with no changeset mention and no dedicated test in this diff — .changeset/emoji-cursor-desync.md suggests it may duplicate work already tracked. If the clamp is needed for Ink 8 say so in the changeset; if it is unrelated, it belongs in its own PR so it can be reverted independently of the upgrade.

⚪ nit · changeset · .changeset/upgrade-ink-v8.md

The changeset is well-formed and the package name @nanocollective/nanocoder resolves against the workspace, and its framing is admirably careful about not claiming #1505/#1664. Two gaps: (1) it describes the Ink-8 behaviour change only for modified Enter, and says nothing about the React floor moving to 19.3 — a downstream packager or anyone tracking React versions needs to know; (2) Ink 8 requires engines: {node: '>=22'} (visible in the lockfile diff) where Ink 6.8 required >=20. If nanocoder still supports Node 20, that is a breaking change for users and belongs in the changeset with an explicit note; if Node 22 was already the floor elsewhere, say so. I could not verify the declared engines in package.json from the diff alone — only the dependency's floor is visible.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:needs-work nc-review found blocking findings label Oct 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hi @DenizOkcu, thanks for this PR! It looks like a maintainer has left feedback
or review activity and there are still some outstanding items to wrap up.

Whenever you get a chance, could you take a look at the open comments?
If anything is unclear or you'd like a hand, just reply here and we'll help you get it across the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:needs-work nc-review found blocking findings area:docs Documentation area:tui Terminal UI stale:nudged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant