Repository navigation
feat(terminal): link file paths that contain spaces as one path - #687
Conversation
Every path pattern ended a path at whitespace, so "/mnt/c/OneDrive - Contoso/a.docx" linked "/mnt/c/OneDrive" plus a relative "Contoso/a.docx", two links to the wrong files, in the terminal and the response viewer alike. findFilePathLinks() now takes a spaced path whole where the line bounds it: the whole line is the path, or it follows a list marker and/or a short "Label:". A relative spaced path needs the marker or label. The candidate must read as one path (no earlier word ends on an extension, starts a new "/" path or is an arrow). A spaced line that is one path but not linkable (unknown root, "." or ".." segment) links nothing instead of a fragment.
|
Thanks for this, aakhter. This PR makes Two things need to change before merge: 1. Prose after a list marker or label becomes part of the path (
In the terminal, A rule that fixed every line above in a quick prototype, with all of your test cases still passing: after const firstSlash = words.findIndex((word) => word.includes('/'));
if (link && firstSlash === words.length - 1 && !path.startsWith('/')) return null;The trade-off: 2. Smaller items:
Once 1 and 2 are in with their tests, I'll take another look and merge. |
|
Addressed the requested changes in efa647d.
The two residual ambiguities ( Validation: 65 targeted tests pass; typecheck, lint, frontend syntax and format checks pass. The branches combine without conflicts and pass all 121 combined link tests. Chromium also verified the prose case against shipped code. CI is green on this head, including the full unit/integration suite and xterm package tests. |
|
Thanks for the quick turnaround, aakhter. This PR makes Point 1 from last round is still open, and that is on the rule I suggested, not on your implementation: it has holes I did not test for. 1. Prose with a
Test counts and Part of this has an exact fix. A word after the first that starts with The rest cannot be solved by word shape: 2. The response viewer never sees the marker or a bold label (
Absolute paths work in all of these shapes. The Smaller items, which I can also fold in at merge time if you prefer:
Once 1 and 2 are in with their tests, I'll take another look and merge. |
|
Addressed the two required items from this round in 74a107c.
Remaining limits are explicit: paths split across other inline elements are still detected per text node; the own-line absolute-path ambiguity remains. I left the two smaller detector changes (unlinkable-span reservation for command-like lines and double-space column gaps) for your offered merge-time handling. Optional Claude markers also remain a follow-up. Validation: 109 targeted tests passed; typecheck, lint, frontend syntax and formatting checks passed. Chromium verified the actual rendered bold label and CI/CD prose case, including visible links and unchanged label text. The full local suite passed 12,341 tests (12 skipped). This branch combines cleanly with the submitted #686 fixes and all 165 combined link/viewer tests pass. CI on this exact head is green, including the full unit/integration suite and xterm package tests. |
|
Thanks again, aakhter. This PR makes One thing needs to change before merge. It is the same class as last round, this time on the absolute side. 1. Prose after an absolute path is still taken into the link ( (a) A
A (b) An absolute path after a generic label. The prefix regex on line 2256 accepts any one-to-four-word label, and line 2260 checks the allowlist only for relative paths, so
Requiring the allowlist after any label, absolute or not, restores all four: if (prefix[2] ? !SPACED_FILE_PATH_LABEL_PATTERN.test(prefix[2]) : !path.startsWith('/')) return null;The cost: I applied both guards locally: every row in both tables returns to master's links and the existing link tests still pass. Please add every line in both tables to Smaller items. I'll fold these in at merge time unless you'd rather take them:
Once 1 is in with its tests, I'll take another look and merge. |
A spaced path is no longer taken across a `path:line` suffix (grep -n, rg -n and compiler output) and an absolute path after a generic label such as `Error:` or `Note:` goes back to the ordinary detectors: any label must now be a path label. `Saved the report to: /mnt/c/My Docs/a.docx` splits as on master, pinned as a documented trade-off. Also: - a rooted candidate needs a directory before its first space, and an unlinkable one reserves its span only while its last word holds a `/`, so `/usr/bin/env node build.js` and `/review the changes in src/foo.ts` keep their links while `/etc/secret files/passwords.txt` still links nothing - a run of spaces is a column gap, not part of a name - a leading `./` is not read as a segment - the response viewer carries a bold path label that starts a later line of a paragraph (after a `<br>`), and degrades if the label pattern is missing - response viewer tests render with the options chat uses
|
Thanks for the careful review. All of it is in, including the smaller items.
Smaller items
Verification: the two spaced-path test files pass (94 tests). I also ran every non-browser test file that loads constants.js or app.js through the CI config: 138 files, 2933 tests, all passing. lint, format:check, check:frontend-syntax, check:public-assets and typecheck are clean. |
|
Thanks for another thorough round, aakhter. This PR makes One thing needs to change before merge. It is the same class as the earlier rounds, this time behind the path labels. 1. Prose after an allowed path label is taken into the link (
The round-one guard caught all of these. In 74a107c it was removed when the allowlist came in, instead of staying next to it, and the const firstSlash = words.findIndex((word) => word.includes('/'));
if (link && firstSlash === words.length - 1 && !path.startsWith('/')) return null;I ran your test file against that change: the only expectation that fails is
Smaller items, which I can fold in at merge time unless you would rather take them:
Once 1 is in with its tests, I'll take another look and merge. |
A path label says a path follows, not that the words before it belong to the path. Restore the guard that defers a relative candidate whose only slash word is its last, so `Wrote: 120 lines to src/foo.ts` links src/foo.ts as on master. `Saved to: My Docs/plan.md` and `Saved to: 03 Attachments/plan.pptx` split again as a documented trade-off; `Files: src/components and utils/x.ts` is pinned as the residual the guard does not catch. Refuse command lines whose executable sits under a bin/ or .bin/ directory, or that carry a flag word, so they keep master's links. Update the JSDoc example, the CLAUDE.md pointer and the architecture-invariants paragraph.
A side-by-side sweep of master and this branch over about 590 realistic lines found more shapes where a spaced candidate swallowed prose or a command: - An earlier word that is a lowercase connective or verb (and, or, to, in, the, is, has, contains, then, as and a few more) now makes the candidate a sentence, for own-line, list-marker and labelled forms alike: `/home/me/proj contains a.md`, `- /tmp/out has the logs in run.log`, `Path: /home/me/proj/out contains a.md`, `Files: src/components and utils/x.ts`, `Wrote: 3 files to docs/design notes/overview.md`. `at` and `of` stay out so screenshot names and `Copy of report.docx` still link whole; a name such as `Pros and Cons.md` splits as on master. - A bare `=` word is refused like the arrows. - An executable under `scripts/` or `tools/` is refused like one under `bin/` (`/home/me/proj/scripts/deploy prod.yml`). - An earlier word holding an extension anywhere (`a.ts#L12`) ends the path. The two residuals pinned earlier (`/home/me/proj contains a.md` and `Files: src/components and utils/x.ts`) now return master's links.
|
Thanks, you're right. The guard should have stayed next to the allowlist rather than being replaced by it. Pushed 5e1daaf and df9f93d. Item 1:
Smaller items, all taken:
Found and fixed before you had to (df9f93d):
Checks:
|
|
Thanks again, aakhter. This PR makes I ran master's and this branch's detectors side by side over another batch of shell, test-runner and chat output. Three things came up. All are small, so I will apply them at merge rather than send you another round. 1. A shell prompt that shows an absolute working directory links the whole command line (
A word with no letter or digit is never part of a file name, apart from a bare if (words[0].endsWith('$')) return null;
// in the earlier-word loop:
if (word !== '-' && !/[\p{L}\p{N}]/u.test(word)) return null;Your OneDrive, 2. A leading 3. The response viewer reads a text node that starts mid-line as a line of its own ( Thank you for sticking with this through six rounds. The side-by-side corpus run in your last push was the right way to close out this class of bug. I will apply the three items above with their tests and merge. |
- constants.js:2296,2301 spacedFilePathOnLine: refuse a first word ending in `$` and an earlier word with no letter or digit (other than a bare `-`), so a shell prompt showing an absolute working directory (`/mnt/c/proj ❯ code notes.md`, `/home/me/proj$ cat notes.md`) keeps master's links; the six rows from the review are pinned, plus OneDrive, `Program Files (x86)` and `Docs - Copy` still linking whole. - constants.js:2312: the leading strip is now `/^(?:~?\/|(?:\.\.?\/)+)/`, so `Saved to: ../My Docs/plan.md` links whole; an interior `..` still links nothing. Tests for the detector and the terminal provider. - app.js:3032 _linkifyFilePaths: carry any leading bold label and let spacedFilePathOnLine() judge it, and give a text node that follows another inline element a placeholder word, so `**Note:** /home/me/proj/out report.pdf` and ``Use `--config` /home/me/proj/cfg data.json`` split as in the terminal; a spaced path inside inline code still links whole. The now-unused label-pattern guard is gone. - JSDoc (findFilePathLinks, SPACED_FILE_PATH_LABEL_PATTERN, spacedFilePathOnLine) and the spaced-path paragraph in docs/architecture-invariants.md updated to match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… spaces as one path 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 sticking with this through six rounds. At merge I applied the three items: a prompt that prints an absolute working directory no longer links the whole command line (the six-row table is now regression tests), a labelled |
Problem
A file path that contains spaces (
/mnt/c/OneDrive - Contoso/report.docx,Saved to: ~/My Docs/plan.md) was cut at the first space, in the terminal and the response viewer alike, so it produced two links to the wrong files.Approach
findFilePathLinks()inconstants.jstakes a spaced path whole when the line bounds it: the whole line is the path, or the path follows a list marker and/or a shortLabel:. A relative spaced path needs the marker or the label. A spaced line that is one path but cannot be linked (an unknown root, a.or..segment) links nothing rather than a fragment. Because this is the shared helper, the terminal link provider and the response viewer behave the same, and an absolute spaced path outside the workspace goes through the existing external-preview route like any other out-of-workspace path.Tests
test/spaced-file-path-links.test.ts: own-line, labelled and bulleted forms, refusal cases, no merging of two paths on one line, and a linear-time check.npm run typecheck,lint,format:check,check:frontend-syntax,check:public-assetsandnpm run test:cipass.Saved to: <dir>/My Big Docs/report.mdand the link provider returned the whole spaced path.