Refresh embedded mouse state when modifiers change#54
Conversation
The embedded adapter suppressed same-position cursor callbacks to avoid phantom macOS mouse moves, but that also hid stationary modifier transitions from core. Link hover and OSC 8 open state depend on the cursor callback seeing Command/Super, so a pointer parked over a link could never become over_link before release. Constraint: Preserve phantom same-coordinate move suppression for unchanged pointer state. Rejected: Add an app-side URL fallback | would duplicate Ghostty link ownership and miss OSC 8 semantics. Confidence: high Scope-risk: narrow Tested: Not run locally; parent cmux XCUITest added for behavior coverage.
The fork main advanced after cmux pinned 22fa801, so the OSC 8 stationary Cmd-click fix is merged forward instead of rewinding the fork. This keeps the parent submodule pointer fast-forwardable from manaflow-ai/ghostty main. Constraint: Parent cmux submodule pointers must reference commits reachable from the fork main branch. Confidence: high Scope-risk: narrow Tested: git diff origin/main..HEAD -- src/apprt/embedded.zig Not-tested: GhosttyKit rebuild not run yet
📝 WalkthroughWalkthroughAdds Surface.cursor_pos_mods and uses it in the cursor callback to avoid redundant updates when position delta < 1 and modifiers are unchanged. Separately, factors shaper-index advancement into advanceShaperCellIndexToX, replaces an inline loop in rebuildRow, and adds a unit test for empty-tail preedit catch-up. ChangesCursor Position Modifier Tracking
Renderer shaper index advance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 923-927: Update the misleading comment above the conditional that
checks cursor movement and modifiers (the if using self.cursor_pos, pos, and
self.cursor_pos_mods.equal(mods)); change it to state that processing is skipped
only when neither the cursor position has changed (within 1 pixel in x and y)
nor the modifier state has changed, i.e., the block suppresses duplicate events
for identical position+modifier state but allows handling when modifiers change
even if position is stationary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| // behavior, we only continue with callback logic if the cursor has | ||
| // actually moved. | ||
| if (@abs(self.cursor_pos.x - pos.x) < 1 and | ||
| @abs(self.cursor_pos.y - pos.y) < 1) return; | ||
| @abs(self.cursor_pos.y - pos.y) < 1 and | ||
| self.cursor_pos_mods.equal(mods)) return; |
There was a problem hiding this comment.
Update the cursor-move suppression comment to match the new logic.
The comment says processing continues only when the cursor moves, but the code now also processes stationary modifier transitions. Please update wording to avoid misleading future changes.
🤖 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/embedded.zig` around lines 923 - 927, Update the misleading comment
above the conditional that checks cursor movement and modifiers (the if using
self.cursor_pos, pos, and self.cursor_pos_mods.equal(mods)); change it to state
that processing is skipped only when neither the cursor position has changed
(within 1 pixel in x and y) nor the modifier state has changed, i.e., the block
suppresses duplicate events for identical position+modifier state but allows
handling when modifiers change even if position is stationary.
Greptile SummaryThis PR extends the same-position cursor-event deduplication in
Confidence Score: 4/5Safe to merge; the change is minimal and well-scoped with no expected regressions for the current macOS platform code. The dedup guard now calls src/apprt/embedded.zig — specifically the dedup guard and its associated comment. Important Files Changed
Sequence DiagramsequenceDiagram
participant Platform as macOS Platform
participant Surface as embedded.Surface
participant Core as core_surface
Note over Platform,Core: Before fix: modifier-only change suppressed
Platform->>Surface: cursorPosCallback(x, y, mods=Cmd)
Surface->>Surface: Check same pos (< 1px)?
Note right of Surface: Old: only checked position
Surface-->>Platform: return (suppressed!)
Note over Platform,Core: After fix: modifier change passes through
Platform->>Surface: cursorPosCallback(x, y, mods=Cmd)
Surface->>Surface: Check pos delta < 1 AND cursor_pos_mods.equal(mods)
Note right of Surface: mods changed, not equal, don't suppress
Surface->>Surface: cursor_pos = pos
Surface->>Surface: cursor_pos_mods = mods
Surface->>Core: cursorPosCallback(pos, mods)
Core-->>Surface: ok
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (@abs(self.cursor_pos.x - pos.x) < 1 and | ||
| @abs(self.cursor_pos.y - pos.y) < 1) return; | ||
| @abs(self.cursor_pos.y - pos.y) < 1 and | ||
| self.cursor_pos_mods.equal(mods)) return; | ||
|
|
||
| self.cursor_pos = pos; | ||
| self.cursor_pos_mods = mods; |
There was a problem hiding this comment.
Mods.equal compares side bits for unpressed modifiers
Mods.equal performs a full u16 bitcast comparison (including the sides sub-field and _padding). The sides bits track left/right identity for each modifier, but their value for an unpressed modifier is logically meaningless. If the macOS platform inconsistently populates side bits for unpressed modifiers across different events (e.g. one call leaves sides.shift = .left, the next leaves it .right while shift=false), the two Mods values would compare unequal even though the semantically visible modifier state is identical. This could cause the dedup to let through phantom events that the original guard was meant to suppress. Using mods.binding().equal(self.cursor_pos_mods.binding()) — which zeroes the side and lock bits — would be a safer anchor for this dedup check.
The Metal crash report points at GenericRenderer(Metal).rebuildRow, and the row preedit catch-up path can advance past the final shaped glyph when preedit covers the only glyph and the row tail is empty. This commit preserves the current behavior behind a small seam and adds the regression that proves the unsafe read before the fix changes it. Constraint: Regression test must be committed before the fix commit for issue ghostty-org#3369 Confidence: high Scope-risk: narrow Tested: zig build test -Demit-macos-app=false -Dtest-filter='renderer rebuild row preedit catch-up tolerates empty tail after covered glyph' fails with index out of bounds Not-tested: Full renderer/AppKit integration remains for CI after the fix
The row rebuild path skipped IME preedit cells and then advanced through shaped glyph cells as though every terminal cell had a corresponding glyph. Empty cells and other no-glyph cells break that assumption, so the catch-up cursor now stops at the shaped slice boundary and lets the existing next-run logic take over. Constraint: Fix follows the failing regression commit for issue ghostty-org#3369 Rejected: Handle this in cmux Swift lifecycle | the unsafe read is in Ghostty's renderer row iterator, independent of AppKit surface ownership Confidence: high Scope-risk: narrow Directive: Keep shaped glyph iteration bounded; terminal-cell count and shaped-glyph count are not equivalent Tested: zig build test -Demit-macos-app=false -Dtest-filter='renderer rebuild row preedit catch-up tolerates empty tail after covered glyph' Not-tested: Full app rendering and CI matrix pending in cmux PR
… into issue-3557-embedded-stationary-cmd-click
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/generic.zig (1)
3415-3431: 💤 Low valueConsider expanding test coverage for edge cases.
The test correctly validates the main "empty tail" scenario, but additional test cases would provide more comprehensive coverage:
- Empty
shaped_cellsarray- Target
xat the same position as a cell (not just after)- Multiple cells requiring advancement
- Target
xbefore the first cell🤖 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 3415 - 3431, Add more unit tests covering edge cases for advanceShaperCellIndexToX: include tests where shaped_cells is empty (verify shaper_cells_i stays 0), where target x equals a cell's x (ensure index points to that cell or correct next position per function contract), where multiple cells must be advanced (create several entries in shaped_cells and assert final shaper_cells_i), and where target x is before the first cell (assert shaper_cells_i remains 0). Use the same test pattern as "renderer rebuild row preedit catch-up tolerates empty tail after covered glyph" and reference the symbols shaped_cells, shaper_cells_i, and advanceShaperCellIndexToX to locate where to add these cases.
🤖 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.
Nitpick comments:
In `@src/renderer/generic.zig`:
- Around line 3415-3431: Add more unit tests covering edge cases for
advanceShaperCellIndexToX: include tests where shaped_cells is empty (verify
shaper_cells_i stays 0), where target x equals a cell's x (ensure index points
to that cell or correct next position per function contract), where multiple
cells must be advanced (create several entries in shaped_cells and assert final
shaper_cells_i), and where target x is before the first cell (assert
shaper_cells_i remains 0). Use the same test pattern as "renderer rebuild row
preedit catch-up tolerates empty tail after covered glyph" and reference the
symbols shaped_cells, shaper_cells_i, and advanceShaperCellIndexToX to locate
where to add these cases.
Adds a cmux-used embedded C API for selecting the semantic line under the cursor.
… issue-3557-embedded-stationary-cmd-click
…tationary-cmd-click
2 participants
Fixes the Ghostty-side root cause for https://github.com/manaflow-ai/cmux/issues/3557.\n\nThe embedded adapter suppressed same-position cursor callbacks to avoid phantom macOS mouse moves. That also hid stationary modifier transitions from core, so a pointer parked over an OSC 8 hyperlink did not refresh link hover/open state when Command was pressed before click release.\n\nThis keeps same-position suppression only when both coordinates and modifier state are unchanged.
Summary by cubic
Refreshes embedded mouse state when modifiers change so stationary Cmd-clicks on OSC 8 links work in embedded mode. Also fixes a renderer crash, adds a crash report subdirectory option, improves spaced file path link detection, and adds a cursor-line selection API. Addresses
cmuxissues 3557 and 3369.New Features
--crash-report-subdir; exported asbuild_config.crash_report_subdirand used by crash dir lookup. Validates non-empty, relative paths.ghostty_surface_select_cursor_lineselects the semantic line under the cursor.Bug Fixes
cursor_pos_modsand firecursorPosCallbackon mod changes; skip only when both position and mods are unchanged.Written for commit 5b0b60b. Summary will update on new commits. Review in cubic
Summary by CodeRabbit