Support indented hard-wrapped links - #124
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:
📝 WalkthroughWalkthroughIntroduces shared hard-wrap-aware link preparation and canonical target resolution. Renderer integration separates preparation from resolution, while embedded rendering gains token-associated presentation callbacks propagated through platform frame lifecycles. ChangesPrepared link resolution
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant Embedder
participant Surface
participant RendererThread
participant Frame
participant PlatformLayer
Embedder->>Surface: renderNowWithToken(token)
Surface->>RendererThread: renderNowWithPresentation
RendererThread->>Frame: drawFrameWithPresentation
Frame->>PlatformLayer: present surface with callback and token
PlatformLayer-->>Embedder: render_presented_cb(userdata, token)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Greptile SummaryThis PR adds support for recognizing terminal links (URLs and file paths) that are split across physical lines by prose hard-wrapping — a real CRLF followed by leading indentation after common link-break punctuation. A new
Confidence Score: 4/5Safe to merge for the common single-wrap case; the one-level expansion limit and sentinel map-entry are edge cases with no data-loss risk. The core normalization logic in link_wrap.zig is well-tested and handles CRLF, null-byte cell padding, and break-punctuation guards correctly. The main gap is that expandHardWrappedLinkSelection only reaches one adjacent row in each direction, so a path split across three terminal lines is only reliably clickable from its middle segment. src/Surface.zig — expandHardWrappedLinkSelection and the four-map caching block in linkAtScreenPin deserve a second look for multi-wrap correctness. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Mouse click / hover on cell] --> B[linkAtScreenPin]
B --> C{hard_wrap_continuations?}
C -- No --> D[selectLine to linkCandidateMap normalize=false]
C -- Yes --> E[selectLine to expandHardWrappedLinkSelection plus-minus 1 adjacent line]
E --> F[linkCandidateMap normalize=true terminate_joined=true]
F --> G[link_wrap.normalize strip CRLF+indent rewrite cell map]
G --> H[searchIterator regex match on normalized string]
D --> H
H --> I{match contains mouse pin?}
I -- No --> J[try next link]
I -- Yes --> K[Return Link with selection and hard_wrap flag]
K --> L{open or preview?}
L --> M[linkText normalize=true terminate_joined=false]
L --> N[link.selection used for cell highlighting]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Mouse click / hover on cell] --> B[linkAtScreenPin]
B --> C{hard_wrap_continuations?}
C -- No --> D[selectLine to linkCandidateMap normalize=false]
C -- Yes --> E[selectLine to expandHardWrappedLinkSelection plus-minus 1 adjacent line]
E --> F[linkCandidateMap normalize=true terminate_joined=true]
F --> G[link_wrap.normalize strip CRLF+indent rewrite cell map]
G --> H[searchIterator regex match on normalized string]
D --> H
H --> I{match contains mouse pin?}
I -- No --> J[try next link]
I -- Yes --> K[Return Link with selection and hard_wrap flag]
K --> L{open or preview?}
L --> M[linkText normalize=true terminate_joined=false]
L --> N[link.selection used for cell highlighting]
Reviews (1): Last reviewed commit: "fix: join indented hard-wrapped links" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
e10e36c to
0768b05
Compare
![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 513-518: Update newSurfaceOptions to copy render_presented_cb and
render_presented_userdata from ghostty_surface_inherited_config into the
inherited surface options, alongside the existing IO and renderer callbacks.
Preserve the callback values when creating child surfaces so tokened renders can
be acknowledged.
🪄 Autofix (Beta)
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
Run ID: 17f12e71-f8fd-4c9d-950e-0e44b8c85934
📒 Files selected for processing (13)
include/ghostty.hsrc/Surface.zigsrc/apprt/embedded.zigsrc/config/url.zigsrc/renderer.zigsrc/renderer/Metal.zigsrc/renderer/OpenGL.zigsrc/renderer/Thread.zigsrc/renderer/generic.zigsrc/renderer/link.zigsrc/renderer/metal/Frame.zigsrc/renderer/metal/IOSurfaceLayer.zigsrc/renderer/opengl/Frame.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/link.zig
2 participants
Recognize built-in URL and path links split by a real CRLF/newline plus indentation after common link punctuation.
The same normalized byte-to-cell map drives hit testing, opened text, and Cmd-hover highlighting. Custom matchers retain literal newline behavior.
Regression coverage uses the exact two-line
.apppath shape, verifies clicks from both rows, excludes indentation and sentence punctuation from hover/opening, and preserves semantic soft-wrap boundaries.Verification:
zig build test -Dtest-filter=hardzig build test -Dtest-filter="url regex"Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds support for URLs and file paths that are hard-wrapped with a real newline + indentation so they resolve, highlight, and copy as one link. Also adds a per-surface, tokened render presentation callback that fires only after the exact frame is shown across
MetalandOpenGL.New Features
hard_wrap_continuationsandhard_wrap_match_delimiter; enabled for built-in URL and path; custom matchers default to literal-newline behavior.ghostty_render_presented_cb; callback runs after platform presentation on bothMetalandOpenGL.Bug Fixes
Written for commit b211341. Summary will update on new commits.
Summary by CodeRabbit