Repository navigation
fix(links): preserve wrapped configuration filenames - #735
opticon454 wants to merge 5 commits into
Conversation
|
Thank you for this, and for the careful tests (the Chromium click test on both rows is a nice touch). The PR makes The single-line and soft-wrap cases work well. One change is needed before this can go in: 1. master links and opens
Two smaller things: 2. 3. Docs for the new rules: Optional, if you are in there anyway: Once point 1 is fixed, |
0cbbb8e to
97d53f6
Compare
|
Addressed the review in 97d53f66, after rebasing onto release 1.47.0 ( The two hard-break shapes do not supply a reliable text-only distinction. I removed Added both reported unspaced examples and a three-row chain, plus Kobo fallback, refusal handling and session-affinity regressions. Real Chromium clicks exercise both rows with the actual registration helper and fixture HTTP responses. Also removed Validation at
The branch was force-pushed with an explicit lease on the previous head. This PR does not carry |
|
Thanks for the quick turnaround, opticon454. This round keeps unspaced hard-wrapped paths working, links a parenthesised path with spaces whole in the terminal and the Response Viewer, adds The detector side is in good shape. I ran One change is needed: 1. master opens the SVG; this branch shows "Cannot preview .svg from outside the session workspace" and opens nothing. I checked this with your
Smaller things, fine to fold into the same push:
One question is still open on my side. Last round I asked you to say so if no text signal separates a break inside a name from a break at a space, rather than pick a reading in code, and you did say so. Resolving it with a filesystem lookup on click, with the existing file winning when both exist, is itself a choice about ambiguous hard wraps, and whether Codeman should do that is a decision I have not made yet. That question does not change point 1: as long as the lookup is in the branch, it has to leave master's behaviour alone whenever the original exists. Next step on your side: the fix for point 1 with its test, plus whichever of the smaller items you want to include. |
|
Fixed point 1 in fd0e5249. The attachment lookup no longer decides whether a workspace file may open. A successful registration, any non-404 refusal, or a connection failure hands the original path to the normal opener. Only an original 404 permits the spaced lookup, and the spaced path opens only after successful registration. The clicked session is retained and hover performs no lookup. The six workspace regressions ( Also skipped Validation at
I understand that whether to keep filesystem-based disambiguation is still your decision. This correction preserves the original opener while that policy is under review. I left the optional attachment-ID reuse out of this change; the normal opener still rechecks its guards. Broader browser, mobile and performance suites were not run, and nothing was deployed. Fresh GitHub CI is pending. |
|
Thank you for the third round, opticon454. This version keeps the concatenated filename for hard wraps, hands every non-404 answer to the normal opener so workspace files open exactly as on master, and covers the six registry-rejected extensions with unit tests (and real Chromium clicks for One change is needed: 1. That comes from my optional suggestion last round, which was about the parenthesised list
A smaller one, fine in the same push: 2. Still open on my side: whether Codeman should resolve an ambiguous hard wrap with a filesystem lookup on click at all. That decision is not made, and this round does not depend on it: point 1 is in the detector and is needed either way. Once point 1 and its tests are in, plus the message fix, the remaining item on this PR is that open question. Thanks again for the careful tests and the clear write-ups. |
|
Addressed both findings in 168c15ae. Comma refusal now uses an explicit parenthesised context, including when checking a hard-wrap alternative. Ordinary own-line, list-marker and labelled candidates retain their existing comma handling, and the colon rule is unchanged. Added your three exact lines beside the parenthesised-list regression; each now links as one full path, while The shared registration helper now checks the path’s workspace scope when explaining an unsupported type. An in-workspace error names the attempted path and the attachment lookup; it does not say the path is external. External-path wording is unchanged. Unit tests cover all six reported extensions, and a Chromium case clicks both rows of the original-404/spaced-SVG-400 flow and checks the displayed error. The alternative still opens only after successful registration. Validation at
The three comma regressions and six workspace-message regressions failed before the correction. This leaves the filesystem-lookup policy question for your decision; this push does not settle it. Fresh GitHub CI is pending. Broader browser, mobile and performance suites were not run, and nothing was deployed. |
|
Thank you for the fourth round, opticon454. This PR makes Both items from last round are in, and I checked them:
Typecheck, lint, frontend syntax, formatting, browser exclusions and public assets pass here, the 13 link test files pass, and the new Chromium test passes (6 of 6). I also ran Two small things in the lookup code. Both only matter if the lookup stays:
The remaining item is the question from my earlier comments: whether Codeman should resolve an ambiguous hard wrap with a filesystem lookup on click at all (the original wins when both exist, the spaced name is tried only after a 404). That decision is still open on my side. The detector changes in this PR do not depend on it. Nothing else is needed from you for the points raised so far. |
|
Addressed both lookup edge cases in 3847e377.
The five new regressions failed before the correction. Chromium now also checks a labelled fallback and the double-404 hand-off using real clicks on both rows. Updated the architecture documentation and patch changeset. Validation at
The filesystem-lookup policy question remains open for your decision. Fresh GitHub CI is pending. Broader browser, mobile and performance suites were not run, and nothing was deployed. |
|
Thank you for the fifth round, opticon454. This PR makes Both items from last round are in, and I checked them:
Typecheck, lint, frontend syntax, formatting, browser exclusions and public assets pass here, the full test suite passes (13,548 tests), and the Chromium test passes 8 of 8. I also ran your new tests against master's sources: 38 of them fail there, so they cover the change. The remaining item is the question from my earlier comments: whether Codeman should resolve an ambiguous hard wrap with a filesystem lookup on click at all (the original wins when both exist, and the spaced name is tried only after a 404). That decision is still open on my side. Nothing else is needed from you for the points raised so far. Thank you again for the careful tests and the clear write-ups in every round. |
A terminal message such as:
linked
/mnt/data/Kobo/Koboas a folder and failed with "Folder not found". The shared path detector did not recognise.confor parenthesised filenames containing spaces.Recognise
.confand reserve each complete parenthesised spaced path before fragment detectors run. This fixes single-line and soft-wrapped paths in the terminal and inline paths in the Response Viewer, retaining existing root, prose and traversal guards. Only parenthesised candidates reject an earlier word ending in a comma; own-line, list-marker and labelled paths keep commas in directory names and filenames.For CLI-inserted hard breaks, preserve the existing concatenation without inserting spaces. The Kobo example is textually indistinguishable from a break inside an unspaced filename. When clicked, check the original filename through the existing guarded attachment registration route, with
notify: false, and try the spaced alternative only after HTTP 404. If both exist, the original wins. Every original result other than HTTP 404 goes to the normal opener, preserving workspace previews and log viewing for types the attachment registry rejects. External files retain the normal opener’s guarded registration and errors. After an original 404, the spaced alternative opens only if registration succeeds; a second 404 hands the original path to the normal opener. Labels, list markers and leading indentation inside parentheses are excluded from the lookup key. Hover performs no lookup and asynchronous resolution retains the clicked session. Both rows retain their correct clickable cells.Regression coverage includes Ark0N's two unspaced filename examples, a three-row chain, the Kobo fallback, refusal handling, session affinity, workspace SVG/Markdown/AVIF/BMP/ICO/ZIP openers after HTTP 400, accurate workspace lookup-error messages, both filenames missing, labelled and list-marked hard wraps, the three reported comma-containing path forms, shared chat linkification, separate parenthesised paths, comma-separated lists, blocked roots and traversal. Chromium uses real xterm mouse clicks on both rows and the real client registration helper, with fixture HTTP responses for missing and existing files. No live filesystem or CLI capture was used to infer a wrapping heuristic.
Removed
BUGS.md, updated the shared detector documentation and architecture invariants, and included a patch changeset.Validation at
3847e377d8a4e2f331b363827470852a99b199b9:Rebased onto release 1.47.0, upstream/master
f1f7eec4d6a54c73822d118262712535eea5fd28. This PR does not carrysrc/web/public/whats-new.json, so regeneration was not needed.The initial regression tests failed before the correction. The remaining browser, mobile and performance suites were not run. No deployment is included.
Fixes #733. Current implementation: 3847e377d8a4e2f331b363827470852a99b199b9. The filesystem-lookup policy remains subject to the maintainer’s decision.