Skip to content

feat: add mode-aware mouse encoding - #12

Open
pmarreck wants to merge 2 commits into
coder:mainfrom
pmarreck:feat/mouse-encoder
Open

pmarreck wants to merge 2 commits into
coder:mainfrom
pmarreck:feat/mouse-encoder

Conversation

@pmarreck

Copy link
Copy Markdown

Summary

  • expose encodeMouse(event, options) on each terminal instance
  • derive the tracking mode and wire format from the child output already fed into Ghostty
  • preserve surface-space coordinates and explicit renderer geometry for SGR-pixels correctness
  • return raw Buffer bytes suitable for writing to the child PTY
  • retain Ghostty motion-dedup state while geometry is unchanged
  • document pressed-button state, wheel mappings, and empty output for suppressed events

Coverage

Tests cover X10 tracking, normal tracking with X10 wire format, UTF-8, URXVT, SGR, SGR-pixels, press/release/motion, modifiers, buttons 1 through 11, mode transitions, disabled and suppressed events, out-of-viewport drags, same-cell motion deduplication, invalid input sets, and disposal.

A deliberate mutation that stopped resynchronization after feed() made the negotiated-mode regression test fail. An earlier implementation that reapplied unchanged geometry made the motion-dedup test fail, which is why the binding caches geometry.

Verification

  • npm run build:native
  • npm run build
  • npm run verify
  • 2 test files, 12 tests passed on x86_64-linux with Node 26.7.0 and the repository-pinned Ghostty commit

@ThomasK33 ThomasK33 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The binding is thin in the right way. It delegates to libghostty-vt's own ghostty_mouse_encoder_* / ghostty_mouse_event_* API instead of reimplementing the protocols, it reads the negotiated mode from the terminal, and the native lifetime handling is careful. I found one blocking issue: extreme (but finite) inputs reach unchecked integer conversions inside Ghostty. Details and verification are below.

Blocking

1. Out-of-range coordinates or geometry trigger illegal behavior inside Ghostty's encoder. The package builds Ghostty with ReleaseFast, so this is undefined behavior.

The binding accepts any finite float for x/y and any u32 for geometry. Ghostty then converts these with unchecked casts:

  • posToCell: @intFromFloat(clamped_x / cell_width) into a u16 (vendor/ghostty/src/renderer/size.zig:144-145). For release events this runs no matter where the pointer is (src/input/mouse_encode.zig:90-104).
  • GridSize.update: @intFromFloat(screen_width / cell_width) into a u16 (size.zig:258-259).
  • subPadding: padding.left + padding.right on u32 values (size.zig:201).
  • posToPixels: the @intFromFloat to i32 for SGR-pixels has the same problem (mouse_encode.zig:279-280).

scripts/build-libghostty-vt.sh defaults to -Doptimize=ReleaseFast, which removes these checks. What I observed on the PR head (x86_64 Linux, Node 25.9, pinned Ghostty 48ccec1):

Input (80x24 terminal, unit geometry unless noted) ReleaseFast output (shipped config) ReleaseSafe build of the same commit
SGR, release left x=1e30 y=3 ESC[<0;1;4m (column 1; the clamp says it should be 80) panic: integer part of floating point value out of bounds, process aborts (exit 134)
SGR-pixels, release left x=1e12 y=3 ESC[<0;-2147483648;3m same panic
SGR, press x=99999, screenWidth: 100000 ESC[<0;34464;4M (wrapped mod 65536) same panic
SGR, paddingLeft: 2**32-1, paddingRight: 2 ESC[<0;1;4M panic: integer overflow

Mouse events often come from a remote client, for example a browser that relays pointer events over a websocket. So one bad or malicious value can send garbage bytes to the child PTY today. With a safety-checked libghostty build, it aborts the Node process. Ghostty's own docs say out-of-range positions are allowed (mouse_encode.zig:65-68), so this is also worth reporting upstream. The binding still needs to guard, because it ships ReleaseFast. A possible fix, in both the TS normalizer and the native parser:

  • Reject geometry where screenWidth / cellWidth or screenHeight / cellHeight exceeds 65535 (the same u16 limit resize() uses), and where paddingLeft + paddingRight or paddingTop + paddingBottom exceeds the screen size. Compute the sum in 64-bit.
  • Clamp x/y so that (x - paddingLeft) / cellWidth stays within the 16-bit cell range and the SGR-pixels value fits in i32. Keep a margin outside the viewport, so out-of-viewport and drag semantics are unchanged.
  • Add regression tests for these inputs. The current invalid-input test would not catch this, because the calls return without throwing.

Non-blocking suggestions

  1. Motion dedup does not survive feed(). Every non-empty feed() sets mouse_modes_dirty_ (native/terminal.cc:258), and ghostty_mouse_encoder_setopt_from_terminal always clears last_cell (vendor/ghostty/src/terminal/c/mouse_encode.zig:195). I confirmed that in any-event mode with trackLastCell: true, motion in the same cell is emitted again after term.feed("output from child"). Child output is continuous in practice, so trackLastCell rarely helps. The PR text says dedup state is kept while geometry is unchanged, which is only true between feeds. Option: read modes with ghostty_terminal_mode_get and apply GHOSTTY_MOUSE_ENCODER_OPT_EVENT/OPT_FORMAT, which only reset last_cell when the value changes (mouse_encode.zig:146,158). Otherwise, document the limitation.

  2. Geometry is not reconciled with cols/rows. With an 80-column terminal and screenWidth: 100, a press at x=95 encodes column 96. At minimum, document that geometry must describe the same grid as resize(). Consider validating it.

  3. Buttons ten and eleven are accepted but never encoded. Ghostty's buttonCode returns null for them (src/input/mouse_encode.zig:225), although xterm defines 130/131. The test asserts empty output. The README should say so, so callers don't assume buttons 1–11 all work.

  4. Wheel "release". Legacy formats encode every release as button 3, so a wheel release looks like a normal button release. Suggest a README line saying wheel buttons should only be sent as press.

  5. supportsMouseInput. This is a new style of export. The existing capability surface is getNativeInfo(), and the constant is not documented in the README's public-contract list. Please either document it there or drop it, since typeof term.encodeMouse === "function" already works without native allocation on a terminal the caller already has.

  6. Test coverage gaps. I verified all of the following locally and they behave correctly, but none have tests:

    • button-event tracking (?1002): motion with a button is reported and motion without one is suppressed
    • legacy release encoding (X10 ESC[M#…, urxvt ESC[35;…M)
    • X10 releases and wheel presses are dropped under ?9
    • the X10 223-column limit (column 223 is encoded, 224 is dropped)
    • UTF-8 multibyte coordinates (column 100 gives 0xC2 0x85)

    Also, expect(invalidCall).toThrow() without a message pattern would pass for any error. The existing tests match on /cols/ and similar.

  7. CHANGELOG. Release notes are generated by Communique in the release/v* flow. Please check with the maintainers whether hand-written [Unreleased] entries are wanted, so the entry isn't duplicated.

What I verified by running it

  • Checked out the PR head fae62e2 locally, with nothing pushed. The PR does not change package.json, package-lock.json, binding.gyp or scripts, so I ran the standard pipeline: npm ci, npm run build:libghostty (Ghostty 48ccec1, Zig 0.15.2, ReleaseFast), npm run build:native (no compiler warnings), npm run build, and npm run verify (typecheck, then vitest with 2 files and 13 tests passing, then smoke). All passed.
  • Checked the tests' byte expectations against the protocols:
    • SGR button codes 0/1/2, wheel 64–67, and extended buttons 128/129
    • modifier bits: shift +4, alt +8, ctrl +16, and motion +32
    • X10 mode drops modifiers
    • SGR m release keeps the button code
    • urxvt adds 32
    • SGR-pixels uses raw pixel coordinates
  • Native code: event allocation and free are balanced on every path (try/catch rethrow). Output is sized with a two-pass OUT_OF_SPACE query, then a Buffer::Copy of written. Encoder and terminal are freed and nulled in DisposeNative, and the constructor's failure path cleans up. GhosttyMouseEncoderSize.size is set. The raw native object is only reached through the TS wrapper, which passes freshly built plain objects, so user getters cannot re-enter dispose() during EncodeMouse.
  • Ran the ReleaseSafe build and probes described above in a separate scratch copy.

CI: this commit has no CI results. The ci and release-changelog runs from 2026-08-19 finished as failure with zero jobs, and GitHub updated them exactly 30 days later. That matches a first-time-contributor approval request that expired. A maintainer needs to re-run or approve the workflows after the next push.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high

Comment thread native/terminal.cc
Comment on lines +300 to +307
size.screen_width = PositiveUint32(env, geometry.Get("screenWidth"), "screenWidth");
size.screen_height = PositiveUint32(env, geometry.Get("screenHeight"), "screenHeight");
size.cell_width = PositiveUint32(env, geometry.Get("cellWidth"), "cellWidth");
size.cell_height = PositiveUint32(env, geometry.Get("cellHeight"), "cellHeight");
size.padding_top = NonNegativeUint32(env, geometry.Get("paddingTop"), "paddingTop");
size.padding_bottom = NonNegativeUint32(env, geometry.Get("paddingBottom"), "paddingBottom");
size.padding_right = NonNegativeUint32(env, geometry.Get("paddingRight"), "paddingRight");
size.padding_left = NonNegativeUint32(env, geometry.Get("paddingLeft"), "paddingLeft");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking (see review item 1). Each field is bounded only to u32. Ghostty then computes screen_width / cell_width into a u16 (vendor/ghostty/src/renderer/size.zig:258-259) and padding.left + padding.right in u32 (size.zig:201). Both are unchecked in the shipped ReleaseFast build. screenWidth: 100000 produced column 34464 for x=99999, and paddingLeft: 2**32-1, paddingRight: 2 hit panic: integer overflow in a ReleaseSafe build. Please reject grids wider or taller than 65535 cells (the same limit as resize()) and padding sums larger than the screen size. Compute the sums in 64-bit.

Comment thread native/terminal.cc
Comment on lines +342 to +343
FiniteFloat(env, input.Get("x"), "mouse x"),
FiniteFloat(env, input.Get("y"), "mouse y"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking (see review item 1). Any finite f32 passes. Ghostty's posToCell then does @intFromFloat(x / cell_width) into a u16 (size.zig:144-145), and for releases it does this wherever the pointer is. release left x=1e30 encodes column 1 instead of the clamped 80 in the shipped build and aborts the process in a ReleaseSafe build. SGR-pixels x=1e12 sends -2147483648 to the child. Please clamp x/y to a range whose cell index fits in 16 bits and whose pixel value fits in i32, with a small margin outside the viewport so out-of-viewport and drag semantics stay the same.

Comment thread native/terminal.cc
size.padding_left = NonNegativeUint32(env, geometry.Get("paddingLeft"), "paddingLeft");

if (mouse_modes_dirty_) {
ghostty_mouse_encoder_setopt_from_terminal(encoder, terminal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: setopt_from_terminal always clears last_cell (vendor/ghostty/src/terminal/c/mouse_encode.zig:195), and every non-empty feed() marks the modes dirty. So trackLastCell dedup is lost whenever the child prints anything. I confirmed a duplicate same-cell motion after term.feed("output from child"). You could read the modes with ghostty_terminal_mode_get and set OPT_EVENT/OPT_FORMAT, which only reset on change (lines 146/158). Otherwise, document the limitation.

Comment thread src/index.ts

// Fill stable defaults while preserving explicit renderer geometry for both
// cell-based and pixel-based terminal protocols.
function normalizeMouseOptions(options: MouseEncoderOptions): MouseEncoderOptions {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the native bounds from review item 1 exist, it would help to mirror them here (grid size of at most 65535 cells, padding sums no larger than the screen, clamped or bounded x/y). Then callers get the same TypeError style as the other validators, before any native call.

Comment thread src/index.ts
} from "./types.js";

/** Import-time capability marker for consumers which must avoid native allocation. */
export const supportsMouseInput = true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: the existing capability surface is getNativeInfo(), and this constant isn't listed in the README's public-contract section. Please document it there or drop it. typeof term.encodeMouse === "function" already works without extra native allocation.

Comment thread test/terminal.test.ts
];

for (const invalidCall of invalidCalls) {
expect(invalidCall).toThrow();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: toThrow() without a pattern passes for any error. The existing tests match on messages (/cols/, /disposed/). Please add regression cases for the extreme inputs in review item 1 once they are bounded, plus the missing tests for ?1002, legacy release encoding, the X10 223-column limit, and UTF-8 multibyte coordinates.

Comment thread README.md
Comment on lines +77 to +80
Buttons `four`, `five`, `six`, and `seven` conventionally represent wheel up,
wheel down, wheel left, and wheel right. The binding keeps Ghostty's names at
this low-level API boundary so consumers can provide their own user-facing
aliases.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: two things worth documenting here. First, ten and eleven are accepted but always return an empty buffer, because Ghostty's buttonCode has no mapping for them (src/input/mouse_encode.zig:225). Second, wheel buttons should only be sent as press, because legacy formats encode any release as button 3.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants