PROTOTYPE: full-file code viewer for code review (design doc phase 1)

[backnotprop]

PROTOTYPE, do not merge. This is evaluation-grade work built to exercise
Phase 1 of DESIGN_full-file-code-review.md end to end and report back what
the design doc got right and what it missed. It is not a shipping change.

Opens a whole file inside code review, annotates any line range in it, and
round-trips that annotation into normal review feedback labelled as outside the
diff. Everything else in the doc (repo-wide tree browsing, tree-sitter nav,
open-in-editor) is deliberately out of scope.

What was built, against the doc

Doc phase 1 item	Status
packages/shared/repo-file.ts (new, vendored)	Done
GET /api/review-file, both runtimes	Done
FileViewer + REVIEW_FILE panel	Done, as ReviewFullFilePanel
Open file from tree rows / code-nav peek	Done, plus git-status rows
Snippet-extraction fallback and feedback label	Done
Retrofit cap + containment on /api/code-nav/file	Done
Focus race fix landing with phase 1 (risk 1)	Done

24 files, +1695 / -60.

The endpoint and the guard

The design doc's central security point is correct and worth restating: the
review side's entire traversal defense was validateFilePath, which rejects
.. substrings and a leading /. That is a check on the shape of a string,
and a symlink is not a shape. A repo containing link -> /etc passes it and
then reads link/passwd straight off the filesystem. Separately,
/api/code-nav/file had no size cap at all, so one request against a large
file in the tree was a memory bomb.

packages/shared/repo-file.ts replaces both with realpath containment:

1. Shape validation first (relative only, no .. segments, no NUL, no Windows
   drive prefix). This is a cheap pre-filter, deliberately not the boundary. It
   also fixes a real false positive: foo..bar.ts is a legal filename that the
   substring check rejected.
2. Realpath the review root and the candidate, then require the canonical
   candidate to live under the canonical root. Because realpath decides,
   symlink escapes fail by construction. Realpathing the root also matters on
   macOS, where /tmp is itself a symlink.
3. stat before read, so an oversized file is refused before it is resident
   in memory rather than after.

Containment uses a trailing-separator comparison, so /repo-evil/x is not
inside /repo, which is the bug a naive startsWith has.

Failure reasons map to one shared status table, so both runtimes answer alike:
invalid-path 400, outside-root 403, not-found 404, not-a-file 400,
too-large 413. A dangling symlink reports not-found, never outside-root,
so the response never confirms what exists outside the repo.

GET /api/review-file?path= returns { filePath, content, size } from the live
working tree, with no snapshot guard. That is deliberate, per the doc: a file panel
should show the file as it is now. The gate (no committed GitButler views, local
access required, PR checkouts must actually be warm) is shared with
/api/code-nav/file through one helper rather than copied, since the two had
already begun to drift between Bun and Pi.

Client

ReviewFullFilePanel renders one Pierre CodeViewFileItem, which is what buys
virtualization, the shared worker pool and line annotations for free. Confirmed
in the live run: a 907-line file keeps ~55 line elements in the DOM.

One reused panel retargeted per file, matching the diff panel (doc open
question 2's recommendation). Content-hash cacheKey, since Pierre 1.3.2
compares nothing else.

Focus arbitration landed with phase 1 as the doc insists: ToolbarHost drafts
live in module-level maps keyed by file path, so the diff panel now yields
isFocused while the full-file panel holds the same path.

Annotation round-trip

Two pieces, matching the doc's option 2:

Snippet fallback. resolveAnnotationSnippet prefers the patch and falls
back to file content. Only the new side falls back, because the working tree
is not the old side; an old-side range with no hunk coverage correctly yields
nothing.

The label. CodeAnnotation.outsideDiff is stamped at creation, where the
patch is known, the way plan-diff annotations stamp diffContext. Export adds
an [Outside diff] marker, a one-line note, and the fenced lines.

In-diff export behavior is unchanged. originalCode stays suggestion-only for
ordinary annotations (it renders as Replaces:); the selected lines travel as a
separate trailing argument and are attached only when the annotation is outside
the diff. An out-of-diff comment with no suggestion prints under
Code at these lines: rather than Replaces:, because nothing is being
replaced.

Test evidence

New: 41 endpoint/guard cases (every one runs against both Bun and Pi) and 11
round-trip cases. All green.

Guard coverage is the interesting part, since these are the cases the code it
replaces gets wrong:

• traversal via ../, embedded ../, and absolute paths
• a symlink to a file outside the repo (lexically clean, no ..)
• a directory symlink out of the repo, so the path has no .. anywhere
• an inside symlink still resolving, so containment is not merely blunt
• a dangling symlink reported as not-found
• the cap at exactly the boundary and one byte over
• /api/code-nav/file answering 403 and 413 where it previously answered 200

Typecheck (bun run typecheck) passes. apps/review/tsconfig.json is not a
clean gate on either side: pristine origin/main reports 96 errors there and
this branch reports 95, with no new error in any touched file.

Full suite: this environment fails ~500 tests on pristine origin/main
(happy-dom window.Headers, socket exhaustion, ECONNREFUSED under a 387-file
single-process run), so the raw number is meaningless. A clean baseline measured
in a separate pristine worktree fails 502 uniquely-named tests. Diffing failing test names against it, the ONLY difference on this branch
is my own 20 endpoint tests, which fail in that run for the same environmental
reason and pass everywhere else: bun test packages/server/ runs 777 tests
across 52 files with 0 failures, mine included. No pre-existing test newly
fails on this branch.

End-to-end proof

Compiled a binary, ran a real review session against a scratch repo, and drove
it with headless Playwright. The fixture is built so the demonstration cannot be
accidental: src/alpha.ts is 906 lines whose only hunk is at lines 1-3, so
an annotation at line 497 is unambiguously outside the diff.

1. Review session open. Three changed files, git-status view.

2. Full file opened from a tree row. Own dock tab, header reads
src/alpha.ts / Full file / 907 lines, syntax highlighting via the shared theme.

3. Scrolled deep into the file. Window at lines 467-532 of 907, still ~66
line elements in the DOM. Virtualization holds.

4. Annotating lines 497-500, hundreds of lines below anything in the diff.

5. Annotation created. Range highlighted, annotation badge on
src/alpha.ts, Send Feedback now offered.

6. Feedback submitted. The captured /api/feedback payload
(full JSON):

{
  "filePath": "src/alpha.ts",
  "lineStart": 497,
  "lineEnd": 500,
  "side": "new",
  "outsideDiff": true,
  "text": "This whole helper is dead code and should be deleted.",
  "originalCode": "export function alphaPart99(n: number): number {\n  // untouched region of alpha.ts, far below the diff hunk\n  return n + 99;\n}"
}

and the feedback the agent actually receives:

## src/alpha.ts

### Lines 497-500 (new) [Outside diff]
_These lines are not part of the diff under review. The file content at review time is quoted below._
This whole helper is dead code and should be deleted.

**Code at these lines:**

export function alphaPart99(n: number): number {
// untouched region of alpha.ts, far below the diff hunk
return n + 99;
}

The snippet is byte-exact against lines 497-500 of the file on disk.

Deviations from the doc, and why

The server does not return language. The doc's response shape includes an
optional language. packages/review-editor/utils/detectLanguage.ts already
maps extensions client-side, and duplicating that map into a vendored shared
module would create two copies to drift. The client derives it.

No Pi files-array change. The doc pairs vendor.sh with "the Pi files array".
apps/pi-extension/package.json already ships generated/ and server/ as
whole directories, so the vendored module and the new handler are included with
no manifest edit. Only the vendor.sh module list needed the entry.

No new @source entry. The repo rule applies to new directories containing
.tsx. The panel lives in dock/panels/, already covered by the recursive
./dock/**/*.tsx glob. Adding a redundant entry would be noise.

The affordance also went on git-status rows. The doc says "file tree rows".
The git-status sections panel is the default view of a code review and renders
its own rows, so tree-only would have made the feature invisible on open.

Panel component is ReviewFullFilePanel, not FileViewer. It follows the
Review*Panel naming every other dock panel uses.

Open questions the prototype surfaced that the doc missed

1. Pierre's drag gesture does not produce a line range on a CodeView file
   item. This is the biggest surprise. The doc assumes full-file line
   selection comes free from Pierre, citing CodeFilePopout. In practice
   neither a gutter drag nor an in-code drag fired onSelectedLinesChange here,
   with controlled or uncontrolled selection. Multi-line selection only works
   because I ported CodeFilePopout's browser-text-selection mapping (read the
   shadow-root Selection, map anchor/focus nodes to [data-line] ancestors).
   Any real implementation needs that mapping, or an answer from Pierre. It is
   not a free win, and phase 1 estimates should account for it.

2. A single-line click and a drag race each other. The drag ends with a
   click on the release line, which silently collapses the range to one line.
   CodeFilePopout guards this with a suppression window; the doc never
   mentions it, and it is invisible in code review: you just get the wrong
   annotation. Ported here.

3. CodeView does not scroll unless the host says so. Without
   overflow-y-auto and containment classes on the CodeView root, the virtual
   window never advances past the first screen and the file silently appears
   truncated. The doc treats virtualization as automatic.

4. originalCode is overloaded. It means "the lines a suggestion replaces",
   not "the annotated lines". It is only populated when a suggestion exists,
   and the exporter renders it as Replaces:. The doc's "fence the selected
   lines" therefore cannot just reuse it without either changing in-diff export
   behavior for every existing comment or threading a separate value. I chose
   the latter; the maintainer may prefer a dedicated field.

5. Where does "outside the diff" get decided? I stamp it at creation because
   only the app holds the patch. That means an annotation's label is fixed at
   authoring time even if the diff later changes underneath it (agent edits
   mid-review). Recomputing at export would need the patch plumbed into the
   exporter. Worth an explicit decision.

6. The doc's open question 1 answers itself once you see the output. The
   per-annotation [Outside diff] label reads naturally inside the existing
   per-file grouping; a separate trailing section would have split one file's
   comments across two places in the same document.

7. Focus arbitration needs a notion of "active surface", not just
   "focused file". focusedFilePath alone cannot express "this path, but the
   other panel owns it". I added a second field. A general fix probably wants
   one owner value rather than a growing set of exclusions.

Not done (out of scope)

Repo-wide tree browsing (phase 2), tree-sitter nav (phase 3), open-in-editor
(phase 4), old-side viewing, share-link semantics.

The guide-viewer manifest was checked and is unchanged, so it was not
regenerated.

AI-assisted (Claude) under maintainer direction.

[backnotprop]

Prototype in motion. Opening a full file from the review, scrolling a few hundred lines past the diff, and commenting on untouched code that lands in review feedback with an outside-diff label. Stills below, full flow in the video.

full-file-viewer-demo.mp4