fix(ui): smart resolution + existence-validation for code-file paths#654
Merged
backnotprop merged 7 commits intomainfrom May 4, 2026
Merged
fix(ui): smart resolution + existence-validation for code-file paths#654backnotprop merged 7 commits intomainfrom
backnotprop merged 7 commits intomainfrom
Conversation
The bare-prose / backtick path detector linkifies anything that looks
like a code path. Two failure modes regularly produce dead links: prose
abbreviations like `editor/App.tsx` (real file is
`packages/editor/App.tsx`) and references to files the plan proposes
but hasn't created yet. Both 404 on click with no UX cue.
Resolves abbreviated paths via a case-insensitive suffix-match against
a cached project walk (`resolveCodeFile` in `packages/shared/resolve-file.ts`),
mirroring what `resolveMarkdownFile` already does for markdown. The walk
is pre-warmed when the plan/annotate server boots and on every
`/api/doc` request, with a 30s TTL so newly-created files can resolve
mid-review. Storing the walk as a Promise makes the cache race-safe —
concurrent callers piggyback rather than starting a second walk.
A new `POST /api/doc/exists` endpoint takes a batch of candidate paths
and reports `found` / `ambiguous` / `missing` / `unavailable` per path.
On the frontend, `useValidatedCodePaths` extracts candidates from the
markdown on load and POSTs once. The renderer reads the result via
`CodePathValidationContext`: `found` opens directly with the resolved
absolute path, `ambiguous` opens a `CodeFilePicker` popover listing all
matches (common in monorepos where `App.tsx` exists in several
packages), `missing` demotes the link to plain code, and `unavailable`
falls back to the optimistic linkification we have today. While
validation is in flight, every detected path renders as a link, so
first paint is unchanged.
The detection itself gets a shape filter (`isPlausibleCodeFilePath`)
that hard-rejects shell brace expansion (`{a,b}`), glob wildcards, and
whitespace, while explicitly allowing `[` / `]` so Next.js dynamic
routes (`app/[slug]/page.tsx`) still resolve. The bare-prose regex moves
out of `InlineMarkdown.tsx` into `code-file.ts` so the renderer and the
new server-side extractor use the same source of truth, and the
extractor strips fenced code blocks, HTML comments, and URL ranges
before scanning so it only emits candidates the renderer would actually
paint.
Pi extension mirrors the Bun changes (handler upgrade, pre-warm,
`/api/doc/exists` route). When the popout's `/api/doc` request 404s the
dialog now surfaces "File not found in repo: <path>" instead of
silently swallowing the error.
Tests: `code-file.test.ts` extended with shape-filter and Next.js-route
cases; new `extract-code-paths.test.ts` covers extraction, dedup,
fenced/HTML/URL exclusion, and the URL-with-parens regression; new
`resolve-file.test.ts` covers the suffix-match strategy, leading `./`
handling, ambiguous results, and ignored-dir behavior.
Out-of-tree linked docs (and annotate-mode files outside cwd) reference files relative to themselves. The validator was resolving against cwd only, so those paths got marked missing and the renderer demoted them to plain text — even though clicks still resolved correctly via base. Also tightens the suffix-match's leading-segment strip so `../foo.ts` no longer silently misresolves to an unrelated `foo.ts` in cwd. Cleanup: delete unused extract-code-paths import in reference-handlers, add the export entry to packages/shared so consumers don't rely on Bun's lenient subpath resolution. Add TODO(security) comments at both handleDocExists sites flagging that absolute paths bypass project-root containment. 223 tests pass (3 new resolver cases for baseDir + ../ regression).
Self-review fallout: 1. The doc-base expression `linkedDocHook.filepath ? dirname(...) : imageBaseDir` lived in two places (click-time URL builder and Viewer prop). If they drift, validator and click resolve against different bases and we silently re-introduce the demote-correct-link bug. Extract to a single useMemo. 2. The handleDocExists security TODO mentioned absolute paths in `paths[]` but I just added `base` acceptance, which has the same shape of leak (hostile sender supplies base=/secret/dir + relative path). Both vectors flagged in one TODO, mirrored Bun + Pi. 223 tests pass; both builds clean.
Review fallout: - `CodeFilePopout` hardcoded "File not found in repo" regardless of cause. The hook already captures the server's error string, so an ambiguous-path 400 (which can happen if a user clicks an optimistic link before validation completes) was surfacing as a misleading not-found message. Render the actual `error` and only show the planned/future-file caveat when the error matches "file not found". - `InlineMarkdown` emitted demoted bare-prose paths as raw strings while every other plain-text branch in `emitPlainTextWithBareUrls` routes through `transformPlainText`. Cosmetic-only today since paths rarely contain transformable content, but the divergence invites copy-paste rot. Routed through the same helper. - CLAUDE.md missed the new POST /api/doc/exists endpoint in both Plan Server and Annotate Server tables. Added. 223 tests pass; both builds clean.
When the validator is ready but a candidate path has no entry in the validated map, the extractor intentionally excluded it — e.g. inside an HTML comment or fenced code block. The renderer was optimistically linking these because gateCodePath returned 'link' for missing entries. Found during manual testing: `<!-- packages/editor/App.tsx -->` inside a paragraph (parser doesn't recognize HTML comments as block-level) was rendered as a clickable link. The extractor correctly stripped the comment, but the renderer's optimistic fallback overrode that. Also adds manual test harness: tests/manual/path-detection/ with sandbox setup + three launcher scripts (plan mode, annotate in-tree, annotate out-of-tree) covering ~30 test cases. 223 tests pass; both builds clean.
The parser doesn't recognize <!-- --> as block-level HTML, so comments inside paragraphs fall through to InlineMarkdown. The scanner then finds paths inside the comment text and linkifies them. The previous gateCodePath fix (demote when not in validated map) didn't help here because the same path appeared elsewhere in the document — the map had an entry from the non-comment occurrence. Fix: match <!-- ... --> at the top of the scanner loop and skip the entire comment. HTML comments should be invisible per CommonMark spec.
backnotprop
force-pushed
the
fix/path-detection-again
branch
from
98d6ab2 to
13a2de1
Compare
The shared ResolveResult type gained an `unavailable` variant for code files. The markdown resolver never returns it, but TS can't narrow past it without an explicit guard. Both Bun and Pi handlers now guard `not_found || unavailable` before accessing `result.path`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/api/doc: a literal miss now falls back to a case-insensitive suffix match against a cached project walk, mirroring what already works for markdown. Abbreviated paths from prose (editor/App.tsx→packages/editor/App.tsx) resolve correctly.POST /api/doc/existsendpoint + frontenduseValidatedCodePathshook. The renderer demotes paths the project doesn't contain (e.g. files the plan proposes creating) to plain code instead of leaving broken-link buttons that 404 on click.App.tsxexists in multiple packages, clicking opens a small dropdown of all matches; selecting one opens that file in the popout.The server walk is pre-warmed at plan/annotate load and on every
/api/docrequest, stored as a Promise (race-safe), with a 30s TTL so newly-created files resolve mid-review. The frontend renders optimistically (every detected path is a link on first paint) and demotes missing ones once the POST returns — first paint is unchanged.A shape filter (
isPlausibleCodeFilePath) hard-rejects shell brace expansion ({a,b}.ts), glob wildcards, and whitespace, while still allowing[/]so Next.js dynamic routes survive. Pi extension mirrors all server changes. The popout now surfaces "File not found in repo: <path>" instead of silently swallowing 404s.Original failure case:
~/.plannotator/plans/salvage-pr-380-shortcut-regist-2026-05-03-approved.mdproduced 404s foreditor/App.tsx,review-editor/App.tsx, andpackages/ui/shortcuts/{core,runtime,index}.ts. After: the first two resolve to theirpackages/...real paths; the shortcuts files render as plain code (they don't exist yet — the plan proposes creating them).Test plan
bun test packages/shared/— 218 pass./breaking suffix match)bun run --cwd apps/review buildandbun run build:hookboth succeed~/.plannotator/plans/salvage-pr-380-shortcut-regist-2026-05-03-approved.mdindev:hookand verify:editor/App.tsx(line 7) is clickable, openspackages/editor/App.tsxreview-editor/App.tsx(line 7) is clickable, openspackages/review-editor/App.tsxpackages/ui/shortcuts/core.tsetc. render as plain<code>, no shimmer, no click handlerpackages/editor/App.tsx(full path, lines 81/82/109/110) still clickable — regression checkPOST /api/doc/existsafter the initial renderapp/[slug]/page.tsxpaths still linkify in a test planApp.tsxin a test plan opens the picker showing bothpackages/editor/App.tsxandpackages/review-editor/App.tsx