Skip to content

fix(terminal): stop gluing the next line onto a modifier-clicked URL - #9100

Merged
nwparker merged 1 commit into
mainfrom
nwparker/fix-8832-wave2
Jul 17, 2026
Merged

nwparker merged 1 commit into
mainfrom
nwparker/fix-8832-wave2

Conversation

@nwparker

Copy link
Copy Markdown
Contributor

Summary

Fixes #8832.

Takes over / combines the best of #8852 (@xianjianlf2): stop path hard-wrap candidates from poisoning HTTP hit-testing, and reconstruct genuine edge-wrapped HTTP URLs safely.

Cmd-clicking a URL that ends mid-row opened it with the first word of the next line appended:

Repo: https://github.com/stablyai/orca/
Description: 123

opened https://github.com/stablyai/orca/Description instead of https://github.com/stablyai/orca/.

Root cause

findHttpLinkAtBufferPosition built logical lines from soft-wrap, framed hard-wrap HTTP, and path hard-wrap reconstruction (buildCandidateLogicalLinesForBufferPosition / #8339).

Path hard-wrap treats a URL suffix ending in / as a path start and joins the next row's path-like prefix (Description:). HTTP extraction then trims trailing : → …/orca/Description. File links never showed this because they verify path existence; HTTP links had no equivalent check.

Fix

HTTP hit-testing no longer consumes path hard-wrap candidates. It uses only:

  1. Native soft-wrap (isWrapped) — long terminal-wrapped URLs
  2. Framed TUI hard-wrap HTTP — existing frame-scoped builder
  3. Edge-wrap HTTP (buildEdgeWrappedHttpLogicalLineCandidates) — join without wrap metadata only when each earlier row reaches the terminal edge (one-cell wide-glyph tolerance), with label-row / adjacent-scheme guards and a row scan bound derived from TERMINAL_HTTP_URL_MAX_LENGTH

Credit: core approach and edge-wrap reconstruction from @xianjianlf2 in #8852 (including the length-derived row bound). This PR takes that over with a permanent #8832 regression suite and adversarial coverage.

Evidence

pnpm exec vitest run --config config/vitest.config.ts \
  src/renderer/src/components/terminal-pane/repro-8832-url-next-line.test.ts \
  src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.test.ts \
  src/renderer/src/components/terminal-pane/terminal-url-link-click.test.ts

# Full pane suite
pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/terminal-pane/

Results: 170 files / 2176 tests passed (focused: repro 12, edge-wrap 18, click 13).

Covered cases:

  • Issue repro (mid-row URL + Description: next line) → unglued URL
  • Path candidates still glue in isolation (documents why HTTP must not consume them)
  • Chinese / Windows / POSIX / relative / UNC next-line labels not glued
  • Soft-wrapped multi-line URLs still join from either row
  • Edge-wrapped multi-line URLs (no wrap metadata) still join from either row
  • Wide-glyph / multi-code-point continuation rows
  • Edge URL + label rows (Description:, HTTP/2: 200, etc.)
  • Adjacent URL on next row stays separate
  • file:// is not treated as HTTP
  • Same-row Windows path does not expand the HTTP link
  • Narrow-terminal URLs spanning >20 rows (length-derived bound)
  • Chinese path continuation on a genuine edge wrap

ELI5

When you Cmd/Ctrl-click a web link in the terminal, Orca sometimes “stitched” the next line onto it because a helper meant for file paths was also used for web links. We stopped using that file-path helper for web links. Real long links that wrap at the screen edge still work; random labels on the next line no longer get glued on.

User-regression-tradeoffs

Case Before After
Mid-row URL + next-line label (Description:) glued correct single-row URL
Soft-wrapped long URL (isWrapped) works still works
Edge-wrapped long URL (no wrap metadata) sometimes via path glue works via dedicated edge builder
Framed TUI hard-wrapped URL works still works
File path hard-wrap links works (existence check) untouched
Full-width URL + next-line path-like token that is not a label could glue still may join if earlier row reaches the edge and next row is not a recognized label/scheme — inherent ambiguity without wrap metadata; prefer soft-wrap when the terminal provides it
OSC 8 hyperlinks separate routing untouched

Supersedes: #8852 (please close in favor of this PR once reviewed).

Path hard-wrap reconstruction (from #8339) joins a URL ending mid-row
with the next line's path-like prefix, so Cmd-click opened
https://…/orca/Description instead of https://…/orca/ (#8832). File
links were protected by existence checks; HTTP links were not.

Stop feeding path hard-wrap candidates into HTTP hit-testing. Reconstruct
edge-wrapped HTTP URLs only when earlier rows reach the terminal edge
(with label-row guards and length-derived scan bounds), and keep native
soft-wrap plus framed TUI hard-wrap HTTP candidates.

Based on work by @xianjianlf2 in #8852.

Fixes #8832

Co-authored-by: MarkXian <mark-xian@foxmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds fallback reconstruction for HTTP(S) URLs spanning terminal edges when wrap metadata is unavailable. The hit-testing pipeline now includes these candidates alongside hard-wrapped candidates. New tests cover wide glyphs, multi-row and length-limited URLs, label and path boundaries, adjacent-line non-gluing, file URLs, Windows paths, and URL activation with or without native wrap metadata.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed, but it omits required template sections like Screenshots, checklist-style Testing, AI Review Report, Security Audit, and Notes. Add the missing template sections, including a screenshots/no-visual-change note, checklist testing status, AI review report, security audit, and platform-specific notes.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: preventing next-line text from being glued onto modifier-clicked terminal URLs.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.test.ts (1)

403-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise the over-limit URL branch.

Current URLs top out at 2,040 characters, so none tests crossing the 2,048-character cutoff. Add exact-limit and limit-plus-one cases, including protection against opening a truncated prefix.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8b078eab-e09c-481b-952d-ce889967916d

📥 Commits

Reviewing files that changed from the base of the PR and between 1cdc8e8 and 734c88f.

📒 Files selected for processing (5)
  • src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.test.ts
  • src/renderer/src/components/terminal-pane/edge-wrapped-terminal-http-links.ts
  • src/renderer/src/components/terminal-pane/repro-8832-url-next-line.test.ts
  • src/renderer/src/components/terminal-pane/terminal-url-link-click.test.ts
  • src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts

@nwparker

Copy link
Copy Markdown
Contributor Author

Final review (ready to merge)

Correctness

  • Root cause fix is sound: HTTP hit-testing no longer consumes path hard-wrap candidates (buildCandidateLogicalLinesForBufferPosition), which was gluing mid-row URLs to next-line labels (Description: → …/orca/Description).
  • Soft-wrap, framed hard-wrap, and dedicated edge-wrap paths cover the real multi-line cases; issue repro + adversarial suite look solid.

/perf

  • findHttpLinkAtBufferPosition only runs on Cmd/Ctrl-click activation paths (mousedown suppression + mouseup open), not hover/mousemove or render.
  • Edge builder bounds the row scan by TERMINAL_HTTP_URL_MAX_LENGTH (2048) with a width-2-aware row cap and per-line translation cache — acceptable for the click path; no continuous polling/hot-path regression.

CI / mergeability

  • verify + track-community-pr green
  • MERGEABLE / CLEAN
  • CodeRabbit nit only (exact 2048 boundary tests) — optional follow-up, not a blocker

Merging and closing superseded #8852 (credit retained for @xianjianlf2).

@nwparker
nwparker merged commit a7c40cd into main Jul 17, 2026
2 checks passed
@nwparker
nwparker deleted the nwparker/fix-8832-wave2 branch July 17, 2026 02:21
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
…tablyai#9100)

Fixes stablyai#8832.

HTTP hit-testing no longer consumes path hard-wrap candidates that poisoned mid-row URLs with next-line labels. Uses soft-wrap, framed hard-wrap HTTP, and dedicated edge-wrap reconstruction instead.

Supersedes stablyai#8852 (edge-wrap approach credit: @xianjianlf2).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Cmd-click on a URL opens it with the next line's text glued on

1 participant