Add physical-row text read API + host-owned external-hover rendering/diagnostics - #197
Open
YosukeIida wants to merge 17 commits into
Open
Add physical-row text read API + host-owned external-hover rendering/diagnostics#197YosukeIida wants to merge 17 commits into
YosukeIida wants to merge 17 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds physical-row text reads and a host-controlled external link-hover lifecycle. It includes validation, pointer invalidation, rendering arbitration, action delivery, acknowledgment handling, diagnostics, and tests. ChangesExternal link hover and physical-row text
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: ⚪ Minimal · up to No actionable merge-blocking risk remains. The remaining items are a localized test-probe initialization cleanup and the standard formatting pass, with no supplied indication of production impact. Sequence Diagram(s)sequenceDiagram
participant Host
participant SurfaceAPI
participant Surface
participant Renderer
participant AppRuntime
Host->>SurfaceAPI: Set external link hover
SurfaceAPI->>Surface: Validate scope, snapshot, pointer, and ranges
Surface->>Renderer: Schedule hover render
Renderer->>Renderer: Revalidate and merge external cells
Renderer->>AppRuntime: Deliver external_link_hover action
AppRuntime-->>Renderer: Acknowledge transition
Renderer-->>Host: Publish or clear hover token
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/apprt/embedded.zig`:
- Around line 2847-2854: Update the documentation for
ghostty_surface_read_text’s cmux variant to remove the claim that every physical
screen row emits its own newline. State instead that using unwrap = false
prevents soft-wrapped rows from being joined, and callers must validate output
row counts before mapping screen coordinates to returned-text offsets; leave the
existing false argument unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ed82e88-6684-4872-9389-3d38200dff79
📥 Commits
Reviewing files that changed from the base of the PR and between f76c132 and e196afa436e3e722fb120bc1431ac783fd4aa3b3.
📒 Files selected for processing (4)
include/ghostty.hsrc/Surface.zigsrc/apprt/embedded.zigsrc/terminal/Screen.zig
YosukeIida
force-pushed
the
cmux/physical-row-read
branch
from
August 10, 2026 13:14
e196afa to
7c8a205
Compare
…aries cmux fork addition, reviewed and scoped down from a wider proposal (the review kept only the physical-row read API — (A) — and explicitly rejected porting the accompanying ExternalHover mechanism (B), its BFS candidate search, and its mouse-press replay arbitration; those stay host-side or out of scope). `Screen.selectionString` always unwraps soft-wrapped rows into one logical line — correct for clipboard-style reads, but it means a host that maps a screen row/column (e.g. from a mouse click) back into the returned text loses row identity whenever the clicked text soft-wraps across two physical rows: the two rows collapse into one line and any row-index-based lookup desyncs. Adds an `unwrap: bool = true` option to `Screen.SelectionString`, threaded through `Surface.dumpTextLocked` and the embedded apprt's `readTextLocked`, defaulting to the historical `true` everywhere except the one new call site. `ghostty_surface_read_text_physical_rows` is the only caller that passes `false`; every existing caller (`ghostty_surface_read_text`, `ghostty_surface_read_selection`, the inspector's word-selection read) is updated to pass `true` explicitly and is behaviorally unchanged. Named `_physical_rows` rather than mirroring an earlier `_no_unwrap` working name — a negative boolean embedded in a public C symbol name reads worse than what the API actually guarantees. The contract is deliberately "does not unwrap soft-wrapped boundaries," not "exactly one line per physical row": formatters can still represent an unrendered trailing row or a final newline differently, and this API doesn't change any of that pre-existing behavior — it only stops joining wrapped rows into one line. Callers that need a fixed row count must still validate the split themselves (row-count/pad/fail-closed policy is the host's responsibility, not this API's). Tests added to `Screen.zig`: - Existing `selectionString soft wrap` test extended with an `unwrap = false` case proving the wrap boundary now round-trips as a newline (`"2EFGH\n3IJ"`) instead of the joined `"2EFGH3IJ"`. - Hard newlines and leading/inner blank rows survive a selection spanning several real (non-wrapped) row boundaries, for both `unwrap` values — proving this option only affects soft-wrap joining, never a real line break. - A wide character sitting exactly at the wrap boundary and a combining mark composed onto the row's last character both preserve row identity across the wrap under `unwrap = false`: neither the wide cell's trailing spacer nor the zero-width combining mark shifts which physical row later text lands in. All pre-existing `selectionString`/`dumpText`/clipboard/inspector callers pass the historical `true` explicitly and their existing tests are unaffected (`zig build test -Dtest-filter="Screen:"`, `-Dtest-filter="dumpText"`, `-Dtest-filter="selectionString"`: all green). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…ering
Lets the embedding host (cmux) own interactive hover rendering for a
resolved link, replacing a native regex partial-match hover for exactly
that scope, without racing the render thread or resurrecting stale state.
- `renderer/link.zig`: `PhysicalSnapshotToken` (mouse-independent identity
over a row scope fixed at mint time, never re-derived from a "current"
cell) and `HoverActivationToken` (snapshot + pointer cell/mods/epoch at
mint time). `ExternalHover` destructively invalidates on any token
mismatch (ABA-safe) and re-projects its cell ranges against current grid
bounds before rendering.
- `renderer/State.zig`: new `Mouse` fields — `pointer_cell`/`hover_input_epoch`
(native-hover-outcome-independent, unlike the pre-existing `point`),
`hover_eligible` (computed once under the mutex by the input path, never
re-derived by the renderer thread), the `external_hover` state itself, and
a last-delivered/pending-transition pair for cross-thread handoff.
- `Surface.zig`: `setExternalLinkHover`/`clearExternalLinkHover` mint/release
a token entirely inside the renderer mutex and never call the apprt
inline — Ghostty's own `processLinks -> openUrl -> performAction` already
runs synchronously under this same mutex, so a caller can only ever
observe the transition after the setter returns. `cursorPosCallback`/
`modsChanged` bump `hover_input_epoch` only on a real cell or normalized-
mods change, never sub-cell jitter.
- `renderer/generic.zig`: render-loop priority — OSC8 always invalidates and
wins outright; otherwise the stored scope is re-fingerprinted from current
screen content every frame (catches an in-place text rewrite under a
stationary pointer) and `validateOrInvalidate`d; an active override
supplies its own cell set and suppresses interactive regex hover (the
always-on `regex_always` layer is untouched). A transition is queued for
delivery only on an actual token/active change.
- `renderer/Thread.zig`: `notifyExternalHoverTransition` fetches-and-clears
the pending transition under a brief separate lock, then calls
`performAction` after releasing it — mirrors the existing
`notifySelectionChanged` precedent exactly, reusing `state`/`lockDemand`
rather than a new lock. Wired into the same 3 call sites
`notifySelectionChanged` already had.
- `apprt/action.zig`: new `external_link_hover` action carrying only the
bounded `{token_bits: [4]u64, active: bool}` value — never a path string
across the C ABI. Updated the `Action.CValue` size sentinel (this is now
the largest union member).
- `include/ghostty.h` / `apprt/embedded.zig`: new
`ghostty_surface_set_external_link_hover`/`_clear_external_link_hover`
exports and the `GHOSTTY_ACTION_EXTERNAL_LINK_HOVER` mirror. The setter
takes the caller's row scope and physical-row text (same non-unwrapped
form as `ghostty_surface_read_text_physical_rows`) and returns an opaque
activation token; the export wrappers do not take the renderer mutex
themselves since the Surface methods already do.
New tests (all passing): physical snapshot token identity across content/
scope/screen-key change and out-of-bound rejection; hover activation token
identity across pointer cell/mods/epoch; ExternalHover's destructive
invalidation on mismatch; `.set` rejecting out-of-bound ranges; and
`.replaceCells` re-validating ranges against current grid bounds.
This is new API surface with no prior behavior to regress against, so
there is no meaningful failing-test-first split for it (same as the prior
physical-row-read C export); tests are included in this commit and were
green before it landed. `zig build -Demit-macos-app=false` and every
targeted `zig build test -Dtest-filter=...` above are green, including
`ghostty.h Action.Key` (confirms the new enum entry and the updated
`CValue` size sentinel are both correct).
Ghostty side of cmux issue manaflow-ai/cmux#8810.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
ghostty-org#8810) `zig build test` caught this: `external_link_hover` had been inserted right after `mouse_over_link`, shifting every later tag's ordinal value by +1 and breaking "Action.Key preserves the public C ABI" (expected 64, found 65 for copy_title_to_clipboard). New actions must be appended at the end per the file's own "GUIDE TO ADDING NEW ACTIONS" comment, exactly to avoid this. Moved the union field, the Key enum tag, and the matching ghostty.h entries (tag enum + union field) to the end, after selection_changed — copy_title_to_clipboard and selection_changed are back at their pinned 64/65, and external_link_hover is now 66. zig build test: green (the one pre-existing test-lib-vt failure, "kitty temporary file medium preserves bool ABI", reproduces identically on the abcf569 base commit with none of this fork's changes applied — confirmed via a throwaway worktree — so it is unrelated and pre-existing, not a regression from this work). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…e range row (ghostty-org#8810) review-B-wiring-plan.md Blocking 2: range.row is an absolute viewport row (see replaceCells, which uses it directly as the drawn cell's .y with no offset by top_row), but .set() never checks it actually falls within [top_row, top_row + row_count) — the scope the call is claiming. This test proves the gap; the fix lands in the next commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…hostty-org#8810) .set() now checks r.row against [top_row, top_row + row_count) alongside the existing inverted/oversized-range checks. Since range.row is drawn directly as the cell's .y with no top_row offset (replaceCells), a range outside its own claimed scope could otherwise get accepted and later draw an underline on the wrong row once render-time bounds checking alone happened to still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…iewport-relative implementation (ghostty-org#8810) review-B-wiring-plan.md Blocking 2: ghostty.h claimed top_row was an absolute screen row and range.row was relative to top_row. Neither matches the actual Zig implementation: Surface.setExternalLinkHover's bound check and the render loop's re-fingerprint both pin top_row as .viewport, and replaceCells draws range.row directly with no top_row offset (i.e. range.row is itself an absolute VIEWPORT row). Following the wrong header contract would read/underline the wrong row on any scrolled-away-from-bottom viewport. Fixed the doc comments to describe the real contract, and added a test exercising the exact pin(.{.viewport=...}) lookup those call sites use at a nonzero viewport scroll offset and nonzero top_row, confirming it tracks the viewport rather than a fixed absolute row. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…n ineligible (ghostty-org#8810) review-B-wiring-plan.md Blocking 6: Surface wrote hover_eligible but nothing consumed it — the render loop never checked it and the epoch only bumped on pointer_cell/mods changes, so a selection starting (or mouse capture beginning) while the pointer sat still over the same cell would neither invalidate an in-flight token nor stop a new one from being minted. - cursorPosCallback now bumps hover_input_epoch when hover_eligible itself flips, independent of pointer_cell. - setExternalLinkHover rejects (returns zero) outright while !hover_eligible, mirroring the same gate native link hover already respects. - The render loop's external_active block destructively invalidates on !hover_eligible, the same as an OSC8 link taking over. No dedicated unit test: exercising cursorPosCallback/setExternalLinkHover end to end needs a full Surface+apprt harness this file has none of (its existing tests all operate on terminal.Terminal directly); zig build test and zig build -Demit-macos-app=false are both green with this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…n's result (ghostty-org#8810) review-B-wiring-plan.md Blocking 5 (Ghostty half): Thread.notifyExternalHoverTransition called performAction and threw the result away with '_ ='. Without consuming it, the core has no way to know whether the host actually committed a transition into logical ownership, and final-spec's last_published/pending_inactive ack table had no implementation at all. - New State.Mouse fields: external_hover_ack_last_published, external_hover_ack_pending_retry, external_hover_ack_retry_attempted. - externalHoverAckReducer: the pure decision from final-spec's ack table, split out so it's unit-testable without a live Thread/Surface/apprt (this file has none). active(true) commits lastPublished unconditionally on success, leaves it untouched on failure/error (no retry — the next real state change re-derives its own outcome). inactive(true) clears lastPublished only if it still matches; a failed inactive stages exactly one bounded retry via external_hover_ack_retry_attempted, never an unconditional resend loop. - notifyExternalHoverTransition now prefers a genuinely new pending transition over a staged retry (and resets retry_attempted when it fetches one, since that's real fresh state superseding whatever the retry was chasing), calls applyExternalHoverAck with performAction's actual result, and only calls wakeup.notify() after releasing the state lock. 6 new deterministic tests on externalHoverAckReducer: active mismatch (false/error) leaves lastPublished untouched, active accepted commits unconditionally even over a different prior token, inactive mismatch (succeeding for a token that isn't the current lastPublished) leaves it untouched, inactive accepted for the matching token clears it, a failed inactive stages exactly one retry and never re-arms a second time, and a new active transition is unaffected by a prior retry_attempted flag. zig build test and zig build -Demit-macos-app=false are both green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…hostty-org#8810) review-B-primitives-final.md Blocking 6: the C export doc comment still said cell range rows were relative to top_row. ghostty.h and renderer/link.zig's ExternalHover.set/replaceCells were already fixed to describe the real absolute-viewport-row contract; this was the third, still-stale copy. Now all three agree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…nt (ghostty-org#8810) Real dogfood regression: moving the pointer along a resolved hard-wrapped path (staying within the SAME registered underline ranges the whole time) makes the bottom-left indicator flicker between the full external path and a native regex fragment on every cell crossed. Root cause: `Surface.cursorPosCallback` bumps `hover_input_epoch` on every pointer cell change, and `buildHoverActivationToken` hashes that epoch together with the pointer cell itself into ONE opaque `HoverActivationToken`. `ExternalHover.validateOrInvalidate` compares this opaque token by a single memcmp, so it cannot express "the pointer cell changed but is still inside the same link's ranges, so stay valid" — any cell change looks identical to a real invalidation. This test pins the exact repro at the `ExternalHover` unit level: an override active for a 2-cell-wide range, with the pointer moving from cell A to cell B (both inside the SAME registered range), must stay valid. It fails against the current source (`validateOrInvalidate` returns `false`), confirming the regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…oken/physical/context identity (ghostty-org#8810) Fixes the regression the previous commit's failing test pins: the bottom-left hover indicator flickered between the full external path and a native regex fragment on every cell the pointer crossed while moving along a stable, still-valid resolved link. Implements review-flicker-fix-confirm.md's 5 required conditions for approving candidate (b), verbatim: 1. Input-time range-exit destructive invalidation. `Surface.cursorPosCallback`'s `pointer_cell` update and the new `Mouse.updateExternalHoverPointerCell` (renderer/State.zig) it calls destructively invalidate an active override the moment the pointer leaves its registered ranges (or the viewport), rather than waiting for the next render frame — mouse events and render frames aren't 1:1 (they can coalesce), so a render-time-only check can miss an A->outside->A sequence within a single frame. Also fixes an existing latent bug review found while implementing this: the negative-position branch set `pointer_cell = null` but did not return, so control fell through to the common path's unconditional `pointer_cell = pos_vp` a few lines later — and `posToViewport` clamps negative coordinates to `(0, 0)` rather than signaling out-of-bounds, silently resurrecting a real cell where `null` should have stuck. Two epoch bumps happened to invalidate the token anyway before this fix, masking it; removing the cell-driven epoch bump (item 3 below) would have exposed it. Fixed by computing `is_out_of_viewport` once and guarding the common path's reassignment on `!is_out_of_viewport`. 2. Setter containment guard. `ExternalHover.set` now requires `pointer_cell != null && ranges.contains(pointer_cell)`, checked at the moment of the call — the pointer can move between the host's currentness check and the C-boundary setter call. 3. Activation identity split from validity proof. `ExternalHover` now holds `token` (host-visible clear/ack identity only), `physical` (fixed scope/content/viewport identity), and `context_epoch` (mods/ eligibility ABA guard) as separate fields — `validateOrInvalidate` independently checks live pointer/ranges containment, physical identity, context epoch, and eligibility, instead of reconstructing and memcmp'ing one opaque hash that conflated "which cell" with "still the same link." `hover_input_epoch` renamed to `hover_context_epoch`, no longer bumped on a plain in-bounds cell change (range containment decides that now). 4. Viewport identity. `buildPhysicalSnapshotToken` folds in `pages.scrollbar()`'s `row_space_revision` and `offset`, at both the setter and the render-loop re-fingerprint, so a token minted at one scroll position can never validate at a different one — neither `ScreenSet.generation` nor `row_space_revision` alone changes on an ordinary scroll. 5. No performance claim bundled in. `ExternalHoverWorkService.process` (cmux host side) is untouched — this is a correctness-only fix. `Mouse.updateExternalHoverPointerCell` is a new pure method on the otherwise plain `Mouse` struct, extracted specifically so the exact control flow `cursorPosCallback` runs (including the negative-position clamp fix) is unit-testable without a live `Surface` — there is no lightweight `Surface` test fixture in this codebase. 14 new tests (review §5, verbatim) beyond the repro test from the previous commit: 2-row candidate range transitions, setter containment, mods/eligibility ABA without render-time validation, physical mismatch, viewport-identity mismatch (offset and revision independently), the existing stale-clear contract, gap/out-of-scope/viewport-exit invalidation with the (0,0)-clamp non-resurrection check, and the A->outside->A input-only ABA case — plus 3 cmux-side integration tests (same-range moves never transition away from external / real exit promotes deferred native, no revival without a fresh activation, and scroll-triggered withdrawal racing core invalidation converging idempotently) landed separately in the CmuxTerminalCore Swift package. zig build test: 3350/3366 passed, 0 failed, 16 skipped (pre-existing, unrelated to this fix). No push, no submodule pointer commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDsDZHZ4aFKQH3SoE6y2du
…ured verdicts (ghostty-org#8810) Adds bug C diagnostics (hover underline/indicator not showing) per design-hover-diagnostics-v4-final.md, without changing resolver/reducer/ setter production behavior — every early-return still rejects/accepts exactly as before; the change is that each one now also returns (and, for the setter, the caller pushes) a structured reason/verdict instead of a bare bool. link.zig: - New `ExternalHoverDiagSource`/`ExternalHoverDiagReason`/ `ExternalHoverDiagVerdict` enums, a 16-byte POD `ExternalHoverDiagEntry` (event/source/reason/verdict/flags/seq), and a fixed 64-entry `ExternalHoverDiagRing` (push/pushUnchecked/drain, no allocation, no string formatting — string formation is host-only, after drain releases the mutex). - `externalHoverDiagnosticsEnabled()`: a runtime env-var gate (`CMUX_EXTERNAL_HOVER_DIAGNOSTICS=1`), memoized once per process via an atomic, present in ALL build modes — deliberately NOT `builtin.mode`-gated, since dogfood runs a Debug cmux app against a ReleaseFast GhosttyKit. - `ExternalHover.set` now returns `ExternalHoverDiagReason` (`.none` on success) instead of `bool`, and takes a new `event: u64` parameter stored into `diagnostic_event` (only when the gate is on — received but not stored when off) alongside resetting per-activation verdict- suppression bookkeeping. - `ExternalHover.validateOrInvalidate` now returns `ExternalHoverDiagVerdict` and pushes a `source=render` diagnostic entry (via new `recordRenderVerdict`, which suppresses a repeated identical verdict after the first-for-activation one) BEFORE any `invalidate()` call clears the bookkeeping it needs. - `ExternalHover.invalidateIfPointerLeftRanges` similarly pushes a `source=input` entry (`viewportExit`/`pointerNotInRanges`) before invalidating. - Every early-return in both functions reuses the SAME structured value for the production accept/reject decision and the diagnostic entry — no parallel "diagnostic reason" re-derivation anywhere (the correctness guard this design most insists on). State.zig: `Mouse.external_hover_diag` ring field, threaded through `updateExternalHoverPointerCell`. Surface.zig: `setExternalLinkHover` takes `host_event_id: u64`, computes one structured `ExternalHoverDiagReason` across every one of its own early checks (zeroRowCount/hoverIneligible/scopeOutOfBounds/ snapshotBuildFailed) plus `ExternalHover.set`'s own, pushes it to the ring on rejection, and separately pushes `renderQueueFailed` if `queueRender()` fails right after an accepted setter (the direct reason no render/ack would otherwise follow). generic.zig: the render loop's OSC8/eligibility/scope/pin/snapshot-build checks each now push their own verdict before invalidating, and the final `validateOrInvalidate` call's returned verdict drives `external_active` (`== .valid`). embedded.zig / include/ghostty.h: `ghostty_surface_set_external_link_hover` gains a `host_event_id` parameter (same signature across Debug/Release/ ReleaseFast); new `ghostty_surface_drain_external_hover_diagnostics` destructively drains the ring under the renderer mutex into a caller-provided buffer, returning entries copied and the ring's monotonic cumulative dropped-count (the host computes its own delta). Returns 0 immediately, without touching the mutex, when the gate is off. Tests: existing `ExternalHover`/`Mouse` tests updated for the new return types; new ring FIFO/wrap/partial-drain/overflow tests (via `pushUnchecked`, since the gate's process-wide memoization makes `push` unusable for behavior tests within one test binary), POD size/ alignment + enum raw-value pinning tests, a gate-off no-op test, and a focused multi-row (nonzero top_row) selection-read test covering hard newlines, a blank row, and wide/combining glyphs with trim=false/ unwrap=false. Held for user approval per repo convention: this commit stays local to `cmux/external-hover` — not pushed, no PR, no parent submodule pointer update.
…ped_count saturates Review round2 B6: two follow-up fixes to the (C) ExternalHover diagnostics ring, left as uncommitted working-tree changes after commit 4e5fb0cd0. - Surface.zig: setExternalLinkHover's renderQueueFailed push stored its failure in .verdict (ExternalHoverDiagVerdict, source=render-only per this file's own field contract) instead of .reason (ExternalHoverDiagReason, source=setter-only). ABI-shape-consistent (both u8) but semantically wrong for a host decoder keying off source. The accept/reject decision itself is unchanged. - renderer/link.zig: ExternalHoverDiagRing.dropped_count used a wrapping add (+%=), which could roll a saturated counter back to 0 and read to the host as "nothing has ever been dropped" — the opposite of what happened. Switched to a saturating add (+|=), plus a boundary test pinning the max-value case. Local commit only, per the standing constraint holding all Ghostty submodule push/pointer-update actions for explicit user approval. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgHfFt4vvsSdhzKgPvsBcR
…hostty-org#8810 426ms delay investigation Diagnostics-only, no behavior change. Adds a new flags bit, external_hover_diag_flag_transition_snapshot (bit 1, independent of the existing first-for-activation bit 0), and pushes a source=render ring entry with it set at the exact point generic.zig's render loop creates a transition value snapshot (state.mouse.external_hover_pending_transition = ...) -- a different point in the same render-loop pass than the existing per-frame validation entry recordRenderVerdict already pushes a few lines earlier. Reuses the existing ExternalHoverDiagRing/gate/push path unconditionally (push() already checks the diagnostics env var itself); no new mechanism. This is one of two new timestamp points a design investigation asked for to localize a ~426ms delay between setter acceptance and transition delivery to the host (the left-bottom hover indicator). The other is a new stage=callbackEntry Swift-side log at the GHOSTTY_ACTION_EXTERNAL_ LINK_HOVER callback entry, committed separately in the parent cmux repo. Adds a boundary test pinning the new flag's raw value and its independence/composability with the existing first-for-activation flag. Local commit only, per the standing constraint holding all Ghostty submodule push/pointer-update actions for explicit user approval. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgHfFt4vvsSdhzKgPvsBcR
YosukeIida
force-pushed
the
cmux/physical-row-read
branch
from
August 10, 2026 14:55
7c8a205 to
0100e59
Compare
YosukeIida
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/Thread.zig (1)
2049-2061: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA transition produced by
updateFramewaits for an unrelated wake.
notifyExternalHoverTransitionruns beforeupdateFrame.generic.zigsetsexternal_hover_pending_transitioninsideupdateFrame, and it does not notifywakeup. So the transition produced by this frame is delivered only on the next renderer wake.
Surface.setExternalLinkHoverqueues exactly one render. That render produces the transition and then ends without delivery. Delivery then waits for an unrelated wake, for example the cursor blink timer at up to 600 ms. This matches the delay the diagnostics comment insrc/renderer/generic.zig(lines 1716-1728) describes.Deliver again after
updateFrameso a transition produced in this pass leaves in the same pass.🐛 Proposed fix
// Update our frame data t.updateFrame(t.flags.cursor_blink_visible) catch |err| log.warn("error rendering err={}", .{err}); + // `updateFrame` is the only producer of an external-hover transition + // (see `generic.zig`), and it cannot call the apprt itself. Deliver + // here so a transition produced by this pass is not held until an + // unrelated wake (cursor blink, mailbox traffic). + t.notifyExternalHoverTransition(); + // Draw _ = t.drawFrame(false);Apply the same ordering in
renderNow(line 982) andrenderNowWithPresentation(line 1006).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/Thread.zig` around lines 2049 - 2061, Update the render flow in the shown renderer path and in renderNow and renderNowWithPresentation so notifyExternalHoverTransition is called again immediately after updateFrame completes. Preserve the existing pre-update notification, visibility/realization guard, and error handling so transitions produced during updateFrame are delivered in the same render pass.
🧹 Nitpick comments (4)
src/apprt/action.zig (1)
480-487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider pinning the new tag ordinal in the ABI test.
The comment for
external_link_hoverstates that the tag must stay last so existing ordinals do not shift. The testAction.Key preserves the public C ABIpins onlycopy_title_to_clipboardandselection_changed. Add the new tag so a future insertion before it fails the test.♻️ Proposed test addition (outside the selected range, near line 443)
try std.testing.expectEqual( `@as`(c_int, 66), `@intFromEnum`(Key.external_link_hover), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/apprt/action.zig` around lines 480 - 487, Update the `Action.Key preserves the public C ABI` test to also assert that `Key.external_link_hover` has ordinal 66, alongside the existing ordinal checks for `copy_title_to_clipboard` and `selection_changed`, ensuring the tag remains last without shifting prior values.include/ghostty.h (1)
1723-1733: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd ABI layout assertions for the external hover structs.
ghostty_external_hover_cell_range_smirrorsrenderer/link.zig’sExternalHoverCellRange, andghostty_external_hover_diag_entry_smirrorsExternalHoverDiagEntry; the latter already has Zig compile-time assertions. Add equivalent compile-time size and offset checks here as well to prevent an ABI drift forghostty_external_hover_cell_range_s.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/ghostty.h` around lines 1723 - 1733, Add compile-time ABI layout assertions for ghostty_external_hover_cell_range_s, matching renderer/link.zig’s ExternalHoverCellRange size and field offsets; also add equivalent size and offset checks for ghostty_external_hover_diag_entry_s consistent with its existing Zig assertions. Keep the assertions adjacent to the struct definitions and validate each relevant member’s offsetof value.src/renderer/generic.zig (1)
1596-1623: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
row_count == 0where the state is stored.Line 1618 computes
top_row + row_count - 1onu32values. Ifrow_countis0, this underflows and panics in safe builds.Surface.setExternalLinkHovercurrently rejectsrow_count == 0before it callsset, so this path is not reachable today.ExternalHover.setis public and does not enforce the invariant itself, so a second caller would introduce the panic.Reject
row_count == 0insideExternalHover.setinsrc/renderer/link.zigso the invariant lives with the stored state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/generic.zig` around lines 1596 - 1623, Update ExternalHover.set in link.zig to reject row_count == 0 before storing the hover state, preserving the existing behavior for positive row counts. Keep the invariant at the state-storage boundary so the rendering logic in generic.zig cannot reach the top_row + row_count - 1 underflow.src/renderer/link.zig (1)
2575-2592: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider emitting the spacer tail for wide cells.
putHoverCell(line 137) addsx + 1for a wide cell so the underline covers both columns.replaceCellsdraws only the host-supplied columns. If a host reports a wide glyph as one column, that glyph renders half-underlined. Confirm the host contract requires both columns of a wide cell, or clamp/extend here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/link.zig` around lines 2575 - 2592, Update ExternalHover.replaceCells to ensure wide-cell hover underlines cover the spacer column, matching putHoverCell’s x + 1 behavior. Either validate and rely on the host contract that wide ranges include both columns, or extend each eligible range by one column when needed, clamped to cols, while preserving bounds and existing range filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/generic.zig`:
- Around line 1700-1735: Update the external-hover transition tracking around
external_hover_last_delivered_token so the inactive transition stores the
current invalidated token identity (zero) as the next comparison token, rather
than retaining the previously delivered active token. Preserve the pending
transition payload as { T, false } while ensuring the condition in this
render-loop block becomes false on subsequent inactive frames until a new
activation.
In `@src/renderer/link.zig`:
- Around line 1760-1795: Update buildPhysicalSnapshotToken and the corresponding
bound test to enforce the joined text limit using a byte budget derived from the
maximum columns and the configured maximum UTF-8 bytes per cell, rather than
treating max_snapshot_row_columns as a byte count. Preserve the column limit
semantics and ensure valid wide or combining-glyph content is not rejected
solely because its UTF-8 representation is larger.
In `@src/Surface.zig`:
- Around line 5584-5594: Update the scope validation in the Reason computation
to avoid adding top_row and row_count; after confirming top_row is within rows,
compare row_count against the remaining rows (rows - top_row). Apply the same
overflow-safe remaining-range comparison in renderer/link.zig’s
ExternalHover.set validation so both checks reject oversized ranges
consistently.
- Around line 5644-5686: Update the queueRender error branch in the external
hover setter to invalidate the newly created activation immediately, return zero
instead of the live hover token, and retain the renderQueueFailed diagnostic.
Keep the successful queueRender path returning
self.renderer_state.mouse.external_hover.token unchanged.
---
Outside diff comments:
In `@src/renderer/Thread.zig`:
- Around line 2049-2061: Update the render flow in the shown renderer path and
in renderNow and renderNowWithPresentation so notifyExternalHoverTransition is
called again immediately after updateFrame completes. Preserve the existing
pre-update notification, visibility/realization guard, and error handling so
transitions produced during updateFrame are delivered in the same render pass.
---
Nitpick comments:
In `@include/ghostty.h`:
- Around line 1723-1733: Add compile-time ABI layout assertions for
ghostty_external_hover_cell_range_s, matching renderer/link.zig’s
ExternalHoverCellRange size and field offsets; also add equivalent size and
offset checks for ghostty_external_hover_diag_entry_s consistent with its
existing Zig assertions. Keep the assertions adjacent to the struct definitions
and validate each relevant member’s offsetof value.
In `@src/apprt/action.zig`:
- Around line 480-487: Update the `Action.Key preserves the public C ABI` test
to also assert that `Key.external_link_hover` has ordinal 66, alongside the
existing ordinal checks for `copy_title_to_clipboard` and `selection_changed`,
ensuring the tag remains last without shifting prior values.
In `@src/renderer/generic.zig`:
- Around line 1596-1623: Update ExternalHover.set in link.zig to reject
row_count == 0 before storing the hover state, preserving the existing behavior
for positive row counts. Keep the invariant at the state-storage boundary so the
rendering logic in generic.zig cannot reach the top_row + row_count - 1
underflow.
In `@src/renderer/link.zig`:
- Around line 2575-2592: Update ExternalHover.replaceCells to ensure wide-cell
hover underlines cover the spacer column, matching putHoverCell’s x + 1
behavior. Either validate and rely on the host contract that wide ranges include
both columns, or extend each eligible range by one column when needed, clamped
to cols, while preserving bounds and existing range filtering.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d268e62-978c-4cef-8f3f-c3a40844559b
📥 Commits
Reviewing files that changed from the base of the PR and between 7c8a20533120f5574597659945af18d39e300cbc and 0100e59.
📒 Files selected for processing (9)
include/ghostty.hsrc/Surface.zigsrc/apprt/action.zigsrc/apprt/embedded.zigsrc/renderer.zigsrc/renderer/State.zigsrc/renderer/Thread.zigsrc/renderer/generic.zigsrc/renderer/link.zig
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/Thread.zig (1)
3094-3100: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake
external_link_hovercallback-safe.The embedded action callback has no general reentrancy restriction, and
ghostty_surface_freecan tear down the surface during this synchronous call. This path then dereferencesselfinapplyExternalHoverAck, which can use destroyed renderer state or self-join the renderer thread. Defer the acknowledgment through a lifetime-safe owner, or prohibit surface destruction during this callback and document the contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/Thread.zig` around lines 3094 - 3100, Make the external_link_hover handling around performAction callback-safe: do not dereference self in applyExternalHoverAck after the synchronous action, because the callback may destroy the surface; instead defer the acknowledgment through a lifetime-safe owner, or enforce and document a surface-lifetime restriction covering this callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/link.zig`:
- Around line 2572-2573: Update the second range check in the surrounding
row-scope logic to explicitly cast r.row to u32 before comparing it with
top_row, preserving the existing row_count boundary behavior and the
.rangeOutOfScope return.
In `@src/renderer/Thread.zig`:
- Around line 1714-1722: Initialize ExternalHoverUpdateOrderProbe with its
default fields before calling updateFrameAndNotifyExternalHover, ensuring
event_count starts at 0. Replace the undefined initialization or explicitly set
event_count before the callback while preserving the existing event-order
assertions.
---
Outside diff comments:
In `@src/renderer/Thread.zig`:
- Around line 3094-3100: Make the external_link_hover handling around
performAction callback-safe: do not dereference self in applyExternalHoverAck
after the synchronous action, because the callback may destroy the surface;
instead defer the acknowledgment through a lifetime-safe owner, or enforce and
document a surface-lifetime restriction covering this callback.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de877cf7-7035-40db-a09b-e648368fcd0b
📒 Files selected for processing (6)
include/ghostty.hsrc/Surface.zigsrc/apprt/action.zigsrc/renderer/Thread.zigsrc/renderer/generic.zigsrc/renderer/link.zig
🚧 Files skipped from review as they are similar to previous changes (4)
- src/apprt/action.zig
- src/renderer/generic.zig
- include/ghostty.h
- src/Surface.zig
Comment on lines
+2572
to
+2573
| if (r.row < top_row) return .rangeOutOfScope; | ||
| if (@as(u32, r.row) - top_row >= row_count) return .rangeOutOfScope; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the mixed-width comparison and the adjacent explicit conversion.
sed -n '2568,2576p' src/renderer/link.zigRepository: manaflow-ai/ghostty
Length of output: 825
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- relevant declarations and function context ---'
rg -n -C 8 'ExternalHoverCellRange|fn set|top_row|r\.row < top_row' src/renderer/link.zig
printf '%s\n' '--- Zig availability ---'
if command -v zig >/dev/null 2>&1; then
zig version
printf '%s\n' '--- formatter check ---'
zig fmt --check src/renderer/link.zig
printf '%s\n' '--- standalone mixed-width comparison check ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/mixed_width.zig" <<'ZIG'
const std = `@import`("std");
pub fn main() void {
const row: u16 = 1;
const top_row: u32 = 0;
if (row < top_row) std.debug.print("{}\n", .{row});
}
ZIG
if zig build-exe "$tmpdir/mixed_width.zig" -femit-bin="$tmpdir/mixed_width" >/dev/null 2>&1; then
echo 'mixed-width comparison compiled'
else
echo 'mixed-width comparison failed to compile'
fi
else
echo 'zig is not available'
fiRepository: manaflow-ai/ghostty
Length of output: 16733
🌐 Web query:
Zig language reference comparison operators operands integer types u16 u32 compile error
💡 Result:
In Zig, arithmetic and comparison operators do not automatically widen integer types, which is a common source of compile errors when attempting to mix different integer types like u16 and u32 [1][2][3]. ### Comparison Operators and Type Compatibility Zig's comparison operators (e.g., ==, <, >, <=, >=) require the operands to have compatible types [1][2][4]. When you attempt to compare different integer types, such as u16 and u32, the compiler will trigger a compile error because it does not perform implicit type coercion (widening) for these operations [1][5][3]. To compare operands of different integer types, you must explicitly cast one of the operands to match the other using @intCast [3][6]: // Example of fixing a compile error when comparing u16 and u32 var a: u16 = 100; var b: u32 = 200; // if (a < b) // This causes a compile error if (@as(u32, a) < b) { // Correct: explicit cast to u32 //... } ### Peer Type Resolution While Zig uses Peer Type Resolution for many expressions (determining a common type for multiple possible branches or operands), this mechanism does not perform widening for arithmetic or comparison operators to prevent ambiguity [1][5]. If the compiler were to automatically widen types, it could lead to unexpected behavior or hide bugs where a larger type is truncated, which contradicts Zig's philosophy of explicit behavior [5][6]. ### Exceptions and Comptime * Comptime-known values: If one or both operands are compile-time constants (comptime_int), the compiler may allow the comparison if the values can be evaluated and validated at compile time [6][7]. * Nullable types: Comparing optional types (e.g.,?u32) directly with a non-optional integer literal can also result in compile errors, as the types are distinct [8][9]. These must be handled by unwrapping or explicit casting to a compatible type [8]. In summary, if you encounter a compile error involving u16 and u32 in an expression, it is almost certainly because Zig requires you to be explicit about the type conversion rather than implicitly widening the smaller type to the larger one [5][3][6].
Citations:
- 1: https://ziglang.org/documentation/master/
- 2: https://ziglang.org/documentation/0.16.0/
- 3: bug? integer widening doesn't happen during arithmetic ziglang/zig#18214
- 4: https://ziglang.org/documentation/0.12.0/
- 5: Proposal: allow peer type resolution to expand integer sizes ziglang/zig#15737
- 6: always allow integer comparison operations no matter the bit width, signedness, or comptime-ness of operands ziglang/zig#2133
- 7: Sema: elide integer comparisons with guaranteed outcomes ziglang/zig#13552
- 8: incompatible comparison '?u32' and 'comptime_int' ziglang/zig#12609
- 9: peer type resolution not coercing properly ziglang/zig#12608
Cast r.row to u32 before comparing it with top_row.
r.row is u16, and top_row is u32. Zig does not implicitly widen integer operands in comparisons.
Proposed fix
- if (r.row < top_row) return .rangeOutOfScope;
+ if (`@as`(u32, r.row) < top_row) return .rangeOutOfScope;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (r.row < top_row) return .rangeOutOfScope; | |
| if (@as(u32, r.row) - top_row >= row_count) return .rangeOutOfScope; | |
| if (@as(u32, r.row) < top_row) return .rangeOutOfScope; | |
| if (@as(u32, r.row) - top_row >= row_count) return .rangeOutOfScope; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/link.zig` around lines 2572 - 2573, Update the second range
check in the surrounding row-scope logic to explicitly cast r.row to u32 before
comparing it with top_row, preserving the existing row_count boundary behavior
and the .rangeOutOfScope return.
Comment on lines
+1714
to
+1722
| test "external hover transition delivery follows the same successful render pass" { | ||
| var state: u8 = 0; | ||
| var probe: ExternalHoverUpdateOrderProbe = undefined; | ||
| probe.state = &state; | ||
| probe.renderer = .{ .probe = &probe }; | ||
|
|
||
| try updateFrameAndNotifyExternalHover(&probe, false); | ||
| try std.testing.expectEqual(@as(usize, 2), probe.event_count); | ||
| try std.testing.expectEqual([2]u8{ 1, 2 }, probe.events); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
if rg -n 'var probe: ExternalHoverUpdateOrderProbe = undefined' src/renderer/Thread.zig; then
echo "ExternalHoverUpdateOrderProbe is still initialized with undefined" >&2
exit 1
fiRepository: manaflow-ai/ghostty
Length of output: 286
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- relevant test and probe definitions ---'
rg -n -A35 -B15 'ExternalHoverUpdateOrderProbe|updateFrameAndNotifyExternalHover|notifyExternalHoverTransition' src/renderer/Thread.zig
printf '%s\n' '--- surrounding test lines ---'
sed -n '1660,1745p' src/renderer/Thread.zigRepository: manaflow-ai/ghostty
Length of output: 17206
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- Zig availability ---'
if command -v zig >/dev/null 2>&1; then
zig version
printf '%s\n' '--- formatting check ---'
zig fmt --check src/renderer/Thread.zig
else
printf '%s\n' 'zig is not available'
fi
printf '%s\n' '--- all probe field writes and reads ---'
rg -n -A18 -B4 'const ExternalHoverUpdateOrderProbe|var probe: ExternalHoverUpdateOrderProbe|probe\.event_count|probe\.events|self\.probe\.event_count' src/renderer/Thread.zigRepository: manaflow-ai/ghostty
Length of output: 2184
Initialize ExternalHoverUpdateOrderProbe before the first callback.
= undefined bypasses the default event_count = 0. updateFrame uses event_count as an index before the assertions, so the test can fail with an out-of-bounds access. Use an explicit initializer or set event_count before the callback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/Thread.zig` around lines 1714 - 1722, Initialize
ExternalHoverUpdateOrderProbe with its default fields before calling
updateFrameAndNotifyExternalHover, ensuring event_count starts at 0. Replace the
undefined initialization or explicitly set event_count before the callback while
preserving the existing event-order assertions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch now carries three related additions for
manaflow-ai/cmux#9868(the Cmd-click wrapped-path resolver):(A) Physical-row text read API
ghostty_surface_read_text_physical_rows— reads terminal text without unwrapping soft-wrapped row boundaries, so a host that maps a screen row/column back into text doesn't lose row identity when the clicked text soft-wraps.Screen.SelectionStringgrows anunwrap: bool = trueoption; every existing caller keeps the historicaltruebehavior.(B) Host-owned external-hover rendering
ghostty_surface_set_external_link_hover/ghostty_surface_clear_external_link_hover— lets the embedding host claim interactive hover rendering for a resolved link over a physical-row range, instead of Ghostty's own regex-based hover. Ghostty fingerprints and re-validates the supplied text every frame, so a stale/mismatched argument fails safe.(C) External-hover diagnostics
ghostty_surface_drain_external_hover_diagnostics/ghostty_external_hover_diag_entry_s— fixed-size POD hover-lifecycle tracing ring entries for host-side debugging, gated off by default.Note on scope history: an earlier commit on this branch described (B) as reviewed-and-rejected, intended to stay host-side only. That didn't hold up against the actual consumer code — cmux's
ExternalHoverWorkService/TerminalSurfaceRuntimeTeardownCoordinatorgenuinely call into (B) and (C), so both are real, in-scope additions here.Tests
Screen.zig:selectionString soft wrapextended with anunwrap = falsecase, plus hard-newline/blank-row and wrap-boundary (wide char, combining mark) coverage.zig build test -Dtest-filter="Screen:": 261/261 pass.zig build test -Dtest-filter="ExternalHover": 92/92 pass.Consumer
manaflow-ai/cmux#9868. That PR's submodule pin currently points at this branch's tip pending merge here; it will be re-pinned to the resulting fork-main merge commit once this lands.🤖 Generated with Claude Code
Summary by CodeRabbit