From 0a5b0780f5f96eff39caef91ec853cfeba807bb7 Mon Sep 17 00:00:00 2001 From: alt-glitch Date: Wed, 10 Jun 2026 18:48:51 +0530 Subject: [PATCH] opentui(v6): clamp negative draw coords at the node:ffi seam (diff crash fix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expanding a tall 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. --- ui-opentui/src/boundary/ffiSafe.ts | 87 ++++++++++++++++++ ui-opentui/src/boundary/renderer.ts | 5 ++ ui-opentui/src/test/ffiSafe.test.tsx | 129 +++++++++++++++++++++++++++ ui-opentui/src/test/lib/render.ts | 7 ++ 4 files changed, 228 insertions(+) create mode 100644 ui-opentui/src/boundary/ffiSafe.ts create mode 100644 ui-opentui/src/test/ffiSafe.test.tsx diff --git a/ui-opentui/src/boundary/ffiSafe.ts b/ui-opentui/src/boundary/ffiSafe.ts new file mode 100644 index 00000000000..6d34ef2ab8f --- /dev/null +++ b/ui-opentui/src/boundary/ffiSafe.ts @@ -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 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 + * `` 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) + } +} diff --git a/ui-opentui/src/boundary/renderer.ts b/ui-opentui/src/boundary/renderer.ts index bfbc3143f0b..aa5577075b9 100644 --- a/ui-opentui/src/boundary/renderer.ts +++ b/ui-opentui/src/boundary/renderer.ts @@ -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 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 diff --git a/ui-opentui/src/test/ffiSafe.test.tsx b/ui-opentui/src/test/ffiSafe.test.tsx new file mode 100644 index 00000000000..7904d9a3737 --- /dev/null +++ b/ui-opentui/src/test/ffiSafe.test.tsx @@ -0,0 +1,129 @@ +/** + * Regression: tall 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 + * . 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 + +// 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 { + 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( + () => ( + store.state.theme}> + + + ), + { 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) +}) diff --git a/ui-opentui/src/test/lib/render.ts b/ui-opentui/src/test/lib/render.ts index c0c305eccc0..c73d933b26f 100644 --- a/ui-opentui/src/test/lib/render.ts +++ b/ui-opentui/src/test/lib/render.ts @@ -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 () => {