Skip to content

fix: open gitignored & untracked files from terminal click#996

Draft
srid wants to merge 9 commits into
masterfrom
fix-gitignored-fileref-click
Draft

fix: open gitignored & untracked files from terminal click#996
srid wants to merge 9 commits into
masterfrom
fix-gitignored-fileref-click

Conversation

@srid

@srid srid commented May 28, 2026

Copy link
Copy Markdown
Member

Clicking a path:line reference in the terminal now opens the file in the Code-tab preview even when git doesn't track it — build artifacts, generated docs, scratch HTML, log files, freshly-created untracked files the fsListAll snapshot hasn't picked up yet. Previously the click silently failed with a "File reference not found" toast for anything outside the tree-display set, even though the server's readFile would have happily served the content.

Why it was broken

The resolver in lineRef.ts:resolveLineRefPath gated on a repoPaths set that the call site filled from app.streams.fsListAll — which on the server runs git ls-files --cached --others --exclude-standard. The --exclude-standard flag drops gitignored files, and the stream snapshot is read-once-per-mount so anything created post-snapshot is also missing. The tree-display filter is correct (the user explicitly wants gitignored files hidden from "All files"); the resolver had borrowed the wrong oracle.

Two-stage resolution

                    ┌──────────────────────────────────────┐
click ─▶ resolve ──▶│ stage 1: tree-set lookup (sync)      │──▶ hit ─▶ open
                    │   preserves basename-uniqueness      │
                    │   fallback for `Foo.hs:42`           │
                    └──────────────────────────────────────┘
                                    │ miss
                                    ▼
                    ┌──────────────────────────────────────┐
                    │ stage 2: fsExists probe (async)      │──▶ exists ─▶ open
                    │   one RPC per composed candidate     │           (out-of-tree)
                    │   path-traversal guard reused from   │──▶ miss   ─▶ toast
                    │   `readFile`'s `resolveUnder`        │
                    └──────────────────────────────────────┘

The probe stays out of the synchronous effect body — SolidJS reactive tracking ends at the first await — so it fires fire-and-forget through .then, with pendingOpen() !== req + handled()?.stage !== "probing" staleness checks before any write so a superseding click can't get stomped by a stale fallback result.

Pieces that landed

  • fsExists on TerminalBackendFs(repoPath, filePath) → Promise<boolean>, implemented in localFs delegating to the new kolu-git helper, exposed on the wire as git.fsExists. Reuses resolveUnder (same path-traversal guard readFile/statFileMtimeMs already trust) and matches the abstraction every sibling fs op already routes through.
  • lineRefCandidates(args) → string[] — sibling of resolveLineRefPath, materializes the existing private candidates() generator so callers can probe candidates against an authority other than the tree's file list. Basename fallback intentionally omitted — it requires the file list to know what's unique, and a gitignored file whose basename collides with a tracked one shouldn't quietly take over the click target.
  • handled as a discriminated unionpending (signal-level null), probing, in-tree, out-of-tree, miss. The old {resolvedPath: string | null; outOfTree: boolean} shape admitted an illegal (null, true) combination; the tagged union makes that unrepresentable and gives the mid-probe state its own stage so it's distinguishable from a confirmed miss at the type level.
  • Membership-clear effect respects probing and out-of-tree — out-of-tree selections legitimately won't appear in treePaths(), and a mid-flight probe shouldn't wipe the previously-displayed file for the duration of an RPC round-trip.

Refinements during review

The first commit landed the user-visible fix; hickey/lowy and code-police caught a handful of state-machine correctness issues that landed as follow-up commits before merge:

What was off Fix
outOfTree: boolean paired with nullable resolvedPath admitted an illegal state Discriminated union with explicit probing / miss stages
fsExists router handler imported from kolu-git directly, bypassing TerminalBackendFs Routed through localBackend.fs.fsExists like every other fs op
Membership-clear effect wiped selectedPath the instant probing started probing joins out-of-tree as a "trust the current selection" signal
In-flight probe overwrote an explicit user tree-click handleSelect clears handled on a probing-stage click; probe staleness-checks handled().stage !== "probing"
RPC error fired three toasts (probe-failed × N + misleading "not found") try/catch with early return — one accurate toast, no spurious follow-up
RPC error left handled stuck at probing forever Transition to miss on error too — miss is the right terminal stage for "couldn't confirm existence either way"

Tests

  • browse.test.ts — five fsExists unit tests covering present file, missing file, directory-not-a-regular-file, path traversal, and the ENOTDIR case (probing under a non-directory leaf).
  • lineRef.test.ts — five lineRefCandidates unit tests covering subdirectory cwd composition, absolute repo-stripped form, escape rejection, undefined cwd, and parent-escape.
  • file-ref-link.feature:75 — new e2e scenario: gitignored build artifact opens from terminal click at the right line.

Try it locally

nix run github:juspay/kolu/fix-gitignored-fileref-click

Generated by /do on Claude Code (model claude-opus-4-7).

srid added 9 commits May 27, 2026 19:47
`resolveLineRefPath` was gating click resolution on membership in
`fsListAll`, which lists tracked + untracked-but-not-ignored paths
(`git ls-files --cached --others --exclude-standard`). Clicking a
gitignored file (build artifact, scratch HTML, log) or a freshly-created
file the stream snapshot hadn't yet picked up would silently fail with
a "File reference not found" toast, even though the server's `readFile`
would have happily served the content.

The tree-display filter is intentional and stays. Decouple it from
click resolution: on a tree-set miss, fall through to a server-side
`fsExists` probe against each composed candidate (cwd-relative,
then repo-relative — same generator that drives the existing in-tree
resolver). First hit opens in preview; out-of-tree selections carry an
`outOfTree` flag so the membership-clear effect doesn't immediately
wipe them.

The probe runs in `.then` from inside the synchronous effect body —
SolidJS reactive tracking ends at the first `await`, and the probe
re-checks `pendingOpen()` before writing so a superseding click can't
get stomped by a stale fallback result.
Encoded the four reachable resolution states (`probing`, `in-tree`,
`out-of-tree`, `miss`) as a tagged union instead of the `{resolvedPath
| null; outOfTree: boolean}` shape that admitted an illegal `(null,
true)` combination. The mid-probe placeholder now has its own `probing`
stage, so a stale-probe state is distinguishable from a confirmed miss
at the type level rather than relying solely on the runtime
`pendingOpen() !== req` check. `handled() === null` stays reserved for
"no request handled yet" — freshness lives at the signal-level absence,
not in the payload.
The `fsExists` probe the comment referenced as "(#TODO)" is implemented
in this same change set — leaving the marker would mislead future
readers into thinking work is outstanding.
The `.catch((err: Error) => ...)` annotation was a runtime-unchecked
cast — oRPC's promise can reject with a non-Error value (string, plain
object, network-layer rejection) and `err.message` would be `undefined`,
surfacing `"File reference probe failed: undefined"`. Match the
defensive pattern `browse.ts:35` already uses on the server side:
`err instanceof Error ? err.message : String(err)`.
The first version's router handler imported `fsExists` from `kolu-git`
directly, bypassing the `TerminalBackendFs` interface that every other
filesystem op (`listAll`, `readFile`, `statFileMtimeMs`) dispatches
through. The "file access on the backend's host machine" volatility
axis is encapsulated by `TerminalBackendFs`; the bypass meant a remote/
container backend adding file-access support would need a separate code
path for this one verb.

Move `fsExists` onto the interface, implement in `localFs` (delegating
to `kolu-git`'s `fsExists` the same shape as the sibling ops), and
rewrite the handler to call `getTerminalBackendFor({kind:"local"}).fs
.fsExists(...)`. Contract entry stays at `git.fsExists` — a dedicated
`contract.fs` namespace would be a single-entry stub; create it the day
a second raw-fs procedure exists.
The membership-clear effect was wiping `selectedPath` the instant a new
probe started: `setHandled({stage:"probing"})` fires synchronously
before the probe resolves, the effect re-evaluates with the new
handled record (no resolvedPath yet, not in `treePaths`), and the
previously-displayed file unmounts — the gutter flashes "Select a
file" for the RPC round-trip, then the resolved file mounts.

Treat `stage === "probing"` as authoritative for the existing
selection, same as `out-of-tree`: keep whatever's currently selected
visible until the probe lands.
If the user tree-clicked another file while a disk probe was in flight,
the probe would still complete and overwrite the user's choice with the
fallback-resolved path. `handleSelect`'s clear guard only fired when
there was a resolved path to compare against — `probing` has none, so
the guard was a no-op for the most common stale-result case.

Clear `handled` unconditionally on a probing-stage tree-click, and
extend the probe's staleness check to also bail when `handled().stage`
is no longer `probing` (either because the user navigated away or a
superseding click landed). Latest user intent wins.
…te toast

The `.catch` form returned `{exists: false}` and let the probe loop
continue to the next candidate. With two candidates and a server
outage, the user saw three toasts: "probe failed" (cand1), "probe
failed" (cand2), and a false "File reference not found" — the last
one is wrong (the file may exist; the server just wasn't reachable).

Switch to try/catch with an early return: one accurate toast per
network failure, no spurious "not found" follow-up.
When the disk-probe RPC threw (server unreachable), the early `return`
left `handled()` stuck at `{stage:"probing"}` for the lifetime of the
request. The membership-clear effect's `outOfTree` suppression treated
this as an active probe and kept guarding selection forever — the bug
self-healed on the next `handleSelect` but the dangling state was
still wrong.

Transition to `miss` on RPC error too: existence was never confirmed
either way, and `miss` is the correct terminal stage for "probe
completed without resolving a path".
@srid

srid commented May 28, 2026

Copy link
Copy Markdown
Member Author

Hickey/Lowy Analysis

# Lens Finding Disposition
1 Hickey outOfTree bool + nullable resolvedPath encoded illegal state Fixed in this PR
2 Hickey Stale (#TODO) marker in lineRefCandidates JSDoc Fixed in this PR
3 Hickey Unsafe (err: Error) cast in disk-probe .catch() Fixed in this PR
4 Lowy fsExists bypassed TerminalBackendFs, misplaced in git Fixed in this PR
5 Hickey CV contract.fs stub namespace — single-entry premature split ⚠️ No-op
6 Lowy CV Hickey's pending tag conflated provenance with freshness Fixed in this PR
7 Lowy CV Mid-probe state indistinguishable from miss Fixed in this PR

Hickey rationale

The two-stage resolution architecture itself (sync tree lookup → async disk probe with pendingOpen() staleness guard) is structurally clean: DAG data flow, no cycles, no temporal coupling beyond the explicit freshness check.

Finding 1 — illegal state in handled payload. The {request; resolvedPath: string | null; outOfTree: boolean} shape gave four (resolvedPath, outOfTree) combinations of which one — (null, true) — was illegal and only excluded by write-site convention. The reachable states (probing, in-tree, out-of-tree, miss) collapsed to a discriminated union, making the illegal combination unrepresentable at the type level. Read sites became single tag checks instead of multi-field joins.

Finding 2 — #TODO marker for work this PR completes. The lineRefCandidates JSDoc referenced its consumer with a (#TODO) annotation; trivial doc fix.

Finding 3 — unchecked cast in .catch(). TypeScript's .catch receives unknown at runtime; the (err: Error) annotation was a cast with no guarantee. Matched the defensive err instanceof Error ? err.message : String(err) pattern the server side already uses.

Cross-validation — contract.fs stub namespace (No-op). Lowy's recommendation included moving the contract entry from contract.git.fsExists to a new contract.fs.fsExists. Hickey CV flagged that as premature concept multiplication: today contract.fs would hold exactly one entry, and the volatility win Lowy is chasing is captured entirely by routing through TerminalBackendFs — the contract location is independent of backend dispatch. Revisit when a second raw-fs procedure exists.

Lowy rationale

Finding 4 — fsExists bypassed the backend abstraction. The first version's router handler imported fsExists from kolu-git directly, while every sibling file op (listAll, readFile, statFileMtimeMs) dispatched through localBackend.fs.*. The "file access on the backend's host machine" volatility axis is encapsulated by TerminalBackendFs; a remote/container backend adding file-access support would have needed a separate code path for this one verb. Fixed by adding fsExists to the interface, implementing in localFs, and routing the handler through getTerminalBackendFor({kind:"local"}).fs.fsExists(...).

Cross-validation — Hickey's pending tag was wrong axis (Findings 6 & 7). Lowy CV caught that Hickey's proposed three-tag union (pending | in-tree | out-of-tree) conflated provenance (how the path was found) with freshness (has resolution been attempted). The corrected form uses handled() === null for "no request handled yet" — freshness stays at the signal-level absence — and the payload union becomes probing | in-tree | out-of-tree | miss. The probing stage also addresses the separate concern that the original mid-probe placeholder ({resolvedPath: null, outOfTree: false}) was structurally indistinguishable from a confirmed miss; the type now makes the difference explicit.

@srid

srid commented May 28, 2026

Copy link
Copy Markdown
Member Author

/do results

Step Status Duration Verification
sync 0s git fetch ok; forge=github
research 3m 37s Traced terminal click → CodeTab gated on fsListAll-filtered repoPaths
branch 6s Switched to fix-gitignored-fileref-click off origin/master
implement 3m 59s fsExists RPC + lineRefCandidates export + CodeTab two-stage resolver
check 2m 47s just check passed (typecheck 24 projects + biome lint 506 files)
docs 48s Updated packages/surface/README.md raw-oRPC inventory
fmt 15s just fmt (biome + nixpkgs-fmt) clean
commit 24s Primary feature commit pushed
hickey+lowy 16m 8s 4 findings (2 Hickey + 2 Lowy) applied as 4 commits; cross-validation refined Hickey's union shape and rejected Lowy's contract.fs namespace as stub
police 30m 42s Two passes surfaced 4 correctness findings (state-machine bugs in the disk-probe lifecycle); all addressed as 4 separate commits; third pass clean
test 1m 38s file-ref-link.feature 8/8 + code-tab.feature 61/61 regression green
create-pr 1m 20s Draft PR + hickey/lowy analysis comment
ci 8m 7s justci 24/24 nodes green; all 22 protect-required contexts pass on HEAD 079463ce
evidence 1m 47s Judgment-call skip — covered by new e2e scenario; user's dev session occupied both default ports
Total 71m 49s

Slowest step: police (30m 42s)

Optimization suggestions

  • police dominated at 43% of total — both passes caught real state-machine bugs in resolveByDiskProbe (selection flash during probe, in-flight probe overwriting tree-click, RPC-error toast cascade, stuck-probing state). The pattern that bit four times: async effects that close over reactive signals with multi-stage handled-records benefit from a deliberate "all stale-check paths in/out" audit during implement — would have shifted ~half the police-pass cost into implement.
  • hickey+lowy at 16m is mostly unavoidable when both reviewers surface findings + cross-validation — but the diff was small enough (11 files, +275/-13) that running the reviewers on the talk-mode sketch already moved the structural argument upstream. Re-running on the post-implement diff still earned its keep here (the TerminalBackendFs boundary finding only surfaced once the actual router import existed).
  • First CI run wasted ~3m on a dirty-tree refusal — the talk-mode .html artifact left behind by /talk --html failed CI=true's clean-tree precondition. Moving such artifacts out of $PWD or adding them to .gitignore before invoking /do saves the re-run.
  • For re-runs, /do --from ci-only skips the 60+ minutes of earlier steps and runs only CI against the latest commit.

Workflow completed at 2026-05-27 14:42 UTC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant