feat: strip shift modifier for link detection and highlighting#13
Open
dortort wants to merge 1 commit intomanaflow-ai:mainfrom
Open
feat: strip shift modifier for link detection and highlighting#13dortort wants to merge 1 commit intomanaflow-ai:mainfrom
dortort wants to merge 1 commit intomanaflow-ai:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThe changes modify link detection across two modules to ignore the shift modifier. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
7 tasks
Allow Shift alongside Cmd for terminal link activation. Shift toggles the browser destination (cmux vs system) in the apprt layer, so the core link detection and renderer highlighting should treat Cmd+Shift the same as Cmd. - linkAtPos: strip shift before OSC8 and configured-link checks - modsChanged: invalidate link_point cache so cursor callback re-evaluates with new mods - renderer/generic: strip shift before OSC8 highlight check and renderCellMap call Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dortort
force-pushed
the
feat/cmd-shift-link-toggle
branch
from
068064f to
1facf99
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cli/list_themes.zig">
<violation number="1" location="src/cli/list_themes.zig:824">
P2: In cmux mode, Enter exits without applying the currently highlighted theme if no prior navigation occurred. Call the cmux sync before setting `outcome = .apply` so Enter always applies the visible selection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
src/cli/list_themes.zig
Outdated
Comment on lines
+824
to
+825
| self.outcome = .apply; | ||
| self.should_quit = true; |
There was a problem hiding this comment.
P2: In cmux mode, Enter exits without applying the currently highlighted theme if no prior navigation occurred. Call the cmux sync before setting outcome = .apply so Enter always applies the visible selection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/list_themes.zig, line 824:
<comment>In cmux mode, Enter exits without applying the currently highlighted theme if no prior navigation occurred. Call the cmux sync before setting `outcome = .apply` so Enter always applies the visible selection.</comment>
<file context>
@@ -432,40 +803,71 @@ const Preview = struct {
- self.mode = .save;
+ if (key.matchesAny(&.{ vaxis.Key.enter, vaxis.Key.kp_enter }, .{})) {
+ if (self.cmux != null) {
+ self.outcome = .apply;
+ self.should_quit = true;
+ } else {
</file context>
Suggested change
| self.outcome = .apply; | |
| self.should_quit = true; | |
| try self.applyCmuxSelectionForCurrentTheme(); | |
| self.outcome = .apply; | |
| self.should_quit = true; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/cli/list_themes.zig (1)
562-589:⚠️ Potential issue | 🟠 MajorSeed cmux mode from the active theme.
currentstill starts at the first filtered row, whilecmux_preview_*/cmux_applied_*start from the incoming cmux state. On the first render the highlighted row and local preview can disagree with the live cmux preview, andEnterthen “applies” the old cmux theme if the user never moves. Pick the initial row from the active cmux theme (initial_lightvsinitial_dark, usingui_color_scheme) afterupdateFiltered().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/list_themes.zig` around lines 562 - 589, After calling try self.updateFiltered(), if self.cmux is not null, determine the active cmux theme name using self.color_scheme (choose cmux_applied_light when .light else cmux_applied_dark), search self.filtered for the theme entry whose identifier/name matches that active value, and set self.current to that index (fall back to 0 if not found); ensure you use the existing fields CmuxThemePicker.load / self.cmux_applied_light / self.cmux_applied_dark, self.color_scheme, self.filtered, and the updateFiltered() call to locate and seed the initial highlighted row so preview/applied state and the highlighted row agree.
🧹 Nitpick comments (1)
src/Surface.zig (1)
1545-1547: Add focused regression coverage for modifier-only hover/link transitions.Please add tests that cover:
- Cmd+Shift link detection equals Cmd behavior, and
- modifier-only changes (no cursor movement) trigger link reevaluation via
link_pointinvalidation.Also applies to: 4499-4515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Surface.zig` around lines 1545 - 1547, Add regression tests that exercise modifier-only hover/link transitions by (1) verifying that Cmd+Shift produces the same link detection as Cmd alone and (2) verifying that changing modifiers without moving the cursor forces reevaluation via invalidation of Surface.mouse.link_point; specifically add tests that simulate setting cursor over a link, record link detection for Cmd, then toggle Shift while keeping Cmd and assert mouseRefreshLinks (or the code path that reads mouse.link_point) returns the same link result, and add another test that toggles modifiers while not moving the cursor and asserts that link_point has been cleared/invalidated and that a subsequent mouseRefreshLinks call re-evaluates the link. Ensure tests target the Surface code paths that touch mouse.link_point and mouseRefreshLinks so the invalidation at the assignment self.mouse.link_point = null is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/list_themes.zig`:
- Around line 1142-1171: When self.cmux != null you create a footer occupying
the last row but you do not reduce the usable content height; update layout and
scrolling math to subtract one row when cmux is active. Locate places that use
win.height (and any derived content_height/viewport_height variables used by the
list pane, preview pane, and scroll/window calculations) and change them to use
(win.height - 1) when self.cmux != null (keep footer creation via win.child and
footer.fill/UI calls unchanged); ensure any calculations of max visible rows,
index clamping, and printSegment offsets all use the reduced height so the
bottom theme row is not hidden by the footer.
- Around line 822-829: When handling Enter in list_themes.zig, don't switch to
.save if there are zero filtered themes; add a guard in the key.matchesAny block
so that in the non-cmux branch you only set self.mode = .save when the filtered
collection is non-empty (e.g., check self.filtered.items.len > 0 or equivalent
before assigning .save), otherwise treat Enter as a no-op; reference
self.filtered.items and self.current to ensure you avoid the out-of-bounds
access later in draw.
- Around line 843-850: The keypad-Down key (vaxis.Key.kp_down) is present in
both the single-row and page-down matches causing kp_down to trigger both
branches; update the page-down matcher in the key.matchesAny call that currently
lists vaxis.Key.page_down and vaxis.Key.kp_down to use the distinct keypad
page-down key (or remove kp_down from the page-down branch) so that
vaxis.Key.kp_down only appears in the one-row handler; keep the calls to
self.down(1)/self.down(20) and try self.applyCmuxSelectionForCurrentTheme()
as-is.
In `@src/renderer/generic.zig`:
- Around line 1304-1307: The code reads state.mouse outside the mutex-protected
section, causing mixed-frame link state between OSC8 (critical.links) and regex
link detection; fix by taking a single mouse snapshot while still holding the
mutex (the same snapshot used to populate critical.links) and use that snapshot
for render_link_mods and any subsequent mouse-mod reads instead of re-reading
state.mouse (referencing the variables critical.links and render_link_mods where
state.mouse is currently accessed).
---
Outside diff comments:
In `@src/cli/list_themes.zig`:
- Around line 562-589: After calling try self.updateFiltered(), if self.cmux is
not null, determine the active cmux theme name using self.color_scheme (choose
cmux_applied_light when .light else cmux_applied_dark), search self.filtered for
the theme entry whose identifier/name matches that active value, and set
self.current to that index (fall back to 0 if not found); ensure you use the
existing fields CmuxThemePicker.load / self.cmux_applied_light /
self.cmux_applied_dark, self.color_scheme, self.filtered, and the
updateFiltered() call to locate and seed the initial highlighted row so
preview/applied state and the highlighted row agree.
---
Nitpick comments:
In `@src/Surface.zig`:
- Around line 1545-1547: Add regression tests that exercise modifier-only
hover/link transitions by (1) verifying that Cmd+Shift produces the same link
detection as Cmd alone and (2) verifying that changing modifiers without moving
the cursor forces reevaluation via invalidation of Surface.mouse.link_point;
specifically add tests that simulate setting cursor over a link, record link
detection for Cmd, then toggle Shift while keeping Cmd and assert
mouseRefreshLinks (or the code path that reads mouse.link_point) returns the
same link result, and add another test that toggles modifiers while not moving
the cursor and asserts that link_point has been cleared/invalidated and that a
subsequent mouseRefreshLinks call re-evaluates the link. Ensure tests target the
Surface code paths that touch mouse.link_point and mouseRefreshLinks so the
invalidation at the assignment self.mouse.link_point = null is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5906009-8c7d-4f0a-8388-99498c589d5d
📒 Files selected for processing (5)
build.zigsrc/Surface.zigsrc/cli/list_themes.zigsrc/main_ghostty.zigsrc/renderer/generic.zig
src/cli/list_themes.zig
Outdated
Comment on lines
+822
to
+829
| if (key.matchesAny(&.{ vaxis.Key.enter, vaxis.Key.kp_enter }, .{})) { | ||
| if (self.cmux != null) { | ||
| self.outcome = .apply; | ||
| self.should_quit = true; | ||
| } else { | ||
| self.mode = .save; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't enter .save when there are zero filtered themes.
With an empty result set, the non-cmux branch still switches modes here. The next draw dereferences self.filtered.items[self.current] at Line 1287, so Enter after a no-match search turns into an out-of-bounds panic instead of a no-op.
Possible guard
if (key.matchesAny(&.{ vaxis.Key.enter, vaxis.Key.kp_enter }, .{})) {
+ if (self.filtered.items.len == 0 and self.cmux == null) return;
if (self.cmux != null) {
self.outcome = .apply;
self.should_quit = true;📝 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 (key.matchesAny(&.{ vaxis.Key.enter, vaxis.Key.kp_enter }, .{})) { | |
| if (self.cmux != null) { | |
| self.outcome = .apply; | |
| self.should_quit = true; | |
| } else { | |
| self.mode = .save; | |
| } | |
| } | |
| if (key.matchesAny(&.{ vaxis.Key.enter, vaxis.Key.kp_enter }, .{})) { | |
| if (self.filtered.items.len == 0 and self.cmux == null) return; | |
| if (self.cmux != null) { | |
| self.outcome = .apply; | |
| self.should_quit = true; | |
| } else { | |
| self.mode = .save; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/list_themes.zig` around lines 822 - 829, When handling Enter in
list_themes.zig, don't switch to .save if there are zero filtered themes; add a
guard in the key.matchesAny block so that in the non-cmux branch you only set
self.mode = .save when the filtered collection is non-empty (e.g., check
self.filtered.items.len > 0 or equivalent before assigning .save), otherwise
treat Enter as a no-op; reference self.filtered.items and self.current to ensure
you avoid the out-of-bounds access later in draw.
src/cli/list_themes.zig
Outdated
Comment on lines
+843
to
+850
| if (key.matchesAny(&.{ 'j', '+', vaxis.Key.down, vaxis.Key.kp_down, vaxis.Key.kp_add }, .{})) { | ||
| self.down(1); | ||
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) | ||
| try self.applyCmuxSelectionForCurrentTheme(); | ||
| } | ||
| if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{})) { | ||
| self.down(20); | ||
| if (key.matchesAny(&.{ 'k', '-', vaxis.Key.up, vaxis.Key.kp_up, vaxis.Key.kp_subtract }, .{})) | ||
| try self.applyCmuxSelectionForCurrentTheme(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check: confirm that keypad Down is currently matched in both movement branches.
rg -n "page_down|kp_down|kp_page_down|kp_page_up" src/cli/list_themes.zigRepository: manaflow-ai/ghostty
Length of output: 383
🏁 Script executed:
#!/bin/bash
# Check both locations mentioned: lines 843-850 and actual grep results at 457-459
echo "=== Lines 843-850 ==="
sed -n '843,850p' src/cli/list_themes.zig
echo ""
echo "=== Lines 455-465 (around grep results) ==="
sed -n '455,465p' src/cli/list_themes.zigRepository: manaflow-ai/ghostty
Length of output: 1449
kp_down is matched twice, causing keypad Down to move 21 rows instead of 1.
A keypad-Down event satisfies both the one-row handler at Line 457 and the page-down handler at Line 459, so both branches execute. Please switch the page-down block to a distinct PageDown match so kp_down only moves one row.
if (key.matchesAny(&.{ 'j', '+', vaxis.Key.down, vaxis.Key.kp_down, vaxis.Key.kp_add }, .{}))
self.down(1);
if (key.matchesAny(&.{ vaxis.Key.page_down, vaxis.Key.kp_down }, .{}))
self.down(20);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/list_themes.zig` around lines 843 - 850, The keypad-Down key
(vaxis.Key.kp_down) is present in both the single-row and page-down matches
causing kp_down to trigger both branches; update the page-down matcher in the
key.matchesAny call that currently lists vaxis.Key.page_down and
vaxis.Key.kp_down to use the distinct keypad page-down key (or remove kp_down
from the page-down branch) so that vaxis.Key.kp_down only appears in the one-row
handler; keep the calls to self.down(1)/self.down(20) and try
self.applyCmuxSelectionForCurrentTheme() as-is.
src/cli/list_themes.zig
Outdated
Comment on lines
+1142
to
+1171
| if (self.cmux != null) { | ||
| const footer = win.child(.{ | ||
| .x_off = 0, | ||
| .y_off = win.height - 1, | ||
| .width = win.width, | ||
| .height = 1, | ||
| }); | ||
| footer.fill(.{ .style = self.ui_footer() }); | ||
|
|
||
| const text = try std.fmt.allocPrint( | ||
| alloc, | ||
| " cmux live preview target={s} light={s} dark={s} t cycle target Enter apply q cancel ", | ||
| .{ | ||
| @tagName(self.cmux_target_mode), | ||
| self.cmux_preview_light orelse "inherit", | ||
| self.cmux_preview_dark orelse "inherit", | ||
| }, | ||
| ); | ||
| const max_len = @min(text.len, footer.width); | ||
| _ = footer.printSegment( | ||
| .{ | ||
| .text = text[0..max_len], | ||
| .style = self.ui_footer(), | ||
| }, | ||
| .{ | ||
| .row_offset = 0, | ||
| .col_offset = 0, | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Reserve one content row for the cmux footer.
The footer is drawn on the last terminal row, but the list pane, preview pane, and scroll/window math still treat all win.height rows as usable. In cmux mode the bottom theme row can end up hidden under the footer. Use a reduced content height when self.cmux != null so layout and scrolling match the visible area.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/list_themes.zig` around lines 1142 - 1171, When self.cmux != null you
create a footer occupying the last row but you do not reduce the usable content
height; update layout and scrolling math to subtract one row when cmux is
active. Locate places that use win.height (and any derived
content_height/viewport_height variables used by the list pane, preview pane,
and scroll/window calculations) and change them to use (win.height - 1) when
self.cmux != null (keep footer creation via win.child and footer.fill/UI calls
unchanged); ensure any calculations of max visible rows, index clamping, and
printSegment offsets all use the reduced height so the bottom theme row is not
hidden by the footer.
Comment on lines
+1304
to
+1307
| // our regex results. Strip shift — it toggles browser destination, | ||
| // not link detection. | ||
| var render_link_mods = state.mouse.mods; | ||
| render_link_mods.shift = false; |
There was a problem hiding this comment.
Use the same mouse snapshot as critical.links to avoid mixed-frame link state.
Line 1306 and Line 1313 currently read state.mouse.* after leaving the mutex-protected section. That can combine OSC8 links from one mouse state with regex links from another in the same frame.
Proposed fix
- var render_link_mods = state.mouse.mods;
+ var render_link_mods = critical.mouse.mods;
render_link_mods.shift = false;
@@
- state.mouse.point,
+ critical.mouse.point,
render_link_mods,Also applies to: 1313-1314
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/generic.zig` around lines 1304 - 1307, The code reads
state.mouse outside the mutex-protected section, causing mixed-frame link state
between OSC8 (critical.links) and regex link detection; fix by taking a single
mouse snapshot while still holding the mutex (the same snapshot used to populate
critical.links) and use that snapshot for render_link_mods and any subsequent
mouse-mod reads instead of re-reading state.mouse (referencing the variables
critical.links and render_link_mods where state.mouse is currently accessed).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/renderer/generic.zig (1)
1304-1314:⚠️ Potential issue | 🟡 MinorUse the same mouse snapshot as
critical.linksfor regex link rendering.Line 1306 and Line 1313 re-read
state.mouseafter the mutex scope, which can mix frames between OSC8 and regex link state. Usecritical.mousehere as well.Suggested fix
- var render_link_mods = state.mouse.mods; + var render_link_mods = critical.mouse.mods; render_link_mods.shift = false; @@ - state.mouse.point, + critical.mouse.point, render_link_mods,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/generic.zig` around lines 1304 - 1314, The code re-reads state.mouse after releasing the mutex which can cause inconsistent frames between OSC8 and regex link rendering; use the same mouse snapshot stored in critical (i.e., critical.mouse) when building render_link_mods and when calling self.config.links.renderCellMap so the mouse point and modifiers match the critical.links snapshot. Concretely, replace uses of state.mouse (for modifiers and point) with critical.mouse when creating render_link_mods.shift and passing the mouse point to renderCellMap to ensure both OSC8 and regex link rendering use the identical mouse snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/generic.zig`:
- Around line 1304-1314: The code re-reads state.mouse after releasing the mutex
which can cause inconsistent frames between OSC8 and regex link rendering; use
the same mouse snapshot stored in critical (i.e., critical.mouse) when building
render_link_mods and when calling self.config.links.renderCellMap so the mouse
point and modifiers match the critical.links snapshot. Concretely, replace uses
of state.mouse (for modifiers and point) with critical.mouse when creating
render_link_mods.shift and passing the mouse point to renderCellMap to ensure
both OSC8 and regex link rendering use the identical mouse snapshot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 043ae978-7faf-4401-b078-421c03d909dd
📒 Files selected for processing (2)
src/Surface.zigsrc/renderer/generic.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Surface.zig
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
Allow Shift alongside Cmd for terminal link activation. This supports the cmux feature where Cmd+Shift+Click opens links in the opposite browser (see manaflow-ai/cmux#1470).
Changes
src/Surface.ziglinkAtPos(): Strip shift from mods before OSC8 and configured-link checks so Cmd+Shift is treated like Cmd for link activationmodsChanged(): Invalidatelink_pointcache when mods change somouseRefreshLinksre-evaluates on the next cursor callbacksrc/renderer/generic.zigrenderCellMapso hover underlines appear with Cmd+Shift heldContext
Shift toggles the browser destination in the apprt layer (cmux vs system browser), not link detection itself. The core link detection and renderer highlighting should treat Cmd+Shift the same as Cmd.
🤖 Generated with Claude Code
Summary by cubic
Treat Cmd+Shift like Cmd for terminal link detection and hover highlighting, and add a
cmuxtheme picker to preview/apply themes live.renderCellMap; invalidate cached link points when mods change for instant highlight refresh.cmuxtheme picker inlist_themes: env-driven live preview, atomic writes to a managed theme block, macOS distributed notification reload, supports both/light/dark targets, and restores the original config on cancel.Written for commit 1facf99. Summary will update on new commits.
Summary by CodeRabbit