mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
review(tui): address Copilot feedback on cursorLayout wrap-ansi rewrite
Three small follow-ups from the Copilot review on #27489: 1. Declare `wrap-ansi` as a direct dependency of `ui-tui`. It was a phantom dep that resolved via npm hoisting from `@hermes/ink`'s transitive graph — fine on hoisted installs, but breaks under pnpm or `npm install --no-install-strategy=hoisted` style isolated installs. Now listed as `"wrap-ansi": "^9.0.0"` matching the @hermes/ink version. Lockfile regenerated. 2. Implement the defensive resync the comment promised. Previously the comment claimed the loop would "fall back to advancing by one to stay in lockstep" on wrap-ansi desync, but the code unconditionally advanced `originalIdx` with no actual check — so any future wrap-ansi option change or styled-input caller could silently slide `originalIdx` past the end of `value` and emit garbage line ranges. Now actually compares `value[originalIdx] === ch`, re-syncs via `indexOf` on mismatch, and bails out (returning whatever was built so far) if the desync is unrecoverable. Production paths still hit the equality fast-path on every char. 3. Drop the `visualLines` wrapper. It was a one-line indirection over `visualLinesFromWrappedOutput`. Renamed the implementation to `visualLines` and removed the wrapper — same name, no extra layer. No behavior change beyond the defensive realign; all 791 vitest tests still pass.
This commit is contained in:
parent
3b4dd68326
commit
1c0e59e557
3 changed files with 36 additions and 43 deletions
28
ui-tui/package-lock.json
generated
28
ui-tui/package-lock.json
generated
|
|
@ -14,7 +14,8 @@
|
|||
"ink-text-input": "^6.0.0",
|
||||
"nanostores": "^1.2.0",
|
||||
"react": "^19.2.4",
|
||||
"unicode-animations": "^1.0.3"
|
||||
"unicode-animations": "^1.0.3",
|
||||
"wrap-ansi": "^9.0.0"
|
||||
},
|
||||
"devDependencies": {
|
||||
"@babel/cli": "^7.28.6",
|
||||
|
|
@ -503,31 +504,6 @@
|
|||
"node": ">=6.9.0"
|
||||
}
|
||||
},
|
||||
"node_modules/@emnapi/core": {
|
||||
"version": "1.10.0",
|
||||
"resolved": "https://registry.npmjs.org/@emnapi/core/-/core-1.10.0.tgz",
|
||||
"integrity": "sha512-yq6OkJ4p82CAfPl0u9mQebQHKPJkY7WrIuk205cTYnYe+k2Z8YBh11FrbRG/H6ihirqcacOgl2BIO8oyMQLeXw==",
|
||||
"dev": true,
|
||||
"license": "MIT",
|
||||
"optional": true,
|
||||
"peer": true,
|
||||
"dependencies": {
|
||||
"@emnapi/wasi-threads": "1.2.1",
|
||||
"tslib": "^2.4.0"
|
||||
}
|
||||
},
|
||||
"node_modules/@emnapi/runtime": {
|
||||
"version": "1.10.0",
|
||||
"resolved": "https://registry.npmjs.org/@emnapi/runtime/-/runtime-1.10.0.tgz",
|
||||
"integrity": "sha512-ewvYlk86xUoGI0zQRNq/mC+16R1QeDlKQy21Ki3oSYXNgLb45GV1P6A0M+/s6nyCuNDqe5VpaY84BzXGwVbwFA==",
|
||||
"dev": true,
|
||||
"license": "MIT",
|
||||
"optional": true,
|
||||
"peer": true,
|
||||
"dependencies": {
|
||||
"tslib": "^2.4.0"
|
||||
}
|
||||
},
|
||||
"node_modules/@emnapi/wasi-threads": {
|
||||
"version": "1.2.1",
|
||||
"resolved": "https://registry.npmjs.org/@emnapi/wasi-threads/-/wasi-threads-1.2.1.tgz",
|
||||
|
|
|
|||
|
|
@ -22,7 +22,8 @@
|
|||
"ink-text-input": "^6.0.0",
|
||||
"nanostores": "^1.2.0",
|
||||
"react": "^19.2.4",
|
||||
"unicode-animations": "^1.0.3"
|
||||
"unicode-animations": "^1.0.3",
|
||||
"wrap-ansi": "^9.0.0"
|
||||
},
|
||||
"devDependencies": {
|
||||
"@babel/cli": "^7.28.6",
|
||||
|
|
|
|||
|
|
@ -26,14 +26,14 @@ const graphemes = (value: string) =>
|
|||
// boundaries — it never drops, reorders, or substitutes existing characters —
|
||||
// so a parallel walk uniquely identifies each line's source range.
|
||||
//
|
||||
// This used to be a hand-rolled word-wrap (visualLines below) whose break
|
||||
// points disagreed with wrap-ansi in subtle but visible ways: exact-fill rows
|
||||
// pushed the cursor to a phantom next line, mid-word breaks landed one
|
||||
// grapheme off, etc. The composer's TextInput renders text via Ink's
|
||||
// <Text wrap="wrap">, which delegates to wrap-ansi — so any drift between the
|
||||
// two algorithms parks the hardware cursor several cells away from the last
|
||||
// rendered character. Sourcing both from wrap-ansi guarantees agreement.
|
||||
function visualLinesFromWrappedOutput(value: string, cols: number): VisualLine[] {
|
||||
// This used to be a hand-rolled word-wrap whose break points disagreed with
|
||||
// wrap-ansi in subtle but visible ways: exact-fill rows pushed the cursor to
|
||||
// a phantom next line, mid-word breaks landed one grapheme off, etc. The
|
||||
// composer's TextInput renders text via Ink's <Text wrap="wrap">, which
|
||||
// delegates to wrap-ansi — so any drift between the two algorithms parks the
|
||||
// hardware cursor several cells away from the last rendered character.
|
||||
// Sourcing both from wrap-ansi guarantees agreement.
|
||||
function visualLines(value: string, cols: number): VisualLine[] {
|
||||
if (!value.length) {
|
||||
return [{ start: 0, end: 0 }]
|
||||
}
|
||||
|
|
@ -65,10 +65,30 @@ function visualLinesFromWrappedOutput(value: string, cols: number): VisualLine[]
|
|||
continue
|
||||
}
|
||||
|
||||
// Defensive: if wrap-ansi's emitted character ever desyncs from
|
||||
// `value[originalIdx]` (would only happen if it substituted, which it
|
||||
// doesn't for the wrap+hard option set we use), fall back to advancing
|
||||
// by one to stay in lockstep. The lines/cursor map still terminates.
|
||||
// Defensive sync check. wrap-ansi (with `hard: true, trim: false`, no
|
||||
// styled input) is documented to only insert '\n' at break points and
|
||||
// never substitute, drop, or reorder source characters — so under those
|
||||
// options `wrapped[i]` should always equal `value[originalIdx]`. But
|
||||
// future option changes, library upgrades, or callers that start passing
|
||||
// styled input (ANSI escapes) could violate that invariant silently. If
|
||||
// they do, we'd slide `originalIdx` past the end of `value` and emit
|
||||
// garbage line ranges with no diagnostic. Realign by scanning forward
|
||||
// for the matching character; bail out (return whatever we have) if the
|
||||
// sync is unrecoverable rather than producing wrong-but-plausible output.
|
||||
if (originalIdx >= value.length) {
|
||||
break
|
||||
}
|
||||
|
||||
if (value[originalIdx] !== ch) {
|
||||
const reSync = value.indexOf(ch, originalIdx)
|
||||
|
||||
if (reSync === -1) {
|
||||
break
|
||||
}
|
||||
|
||||
originalIdx = reSync
|
||||
}
|
||||
|
||||
originalIdx += 1
|
||||
}
|
||||
|
||||
|
|
@ -79,10 +99,6 @@ function visualLinesFromWrappedOutput(value: string, cols: number): VisualLine[]
|
|||
return lines.length ? lines : [{ start: 0, end: 0 }]
|
||||
}
|
||||
|
||||
function visualLines(value: string, cols: number): VisualLine[] {
|
||||
return visualLinesFromWrappedOutput(value, cols)
|
||||
}
|
||||
|
||||
function widthBetween(value: string, start: number, end: number) {
|
||||
let width = 0
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue