Code tab: right-click Open path:N jumps to file at that line#891
Merged
Conversation
…ne N"
Generalizes the terminal-link click pipeline into a single `openInCodeTab`
producer-facing function, then exercises the new seam by adding an
`Open path:N` entry to the Code tab's right-click context menu.
- Renames `codeNavigation.ts` → `openInCodeTab.ts`. The new function
encapsulates the paired writes (panel-uncollapse + Code-tab activation
+ browse-mode + pending-request signal) that previously sat at every
call site as documented ordering discipline (`Terminal.tsx:474-489`).
Callers now make one call instead of two.
- `useRightPanel.openCodeBrowser` → `openCodeAt(mode)`: the atomic
three-field preferences patch now takes the target sub-mode as a
parameter so future producers (comment-revisit in phase 1) can route
back to the mode the artifact was authored in.
- `CodeContextMenuItem` becomes a discriminated union (`kind: "copy"`
vs `kind: "action"`). Copy items still write to the clipboard; action
items fire a callback. `CodeContextMenu` dispatches on `kind`.
- `useLineSelection` accepts an `onOpen` accessor option; when set and
a range is selected, it emits an `Open path:N` action entry that
routes the ref through the host's open dispatcher.
- The Code tab's diff-view `CodeMenuFrame` passes `onOpen` that calls
`openInCodeTab({ ref, repoRoot, targetMode: "browse" })`. Reviewing a
diff and jumping to the full file at the same line is now one click.
The browse-view `CodeMenuFrame` deliberately omits `onOpen` — the
file is already on screen at line precision.
First phase of #881's redo. No new types beyond `OpenInCodeTabRequest`
(rename of `CodeOpenRequest`).
…atch.exhaustive Replaces an `if (item.kind === "copy") … else …` two-arm with `match(item).with(…).exhaustive()`. The else-arm silently absorbed any future `CodeContextMenuItem` variant; the exhaustive check makes the dispatch site a compile-time gate for the next variant (Phase 1's "Comment on path:N" item).
`onActivate`'s closure used to call `path()` at click time, so if Pierre remounted to a different file between menu-open and click the action opened a path that disagreed with the rendered label. Snapshot once at the top of `buildItems` and reuse the local in every consumer (label, textToCopy, onActivate capture).
…rget `openInCodeTab` calls `openCodeAt(req.targetMode)` unconditionally, which patches three preference fields and round-trips to the server even when the panel is already in the requested state — e.g. a terminal-link click on `notes.txt:3` while the Code tab is already showing that file in browse mode would have re-sent the same patch. Guard at the function so every caller benefits.
The second sentence ("Same dispatch as a terminal-link click — openInCodeTab
flips the panel to browse mode and surfaces the line range") narrated
what openInCodeTab does, which is already documented at its definition.
Keep only the non-obvious cwd-omission rationale.
…ode jsdoc "Today every producer passes …; later phases may route a comment-revisit click back …" was roadmap speculation that both ages poorly and reads as a justification for the field's existence. The contract is what matters: producers without an authoring mode pass "browse".
Member
Author
Hickey/Lowy AnalysisNo findings — analysis below. Hickey rationaleReviewed
Lowy rationaleReviewed five volatility seams; all came back No-op:
Polish commitsAfter the structural review came back clean, an elegance pass (
|
Open
Member
Author
EvidenceThe visible surface this PR adds is a single new context-menu item plus the navigation it triggers. Both are exercised end-to-end by a new e2e scenario ( What the menu looks like in diff viewAsserted in And I right-click the diff view
Then the context menu items should be "Copy path | Copy file-b.txt:1 | Open file-b.txt:1"What the click doesNew scenario in Scenario: Right-click "Open path:N" in diff view jumps to browse at that line
When I run "git init /tmp/kolu-open-from-diff && cd /tmp/kolu-open-from-diff"
And I run "git commit --allow-empty -m init"
And I run "printf 'first\nsecond\nthird\n' > notes.txt"
And I click the Code tab
Then the Code tab should list a changed file "notes.txt"
When I click the changed file "notes.txt" in the Code tab
Then the diff view should contain "second"
When I click the line number 2 in the diff view
And I right-click the diff view
And I click the context menu item "Open notes.txt:2"
Then the Code tab mode should be "browse"
And the selected file should show content "second"
And line 2 should be selected in the file content
Regression coverage retained
Local: 43/43 scenarios, 503/503 steps. CI: 300/300 on both |
Member
Author
|
| Step | Status | Duration | Verification |
|---|---|---|---|
| sync | ✓ | 0s | git fetch ok; forge=github; noGit=false |
| research | ✓ | 2m 11s | File inventory, paired-write pattern at Terminal.tsx:468-491, CodeContextMenuItem shape, e2e patterns confirmed |
| branch | ✓ | 8s | On feature branch open-in-code-tab rooted at origin/master |
| implement | ✓ | 5m 26s | Renamed codeNavigation.ts→openInCodeTab.ts; OpenInCodeTabRequest carries targetMode; useRightPanel.openCodeBrowser→openCodeAt(mode); CodeContextMenuItem discriminated; useLineSelection emits Open action via accessor; e2e scenario added |
| check | ✓ | 5m 37s | just check exit 0; pnpm typecheck Done for all 19 packages |
| docs | ✓ | 45s | README's clickable-file-refs bullet still accurate; no architecture refs to renamed module |
| fmt | ✓ | 14s | biome reformatted 1 file (line-break); nixpkgs-fmt 0/5 |
| commit | ✓ | 24s | Commit 3d887fdf; 9 files / +178 −91; rename tracked by git |
| hickey+lowy | ✓ | 3m 50s | Hickey: 0 findings. Lowy: 0 findings (5 questions all No-op) |
| police | ✓ | 8m 40s | Rules 1 violation (prefer-ts-pattern), Elegance 5 findings → 5 separate commits (ce1546a4, fc2d1c9b, e267f90a, ee147275, e50f4ac1) |
| test | ✓ | 1m 8s | 43/43 scenarios, 503/503 steps |
| create-pr | ✓ | 1m 8s | Draft PR #891 + Hickey/Lowy analysis comment |
| ci | ✓ | 11m 56s | All 14 contexts success on HEAD; 3 flakes on first run, all passed on ci::e2e retry (reported on #320) |
| evidence | ✓ | 1m 11s | Textual Evidence comment quoting the new e2e scenario's polling assertions |
| done | ✓ | 4s | All 15 steps recorded |
| Total | 43m 12s |
Slowest step: ci (11m 56s)
Optimization suggestions
- CI dominated the run (28%) because of a first-pass flake retry. Two known flakes (OpenCode indicator on
x86_64-linux, session-restore click race onaarch64-darwin) cost ~5m of retry time. Both are unrelated to this diff and now logged on Flaky tests log #320; fixing them would unlock a clean ~7m CI run. - Police took 8m 40s mostly from the
/simplifythree-agent fan-out plus five separate fix/format/commit/push cycles. The granularity is by design (one commit per finding for review clarity) but it does compound: each commit pays the cost ofjust fmtre-running on a clean tree. Worth considering a single-shot lint-after-all-fixes pass in/simplifyitself. - Check at 5m 37s is dominated by
pnpm installround-tripping through the nix devshell on each invocation. Same install runs again in fmt and police's batched fmt — cache invalidation might be too eager. - Resume entry points: re-running this with
/do --from ci-onlywould skip everything before CI and land in ~12m if the flakes hold.
Workflow completed at 2026-05-14T14:16:50Z.
… just verbal "Copy path:N" and "Open path:N" differ only by the first word, which is easy to mis-read when scanning the menu in a hurry. Add a leading glyph to every CodeContextMenuItem — a clipboard glyph for `kind: "copy"` and an external-link arrow for `kind: "action"` — so the verb is readable without parsing the label. CopyIcon and OpenIcon land in the existing Icons.tsx registry per the icons-in-registry rule; useLineSelection populates each item's `icon` at build time.
…ation reveals nested files Pierre's FileTree was constructed with `initialExpandedPaths` from `props.expandPaths` only; an externally-driven `selectedPath` change (e.g. a terminal `path:line` click resolving onto `src/foo/bar.ts`) left the parent directories collapsed and the row off-screen until the user opened them by hand. Merge the ancestors of `props.selectedPath` into the `initialExpandedPaths` set passed to `resetPaths`. Pierre's vanilla `FileTree` class doesn't expose `selectOnlyPath` post-mount, so the tree-row `data-item-selected` highlight is still seeded only at construction — that would need an upstream Pierre API. The content-level highlight (line range selected in the file viewer) already verifies user-meaningful selection. E2e: the existing "Open path:N" scenario now uses `docs/notes.txt` and asserts `the file browser should show a file "docs/notes.txt"` (which fails when the parent directory is collapsed because Playwright's `state: "visible"` rejects collapsed-ancestor descendants).
…ntry point Right-click on the viewer USED to require a prior left-click to set the line range — without that, the menu opened with a lone "Copy path" item, which was both noise (the path is also in every other item) and a UX trap (users mis-read which item they had). `CodeMenuFrame`'s contextmenu handler now walks `event.composedPath()` (piercing Pierre's shadow DOM) for the gutter element carrying `data-column-number` and sets the selection range from there. Right- clicks off any line clear the range; `useLineSelection.buildItems` returns `[]` when no range is set, so `CodeContextMenu.open()` short- circuits without preventing the browser default. Net: right-click on a line shows the 3-item menu; right-click on whitespace shows nothing. E2e: new step `I right-click line N in the diff view` / `... in the file content` (clicks the gutter at `data-column-number=N` with the right mouse button). Replaces the old `click line N + right- click viewer root` idiom in three existing scenarios — they now lead with a single gesture per the new UX.
…aths at construction
The earlier ancestor-expansion fix only ran in the deferred reset
effect — which doesn't fire on the initial mount. When the Code tab
flips from diff to browse mode, `<FileTree>` remounts FRESH (the
`<Switch>/<Match>` predicate flips with the data source), and the
constructor's `initialExpandedPaths` previously only included
`props.expandPaths`. A `setSelectedPath("docs/notes.txt")` carried in
through `initialSelectedPaths`, but `docs/` stayed collapsed and the
row was offscreen.
Compute selected-path ancestors at construction time too, alongside
the existing deferred-effect path. Both paths now use the same
`ancestorsOf` helper.
…Key effect sees the pending request
`openInCodeTab(req)` issues two synchronous writes — a preferences
patch (`openCodeAt`) and `setPending(req)`. The preferences cell uses
`authority: "local"` (wire.ts:56), so `applyLocal` ticks `view()`
synchronously. Without an explicit batch, SolidJS fired CodeTab's
`resetKey` effect immediately on the first write, BEFORE `setPending`
ran. The guard
if (req && req.repoRoot === repoPath() && req.targetMode === view()
&& handled()?.request !== req) return;
saw `pendingOpen() === null` (setPending hadn't fired yet), fell
through to `setSelectedPath(null)`, and cleared the path the user had
just navigated to. Symptom: in the test scenario, the file row in the
tree showed `aria-selected="false"` after a `Open path:N` jump,
because the rebuilt browse-mode tree had `selectedPath = null` at
mount time.
The old paired-write at `Terminal.tsx`'s call site escaped this by
sitting inside a JSX click handler — Solid's event delegation
auto-batches writes within a single user event. Once the writes moved
into a standalone function called from a non-event context (an item
callback two layers deep), the implicit batch was lost.
Wrap with `batch()` so both writes commit before any dependent effect
fires. The `resetKey` effect now sees `pendingOpen() = req` on the
tick `view()` changes, the guard passes, and selectedPath survives the
mode flip.
E2e: the "Open path:N" scenario now uses `docs/notes.txt` (nested), and
asserts `the file "docs/notes.txt" should be selected in the file
browser` via `aria-selected="true"`. The assertion polls until visible,
which requires both ancestor expansion AND the selection flag.
… upstream Pierre's `scrollFocusedRowIntoView` is gated on `shouldOwnDomFocus`, which is set only by user-initiated handlers — programmatic selection via `initialSelectedPaths` / `getItem(path)?.select()` doesn't trigger it. For small trees the row sits in the visible window; for large worktrees the selected row stays virtualized off-screen until the user scrolls. Comment near `onMount` documents the limitation; upstream request is pierrecomputer/pierre#676.
… into one helper The two helpers had identical bodies modulo the mouse button — same selector, same bounding-box poll, same waitForFrame. Drift hazard. Extract `interactWithGutterLine(world, root, line, button)` parameterized on `"left" | "right"`. Three step registrations now call through it with the appropriate button literal. ~25 lines of near-duplicate body removed.
…face effect-ordering coupling The resetKey effect's guard checked four conditions inline. Naming the predicate `isPendingOpenAboutToLand()` gives the cross-effect temporal coupling — resetKey runs BEFORE pendingOpen by registration order, both effects observe state populated by `openInCodeTab`'s `batch()` — a discoverable surface. A future editor changing one effect in isolation now reads a single named function rather than re-deriving the guard from inline ands. No behavior change.
Module-level signal is fine while right-panel state is a singleton. Comment makes the assumption explicit so a future split-panel / multi- window change doesn't silently race two CodeTab instances on each other's pending requests.
…cross mount and effect `onMount` reads `props.selectedPath` as a snapshot at construction time; the deferred resetPaths effect reads it reactively. The split is forced by Pierre — there's no API to re-feed `initialExpandedPaths` after the constructor. The two sites encapsulate two lifecycle moments. Today's call site initializes selectedPath to null and updates it post-stream-settle, so the snapshot is always null at mount; a future caller passing a non-null selectedPath on first render would rely on the snapshot, NOT the deferred effect (which skips its initial run).
…defs The two `I right-click the file content` / `I right-click the diff view` step handlers (and the shared `rightClickViewRoot` helper) had no remaining feature-file callers after the new right-click-on-line idiom landed. Dead surface that invites future scenarios to re-introduce the two-step "click line + right-click viewer" pattern.
…id-pierre `fileSearch.ts` already had `ancestorDirectoryPaths` for its search- expansion needs; the Phase 0 work introduced `ancestorsOf` in `solid-pierre/FileTree.tsx` for the same shape. Both mirrored Pierre's internal `getAncestorDirectoryPaths` and both fed Pierre's `initialExpandedPaths`. The natural home is the Pierre wrapper package (`@kolu/solid-pierre`) — kolu-client already depends on it, and the helper conceptually belongs next to the FileTree wrapper that needs it. Take the more capable of the two implementations (the fileSearch.ts one tolerates trailing slashes), rename `ancestorsOf` → it, export from `@kolu/solid-pierre`, and consume from `fileSearch.ts`.
…eTab jsdoc "The Terminal call site used to escape this by being inside a JSX click handler … but once the writes moved into a standalone function the implicit batch was lost." Is change-narration. Future readers need the invariant (why batch is required), not the refactor history.
…Mount comment The "Today's only call site (CodeTab) initializes selectedPath to null and updates it via the pendingOpen effect AFTER the FileTree's paths stream settles, so this snapshot is always null at mount …" paragraph named a specific caller (will rot when others wire up) and described hypothetical futures. Keep the invariant: snapshot here vs reactive in the deferred effect, because Pierre doesn't expose a re-feed hook.
…ne comments Two blocks were merged into one and the surviving comment carried a "One shared interaction for any mouse button on a gutter line — only the button differs" preamble that narrated the refactor. The right-click-line step comment had "Replaces the old two-step click- line-then-right-click idiom" — change-narration that won't age. Keep the load-bearing rationales: why the gutter selector is correct, why pointerdown/pointerup go through Playwright's mouse API, why the bounding box needs polling. Drop the narration.
srid
added a commit
that referenced
this pull request
May 14, 2026
**The Code tab now resolves a bare filename like `Foo.hs:42` to the unique repo file ending in that basename.** Compiler output frequently emits source locations without the full path prefix; previously those clicks fell through to a "File reference not found" toast even when exactly one file in the repo could match. Closes #898. Resolution still tries the path-based candidates first (repo-relative, cwd-relative, absolute-stripped) — the basename scan only fires when all of them miss, and only commits to a result when there's exactly one match. _Ambiguous basenames keep returning `null`; opening the wrong file is worse than the toast._ ``` repo: resolveLineRefPath("Foo.hs"): src/lib/Foo.hs 1. cwd-rel → miss src/lib/Bar.hs 2. repo-rel → miss ... 3. basename → src/lib/Foo.hs ✓ (unique) ``` The fallback covers terminal-link clicks and right-click _Open path:N_ alike — both producers route through the single `openInCodeTab` resolver added in #891. ### Coverage - **Unit** — `lineRef.test.ts`: unique basename, ambiguous basename → null, slash-with-bad-prefix falls back, exact path candidate still preferred over basename - **E2E** — `file-ref-link.feature`: terminal emits `notes.txt:2` while the file lives at `src/lib/notes.txt`; click opens the file in browse mode at line 2 ### Try it locally \`\`\`sh nix run github:juspay/kolu/fix-898-basename-fileref \`\`\` _Generated by [\`/do\`](https://github.com/srid/agency) on Claude Code (model \`claude-opus-4-7\`)._
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.
The Code tab's right-click menu gains an
Open <path>:<line>entry. Reviewing a diff and wanting the full file at the same line was previously two steps — copypath:N, switch to browse mode, paste-navigate. Now it's one click; the menu item dispatches through the same pipeline a terminal-link click does.This is Phase 0 of #881's redo — small user-visible win on top of a seam Phase 1 (line-anchored comments) will reuse. The structural review behind the redo is in #881's body and replaces #879.
Before / after
The seam
The terminal-link click pipeline used to require a paired write at every call site:
The ordering hazard sat at the call site as documented discipline, which meant every new producer (right-click Open, future Comment on) inherited the same wall-of-text rationale. Phase 0 collapses both writes into one function:
openInCodeTab(replacingcodeNavigation.ts) encapsulates both writes;useRightPanel.openCodeBrowser()is generalized toopenCodeAt(mode: CodeTabView)so a Phase 1 comment authored while reviewing a diff can re-open in that diff mode rather than always browse.What landed
openInCodeTab(req)— single call replaces the paired-write pattern at every siteuseRightPanel.openCodeAt(mode)— atomic three-field preferences patch parameterized on the target sub-mode; skips the patch when already at targetCodeContextMenuItembecomes a discriminated union (kind: "copy"vskind: "action");handleItemdispatches viats-pattern.exhaustive()useLineSelectiongainsonOpen?: Accessor<((ref) => void) | undefined>; when present and a range is selected, an Open path:N action item is emittedCodeMenuFramepassesonOpenthat routes throughopenInCodeTab. The browse-view path deliberately omits it — the file is already on screen at line precisionThe
Accessor<…>shape ononOpenmirrorsinitialRange— buildItems runs at menu-open time, so a host whose prop arrives late in the SolidJS lifecycle still flips the item in.Test coverage
code-tab.feature— new scenario: right-click in diff view, click Open notes.txt:2, assert mode flipped to browse + file selected + line 2 highlightedcode-tab.feature— updated multi-file diff scenario to assert the three-item menu (Copy path | Copy file-b.txt:1 | Open file-b.txt:1)file-ref-link.feature— terminal-link clicks now flow throughopenInCodeTab; all three pre-existing scenarios still pass43/43 scenarios, 503/503 steps locally.
Try it locally
Generated by
/doon Claude Code (modelclaude-opus-4-7).