Repository navigation
feat(terminal): link URLs and paths that wrap across rows or sit in table cells - #686
Conversation
…able cells
A CLI that word-wraps inside a markdown table cell or an indented paragraph
breaks a long URL or path short of the right edge and indents the rest, so
nothing is flagged isWrapped and no row is full width. Each row linked only
a fragment, and the fragment opened the wrong target.
- findTerminalUrls() (constants.js) is the URL grammar for the link provider:
a ")" that closes a "(" inside the URL is kept, an unmatched one still ends
it, so (https://...) and [text](https://...) stay clean.
- terminalUrlContinuationExtent() joins rows the CLI wrapped a URL across,
only when the fragment ends on a wrap-break character, the next row is
indented no further right than the URL started, and its first token looks
like URL material. Table columns after the first fragment are left out.
- terminalLinkLine() widens terminalPathLine() to that extent; a URL that
spans rows is emitted as one link per row, each opening the whole URL, so
the indent and other columns between the pieces are not underlined.
- terminalCellWrappedPaths() follows an absolute path down its own table
cell (space-separated or box-drawn), so two-column tables link both paths.
- A full-width table divider no longer counts as a hard-wrapped line.
- OSC 8 hyperlinks open through the same opener, web URLs only.
|
Thank you, aakhter, for this one. It teaches the terminal link provider to follow URLs and absolute paths that a CLI word-wrapped inside table cells or indented paragraphs, keeps balanced parens inside URLs, treats full-width dividers as row boundaries, and routes OSC 8 links through our own opener. The paren grammar, the divider boundary and the single 1. Cell-wrapped paths join the next listing entry onto a directory row (must fix)
This breaks the rule in 2. URL continuation glues list numbers, versions and status codes onto a URL ending in
3. OSC 8 links open with no confirmation
4. Smaller things
Once 1 and 2 are fixed with their negative tests and we have settled 3, I will review again and merge. If you would rather land the clean parts first, the paren grammar, the divider boundary and the |
|
Addressed the requested changes in 40a9528.
Validation: 88 targeted tests pass; typecheck, lint, frontend syntax and format checks pass. Both PR branches combine without conflicts and pass all 121 combined link tests. CI is green on this head, including 12,325 unit/integration tests and 261 xterm package tests. The shared working checkout was not modified. |
|
Thank you, aakhter, for the quick and thorough second round. This PR teaches the terminal link provider to follow URLs and absolute paths that a CLI word-wrapped inside table cells or indented paragraphs, keeps balanced parens inside URLs, treats full-width dividers as row boundaries, and routes OSC 8 links through our own opener. Everything from the first round landed. I re-ran the side-by-side comparison against master: the One thing still needs a change before merge, and two small ones are optional. 1. Prose words with
The first row is the shape of a Claude Code reply paragraph, so this shows up in everyday output. One rule that clears all seven: a continuation token with none of const rest = nextText.slice(indent + token.length);
if (!/[%&=/?]/.test(token) && rest.trim() !== '' && !/^ {2,}/.test(rest)) break;I ran it on a copy of this branch: all 56 tests in your three test files still pass, and all seven shapes go back to master's link. The trade-off is that a URL broken at 2. Links in the first row's other columns disappear (small)
master links 3. Hover underline on soft-wrapped URLs (nit)
Once 1 is in with its negative tests, I will merge. 2 and 3 are welcome in this PR; if they are not in by then, I will pick them up as a follow-up. |
|
Addressed required item 1 in 27c93b2 using the suggested conservative cell-end guard. A continuation with none of Validation: 111 targeted tests passed; typecheck, lint, frontend syntax and formatting checks passed; Chromium exercised the shipped detector. The branch combines cleanly with #687 at 74a107c, with all 188 combined link/viewer tests passing. CI for this exact head is green, including the full unit/integration suite and xterm package tests. Local full-suite result: 12,347 passed, 12 skipped, one failure in the untouched quick-start skill-injection test (missing injected SKILL.md). The same failure reproduces on the pre-fix head 40a9528 as well as this head; no unrelated source was changed to work around it. Optional items 2 and 3 (links in trailing table columns and whole-URL soft-wrap hover underlining) are unchanged for your offered follow-up. No shared checkout or unrelated PR was modified. |
|
Thank you, aakhter, for the third round. This PR makes terminal links follow URLs and absolute paths that a CLI word-wrapped inside table cells or indented paragraphs, keeps balanced parens inside URLs, treats full-width dividers as row boundaries, and routes OSC 8 links through our own opener with a confirmation. The cell-end rule landed exactly as discussed: all seven prose shapes from the last round keep master's link again, at detector and provider level, and the positive cases on both sides of the rule make the trade-off easy to see. Thank you for that. 1. Shell prompts, HTTP status lines and home paths still continue a URL (must fix)
The first two rows are A guard right after the scheme check at // A shell prompt (`user@host:~/dir$`), a home path and an HTTP status line
// are never the rest of a URL, however much URL punctuation they carry.
if (/^[\w.-]+@[\w.-]+:|^~\/|^HTTP\/\d/i.test(token)) break;Optional, and your call on the fixtures: refusing an unindented hard continuation (add Please add these rows as negative tests at detector and provider level, as you did for the last round. 2. Smaller things (optional)
Once 1 is in with its negative tests, I will merge. Round-2 items 2 and 3 (trailing-column links on the first row, soft-wrap hover underline) stay on my follow-up list, together with anything from 2 you leave out. |
|
@aakhter, one correction to my last review, so you don't chase a test that cannot pass. I asked for all nine rows as negative tests, but the indented |
A URL ending a row on `/` took the next row's first word whenever it carried URL punctuation, so a shell prompt, a home path or an HTTP status line could be appended to it. Refuse those tokens, and refuse a column-0 row as a hard continuation (real continuations are indented), which also keeps `status=200`, `24/7` and `really?` out. A divider row no longer continues the row above it, so a URL reaching the right edge does not absorb a full-width rule below it. Pin the new negatives at detector and provider level, re-indent the positive continuation fixtures, and pin the `self-hosted.` and `and/or` trade-offs.
|
Thanks for the careful review. Pushed a follow-up commit (fix(terminal): keep shell prompts and status lines out of wrapped URLs) that covers all of it:
Mutation check (terminal-url-continuation.test.ts, 76 tests): removing only the guard fails 5 tests, removing only |
|
Thank you, aakhter, for the fourth round. This PR makes terminal links follow URLs and absolute paths that a CLI word-wrapped inside table cells or indented paragraphs, keeps balanced parens inside URLs, treats full-width dividers as row boundaries, and routes OSC 8 links through our own opener with a confirmation. Everything from the last round landed: the prompt, home path and status line guard, the 1. A URL in the last column of a space-aligned table takes the next row's first cell (must fix)
The joined link also covers the next row's first cell, so clicking a service name opens the row above's URL. The last row is a codex table whose first column wrapped. The rule that clears them: the next row must start inside the URL's own column. A gap of two or more spaces between where the next row starts and where the URL starts on the first row means it opens an earlier column, the next record of a table. That is the test the trailing-columns branch already makes at // The next row must start inside the URL's own cell: a column gap between
// its indent and the URL on the first row means it opens an earlier
// column (the next record of a table), not the rest of the URL.
if (/ {2,}/.test(first.text.slice(indent, last.startIndex).trimStart())) break;The 2. An indented email address continues a URL (optional)
if (/^[\w.+-]+@[\w-]+(?:\.[\w-]+)+$/.test(token)) break;Once 1 is in with its negative tests, I will review again and merge. |
A URL in the last column of a space-aligned table ended on a slash, so the first cell of the next record (billing-api, CHANGELOG.md, ...) passed every continuation test and was joined onto it. The alignment check that refused this only ran when other columns followed the URL. A continuation row must now start inside the URL's own cell: a gap of two or more spaces between its indent and where the URL starts on the first row means it opens an earlier column. An indented token that is a whole email address is also refused. Adds the six table shapes and the email case as negative tests at detector and provider level, and documents the rule.
A URL ending in `/` still took the next indented row when that row was code, a label, a log or config field, a measurement or the end of a sentence. A side-by-side sweep against master turned up shapes such as `ready.` under a codex `Listening on` line, `expect(x).toBe(y)` under a jest title, `Failure/Error:` under an rspec title, `API_KEY=...` under `BASE_URL=...`, `level=info` log fields, `45%|####` tqdm bars, `12.3MB/45.0MB`, `12.3ms` and `10:00:00.123`. A continuation is now refused when it ends with a colon, when it is a measurement or clock time, or when it is a `name=value` token under a URL ending in `/` (a query needs `?` first). A token with only `-`, `_` or `.` is also refused when it carries a call paren or ends a sentence. Wiki titles such as `Mercury_(planet)`, a file name before its period, query parameters after `?`, `&` or `_`, and percent-encoding that starts with digits still join. `self-hosted.` moves from the pinned trade-offs to the negatives; the trade-off is now a lone word without sentence punctuation.
|
Thanks for the careful round again, and for the exact table of shapes. Both items are in 2a90fb8, and a few more shapes are in 1bdc341.
Mutation check: with the column guard removed, the 12 new table tests (6 detector, 6 provider) fail and the rest pass. With the email guard removed, the 2 email tests fail. Both are restored in the commit. For the new guards: colon 4 tests fail, Gate: the link test files plus every non-browser test that reads |
|
Thank you, aakhter, for the fifth round. This PR makes terminal links follow URLs and absolute paths that a CLI word-wrapped inside table cells or indented paragraphs, keeps balanced parens inside URLs, treats full-width dividers as row boundaries, and routes OSC 8 links through our own opener with a confirmation. Everything from the last round landed: the last-column rule with all six table shapes at detector and provider level ( 1. A directory value in a table cell takes the next row's file name (must fix)
Tapping On master, One rule that clears all five: a wrapper breaks a path after // A wrapper breaks a path after `/` or `-`; a fragment that ends inside a
// name (`/home/u/proj` over `config.yaml`) is a whole value, not a piece.
if (!/[/-]$/.test(joined)) break;I ran it on a copy of this branch: all 276 tests in the eight link test files pass (your two new files, Please add these shapes as negative tests at detector and provider level (checking that Once 1 is in with its negative tests, I will review again and merge. |
…name A table cell holding a whole directory (`/home/u/proj` over `config.yaml`) was joined onto the file name in the same column on the next row, which is the next record, and the joined link then replaced the correct links on both cells. A wrapper breaks a path after `/` or `-` (codex wraps cells on Unicode line-break opportunities plus a hyphen splitter, which never break at `_`), so a cell fragment must end on one of those before the next row can continue it. A directory written with its trailing slash over a relative path in the same column still joins, since it reads exactly like a wrap at `/`; pinned as a known trade-off.
A wrapper wraps every column of one table row together, so the row that carries a cell's continuation is blank to the left of that cell unless a path in a left column continued on it too. A label there (`Entry`, `│ Helper │`) opens the next record, so a directory ending in `/` or `-` (`Workspace /home/u/proj/app/` over `Entry main.py`) no longer takes the next record's file name. What still joins is a trailing-slash directory over a relative path in the same column with nothing to its left, which reads exactly like a wrap at `/`; that trade-off stays pinned.
|
Thanks for round 6. Two commits on top of 1bdc341. 1. A directory cell taking the next row's file name (8ad60fa) Your rule is in, right before the
Trade-off. The comment at the rule and a test pin the shape that still joins. While sweeping I narrowed that shape (next section), so the pinned version is the unlabelled one: Docs. I rewrote the Path-in-table sweep (ae3d314) I ran the side-by-side harness, master vs this branch, on 188 path-focused samples, at provider and detector level. They cover:
With the first commit alone, 17 samples differed from master. 13 were intended joins. One was a regression of the same kind as yours: One narrow rule fixes all four. A wrapper wraps every column of a table row together, so the row that carries a cell's continuation is blank to the left of that cell. The one exception is when a path in a left column also continued on that row, as in the two-column BLDMESH table. So a label there means the next record. There are three negative tests for it at each level. Without the rule, 6 tests fail. If I keep the rule but drop the left-column exception, the 5 two-column BLDMESH and box tests fail. If I remove both rules, 17 tests fail. After the fix, 13 samples differ from master. 12 are intended joins (box, pipe, codex narrow and two-column wraps at One thing I left alone because master does it too: if a left-column path is wrapped and a URL ending in Checks: the eight link test files pass (294 tests). Every non-browser test that reads |
|
Thank you, aakhter, for the sixth round. This PR makes terminal links follow URLs and absolute paths that a CLI word-wrapped inside table cells or indented paragraphs, keeps balanced parens inside URLs, treats full-width dividers as row boundaries, and routes OSC 8 links through our own opener with a confirmation. Round 6 landed. I re-ran the side-by-side comparison against master. The five whole-value shapes, the three labelled records ( Two small things are left. I will apply both at merge time, so no new round is needed:
Master links
The static checks and the 349 tests in the twelve link test files pass locally, and CI is green on this head. I will merge with those two changes on top. |
- constants.js:2618 terminalUrlContinuationExtent: trailingColumns now needs a one-row first line (first.startRow === first.endRow), so a URL row that soft-wraps with a column on its wrapped part no longer glues the next row onto that column (`/tmp/report.txtccc/x.html` at 41 columns); master's links come back. Negative tests for the extent and the link provider. - constants.js:2847 terminalCellWrappedPaths: comment reworded to match the JSDoc (the exception is a left-column PATH), plus the wrapped-prose case. - docs/architecture-invariants.md, CLAUDE.md: one clause that a left column of wrapped prose (a Description | Path table) also fills the continuation row, so the path beside it keeps its per-row fragment links, the safe direction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…p across rows or sit in table cells Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged in 1.46.0 (https://github.com/Ark0N/Codeman/releases/tag/codeman%401.46.0). Thank you for six rounds and for the side-by-side corpus runs; they are why this could land. I applied the two items from the last review: the |
Problem
Terminal links often opened only part of their target:
/,?or-short of the right edge and indents the rest. Nothing is flaggedisWrappedand no row reaches the last column, so each row linked a fragment.), so.../Team-Space(QA)Archive/...linked only as far as(QA.Approach
All detection is pure and lives in
constants.js; the xterm link provider interminal-ui.jsconsumes it.findTerminalUrls()is the URL grammar: a)that closes a(inside the URL is kept, an unmatched one still ends it, so(https://...)and[text](https://...)stay clean.terminalUrlContinuationExtent()joins rows a CLI wrapped a URL across, only when the fragment ends on a wrap-break character, the next row is indented no further right than the URL started, and its first token looks like URL material (not prose, not a newscheme://). Table columns after the first fragment are left out.terminalLinkLine()widensterminalPathLine()to that extent.terminalCellWrappedPaths()follows an absolute path down its own cell (space-separated or box-drawn tables). The join is accepted only when the joined text matches the file path pattern from its first character. These links displace any fragment link on the same cells.terminalLogicalLine()treats a row made only of divider characters as a boundary._openTerminalUrl(), which keeps the loopback web-tab routing. OSC 8 accepts web URLs only. Grid tiles and the split pane use the same handler.Tests
test/terminal-url-continuation.test.ts(grammar, wrap joining, divider, OSC 8 handler, provider ranges),test/terminal-cell-wrapped-paths.test.ts(two-column and box-drawn cells, negative alignment cases, provider).test/webview-loopback-links.test.tsnow reads_openTerminalUrl()for the loopback check.npm run typecheck,lint,format:check,check:frontend-syntax,check:public-assetsandnpm run test:cipass.Paths that contain spaces are a separate PR.