From 3d887fdf0aaaf2e5241a45ea00cb4e3be85d4766 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 09:48:45 -0400 Subject: [PATCH 01/21] feat(right-panel): openInCodeTab front door + right-click "Open at line N" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`). --- .../client/src/right-panel/CodeMenuFrame.tsx | 7 +++ packages/client/src/right-panel/CodeTab.tsx | 56 +++++++++++++------ .../client/src/right-panel/codeNavigation.ts | 39 ------------- .../client/src/right-panel/openInCodeTab.ts | 51 +++++++++++++++++ .../client/src/right-panel/useRightPanel.ts | 12 ++-- packages/client/src/terminal/Terminal.tsx | 15 +---- packages/client/src/ui/CodeContextMenu.tsx | 34 ++++++++--- packages/client/src/ui/useLineSelection.ts | 33 +++++++++-- packages/tests/features/code-tab.feature | 22 +++++++- 9 files changed, 178 insertions(+), 91 deletions(-) delete mode 100644 packages/client/src/right-panel/codeNavigation.ts create mode 100644 packages/client/src/right-panel/openInCodeTab.ts diff --git a/packages/client/src/right-panel/CodeMenuFrame.tsx b/packages/client/src/right-panel/CodeMenuFrame.tsx index 7accc9b30..06029276b 100644 --- a/packages/client/src/right-panel/CodeMenuFrame.tsx +++ b/packages/client/src/right-panel/CodeMenuFrame.tsx @@ -11,6 +11,7 @@ import { CodeContextMenu, type CodeContextMenuController, } from "../ui/CodeContextMenu"; +import type { LineRef } from "../ui/lineRef"; import { type LineSelection, useLineSelection } from "../ui/useLineSelection"; export type CodeMenuFrameProps = { @@ -25,12 +26,18 @@ export type CodeMenuFrameProps = { * controller so a terminal `path:line` click drives both the * Pierre highlight AND the right-click menu's "Copy path:N" item. */ initialSelectedLines?: SelectedLineRange | null; + /** When provided, adds an "Open :" entry to the context + * menu that dispatches the selected ref to the host (typically a + * call to `openInCodeTab`). Omit for viewers where "open" is a + * no-op (the file is already on screen at line precision). */ + onOpen?: (ref: LineRef) => void; }; export const CodeMenuFrame: Component = (props) => { let menuCtrl: CodeContextMenuController | undefined; const selection = useLineSelection(() => props.path, { initialRange: () => props.initialSelectedLines, + onOpen: () => props.onOpen, }); return (
= (props) => { setSearchQuery(""); // Skip the selectedPath clear when an incoming request is // about to land in the new mode — the resetKey effect runs - // before the pendingCodeOpen effect (registration order), and - // an unconditional clear would null what we're about to set. + // before the pendingOpen effect (registration order), and an + // unconditional clear would null what we're about to set. // Reading `req.targetMode` (not `view()`) makes the guard // robust to user-driven mode flips that race the click. - const req = pendingCodeOpen(); + const req = pendingOpen(); if ( req && req.repoRoot === repoPath() && @@ -202,31 +206,32 @@ const CodeTab: Component<{ meta: TerminalMetadata | null }> = (props) => { ), ); - // Consume-once record for the latest pendingCodeOpen tick. Holds - // the full request object (reference identity discriminates two - // structurally-identical clicks — `requestCodeOpen` mints a fresh + // Consume-once record for the latest pendingOpen tick. Holds the + // full request object (reference identity discriminates two + // structurally-identical clicks — `openInCodeTab` mints a fresh // object per call) alongside the resolved path. Storing the // request here lets `selectedRange` derive its value without // re-running `resolveLineRefPath` (single resolution site per // request) and lets `resetKey` know whether a pending request // has already been applied. const [handled, setHandled] = createSignal<{ - request: CodeOpenRequest; + request: OpenInCodeTabRequest; resolvedPath: string | null; } | null>(null); - // Honor terminal file-ref clicks. The effect waits for the live - // `fsListAll` stream to settle so resolution can validate against - // a complete file list — otherwise a request fired during boot - // would toast "not found" on a path that just hasn't been - // enumerated yet. The terminal click handler is the sole site that - // flips the panel to browse mode; this effect only sets + // Honor every `openInCodeTab` request — terminal file-ref clicks, + // right-click "Open path:N" entries, and any future producer. The + // effect waits for the live `fsListAll` stream to settle so + // resolution can validate against a complete file list — otherwise + // a request fired during boot would toast "not found" on a path + // that just hasn't been enumerated yet. `openInCodeTab` flips the + // panel to browse mode itself; this effect only sets // `selectedPath`. The `resetKey` effect above guards against // clearing selectedPath when this effect is about to set it. createEffect( on( () => { - const req = pendingCodeOpen(); + const req = pendingOpen(); const paths = treePaths(); const isPending = allPaths.pending(); return { req, repo: repoPath(), paths, isPending }; @@ -263,7 +268,7 @@ const CodeTab: Component<{ meta: TerminalMetadata | null }> = (props) => { // // No `equals` override: two clicks on the same `path:line` produce // structurally identical `{start, end}` but distinct request - // objects (`requestCodeOpen` mints a fresh one per call), so the + // objects (`openInCodeTab` mints a fresh one per call), so the // memo emits a fresh value on every click. Pierre's // `InteractionManager.setSelection` re-renders when the selection // is "dirty" — and tearing down the gutter (panel collapse, @@ -275,7 +280,7 @@ const CodeTab: Component<{ meta: TerminalMetadata | null }> = (props) => { start: number; end: number; } | null>(() => { - const req = pendingCodeOpen(); + const req = pendingOpen(); if (!req) return null; const h = handled(); if (!h || h.request !== req || h.resolvedPath === null) return null; @@ -538,7 +543,22 @@ const CodeTab: Component<{ meta: TerminalMetadata | null }> = (props) => { {(d) => ( - + { + // Diff path is already repo-relative; cwd is + // irrelevant. Same dispatch as a terminal-link + // click — `openInCodeTab` flips the panel to + // browse mode and surfaces the line range. + const repo = repoPath(); + if (repo === null) return; + openInCodeTab({ + ref, + repoRoot: repo, + targetMode: "browse", + }); + }} + > {(selection) => ( // `` is the scroll container — // `` consumes its context and diff --git a/packages/client/src/right-panel/codeNavigation.ts b/packages/client/src/right-panel/codeNavigation.ts deleted file mode 100644 index e77179dc5..000000000 --- a/packages/client/src/right-panel/codeNavigation.ts +++ /dev/null @@ -1,39 +0,0 @@ -/** Cross-component code-tab navigation requests. - * - * Terminal link-click handlers (and any future caller that needs to - * open a file from outside the right panel) publish a request here; - * `CodeTab` observes the signal and reacts. Latest request wins; - * callers don't need to clear it. Each call mints a fresh request - * object, so two clicks on the same `path:line` are distinct by - * reference — that's what lets `CodeTab` tell them apart even when - * their `ref` content matches. */ - -import { createSignal } from "solid-js"; -import type { LineRef } from "../ui/lineRef"; - -export interface CodeOpenRequest { - /** Parsed `path:line[-end]` the user clicked. */ - ref: LineRef; - /** Per-terminal git repo root that `ref.path` is relative to (when - * relative). Absolute paths beneath this root are also accepted — - * the resolver normalizes both shapes. */ - repoRoot: string; - /** Terminal cwd at the time of click. Drives the "user typed - * `bar.ts:42` while standing in a subdirectory of the repo" case; - * undefined falls back to repo-relative interpretation only. */ - cwd: string | undefined; - /** Code-tab mode the request expects to land in. Today only - * `"browse"` makes sense (highlighting a line in a diff doesn't - * generalize), but encoding the assumption on the type lets the - * consumer guard explicitly instead of relying on the click - * handler having pre-called `openCodeBrowser` in the right order. */ - targetMode: "browse"; -} - -const [pending, setPending] = createSignal(null); - -export const pendingCodeOpen = pending; - -export function requestCodeOpen(req: CodeOpenRequest): void { - setPending(req); -} diff --git a/packages/client/src/right-panel/openInCodeTab.ts b/packages/client/src/right-panel/openInCodeTab.ts new file mode 100644 index 000000000..a571f55ca --- /dev/null +++ b/packages/client/src/right-panel/openInCodeTab.ts @@ -0,0 +1,51 @@ +/** Front door for "open this file:line in the Code tab". Every producer + * — terminal-link click, right-click "Open path:N" context-menu entry, + * future surfaces — calls `openInCodeTab(...)` instead of writing the + * preferences patch and pending-request signal separately. The function + * encapsulates the paired writes (panel-uncollapse + tab + browse-mode + + * pending request) so the SolidJS effect-ordering invariant lives here, + * not at every call site. + * + * Latest request wins; callers don't clear it. Each call mints a fresh + * request object — two clicks on the same `path:line` are distinct by + * reference, which is what lets `CodeTab` tell them apart even when + * their `ref` content matches and re-paint the highlight. */ + +import type { CodeTabView } from "kolu-common/surface"; +import { createSignal } from "solid-js"; +import type { LineRef } from "../ui/lineRef"; +import { useRightPanel } from "./useRightPanel"; + +export interface OpenInCodeTabRequest { + /** Parsed `path:line[-end]` to navigate to. The path is interpreted + * relative to `repoRoot` (or, when present, cwd-relative under + * `repoRoot`) by `CodeTab` via `resolveLineRefPath`. */ + ref: LineRef; + /** Per-terminal git repo root that `ref.path` is relative to (when + * relative). Absolute paths beneath this root are also accepted — + * the resolver normalizes both shapes. */ + repoRoot: string; + /** Terminal cwd at the time of the request. Drives the "user typed + * `bar.ts:42` while standing in a subdirectory of the repo" case; + * undefined falls back to repo-relative interpretation only. */ + cwd?: string; + /** Which Code-tab sub-mode the request expects to land in. Today + * every producer passes `"browse"`; later phases may route a + * comment-revisit click back to the diff mode the comment was + * authored in. */ + targetMode: CodeTabView; +} + +const [pending, setPending] = createSignal(null); + +export const pendingOpen = pending; + +/** Open the right panel's Code tab at `req.targetMode` showing `req.ref`. + * The two reactive writes (preferences patch + pending-request signal) + * fire inside the same call, so `CodeTab`'s `resetKey` effect and + * `pendingOpen` effect both observe the request on the tick the view + * changes — no call-site ordering discipline required. */ +export function openInCodeTab(req: OpenInCodeTabRequest): void { + useRightPanel().openCodeAt(req.targetMode); + setPending(req); +} diff --git a/packages/client/src/right-panel/useRightPanel.ts b/packages/client/src/right-panel/useRightPanel.ts index 5a4087603..0153f3bdb 100644 --- a/packages/client/src/right-panel/useRightPanel.ts +++ b/packages/client/src/right-panel/useRightPanel.ts @@ -42,16 +42,16 @@ export function useRightPanel() { ...(mode !== undefined && { codeMode: mode }), }, }), - /** Atomic "open the code browser at this file" — uncollapse the - * panel, switch to Code, force browse mode. Single patch so the - * UI ticks once instead of three times when callers need all - * three transitions together. */ - openCodeBrowser: () => + /** Atomic "open the Code tab at `mode`" — uncollapse the panel, + * switch to Code, set the requested sub-mode. Single preferences + * patch so the UI ticks once instead of three times when callers + * need all three transitions together. */ + openCodeAt: (mode: CodeTabView) => updatePreferences({ rightPanel: { collapsed: false, activeTab: "code", - codeMode: "browse", + codeMode: mode, }, }), /** Change the sub-mode within the Code tab. */ diff --git a/packages/client/src/terminal/Terminal.tsx b/packages/client/src/terminal/Terminal.tsx index 3312ad264..fe7be8d1a 100644 --- a/packages/client/src/terminal/Terminal.tsx +++ b/packages/client/src/terminal/Terminal.tsx @@ -41,8 +41,7 @@ import { streamCall } from "@kolu/surface/solid"; import { client } from "../wire"; import { isExpectedCleanupError } from "../rpc/streamCleanup"; import { createScrollLock } from "../scrollLock"; -import { requestCodeOpen } from "../right-panel/codeNavigation"; -import { useRightPanel } from "../right-panel/useRightPanel"; +import { openInCodeTab } from "../right-panel/openInCodeTab"; import { preferences } from "../wire"; import { isTouch } from "../useMobile"; import { createFileRefLinkProvider } from "./fileRefLinkProvider"; @@ -163,7 +162,6 @@ const Terminal: Component<{ const [searchAddon, setSearchAddon] = createSignal(null); const scrollLock = createScrollLock(() => preferences().scrollLock); const terminalStore = useTerminalStore(); - const rightPanel = useRightPanel(); let fitRaf = 0; /** Debounce fit() to one call per animation frame — ResizeObserver fires rapidly. */ @@ -471,16 +469,7 @@ const Terminal: Component<{ const meta = terminalStore.getMetadata(props.terminalId); const repoRoot = meta?.git?.repoRoot ?? null; if (!repoRoot) return; - // Issue both writes in the same DOM-event tick: the - // panel-mode change and the click request invalidate - // CodeTab's `resetKey` and `pendingCodeOpen` effects - // together, and Solid runs them in registration order - // (resetKey clears, then pendingCodeOpen sets), so - // `selectedPath` lands on the click's target. Moving - // `openCodeBrowser` inside CodeTab's effect would - // invert that ordering and null the path. - rightPanel.openCodeBrowser(); - requestCodeOpen({ + openInCodeTab({ ref, repoRoot, cwd: meta?.cwd, diff --git a/packages/client/src/ui/CodeContextMenu.tsx b/packages/client/src/ui/CodeContextMenu.tsx index bb76619a0..269464c30 100644 --- a/packages/client/src/ui/CodeContextMenu.tsx +++ b/packages/client/src/ui/CodeContextMenu.tsx @@ -10,11 +10,23 @@ import { type Component, createSignal, For, onCleanup, Show } from "solid-js"; import { Portal } from "solid-js/web"; import { toast } from "solid-sonner"; -export type CodeContextMenuItem = { - label: string; - /** Returned text gets copied; success toast names the item. */ - textToCopy: string; -}; +/** Two verbs over the same selection noun: copy a string to the clipboard, + * or invoke an action callback. The discriminator keeps the dispatch + * explicit so adding a third verb (e.g. "share") doesn't tempt the + * handler into reading every field on every item. */ +export type CodeContextMenuItem = + | { + kind: "copy"; + label: string; + /** Text written to the clipboard; success toast names the item. */ + textToCopy: string; + } + | { + kind: "action"; + label: string; + /** Fired on click. The item is closed regardless of completion. */ + onActivate: () => void; + }; export type CodeContextMenuController = { /** Bind to a host element's `oncontextmenu`. */ @@ -67,10 +79,14 @@ export const CodeContextMenu: Component<{ }); const handleItem = (item: CodeContextMenuItem) => { - navigator.clipboard - .writeText(item.textToCopy) - .then(() => toast.success(`Copied: ${item.textToCopy}`)) - .catch((err: Error) => toast.error(`Failed to copy: ${err.message}`)); + if (item.kind === "copy") { + navigator.clipboard + .writeText(item.textToCopy) + .then(() => toast.success(`Copied: ${item.textToCopy}`)) + .catch((err: Error) => toast.error(`Failed to copy: ${err.message}`)); + } else { + item.onActivate(); + } close(); }; diff --git a/packages/client/src/ui/useLineSelection.ts b/packages/client/src/ui/useLineSelection.ts index 59ef8d999..700a3b1f3 100644 --- a/packages/client/src/ui/useLineSelection.ts +++ b/packages/client/src/ui/useLineSelection.ts @@ -14,7 +14,7 @@ import type { SelectedLineRange } from "@pierre/diffs"; import { type Accessor, createEffect, createSignal, on } from "solid-js"; import type { CodeContextMenuItem } from "./CodeContextMenu"; -import { formatLineRef } from "./lineRef"; +import { formatLineRef, type LineRef } from "./lineRef"; export type LineSelection = { /** Current selection range — bind to Pierre's `selectedLines` prop @@ -24,7 +24,9 @@ export type LineSelection = { * selection commit (single-line click or drag end). */ handleSelect: (range: SelectedLineRange | null) => void; /** Bind to ``. Returns "Copy path" plus, when - * a line is selected, "Copy :" with the rendered ref. */ + * a line is selected, "Copy :" with the rendered ref — + * and, when `options.onOpen` is provided, an "Open :" + * action entry that dispatches to the host. */ buildItems: () => CodeContextMenuItem[]; }; @@ -34,6 +36,14 @@ export interface LineSelectionOptions { * effect below pushes it into the controller's range, which means * the right-click menu and the Pierre highlight stay in sync. */ initialRange?: Accessor; + /** Accessor — re-read at `buildItems()` time. Returning a handler + * adds an "Open :" entry to the menu while a range is + * selected; returning undefined omits it. Accessor (not bare + * callback) so a host whose prop arrives later in the lifecycle + * still flips the item in once it's available. The host is + * responsible for routing the ref through the Code-tab open + * dispatcher. */ + onOpen?: Accessor<((ref: LineRef) => void) | undefined>; } export function useLineSelection( @@ -62,12 +72,25 @@ export function useLineSelection( handleSelect: (r) => setRange(r), buildItems: () => { const items: CodeContextMenuItem[] = [ - { label: "Copy path", textToCopy: path() }, + { kind: "copy", label: "Copy path", textToCopy: path() }, ]; const r = range(); if (r) { - const ref = formatLineRef(path(), r.start, r.end); - items.push({ label: `Copy ${ref}`, textToCopy: ref }); + const refStr = formatLineRef(path(), r.start, r.end); + items.push({ + kind: "copy", + label: `Copy ${refStr}`, + textToCopy: refStr, + }); + const onOpen = options.onOpen?.(); + if (onOpen) { + items.push({ + kind: "action", + label: `Open ${refStr}`, + onActivate: () => + onOpen({ path: path(), startLine: r.start, endLine: r.end }), + }); + } } return items; }, diff --git a/packages/tests/features/code-tab.feature b/packages/tests/features/code-tab.feature index a5a8bf9eb..ca160a46d 100644 --- a/packages/tests/features/code-tab.feature +++ b/packages/tests/features/code-tab.feature @@ -330,11 +330,31 @@ Feature: Code tab (review + browse) Then the diff view should contain "b-one" When I click the line number 1 in the diff view And I right-click the diff view - Then the context menu items should be "Copy path | Copy file-b.txt:1" + Then the context menu items should be "Copy path | Copy file-b.txt:1 | Open file-b.txt:1" When I click the context menu item "Copy file-b.txt:1" Then the clipboard should contain "file-b.txt:1" And the clipboard should not contain "file-a.txt" + # ── Right-click "Open" jumps from diff to full file (#881 phase 0) ── + # Reviewing a diff and wanting full-file context at the same line was + # previously two manual steps: copy `path:N`, switch to browse mode, + # paste-navigate. The "Open path:N" context-menu entry dispatches via + # the same `openInCodeTab` front door the terminal-link click uses. + 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 + # ── Live updates: filesystem changes propagate without manual refresh ── # The Code view subscribes to a watcher that observes four axes (HEAD, # reflog, index, working tree) and pushes snapshot updates whenever any From ce1546a4737f35d88ceaea7bbe4143b106739b46 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 09:58:50 -0400 Subject: [PATCH 02/21] =?UTF-8?q?fix(police):=20prefer-ts-pattern=20?= =?UTF-8?q?=E2=80=94=20dispatch=20CodeContextMenu=20items=20via=20match.ex?= =?UTF-8?q?haustive?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- packages/client/src/ui/CodeContextMenu.tsx | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/client/src/ui/CodeContextMenu.tsx b/packages/client/src/ui/CodeContextMenu.tsx index 269464c30..3296688d8 100644 --- a/packages/client/src/ui/CodeContextMenu.tsx +++ b/packages/client/src/ui/CodeContextMenu.tsx @@ -9,6 +9,7 @@ import { type Component, createSignal, For, onCleanup, Show } from "solid-js"; import { Portal } from "solid-js/web"; import { toast } from "solid-sonner"; +import { match } from "ts-pattern"; /** Two verbs over the same selection noun: copy a string to the clipboard, * or invoke an action callback. The discriminator keeps the dispatch @@ -79,14 +80,15 @@ export const CodeContextMenu: Component<{ }); const handleItem = (item: CodeContextMenuItem) => { - if (item.kind === "copy") { - navigator.clipboard - .writeText(item.textToCopy) - .then(() => toast.success(`Copied: ${item.textToCopy}`)) - .catch((err: Error) => toast.error(`Failed to copy: ${err.message}`)); - } else { - item.onActivate(); - } + match(item) + .with({ kind: "copy" }, ({ textToCopy }) => { + navigator.clipboard + .writeText(textToCopy) + .then(() => toast.success(`Copied: ${textToCopy}`)) + .catch((err: Error) => toast.error(`Failed to copy: ${err.message}`)); + }) + .with({ kind: "action" }, ({ onActivate }) => onActivate()) + .exhaustive(); close(); }; From fc2d1c9bfb2f730b7effe1d021c611b5241d293b Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 09:59:13 -0400 Subject: [PATCH 03/21] =?UTF-8?q?refactor(police):=20elegance=20=E2=80=94?= =?UTF-8?q?=20snapshot=20path()=20once=20in=20buildItems?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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). --- packages/client/src/ui/useLineSelection.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/client/src/ui/useLineSelection.ts b/packages/client/src/ui/useLineSelection.ts index 700a3b1f3..d56cc563a 100644 --- a/packages/client/src/ui/useLineSelection.ts +++ b/packages/client/src/ui/useLineSelection.ts @@ -71,12 +71,18 @@ export function useLineSelection( range, handleSelect: (r) => setRange(r), buildItems: () => { + // Snapshot `path()` once so the label, copy text, and onActivate + // capture all agree. Reading the accessor inside `onActivate` + // would resolve at click time and could disagree with the + // already-rendered label if Pierre remounts onto a different + // file between menu-open and click. + const p = path(); const items: CodeContextMenuItem[] = [ - { kind: "copy", label: "Copy path", textToCopy: path() }, + { kind: "copy", label: "Copy path", textToCopy: p }, ]; const r = range(); if (r) { - const refStr = formatLineRef(path(), r.start, r.end); + const refStr = formatLineRef(p, r.start, r.end); items.push({ kind: "copy", label: `Copy ${refStr}`, @@ -88,7 +94,7 @@ export function useLineSelection( kind: "action", label: `Open ${refStr}`, onActivate: () => - onOpen({ path: path(), startLine: r.start, endLine: r.end }), + onOpen({ path: p, startLine: r.start, endLine: r.end }), }); } } From e267f90a292c649a9acb882eef5cc9d75c644937 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 09:59:34 -0400 Subject: [PATCH 04/21] =?UTF-8?q?refactor(police):=20elegance=20=E2=80=94?= =?UTF-8?q?=20skip=20openCodeAt=20patch=20when=20already=20at=20target?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- packages/client/src/right-panel/useRightPanel.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/client/src/right-panel/useRightPanel.ts b/packages/client/src/right-panel/useRightPanel.ts index 0153f3bdb..7a3a81188 100644 --- a/packages/client/src/right-panel/useRightPanel.ts +++ b/packages/client/src/right-panel/useRightPanel.ts @@ -45,15 +45,23 @@ export function useRightPanel() { /** Atomic "open the Code tab at `mode`" — uncollapse the panel, * switch to Code, set the requested sub-mode. Single preferences * patch so the UI ticks once instead of three times when callers - * need all three transitions together. */ - openCodeAt: (mode: CodeTabView) => + * need all three transitions together. Skips the patch when the + * panel is already in the target state (every diff→browse and + * browse→browse `openInCodeTab` would otherwise round-trip a + * three-field preferences write to the server). */ + openCodeAt: (mode: CodeTabView) => { + const cur = rp(); + if (!cur.collapsed && cur.activeTab === "code" && cur.codeMode === mode) { + return; + } updatePreferences({ rightPanel: { collapsed: false, activeTab: "code", codeMode: mode, }, - }), + }); + }, /** Change the sub-mode within the Code tab. */ setCodeMode: (mode: CodeTabView) => updatePreferences({ rightPanel: { codeMode: mode } }), From ee14727559101ea301b41a58d21fadd466b0042b Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 10:00:00 -0400 Subject: [PATCH 05/21] =?UTF-8?q?refactor(police):=20elegance=20=E2=80=94?= =?UTF-8?q?=20trim=20narrating=20comment=20on=20diff-view=20onOpen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/client/src/right-panel/CodeTab.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/client/src/right-panel/CodeTab.tsx b/packages/client/src/right-panel/CodeTab.tsx index 9f8185b6c..4e8b40632 100644 --- a/packages/client/src/right-panel/CodeTab.tsx +++ b/packages/client/src/right-panel/CodeTab.tsx @@ -546,10 +546,7 @@ const CodeTab: Component<{ meta: TerminalMetadata | null }> = (props) => { { - // Diff path is already repo-relative; cwd is - // irrelevant. Same dispatch as a terminal-link - // click — `openInCodeTab` flips the panel to - // browse mode and surfaces the line range. + // Diff paths are repo-relative; cwd is irrelevant. const repo = repoPath(); if (repo === null) return; openInCodeTab({ From e50f4ac15a4c74b36410cbb4406133d0ab7f4104 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 10:00:07 -0400 Subject: [PATCH 06/21] =?UTF-8?q?refactor(police):=20elegance=20=E2=80=94?= =?UTF-8?q?=20drop=20future-tense=20narration=20from=20targetMode=20jsdoc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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". --- packages/client/src/right-panel/openInCodeTab.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/client/src/right-panel/openInCodeTab.ts b/packages/client/src/right-panel/openInCodeTab.ts index a571f55ca..d9bd8bfd1 100644 --- a/packages/client/src/right-panel/openInCodeTab.ts +++ b/packages/client/src/right-panel/openInCodeTab.ts @@ -29,10 +29,8 @@ export interface OpenInCodeTabRequest { * `bar.ts:42` while standing in a subdirectory of the repo" case; * undefined falls back to repo-relative interpretation only. */ cwd?: string; - /** Which Code-tab sub-mode the request expects to land in. Today - * every producer passes `"browse"`; later phases may route a - * comment-revisit click back to the diff mode the comment was - * authored in. */ + /** Which Code-tab sub-mode the request expects to land in. + * Producers that don't track an authoring mode pass `"browse"`. */ targetMode: CodeTabView; } From e268a5baf5af6d68d65fe857618ef9c6b7a73161 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 11:00:07 -0400 Subject: [PATCH 07/21] feat(ui): icons on context-menu items so Copy vs Open is glyphic, not just verbal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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. --- packages/client/src/ui/CodeContextMenu.tsx | 17 ++++++++--- packages/client/src/ui/Icons.tsx | 33 ++++++++++++++++++++++ packages/client/src/ui/useLineSelection.ts | 5 +++- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/packages/client/src/ui/CodeContextMenu.tsx b/packages/client/src/ui/CodeContextMenu.tsx index 3296688d8..0fc864f99 100644 --- a/packages/client/src/ui/CodeContextMenu.tsx +++ b/packages/client/src/ui/CodeContextMenu.tsx @@ -7,24 +7,29 @@ * line selection — items can return null to omit themselves. */ import { type Component, createSignal, For, onCleanup, Show } from "solid-js"; -import { Portal } from "solid-js/web"; +import { Dynamic, Portal } from "solid-js/web"; import { toast } from "solid-sonner"; import { match } from "ts-pattern"; /** Two verbs over the same selection noun: copy a string to the clipboard, * or invoke an action callback. The discriminator keeps the dispatch * explicit so adding a third verb (e.g. "share") doesn't tempt the - * handler into reading every field on every item. */ + * handler into reading every field on every item. Every item carries + * a leading icon — verbs like "Copy" and "Open" share the same label + * shape (` path:N`) and disambiguate poorly when only the first + * word differs; the glyph makes the verb readable at a glance. */ export type CodeContextMenuItem = | { kind: "copy"; label: string; + icon: Component<{ class?: string }>; /** Text written to the clipboard; success toast names the item. */ textToCopy: string; } | { kind: "action"; label: string; + icon: Component<{ class?: string }>; /** Fired on click. The item is closed regardless of completion. */ onActivate: () => void; }; @@ -106,10 +111,14 @@ export const CodeContextMenu: Component<{ )} diff --git a/packages/client/src/ui/Icons.tsx b/packages/client/src/ui/Icons.tsx index bb18d7fa5..47187d10e 100644 --- a/packages/client/src/ui/Icons.tsx +++ b/packages/client/src/ui/Icons.tsx @@ -126,6 +126,39 @@ export const DiffBranchIcon: Component<{ class?: string }> = (props) => ( ); +export const CopyIcon: Component<{ class?: string }> = (props) => ( + +); + +export const OpenIcon: Component<{ class?: string }> = (props) => ( + +); + export const CloseIcon: Component<{ class?: string }> = (props) => ( onOpen({ path: p, startLine: r.start, endLine: r.end }), }); From c1bb9b71b5cb07f5a4a75b5b2a0fa62f2ba9e7f4 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 11:04:17 -0400 Subject: [PATCH 08/21] fix(solid-pierre): expand ancestors of selectedPath so external navigation reveals nested files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- packages/solid-pierre/src/FileTree.tsx | 34 ++++++++++++++++++++---- packages/tests/features/code-tab.feature | 9 ++++--- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/packages/solid-pierre/src/FileTree.tsx b/packages/solid-pierre/src/FileTree.tsx index 86734d869..e27561b3a 100644 --- a/packages/solid-pierre/src/FileTree.tsx +++ b/packages/solid-pierre/src/FileTree.tsx @@ -28,6 +28,19 @@ type FileTreeOptions = ConstructorParameters[0]; type Composition = NonNullable; type FileTreeContextMenu = NonNullable; +/** Directory paths that contain `path`, formatted with the trailing + * slash Pierre uses for folder keys (`src/`, `src/right-panel/`). The + * list is leaf-first → root, which is the order Pierre's + * `initialExpandedPaths` walks. */ +function ancestorsOf(path: string): string[] { + const parts = path.split("/").filter(Boolean); + const out: string[] = []; + for (let i = 1; i < parts.length; i++) { + out.push(`${parts.slice(0, i).join("/")}/`); + } + return out; +} + export type FileTreeProps = { paths: string[]; gitStatus?: GitStatusEntry[]; @@ -110,15 +123,26 @@ export const FileTree: Component = (props) => { // `resetPaths` takes the new path inventory and the directories to // open in one call (Pierre's `FileTreeResetOptions.initialExpandedPaths`). - // Tracking both inputs in the same effect means a paths-and-ancestors + // Tracking the inputs in the same effect means a paths-and-ancestors // swap lands atomically — no second effect, no ordering invariant - // between "rebuild tree" and "open ancestors". + // between "rebuild tree" and "open ancestors". The selected path's + // ancestors are merged in too: when an external caller drives selection + // (e.g. a terminal `path:line` click resolving into a nested file), + // the parents must be expanded for the row to be visible. Pierre's + // public surface doesn't expose `expandDirectory` directly, so the + // expand-on-select is routed through this same `resetPaths` call. createEffect( on( - [() => props.paths, () => props.expandPaths], - ([paths, expandPaths]) => { + [ + () => props.paths, + () => props.expandPaths, + () => props.selectedPath ?? null, + ], + ([paths, expandPaths, selectedPath]) => { try { - tree?.resetPaths(paths, { initialExpandedPaths: expandPaths }); + const ancestors = selectedPath ? ancestorsOf(selectedPath) : []; + const expanded = [...(expandPaths ?? []), ...ancestors]; + tree?.resetPaths(paths, { initialExpandedPaths: expanded }); } catch (e) { props.onError(toError(e)); } diff --git a/packages/tests/features/code-tab.feature b/packages/tests/features/code-tab.feature index ca160a46d..09aa83761 100644 --- a/packages/tests/features/code-tab.feature +++ b/packages/tests/features/code-tab.feature @@ -343,17 +343,18 @@ Feature: Code tab (review + browse) 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 run "mkdir -p docs && printf 'first\nsecond\nthird\n' > docs/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 Code tab should list a changed file "docs/notes.txt" + When I click the changed file "docs/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" + And I click the context menu item "Open docs/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 + And the file browser should show a file "docs/notes.txt" # ── Live updates: filesystem changes propagate without manual refresh ── # The Code view subscribes to a watcher that observes four axes (HEAD, From 623437d6262b5e693c0dff63c920e18e9402c23d Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 12:54:23 -0400 Subject: [PATCH 09/21] feat(right-panel): right-click on a line is the single context-menu entry point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../client/src/right-panel/CodeMenuFrame.tsx | 34 ++++++++++++++- packages/client/src/ui/useLineSelection.ts | 42 +++++++++++-------- packages/tests/features/code-tab.feature | 14 +++---- .../tests/step_definitions/code_tab_steps.ts | 38 +++++++++++++++++ 4 files changed, 100 insertions(+), 28 deletions(-) diff --git a/packages/client/src/right-panel/CodeMenuFrame.tsx b/packages/client/src/right-panel/CodeMenuFrame.tsx index 06029276b..cb653c3ed 100644 --- a/packages/client/src/right-panel/CodeMenuFrame.tsx +++ b/packages/client/src/right-panel/CodeMenuFrame.tsx @@ -33,6 +33,23 @@ export type CodeMenuFrameProps = { onOpen?: (ref: LineRef) => void; }; +/** Walk the contextmenu event's composed path (which pierces Pierre's + * open shadow DOM, where `event.target` would otherwise be retargeted + * to the shadow host) and return the line number from the first + * element carrying `data-column-number`. Returns null when the + * right-click landed outside any gutter line — empty area, scrollbar, + * decoration row — so the host can skip opening a menu entirely. */ +function lineFromContextMenu(event: MouseEvent): number | null { + for (const node of event.composedPath()) { + if (!(node instanceof Element)) continue; + const raw = node.getAttribute("data-column-number"); + if (raw === null) continue; + const n = Number(raw); + if (Number.isFinite(n) && n >= 1) return n; + } + return null; +} + export const CodeMenuFrame: Component = (props) => { let menuCtrl: CodeContextMenuController | undefined; const selection = useLineSelection(() => props.path, { @@ -44,7 +61,22 @@ export const CodeMenuFrame: Component = (props) => { // Attach contextmenu via addEventListener so the host div doesn't // carry interactive JSX props — the inner Pierre canvas is the // actual interactive surface; the host is layout only. - ref={(el) => el.addEventListener("contextmenu", (e) => menuCtrl?.open(e))} + ref={(el) => + el.addEventListener("contextmenu", (e) => { + // Right-click on a gutter line is the single entry point for + // the context menu: it both selects the line and opens the + // menu in one gesture. Right-clicks elsewhere (whitespace, + // scrollbar, decoration row) clear the range and produce no + // menu — `buildItems` returns empty when no range is set, + // so `menuCtrl.open` short-circuits without preventing the + // browser default. + const line = lineFromContextMenu(e); + selection.handleSelect( + line === null ? null : { start: line, end: line }, + ); + menuCtrl?.open(e); + }) + } class="h-full w-full" > {props.children(selection)} diff --git a/packages/client/src/ui/useLineSelection.ts b/packages/client/src/ui/useLineSelection.ts index 7ab3e6bbd..c1e1ec858 100644 --- a/packages/client/src/ui/useLineSelection.ts +++ b/packages/client/src/ui/useLineSelection.ts @@ -72,34 +72,40 @@ export function useLineSelection( range, handleSelect: (r) => setRange(r), buildItems: () => { - // Snapshot `path()` once so the label, copy text, and onActivate + // The menu is line-anchored: every entry refers to a specific + // `path:line` ref, so a build without a selected range returns + // empty. ``'s `open()` short-circuits on empty + // items, which means the right-click on whitespace produces no + // menu — the path-only menu was actionable noise (just one + // "Copy path" item, redundant with the in-menu Copy entries + // that ALSO copy the path bundled with the line). + const r = range(); + if (!r) return []; + // Snapshot `path()` once so label, copy text, and onActivate // capture all agree. Reading the accessor inside `onActivate` // would resolve at click time and could disagree with the - // already-rendered label if Pierre remounts onto a different - // file between menu-open and click. + // rendered label if Pierre remounts to a different file between + // menu-open and click. const p = path(); + const refStr = formatLineRef(p, r.start, r.end); const items: CodeContextMenuItem[] = [ { kind: "copy", label: "Copy path", icon: CopyIcon, textToCopy: p }, - ]; - const r = range(); - if (r) { - const refStr = formatLineRef(p, r.start, r.end); - items.push({ + { kind: "copy", label: `Copy ${refStr}`, icon: CopyIcon, textToCopy: refStr, + }, + ]; + const onOpen = options.onOpen?.(); + if (onOpen) { + items.push({ + kind: "action", + label: `Open ${refStr}`, + icon: OpenIcon, + onActivate: () => + onOpen({ path: p, startLine: r.start, endLine: r.end }), }); - const onOpen = options.onOpen?.(); - if (onOpen) { - items.push({ - kind: "action", - label: `Open ${refStr}`, - icon: OpenIcon, - onActivate: () => - onOpen({ path: p, startLine: r.start, endLine: r.end }), - }); - } } return items; }, diff --git a/packages/tests/features/code-tab.feature b/packages/tests/features/code-tab.feature index 09aa83761..c7f428131 100644 --- a/packages/tests/features/code-tab.feature +++ b/packages/tests/features/code-tab.feature @@ -288,7 +288,7 @@ Feature: Code tab (review + browse) # ── Pierre file/diff viewer right-click menu (Copy path:line) ── - Scenario: Right-click on file content with a selected line copies "path:line" + Scenario: Right-click on a file content line copies "path:line" When I run "git init /tmp/kolu-browse-ctx && cd /tmp/kolu-browse-ctx" And I run "printf 'alpha\nbeta\ngamma\n' > letters.txt" And I run "git add . && git commit -m init" @@ -296,8 +296,7 @@ Feature: Code tab (review + browse) And I click the Code tab mode "browse" When I click the file "letters.txt" in the file browser Then the file content should contain "beta" - When I click the line number 2 in the file content - And I right-click the file content + When I right-click line 2 in the file content And I click the context menu item "Copy letters.txt:2" Then the clipboard should contain "letters.txt:2" @@ -322,14 +321,12 @@ Feature: Code tab (review + browse) And the Code tab should list a changed file "file-b.txt" When I click the changed file "file-a.txt" in the Code tab Then the diff view should contain "a-one" - When I click the line number 1 in the diff view - And I right-click the diff view + When I right-click line 1 in the diff view And I click the context menu item "Copy file-a.txt:1" Then the clipboard should contain "file-a.txt:1" When I click the changed file "file-b.txt" in the Code tab Then the diff view should contain "b-one" - When I click the line number 1 in the diff view - And I right-click the diff view + When I right-click line 1 in the diff view Then the context menu items should be "Copy path | Copy file-b.txt:1 | Open file-b.txt:1" When I click the context menu item "Copy file-b.txt:1" Then the clipboard should contain "file-b.txt:1" @@ -348,8 +345,7 @@ Feature: Code tab (review + browse) Then the Code tab should list a changed file "docs/notes.txt" When I click the changed file "docs/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 + When I right-click line 2 in the diff view And I click the context menu item "Open docs/notes.txt:2" Then the Code tab mode should be "browse" And the selected file should show content "second" diff --git a/packages/tests/step_definitions/code_tab_steps.ts b/packages/tests/step_definitions/code_tab_steps.ts index d8d5e9484..069a153ca 100644 --- a/packages/tests/step_definitions/code_tab_steps.ts +++ b/packages/tests/step_definitions/code_tab_steps.ts @@ -156,6 +156,44 @@ When("I right-click the file content", async function (this: KoluWorld) { await rightClickViewRoot(this, FILE_VIEW); }); +// Right-click on a specific gutter line — selects the line and opens +// the context menu in one gesture (the host's `contextmenu` handler +// in `CodeMenuFrame` derives the line from the click target). Replaces +// the old two-step "click line then right-click viewer root" idiom. +async function rightClickLineIn(world: KoluWorld, root: string, line: number) { + const lineEl = world.page.locator(`${root} [data-column-number="${line}"]`); + await lineEl.first().waitFor({ state: "visible", timeout: POLL_TIMEOUT }); + const box = await pollFor({ + observe: () => lineEl.first().boundingBox(), + isDone: (b) => !!b && b.width > 0 && b.height > 0, + onTimeout: (last, ms) => + new Error( + `gutter ${root} [data-column-number="${line}"] has no usable bounding box after ${ms}ms (last=${JSON.stringify(last)})`, + ), + timeoutMs: POLL_TIMEOUT, + intervalMs: 50, + }); + if (!box) throw new Error("unreachable: pollFor returned without box"); + await world.page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await world.page.mouse.down({ button: "right" }); + await world.page.mouse.up({ button: "right" }); + await world.waitForFrame(); +} + +When( + "I right-click line {int} in the diff view", + async function (this: KoluWorld, line: number) { + await rightClickLineIn(this, DIFF_VIEW, line); + }, +); + +When( + "I right-click line {int} in the file content", + async function (this: KoluWorld, line: number) { + await rightClickLineIn(this, FILE_VIEW, line); + }, +); + When( "I click the line number {int} in the diff view", async function (this: KoluWorld, line: number) { From 325e240aa90f371db4cea0033dc3f8d6cea447ed Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 12:54:33 -0400 Subject: [PATCH 10/21] fix(solid-pierre): include selectedPath ancestors in initialExpandedPaths at construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, `` remounts FRESH (the `/` 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. --- packages/solid-pierre/src/FileTree.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/solid-pierre/src/FileTree.tsx b/packages/solid-pierre/src/FileTree.tsx index e27561b3a..f883d2833 100644 --- a/packages/solid-pierre/src/FileTree.tsx +++ b/packages/solid-pierre/src/FileTree.tsx @@ -95,10 +95,16 @@ export const FileTree: Component = (props) => { onMount(() => { try { + const selectedAncestors = props.selectedPath + ? ancestorsOf(props.selectedPath) + : []; tree = new FileTreeClass({ paths: props.paths, initialExpansion: props.initialExpansion ?? "closed", - initialExpandedPaths: props.expandPaths, + initialExpandedPaths: [ + ...(props.expandPaths ?? []), + ...selectedAncestors, + ], flattenEmptyDirectories: props.flattenEmptyDirectories ?? true, stickyFolders: props.stickyFolders ?? true, icons: props.icons, From 3271b6f0cf3ec20bf8599eb905409f9b140684fd Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 13:57:38 -0400 Subject: [PATCH 11/21] fix(right-panel): batch openInCodeTab's two writes so CodeTab's resetKey effect sees the pending request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../client/src/right-panel/openInCodeTab.ts | 20 +++++++++++++------ packages/tests/features/code-tab.feature | 1 + .../tests/step_definitions/code_tab_steps.ts | 16 +++++++++++++++ 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/packages/client/src/right-panel/openInCodeTab.ts b/packages/client/src/right-panel/openInCodeTab.ts index d9bd8bfd1..731ef2e5c 100644 --- a/packages/client/src/right-panel/openInCodeTab.ts +++ b/packages/client/src/right-panel/openInCodeTab.ts @@ -12,7 +12,7 @@ * their `ref` content matches and re-paint the highlight. */ import type { CodeTabView } from "kolu-common/surface"; -import { createSignal } from "solid-js"; +import { batch, createSignal } from "solid-js"; import type { LineRef } from "../ui/lineRef"; import { useRightPanel } from "./useRightPanel"; @@ -40,10 +40,18 @@ export const pendingOpen = pending; /** Open the right panel's Code tab at `req.targetMode` showing `req.ref`. * The two reactive writes (preferences patch + pending-request signal) - * fire inside the same call, so `CodeTab`'s `resetKey` effect and - * `pendingOpen` effect both observe the request on the tick the view - * changes — no call-site ordering discipline required. */ + * are wrapped in `batch()` so SolidJS defers all dependent effects + * until both have committed. Without the batch, the preferences + * optimistic update ticks `view()` first, which fires `CodeTab`'s + * `resetKey` effect — at that moment `pendingOpen()` is still null + * (setPending hasn't run yet), the guard fails, and selectedPath gets + * cleared. The Terminal call site used to escape this by being inside + * a JSX click handler (Solid's event delegation auto-batches), but + * once the writes moved into a standalone function the implicit batch + * was lost. */ export function openInCodeTab(req: OpenInCodeTabRequest): void { - useRightPanel().openCodeAt(req.targetMode); - setPending(req); + batch(() => { + useRightPanel().openCodeAt(req.targetMode); + setPending(req); + }); } diff --git a/packages/tests/features/code-tab.feature b/packages/tests/features/code-tab.feature index c7f428131..b7719a94f 100644 --- a/packages/tests/features/code-tab.feature +++ b/packages/tests/features/code-tab.feature @@ -351,6 +351,7 @@ Feature: Code tab (review + browse) And the selected file should show content "second" And line 2 should be selected in the file content And the file browser should show a file "docs/notes.txt" + And the file "docs/notes.txt" should be selected in the file browser # ── Live updates: filesystem changes propagate without manual refresh ── # The Code view subscribes to a watcher that observes four axes (HEAD, diff --git a/packages/tests/step_definitions/code_tab_steps.ts b/packages/tests/step_definitions/code_tab_steps.ts index 069a153ca..658f2cfc0 100644 --- a/packages/tests/step_definitions/code_tab_steps.ts +++ b/packages/tests/step_definitions/code_tab_steps.ts @@ -401,6 +401,22 @@ Then( }, ); +// Pierre marks selected rows with `aria-selected="true"` (and a boolean +// `data-item-selected` that may serialize as `""` or `"true"` depending +// on the renderer — `aria-selected` is the reliable string form). The +// row must also be VISIBLE — collapsed-ancestor descendants fail +// `state: "visible"` even when marked selected, so this step implicitly +// verifies ancestor expansion too. +Then( + "the file {string} should be selected in the file browser", + async function (this: KoluWorld, path: string) { + const item = this.page.locator( + `${TREE} [data-item-path="${path}"][data-item-type="file"][aria-selected="true"]:not([data-file-tree-sticky-row])`, + ); + await item.waitFor({ state: "visible", timeout: POLL_TIMEOUT }); + }, +); + Then( "the Code tab content should show the select hint {string}", async function (this: KoluWorld, expected: string) { From 46d7a57cf0d54c4b0d78b380048e7055e314fcdd Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:09:51 -0400 Subject: [PATCH 12/21] docs(solid-pierre): note Pierre's missing scroll-to-path API and link upstream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/solid-pierre/src/FileTree.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/solid-pierre/src/FileTree.tsx b/packages/solid-pierre/src/FileTree.tsx index f883d2833..79f77fe1d 100644 --- a/packages/solid-pierre/src/FileTree.tsx +++ b/packages/solid-pierre/src/FileTree.tsx @@ -93,6 +93,16 @@ export const FileTree: Component = (props) => { // membership in this set is a reliable file-vs-folder discriminator. const fileSet = createMemo(() => new Set(props.paths)); + // Known limitation: Pierre's vanilla `FileTree` doesn't expose a + // public `scrollToPath`. Its `scrollFocusedRowIntoView` (in + // `dist/render/FileTreeView.js`) is gated on `shouldOwnDomFocus`, + // which is set only by user-initiated handlers (row click, keyboard + // nav). Programmatic selection via `initialSelectedPaths` / + // `getItem(path)?.select()` marks the row `aria-selected="true"` but + // does NOT scroll the virtualizer to reveal it. For small trees the + // row happens to sit in the visible window; for large worktrees the + // selected row stays virtualized off-screen until the user scrolls. + // Filed upstream: https://github.com/pierrecomputer/pierre/issues/676 onMount(() => { try { const selectedAncestors = props.selectedPath From 59b4711db1fd25b02d86ddfa6bf42e8b94aeeb1e Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:22:38 -0400 Subject: [PATCH 13/21] =?UTF-8?q?refactor(hickey):=20elegance=20=E2=80=94?= =?UTF-8?q?=20merge=20clickLineGutter=20/=20rightClickLineIn=20into=20one?= =?UTF-8?q?=20helper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../tests/step_definitions/code_tab_steps.ts | 48 +++++++------------ 1 file changed, 18 insertions(+), 30 deletions(-) diff --git a/packages/tests/step_definitions/code_tab_steps.ts b/packages/tests/step_definitions/code_tab_steps.ts index 658f2cfc0..15591a1b0 100644 --- a/packages/tests/step_definitions/code_tab_steps.ts +++ b/packages/tests/step_definitions/code_tab_steps.ts @@ -114,13 +114,20 @@ When( // diff viewer (`DIFF_VIEW`) wrap the same Pierre primitive, so the // gutter selector and mouse dance are identical — only the host // element's CSS root changes. -async function clickLineGutterIn(world: KoluWorld, root: string, line: number) { +// One shared interaction for any mouse button on a gutter line. The +// virtualizer-bounding-box race (Pierre's `VirtualizedFileDiff` is +// keyed on path; switching files makes the element pass +// `waitFor(visible)` and then return a null bounding box on the very +// next call as the virtualizer re-measures) is identical between +// left- and right-click — only the button differs. +async function interactWithGutterLine( + world: KoluWorld, + root: string, + line: number, + button: "left" | "right", +): Promise { const lineEl = world.page.locator(`${root} [data-column-number="${line}"]`); await lineEl.first().waitFor({ state: "visible", timeout: POLL_TIMEOUT }); - // Switching files re-mounts Pierre's `VirtualizedFileDiff` (FileDiff - // is keyed on path), so the line element can pass `waitFor(visible)` - // and then return a null bounding box on the very next call as the - // virtualizer re-measures. Poll until the box is stable. const box = await pollFor({ observe: () => lineEl.first().boundingBox(), isDone: (b) => !!b && b.width > 0 && b.height > 0, @@ -133,8 +140,8 @@ async function clickLineGutterIn(world: KoluWorld, root: string, line: number) { }); if (!box) throw new Error("unreachable: pollFor returned without box"); await world.page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); - await world.page.mouse.down(); - await world.page.mouse.up(); + await world.page.mouse.down({ button }); + await world.page.mouse.up({ button }); await world.waitForFrame(); } @@ -148,7 +155,7 @@ async function rightClickViewRoot(world: KoluWorld, root: string) { When( "I click the line number {int} in the file content", async function (this: KoluWorld, line: number) { - await clickLineGutterIn(this, FILE_VIEW, line); + await interactWithGutterLine(this, FILE_VIEW, line, "left"); }, ); @@ -160,44 +167,25 @@ When("I right-click the file content", async function (this: KoluWorld) { // the context menu in one gesture (the host's `contextmenu` handler // in `CodeMenuFrame` derives the line from the click target). Replaces // the old two-step "click line then right-click viewer root" idiom. -async function rightClickLineIn(world: KoluWorld, root: string, line: number) { - const lineEl = world.page.locator(`${root} [data-column-number="${line}"]`); - await lineEl.first().waitFor({ state: "visible", timeout: POLL_TIMEOUT }); - const box = await pollFor({ - observe: () => lineEl.first().boundingBox(), - isDone: (b) => !!b && b.width > 0 && b.height > 0, - onTimeout: (last, ms) => - new Error( - `gutter ${root} [data-column-number="${line}"] has no usable bounding box after ${ms}ms (last=${JSON.stringify(last)})`, - ), - timeoutMs: POLL_TIMEOUT, - intervalMs: 50, - }); - if (!box) throw new Error("unreachable: pollFor returned without box"); - await world.page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); - await world.page.mouse.down({ button: "right" }); - await world.page.mouse.up({ button: "right" }); - await world.waitForFrame(); -} When( "I right-click line {int} in the diff view", async function (this: KoluWorld, line: number) { - await rightClickLineIn(this, DIFF_VIEW, line); + await interactWithGutterLine(this, DIFF_VIEW, line, "right"); }, ); When( "I right-click line {int} in the file content", async function (this: KoluWorld, line: number) { - await rightClickLineIn(this, FILE_VIEW, line); + await interactWithGutterLine(this, FILE_VIEW, line, "right"); }, ); When( "I click the line number {int} in the diff view", async function (this: KoluWorld, line: number) { - await clickLineGutterIn(this, DIFF_VIEW, line); + await interactWithGutterLine(this, DIFF_VIEW, line, "left"); }, ); From 1f0417180f0d5d6c9c217cd869782999744b95e2 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:27:46 -0400 Subject: [PATCH 14/21] =?UTF-8?q?refactor(hickey):=20elegance=20=E2=80=94?= =?UTF-8?q?=20name=20the=20resetKey=20guard=20predicate=20to=20surface=20e?= =?UTF-8?q?ffect-ordering=20coupling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/client/src/right-panel/CodeTab.tsx | 38 +++++++++++++-------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/packages/client/src/right-panel/CodeTab.tsx b/packages/client/src/right-panel/CodeTab.tsx index 4e8b40632..ee89b2ffe 100644 --- a/packages/client/src/right-panel/CodeTab.tsx +++ b/packages/client/src/right-panel/CodeTab.tsx @@ -180,26 +180,34 @@ const CodeTab: Component<{ meta: TerminalMetadata | null }> = (props) => { // absolute path or `null`, never the empty string that would alias // null. const resetKey = createMemo(() => `${repoPath() ?? ""}::${view()}`); + + /** The resetKey effect runs BEFORE the pendingOpen effect by + * registration order. When a navigation request is about to land in + * the new (repo, mode) — same repoRoot, target mode equals the + * freshly-ticked `view()`, and the request hasn't already been + * consumed by `handled()` — the resetKey effect must skip its clear, + * or the pendingOpen effect would null what we're about to set. + * This predicate names the cross-effect temporal coupling so the + * guard isn't a wall of inline conjunctions a future editor has to + * re-derive. The `batch()` in `openInCodeTab` ensures both writes + * (`view` and `pendingOpen`) commit before either effect fires; the + * registration-order discipline survives ABOVE that. */ + const isPendingOpenAboutToLand = (): boolean => { + const req = pendingOpen(); + return ( + req !== null && + req.repoRoot === repoPath() && + req.targetMode === view() && + handled()?.request !== req + ); + }; + createEffect( on( resetKey, () => { setSearchQuery(""); - // Skip the selectedPath clear when an incoming request is - // about to land in the new mode — the resetKey effect runs - // before the pendingOpen effect (registration order), and an - // unconditional clear would null what we're about to set. - // Reading `req.targetMode` (not `view()`) makes the guard - // robust to user-driven mode flips that race the click. - const req = pendingOpen(); - if ( - req && - req.repoRoot === repoPath() && - req.targetMode === view() && - handled()?.request !== req - ) { - return; - } + if (isPendingOpenAboutToLand()) return; setSelectedPath(null); }, { defer: true }, From 7e89d78e4632ae597fc58e11504e306f6ce6c7e2 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:27:56 -0400 Subject: [PATCH 15/21] docs(right-panel): annotate pendingOpen signal's single-owner assumption 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. --- packages/client/src/right-panel/openInCodeTab.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/client/src/right-panel/openInCodeTab.ts b/packages/client/src/right-panel/openInCodeTab.ts index 731ef2e5c..3b6c0c77f 100644 --- a/packages/client/src/right-panel/openInCodeTab.ts +++ b/packages/client/src/right-panel/openInCodeTab.ts @@ -34,6 +34,12 @@ export interface OpenInCodeTabRequest { targetMode: CodeTabView; } +// Module-level singleton. Right-panel state is a singleton in Kolu — +// one panel, one CodeTab — and the navigation request is meant for +// the unique consumer. If kolu ever mounts multiple CodeTab instances +// (split panels, multi-window), this signal must move into a +// SolidJS context or scope to a per-panel store, otherwise concurrent +// consumers will race on each other's pending requests. const [pending, setPending] = createSignal(null); export const pendingOpen = pending; From c00cb08550b46309e8ee80c7a25e8873d1740bf2 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:27:58 -0400 Subject: [PATCH 16/21] docs(solid-pierre): explain snapshot vs reactive selectedPath reads across mount and effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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). --- packages/solid-pierre/src/FileTree.tsx | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/solid-pierre/src/FileTree.tsx b/packages/solid-pierre/src/FileTree.tsx index 79f77fe1d..0d7ebed87 100644 --- a/packages/solid-pierre/src/FileTree.tsx +++ b/packages/solid-pierre/src/FileTree.tsx @@ -105,6 +105,19 @@ export const FileTree: Component = (props) => { // Filed upstream: https://github.com/pierrecomputer/pierre/issues/676 onMount(() => { try { + // `props.selectedPath` is read once here as a snapshot, then + // again reactively in the deferred resetPaths effect below. The + // two sites encapsulate two distinct lifecycle moments — initial + // construction state vs. subsequent reactive state — and Pierre + // doesn't expose a hook to re-feed `initialExpandedPaths` after + // the constructor. Today's only call site (`CodeTab`) initializes + // `selectedPath` to `null` (CodeTab.tsx) and updates it via the + // pendingOpen effect AFTER the FileTree's paths stream settles, + // so this snapshot is always `null` at mount — the deferred + // effect handles the real cases. A future caller that passes a + // non-null `selectedPath` on first render would also see its + // ancestors expanded because of the snapshot here, NOT the + // deferred effect (which skips its initial run). const selectedAncestors = props.selectedPath ? ancestorsOf(props.selectedPath) : []; From aaf4e5697f78e330890c38a3ce23ecd2d4ba1ba9 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:34:27 -0400 Subject: [PATCH 17/21] =?UTF-8?q?fix(police):=20no-dead-code=20=E2=80=94?= =?UTF-8?q?=20remove=20unused=20right-click=20view-root=20step=20defs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/tests/step_definitions/code_tab_steps.ts | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/packages/tests/step_definitions/code_tab_steps.ts b/packages/tests/step_definitions/code_tab_steps.ts index 15591a1b0..06995b438 100644 --- a/packages/tests/step_definitions/code_tab_steps.ts +++ b/packages/tests/step_definitions/code_tab_steps.ts @@ -145,13 +145,6 @@ async function interactWithGutterLine( await world.waitForFrame(); } -async function rightClickViewRoot(world: KoluWorld, root: string) { - const view = world.page.locator(root); - await view.waitFor({ state: "visible", timeout: POLL_TIMEOUT }); - await view.click({ button: "right" }); - await world.waitForFrame(); -} - When( "I click the line number {int} in the file content", async function (this: KoluWorld, line: number) { @@ -159,10 +152,6 @@ When( }, ); -When("I right-click the file content", async function (this: KoluWorld) { - await rightClickViewRoot(this, FILE_VIEW); -}); - // Right-click on a specific gutter line — selects the line and opens // the context menu in one gesture (the host's `contextmenu` handler // in `CodeMenuFrame` derives the line from the click target). Replaces @@ -189,10 +178,6 @@ When( }, ); -When("I right-click the diff view", async function (this: KoluWorld) { - await rightClickViewRoot(this, DIFF_VIEW); -}); - // Pierre marks selected gutter + content rows with `data-selected-line` // (see @pierre/diffs InteractionManager.renderSelectedLines). The gutter // element also carries `data-column-number`, so we can pinpoint the line From d23d2473ac8ab8debe3f0e4729568800e69def43 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:36:03 -0400 Subject: [PATCH 18/21] =?UTF-8?q?refactor(police):=20reuse=20=E2=80=94=20p?= =?UTF-8?q?romote=20ancestorDirectoryPaths=20to=20@kolu/solid-pierre?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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`. --- packages/client/src/right-panel/fileSearch.ts | 17 ++----------- packages/solid-pierre/src/FileTree.tsx | 24 ++++++++++++------- packages/solid-pierre/src/index.ts | 2 +- 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/packages/client/src/right-panel/fileSearch.ts b/packages/client/src/right-panel/fileSearch.ts index d0156bbd9..da14c8338 100644 --- a/packages/client/src/right-panel/fileSearch.ts +++ b/packages/client/src/right-panel/fileSearch.ts @@ -13,6 +13,8 @@ * the directories the wrapper should ensure are open so matches don't * hide behind a collapsed parent on first paint. */ +import { ancestorDirectoryPaths } from "@kolu/solid-pierre"; + type FileTreeSearchProjection = { projectedPaths: string[]; expandedAncestors: string[]; @@ -39,21 +41,6 @@ function pathContainsTokensInOrder( return true; } -/** Pierre uses `getAncestorDirectoryPaths` internally to drive - * expansion in `hide-non-matches` mode. Mirror that exact shape so - * the wrapper's expansion request reaches every dir Pierre infers - * from the projected paths. */ -function ancestorDirectoryPaths(path: string): string[] { - const normalized = path.endsWith("/") ? path.slice(0, -1) : path; - if (normalized.length === 0) return []; - const segments = normalized.split("/"); - const out: string[] = []; - for (let i = 1; i < segments.length; i += 1) { - out.push(`${segments.slice(0, i).join("/")}/`); - } - return out; -} - export function projectFileTreeSearch( paths: string[], query: string, diff --git a/packages/solid-pierre/src/FileTree.tsx b/packages/solid-pierre/src/FileTree.tsx index 0d7ebed87..074e72bed 100644 --- a/packages/solid-pierre/src/FileTree.tsx +++ b/packages/solid-pierre/src/FileTree.tsx @@ -29,14 +29,18 @@ type Composition = NonNullable; type FileTreeContextMenu = NonNullable; /** Directory paths that contain `path`, formatted with the trailing - * slash Pierre uses for folder keys (`src/`, `src/right-panel/`). The - * list is leaf-first → root, which is the order Pierre's - * `initialExpandedPaths` walks. */ -function ancestorsOf(path: string): string[] { - const parts = path.split("/").filter(Boolean); + * slash Pierre uses for folder keys (`src/`, `src/right-panel/`). + * Tolerates an input that already carries a trailing slash (folder + * path) by stripping it before splitting. Mirrors the shape Pierre's + * internal `getAncestorDirectoryPaths` walks so the result can be + * fed back as `initialExpandedPaths` without surprises. */ +export function ancestorDirectoryPaths(path: string): string[] { + const normalized = path.endsWith("/") ? path.slice(0, -1) : path; + if (normalized.length === 0) return []; + const segments = normalized.split("/").filter(Boolean); const out: string[] = []; - for (let i = 1; i < parts.length; i++) { - out.push(`${parts.slice(0, i).join("/")}/`); + for (let i = 1; i < segments.length; i += 1) { + out.push(`${segments.slice(0, i).join("/")}/`); } return out; } @@ -119,7 +123,7 @@ export const FileTree: Component = (props) => { // ancestors expanded because of the snapshot here, NOT the // deferred effect (which skips its initial run). const selectedAncestors = props.selectedPath - ? ancestorsOf(props.selectedPath) + ? ancestorDirectoryPaths(props.selectedPath) : []; tree = new FileTreeClass({ paths: props.paths, @@ -169,7 +173,9 @@ export const FileTree: Component = (props) => { ], ([paths, expandPaths, selectedPath]) => { try { - const ancestors = selectedPath ? ancestorsOf(selectedPath) : []; + const ancestors = selectedPath + ? ancestorDirectoryPaths(selectedPath) + : []; const expanded = [...(expandPaths ?? []), ...ancestors]; tree?.resetPaths(paths, { initialExpandedPaths: expanded }); } catch (e) { diff --git a/packages/solid-pierre/src/index.ts b/packages/solid-pierre/src/index.ts index 9254b3539..7edb3f2c4 100644 --- a/packages/solid-pierre/src/index.ts +++ b/packages/solid-pierre/src/index.ts @@ -3,7 +3,7 @@ * and routes Pierre throws through a required `onError` prop so silent * failures can't escape into a blank pane. */ -export { FileTree } from "./FileTree"; +export { ancestorDirectoryPaths, FileTree } from "./FileTree"; export type { FileTreeProps } from "./FileTree"; export { FileDiff } from "./FileDiff"; export type { FileDiffProps } from "./FileDiff"; From c0d08b7271da2be8d7db446f1c74a59e4012c5b9 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:40:20 -0400 Subject: [PATCH 19/21] =?UTF-8?q?refactor(police):=20elegance=20=E2=80=94?= =?UTF-8?q?=20trim=20Terminal-archaeology=20from=20openInCodeTab=20jsdoc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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. --- packages/client/src/right-panel/openInCodeTab.ts | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/client/src/right-panel/openInCodeTab.ts b/packages/client/src/right-panel/openInCodeTab.ts index 3b6c0c77f..d9368898c 100644 --- a/packages/client/src/right-panel/openInCodeTab.ts +++ b/packages/client/src/right-panel/openInCodeTab.ts @@ -46,15 +46,11 @@ export const pendingOpen = pending; /** Open the right panel's Code tab at `req.targetMode` showing `req.ref`. * The two reactive writes (preferences patch + pending-request signal) - * are wrapped in `batch()` so SolidJS defers all dependent effects - * until both have committed. Without the batch, the preferences - * optimistic update ticks `view()` first, which fires `CodeTab`'s - * `resetKey` effect — at that moment `pendingOpen()` is still null - * (setPending hasn't run yet), the guard fails, and selectedPath gets - * cleared. The Terminal call site used to escape this by being inside - * a JSX click handler (Solid's event delegation auto-batches), but - * once the writes moved into a standalone function the implicit batch - * was lost. */ + * are wrapped in `batch()` so SolidJS defers dependent effects until + * both have committed. Without the batch, the preferences optimistic + * update ticks `view()` first, fires `CodeTab`'s `resetKey` effect + * when `pendingOpen()` is still null, the guard fails, and the + * selectedPath the user navigated to gets cleared. */ export function openInCodeTab(req: OpenInCodeTabRequest): void { batch(() => { useRightPanel().openCodeAt(req.targetMode); From 26ecf6743f02a2f079b187e35709c172d4b8c4d5 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:40:31 -0400 Subject: [PATCH 20/21] =?UTF-8?q?refactor(police):=20elegance=20=E2=80=94?= =?UTF-8?q?=20trim=20caller=20speculation=20from=20FileTree=20onMount=20co?= =?UTF-8?q?mment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/solid-pierre/src/FileTree.tsx | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/packages/solid-pierre/src/FileTree.tsx b/packages/solid-pierre/src/FileTree.tsx index 074e72bed..dce74dca3 100644 --- a/packages/solid-pierre/src/FileTree.tsx +++ b/packages/solid-pierre/src/FileTree.tsx @@ -109,19 +109,11 @@ export const FileTree: Component = (props) => { // Filed upstream: https://github.com/pierrecomputer/pierre/issues/676 onMount(() => { try { - // `props.selectedPath` is read once here as a snapshot, then - // again reactively in the deferred resetPaths effect below. The - // two sites encapsulate two distinct lifecycle moments — initial - // construction state vs. subsequent reactive state — and Pierre - // doesn't expose a hook to re-feed `initialExpandedPaths` after - // the constructor. Today's only call site (`CodeTab`) initializes - // `selectedPath` to `null` (CodeTab.tsx) and updates it via the - // pendingOpen effect AFTER the FileTree's paths stream settles, - // so this snapshot is always `null` at mount — the deferred - // effect handles the real cases. A future caller that passes a - // non-null `selectedPath` on first render would also see its - // ancestors expanded because of the snapshot here, NOT the - // deferred effect (which skips its initial run). + // Snapshot read of `props.selectedPath` for `initialExpandedPaths`. + // The deferred resetPaths effect below reads it reactively for + // subsequent changes — Pierre doesn't expose a hook to re-feed + // `initialExpandedPaths` after the constructor, so initial and + // reactive paths are unavoidably two sites. const selectedAncestors = props.selectedPath ? ancestorDirectoryPaths(props.selectedPath) : []; From 25382e058896a9f559931a22139af24410dea8d4 Mon Sep 17 00:00:00 2001 From: Sridhar Ratnakumar Date: Thu, 14 May 2026 14:40:39 -0400 Subject: [PATCH 21/21] =?UTF-8?q?refactor(police):=20elegance=20=E2=80=94?= =?UTF-8?q?=20trim=20narration=20from=20interactWithGutterLine=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../tests/step_definitions/code_tab_steps.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/tests/step_definitions/code_tab_steps.ts b/packages/tests/step_definitions/code_tab_steps.ts index 06995b438..9eb746e6c 100644 --- a/packages/tests/step_definitions/code_tab_steps.ts +++ b/packages/tests/step_definitions/code_tab_steps.ts @@ -114,12 +114,11 @@ When( // diff viewer (`DIFF_VIEW`) wrap the same Pierre primitive, so the // gutter selector and mouse dance are identical — only the host // element's CSS root changes. -// One shared interaction for any mouse button on a gutter line. The -// virtualizer-bounding-box race (Pierre's `VirtualizedFileDiff` is -// keyed on path; switching files makes the element pass -// `waitFor(visible)` and then return a null bounding box on the very -// next call as the virtualizer re-measures) is identical between -// left- and right-click — only the button differs. +// +// Poll the bounding box because Pierre's `VirtualizedFileDiff` is keyed +// on path; switching files makes the element pass `waitFor(visible)` +// and then return a null bounding box on the very next call as the +// virtualizer re-measures. async function interactWithGutterLine( world: KoluWorld, root: string, @@ -152,10 +151,9 @@ When( }, ); -// Right-click on a specific gutter line — selects the line and opens -// the context menu in one gesture (the host's `contextmenu` handler -// in `CodeMenuFrame` derives the line from the click target). Replaces -// the old two-step "click line then right-click viewer root" idiom. +// Right-click on a gutter line: `CodeMenuFrame`'s contextmenu handler +// reads the line from `event.composedPath()` and opens a 3-item menu +// scoped to that line. Selection + menu-open in one gesture. When( "I right-click line {int} in the diff view",