mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-09 13:21:42 +00:00
opentui(v6): clamp negative draw coords at the node:ffi seam (diff crash fix)
Expanding a tall <diff showLineNumbers> pinned to the scrollbox bottom froze the TUI with ERR_INVALID_ARG_VALUE looping out of CliRenderer.loop every frame. Root cause: @opentui/core 0.4.0 marshals OptimizedBuffer fillRect/drawText/setCell* coordinates as u32 in the FFI table while LineNumberRenderable.renderSelf passes raw screen coordinates — NEGATIVE when the diff is partially scrolled above the viewport. Bun's FFI silently wraps negatives (native side bounds-checks them into a no-op); Node's experimental node:ffi rejects them. bufferDrawBox already uses i32, which is why ordinary boxes/text scroll fine and only the diff line-background path crashed. Fix at the seam we own: boundary/ffiSafe.ts patches OptimizedBuffer to clip fillRect to the non-negative quadrant and skip negative-origin drawText/setCell*/drawChar before the FFI call (Bun parity). Installed from boundary/renderer.ts (live) and test/lib/render.ts (headless). TODO(upstream): widen those FFI params to i32 so this shim can be deleted.
This commit is contained in:
parent
c4348480f3
commit
0a5b0780f5
4 changed files with 228 additions and 0 deletions
87
ui-opentui/src/boundary/ffiSafe.ts
Normal file
87
ui-opentui/src/boundary/ffiSafe.ts
Normal file
|
|
@ -0,0 +1,87 @@
|
|||
/**
|
||||
* Node-FFI coordinate safety shim for @opentui/core 0.4.0.
|
||||
*
|
||||
* Root cause (live crash, ERR_INVALID_ARG_VALUE looping every frame): several
|
||||
* OptimizedBuffer methods marshal x/y/width/height as **u32** in the FFI table
|
||||
* (zig.ts: `bufferFillRect: ["u32","u32","u32","u32","u32","ptr"]`, same for
|
||||
* `bufferDrawText` / `bufferSetCell*` / `bufferDrawChar`), while renderables
|
||||
* pass RAW SCREEN COORDINATES — which go NEGATIVE inside a <scrollbox> when an
|
||||
* element is partially scrolled above the viewport. Concretely:
|
||||
* `LineNumberRenderable.renderSelf` does `buffer.fillRect(this.x + gutterWidth,
|
||||
* this.y + i, …)` for diff added/removed line backgrounds, so expanding a tall
|
||||
* `<diff showLineNumbers>` pinned to the scrollbox bottom rendered with
|
||||
* `this.y < 0` and threw out of `CliRenderer.loop` on EVERY frame (frozen UI,
|
||||
* console error spam) until a resize forced a fresh layout.
|
||||
*
|
||||
* Upstream-on-Bun this never throws: Bun's FFI silently WRAPS negatives to
|
||||
* huge u32s and the native side bounds-checks them into a no-op. Node's
|
||||
* experimental FFI (node:ffi) instead REJECTS the argument. Other draw entry
|
||||
* points (`bufferDrawBox`, `bufferDrawTextBufferView`) already use i32 — which
|
||||
* is why ordinary text/boxes scroll fine and only the diff gutter path crashed.
|
||||
*
|
||||
* Fix at the seam we own: clamp/skip BEFORE the FFI call.
|
||||
* - fillRect: clip the rect to the non-negative quadrant (the native side
|
||||
* already clips right/bottom against the buffer + scissor) and skip empties.
|
||||
* - drawText/setCell/setCellWithAlphaBlending/drawChar: skip when the origin
|
||||
* is negative (Bun-parity: those cells/rows are off-screen anyway).
|
||||
*
|
||||
* TODO(upstream): file/track an OpenTUI issue to widen these FFI params to i32
|
||||
* (or clamp in core) — then this shim can be deleted.
|
||||
*/
|
||||
import { OptimizedBuffer } from '@opentui/core'
|
||||
|
||||
let installed = false
|
||||
|
||||
/** Patch OptimizedBuffer's u32-coordinate methods to tolerate negative coords. Idempotent. */
|
||||
export function installFfiCoordSafety(): void {
|
||||
if (installed) return
|
||||
installed = true
|
||||
|
||||
const proto = OptimizedBuffer.prototype
|
||||
|
||||
// Prototype monkey-patching: extracting the original methods unbound is the
|
||||
// point — they're re-invoked with `.call(this, …)` on the correct instance.
|
||||
/* eslint-disable @typescript-eslint/unbound-method */
|
||||
|
||||
const origFillRect = proto.fillRect
|
||||
proto.fillRect = function (this: OptimizedBuffer, x, y, width, height, bg) {
|
||||
let x2 = Math.trunc(x)
|
||||
let y2 = Math.trunc(y)
|
||||
let w = Math.trunc(width)
|
||||
let h = Math.trunc(height)
|
||||
if (x2 < 0) {
|
||||
w += x2
|
||||
x2 = 0
|
||||
}
|
||||
if (y2 < 0) {
|
||||
h += y2
|
||||
y2 = 0
|
||||
}
|
||||
if (w <= 0 || h <= 0) return
|
||||
origFillRect.call(this, x2, y2, w, h, bg)
|
||||
}
|
||||
|
||||
const origDrawText = proto.drawText
|
||||
proto.drawText = function (this: OptimizedBuffer, text, x, y, ...rest) {
|
||||
if (x < 0 || y < 0) return
|
||||
origDrawText.call(this, text, x, y, ...rest)
|
||||
}
|
||||
|
||||
const origSetCell = proto.setCell
|
||||
proto.setCell = function (this: OptimizedBuffer, x, y, ...rest) {
|
||||
if (x < 0 || y < 0) return
|
||||
origSetCell.call(this, x, y, ...rest)
|
||||
}
|
||||
|
||||
const origSetCellAlpha = proto.setCellWithAlphaBlending
|
||||
proto.setCellWithAlphaBlending = function (this: OptimizedBuffer, x, y, ...rest) {
|
||||
if (x < 0 || y < 0) return
|
||||
origSetCellAlpha.call(this, x, y, ...rest)
|
||||
}
|
||||
|
||||
const origDrawChar = proto.drawChar
|
||||
proto.drawChar = function (this: OptimizedBuffer, char, x, y, ...rest) {
|
||||
if (x < 0 || y < 0) return
|
||||
origDrawChar.call(this, char, x, y, ...rest)
|
||||
}
|
||||
}
|
||||
|
|
@ -13,8 +13,13 @@ import { createCliRenderer, type CliRenderer, type KeyEvent, type Selection } fr
|
|||
import { Deferred, Effect } from 'effect'
|
||||
|
||||
import { RendererError } from './errors.ts'
|
||||
import { installFfiCoordSafety } from './ffiSafe.ts'
|
||||
import { getLog } from './log.ts'
|
||||
|
||||
// Node-FFI seam: clamp negative draw coordinates BEFORE the u32 FFI marshaling
|
||||
// (see ffiSafe.ts — scrolled-out <diff> line backgrounds crashed the render loop).
|
||||
installFfiCoordSafety()
|
||||
|
||||
/**
|
||||
* The text a finished selection copies: the RENDERED text the user highlighted,
|
||||
* verbatim (`getSelectedText()` does correct same-line merging). Markdown markers
|
||||
|
|
|
|||
129
ui-opentui/src/test/ffiSafe.test.tsx
Normal file
129
ui-opentui/src/test/ffiSafe.test.tsx
Normal file
|
|
@ -0,0 +1,129 @@
|
|||
/**
|
||||
* Regression: tall <diff showLineNumbers> scrolled partially above the
|
||||
* transcript viewport crashed the render loop under node:ffi.
|
||||
*
|
||||
* @opentui/core 0.4.0 marshals OptimizedBuffer.fillRect/drawText coordinates
|
||||
* as u32 (zig.ts FFI table) while LineNumberRenderable passes raw screen
|
||||
* coordinates — NEGATIVE when the diff is partially scrolled out of a
|
||||
* <scrollbox>. Bun's FFI silently wraps negatives (native bounds-check →
|
||||
* no-op); Node's experimental FFI throws ERR_INVALID_ARG_VALUE out of
|
||||
* CliRenderer.loop on EVERY frame (frozen UI + console error spam). Fixed by
|
||||
* boundary/ffiSafe.ts clamping/skipping before the FFI call.
|
||||
*/
|
||||
import { OptimizedBuffer, RGBA } from '@opentui/core'
|
||||
import { describe, expect, test } from 'vitest'
|
||||
|
||||
import { installFfiCoordSafety } from '../boundary/ffiSafe.ts'
|
||||
import { createSessionStore } from '../logic/store.ts'
|
||||
import { App } from '../view/App.tsx'
|
||||
import { ThemeProvider } from '../view/theme.tsx'
|
||||
import { renderProbe, type RenderProbe } from './lib/render.ts'
|
||||
|
||||
type Store = ReturnType<typeof createSessionStore>
|
||||
|
||||
// TALL diff: when expanded inside the sticky-bottom scrollbox the diff's TOP
|
||||
// rows render above the viewport (negative screen y) — the live-crash trigger.
|
||||
const ADDED = Array.from({ length: 40 }, (_, i) => `+def fn_${i}(): pass`)
|
||||
const DIFF = [
|
||||
'--- a//tmp/v6smoke/greet.py',
|
||||
'+++ b//tmp/v6smoke/greet.py',
|
||||
'@@ -1,5 +1,45 @@',
|
||||
' def greet(name):',
|
||||
'- print("hello " + name)',
|
||||
'+ print(f"hello {name}")',
|
||||
...ADDED,
|
||||
' ',
|
||||
' if __name__ == "__main__":',
|
||||
' greet("world")',
|
||||
''
|
||||
].join('\n')
|
||||
|
||||
function seed(store: Store) {
|
||||
store.apply({ type: 'gateway.ready' })
|
||||
store.apply({ type: 'message.start' })
|
||||
store.apply({ type: 'tool.start', payload: { tool_id: 'p1', name: 'patch', context: '/tmp/v6smoke/greet.py' } })
|
||||
store.apply({
|
||||
type: 'tool.complete',
|
||||
payload: {
|
||||
tool_id: 'p1',
|
||||
name: 'patch',
|
||||
args: { path: '/tmp/v6smoke/greet.py', mode: 'replace' },
|
||||
diff_unified: DIFF,
|
||||
duration_s: 0.2,
|
||||
result: JSON.stringify({ success: true, diff: DIFF })
|
||||
}
|
||||
})
|
||||
store.apply({ type: 'message.complete' })
|
||||
}
|
||||
|
||||
async function clickHeader(probe: RenderProbe, name: string): Promise<void> {
|
||||
const frame = await probe.waitForFrame(f => f.includes(name))
|
||||
const rows = frame.split('\n')
|
||||
const y = rows.findIndex(line => line.includes(name))
|
||||
expect(y).toBeGreaterThanOrEqual(0)
|
||||
const x = (rows[y] ?? '').indexOf(name)
|
||||
await probe.click(x, y)
|
||||
}
|
||||
|
||||
describe('node-ffi coordinate safety (boundary/ffiSafe.ts)', () => {
|
||||
test('negative coordinates no longer throw ERR_INVALID_ARG_VALUE', () => {
|
||||
installFfiCoordSafety() // idempotent (test/lib/render.ts installs it too)
|
||||
const buf = OptimizedBuffer.create(20, 10, 'unicode', { id: 'ffi-safety-probe' })
|
||||
const red = RGBA.fromInts(255, 0, 0, 255)
|
||||
try {
|
||||
// each of these threw TypeError ERR_INVALID_ARG_VALUE ("must be a uint32")
|
||||
expect(() => buf.fillRect(2, -3, 5, 2, red)).not.toThrow()
|
||||
expect(() => buf.fillRect(-1, 2, 5, 2, red)).not.toThrow()
|
||||
expect(() => buf.fillRect(2, 2, -5, 2, red)).not.toThrow()
|
||||
expect(() => buf.drawText('hi', -1, 2, red)).not.toThrow()
|
||||
expect(() => buf.drawText('hi', 2, -1, red)).not.toThrow()
|
||||
expect(() => buf.setCell(-1, 0, 'x', red, red)).not.toThrow()
|
||||
expect(() => buf.setCellWithAlphaBlending(0, -1, 'x', red, red)).not.toThrow()
|
||||
// a clipped fillRect still draws its visible part
|
||||
buf.fillRect(-2, -2, 6, 6, red)
|
||||
expect(() => buf.fillRect(0, 0, 4, 4, red)).not.toThrow()
|
||||
} finally {
|
||||
buf.destroy()
|
||||
}
|
||||
})
|
||||
|
||||
test('tall diff expand/collapse + resize churn survives without render-loop errors', async () => {
|
||||
const store = createSessionStore()
|
||||
seed(store)
|
||||
const probe = await renderProbe(
|
||||
() => (
|
||||
<ThemeProvider theme={() => store.state.theme}>
|
||||
<App store={store} />
|
||||
</ThemeProvider>
|
||||
),
|
||||
{ width: 120, height: 35 }
|
||||
)
|
||||
const errors: unknown[] = []
|
||||
const onErr = (e: unknown) => errors.push(e)
|
||||
process.on('uncaughtException', onErr)
|
||||
try {
|
||||
// Expanding renders transient sticky-bottom frames where the diff top sits
|
||||
// ABOVE the viewport (negative y) — the exact live-crash condition.
|
||||
await clickHeader(probe, 'patch')
|
||||
// let tree-sitter + the scrollAnchor's 4x16ms re-asserts land
|
||||
await new Promise(r => setTimeout(r, 200))
|
||||
// added rows only paint when the diff body is actually expanded (the
|
||||
// scrollAnchor holds the viewport at the diff TOP, so assert early rows)
|
||||
const expanded = await probe.waitForFrame(f => f.includes('fn_0'))
|
||||
expect(expanded).toContain('+ def fn_0(): pass')
|
||||
// toggle a few times + resize churn
|
||||
await clickHeader(probe, 'patch')
|
||||
await new Promise(r => setTimeout(r, 100))
|
||||
await clickHeader(probe, 'patch')
|
||||
await new Promise(r => setTimeout(r, 200))
|
||||
probe.resize(100, 30)
|
||||
await new Promise(r => setTimeout(r, 100))
|
||||
probe.resize(120, 35)
|
||||
await new Promise(r => setTimeout(r, 200))
|
||||
expect(errors).toEqual([])
|
||||
} finally {
|
||||
process.off('uncaughtException', onErr)
|
||||
probe.destroy()
|
||||
}
|
||||
}, 30000)
|
||||
})
|
||||
|
|
@ -23,6 +23,13 @@ import { testRender, useRenderer } from '@opentui/solid'
|
|||
import type { JSX } from '@opentui/solid'
|
||||
import { createMemo } from 'solid-js'
|
||||
|
||||
import { installFfiCoordSafety } from '../../boundary/ffiSafe.ts'
|
||||
|
||||
// Headless renders go through the same node:ffi seam as the live TUI — install
|
||||
// the negative-coordinate shim here too (the live path installs it in
|
||||
// boundary/renderer.ts, which tests don't import).
|
||||
installFfiCoordSafety()
|
||||
|
||||
/** Wrap a node in a KeymapProvider whose keymap is bound to the test renderer. */
|
||||
function withKeymap(node: () => JSX.Element): () => JSX.Element {
|
||||
return () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue