From f5311d19d8912da5b9bb513988b28cc119e4dadf Mon Sep 17 00:00:00 2001 From: cjimti Date: Thu, 6 Aug 2026 19:55:58 -0700 Subject: [PATCH] Put a blame column and a control row beside the editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The editor grows a toolbar row under the breadcrumb, and the first two things in it: Locate, which selects the open file in the tree, and Blame, which attributes every line to the commit that last touched it (#52, #56). The row's controls become the one control shape the file tree header also uses (#54), and the selection bug that made a right-click on a project tab highlight its name is fixed underneath them (#55). Four issues on one branch at the maintainer's direction rather than the template's one-issue-per-PR. They interleave in style.css, Icon.tsx, ViewToolbar.tsx and FileTree.tsx, so splitting them would produce commits that do not individually build. Blame reads `git blame --porcelain` and keeps the format's own shape: each commit stated once, and a line-by-line index into them. A 5,000-line file from one commit crosses the bridge as one commit record and 5,000 integers rather than 5,000 copies of an author and a timestamp. What blame measures is the file on disk, and its answer is a line number. One unsaved insertion moves every line below it, so the column clears while the buffer is dirty and comes back on save, with the lines just written now attributed to nobody — which is what they are. The toggle stays pressed across that and the column keeps its width, so turning blame on and typing does not shift the code sideways. Two things in the parser are defensive rather than reachable. An entry is placed at the line number git gave it rather than appended, because if the output ever skips a line one blank entry is a better failure than every attribution below it being off by one. And that line number is bounded by the output's own line count — the file cannot have more lines than the blame has records — so a bad digit cannot size an allocation. internal/git takes a ratchet raise, 900 -> 1100 LOC and 21 -> 25 exported, and the note it lands in argues against itself: the #9 paragraph named this exact case as one that should become its own package. It did not, because a sibling cannot reach runGit and depguard forbids the import — so the seam is the runner, not the parser. That is #53, which blocks #35, rather than something done here. The tree header and the editor toolbar had three looks between them: an icon beside a word with no hover state, a bare icon that turned accent- coloured, and a word with no icon. Control (components/Control.tsx) is the one shape for both, and the naming rule matters more than the look: `show dotfiles` renamed itself to `hide dotfiles`, which leaves a user unable to tell whether the words describe the state they are in or the one the click leads to, and a screen reader announcing a different control each time. It is now `Show hidden` with aria-pressed carrying the state, and the two act-once controls carry no aria-pressed at all rather than a permanent false. The tree header's toggles are icon-only because four labelled controls measure 274px and the sidebar's default is 260, its minimum 180. The row measures 180 exactly; a control added to it has to be compact or it stops fitting. user-select was in the stylesheet four times and unprefixed every time. Unprefixed user-select only reached WebKit in Safari 17, and this window is a WKWebView on macOS and a WebKitGTK one on Linux, so every one of those rules was inert in the app while testing clean in a Chromium browser — which is why right-clicking a tab selected its label. It is now one rule on .shell with the prefix beside it, the panes holding a document opt back in, and style.test.ts fails the build on an unpaired declaration. Locate is not reveal with a file path. reveal expands what it is given, so a file lands in the expanded set as a directory listing nobody will fetch, and the hook then asks the backend to list a file. It also runs its hidden check over the file's own chain rather than its parent's, which is the case a parent-only check gets wrong: .gitignore at the root has no hidden ancestor, so the filter would stay on and the locate would do nothing at all. The tree cannot tell a locate from an ordinary selection by `selected` alone, because locating the file already selected changes nothing about it. TreeState carries a locateRequest counter for it: a locate centres the row, a selection only brings it into view. Centring every selection would drag the tree out from under someone arrowing through it. Three layout defects here were found by measuring in a browser rather than by reading the CSS, after two rounds of shipping arithmetic that looked right: the blame column truncated its date because CodeMirror gutters inherit the UI font, so 13ch was thirteen proportional zeros rather than thirteen monospace characters; the width then still truncated because everything in a CodeMirror view is border-box, so it sized the padded box and left the text a character short; and the toolbar's pressed background bled 4px past the pane's left edge. Closes #52 Closes #54 Closes #55 Closes #56 --- DESIGN.md | 11 +- frontend/src/App.test.tsx | 228 ++++++++++++- frontend/src/App.tsx | 34 +- frontend/src/components/Control.test.tsx | 109 ++++++ frontend/src/components/Control.tsx | 87 +++++ frontend/src/components/EditorPane.test.tsx | 57 +++- frontend/src/components/EditorPane.tsx | 40 ++- frontend/src/components/FileTree.test.tsx | 117 ++++++- frontend/src/components/FileTree.tsx | 91 +++-- frontend/src/components/Icon.tsx | 6 + frontend/src/components/ViewToolbar.test.tsx | 175 ++++++++++ frontend/src/components/ViewToolbar.tsx | 82 +++++ frontend/src/lib/blame.test.ts | 166 ++++++++++ frontend/src/lib/blame.ts | 154 +++++++++ frontend/src/lib/codemirror.test.ts | 141 +++++++- frontend/src/lib/codemirror.ts | 143 +++++++- frontend/src/lib/editorTabs.test.ts | 37 +++ frontend/src/lib/editorTabs.ts | 24 ++ frontend/src/lib/git.ts | 40 +++ frontend/src/lib/gitStatus.test.ts | 43 +++ frontend/src/lib/gitStatus.ts | 19 ++ frontend/src/lib/tree.test.ts | 76 +++++ frontend/src/lib/tree.ts | 42 +++ frontend/src/lib/useBlame.test.ts | 199 +++++++++++ frontend/src/lib/useBlame.ts | 99 ++++++ frontend/src/lib/useEditorTabs.test.ts | 49 +++ frontend/src/lib/useEditorTabs.ts | 8 + frontend/src/lib/useFileTree.test.ts | 48 +++ frontend/src/lib/useFileTree.ts | 20 ++ frontend/src/lib/useGitOps.test.ts | 3 +- frontend/src/style.css | 146 ++++++-- frontend/src/style.test.ts | 18 + frontend/wailsjs/go/app/App.d.ts | 2 + frontend/wailsjs/go/app/App.js | 4 + frontend/wailsjs/go/models.ts | 53 +++ godobject_budget_test.go | 16 +- internal/app/git.go | 17 + internal/app/git_test.go | 34 ++ internal/git/blame.go | 286 ++++++++++++++++ internal/git/blame_test.go | 330 +++++++++++++++++++ package_budget_test.go | 36 +- 41 files changed, 3195 insertions(+), 95 deletions(-) create mode 100644 frontend/src/components/Control.test.tsx create mode 100644 frontend/src/components/Control.tsx create mode 100644 frontend/src/components/ViewToolbar.test.tsx create mode 100644 frontend/src/components/ViewToolbar.tsx create mode 100644 frontend/src/lib/blame.test.ts create mode 100644 frontend/src/lib/blame.ts create mode 100644 frontend/src/lib/useBlame.test.ts create mode 100644 frontend/src/lib/useBlame.ts create mode 100644 internal/git/blame.go create mode 100644 internal/git/blame_test.go diff --git a/DESIGN.md b/DESIGN.md index 8340d0e..babb7e6 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -205,7 +205,9 @@ Single window. Top-level tabs are projects; each project tab contains: diagnostics for Kubernetes kinds (bundled JSON schemas, validated in the Go backend on save/idle — kubeconform-style). Markdown files render to a preview with an edit toggle. This is deliberately "light editing": no refactoring, no - multi-file operations. + multi-file operations. Under the breadcrumb sits a view toolbar carrying the + active file's view toggles — the blame column, and the diff — each shown only + when the file is one it can answer for. - **Cluster panel**: live health (from the watch service + kstatus) for every object declared in the project, and a drift indicator when live objects differ from the checked-in manifests (server-side dry-run comparison, computed on @@ -268,6 +270,13 @@ v1 scope mirrors actual daily use, not a git client: - Status-driven change markers in the file tree: every changed path tinted and badged where it lives, and a tree-header toggle that filters the tree down to just those paths (deletions included, struck through, since they are in no directory listing). There is no separate changes list — a second list of the same paths cost a fixed share of the sidebar to say what the tree already knew. - Pull (rebase per repo config), push, current branch + ahead/behind in the status bar. - Diff viewer for working-tree changes and for a file's last commit. +- A blame column beside the editor, per line: the author's initials and the + date of the commit that last touched it, with the commit's subject and short + SHA on hover. It reads the file on disk, so its entries are hidden while the + buffer holds an unsaved edit — one insertion moves every line number under + it, and a shifted blame names the wrong person. This is not history tooling: + there is no navigating to a commit from it, which stays with log browsing in + v1.x. - Branch switching (existing branches). Branch creation, log browsing, stash, and history tooling are v1.x (§10). diff --git a/frontend/src/App.test.tsx b/frontend/src/App.test.tsx index bddec6d..17e889f 100644 --- a/frontend/src/App.test.tsx +++ b/frontend/src/App.test.tsx @@ -16,7 +16,7 @@ import type { Files } from "./lib/files"; import type { Project, Registry } from "./lib/projects"; import type { Endpoint } from "./lib/stream"; import type { Git, Status } from "./lib/git"; -import { MODIFIED, NOT_A_REPOSITORY, emptyStatus } from "./lib/git"; +import { MODIFIED, NOT_A_REPOSITORY, UNTRACKED, emptyBlame, emptyStatus } from "./lib/git"; /** * A full `Git` seam from the one or two calls a test actually cares about. @@ -28,6 +28,7 @@ import { MODIFIED, NOT_A_REPOSITORY, emptyStatus } from "./lib/git"; function stubGit(overrides: Partial = {}): Git { return { status: () => Promise.resolve(emptyStatus()), + blame: () => Promise.resolve(emptyBlame()), pull: () => Promise.resolve(), push: () => Promise.resolve(), checkout: () => Promise.resolve(), @@ -1065,12 +1066,12 @@ describe("the breadcrumb above the editor (#43)", () => { // changed under it has no row in that list to reveal. it("leaves changed-only mode to show a directory it was filtering out", async () => { const bar = await openTheFile(); - fireEvent.click(screen.getByRole("button", { name: "show changed files only" })); + fireEvent.click(screen.getByRole("button", { name: "Changed only" })); expect(screen.getByText("Nothing has changed in this project.")).toBeDefined(); fireEvent.click(within(bar).getByRole("button", { name: "manifests" })); - expect(screen.getByRole("button", { name: "show changed files only" })).toBeDefined(); + expect(screen.getByRole("button", { name: "Changed only" })).toBeDefined(); expect(screen.getByRole("treeitem", { name: /manifests/ })).toBeDefined(); }); @@ -1081,3 +1082,224 @@ describe("the breadcrumb above the editor (#43)", () => { expect(screen.queryByRole("navigation", { name: /^path of/ })).toBeNull(); }); }); + +describe("the blame column above the editor (#52)", () => { + const blame = { + commits: [ + { + sha: "a1b2c3d4e5f60718293a4b5c6d7e8f9012345678", + author: "Craig Johnston", + authorTime: Math.floor(new Date(2026, 7, 6, 9, 30).getTime() / 1000), + summary: "Add the ingress", + uncommitted: false, + }, + { + sha: "0000000000000000000000000000000000000000", + author: "Not Committed Yet", + authorTime: 0, + summary: "", + uncommitted: true, + }, + ], + lines: [0, 1], + }; + + function stubDirectory(): Directory { + return { + list: (_root, relPath) => + Promise.resolve(relPath === "" ? [{ name: "ingress.yaml", isDir: false }] : []), + create: () => Promise.resolve(), + rename: () => Promise.resolve(), + remove: () => Promise.resolve(), + prefixes: () => Promise.resolve({}), + }; + } + + function stubFiles(): Files { + return { + read: () => + Promise.resolve( + watch.FileContent.createFrom({ + content: "kind: Ingress\nname: web\n", + crlf: false, + mixedEol: false, + readOnly: false, + size: 24, + }), + ), + write: () => Promise.resolve(), + }; + } + + /** Opens the one file, with git answering `status` and `blame` as told. */ + async function openTheFile(git = stubGit({ blame: () => Promise.resolve(blame) })) { + render( + , + ); + fireEvent.click(await screen.findByRole("treeitem", { name: /ingress\.yaml/ })); + await screen.findByRole("navigation", { name: "path of ingress.yaml" }); + } + + /** The rendered entries of the blame column, in line order. */ + function entries(): string[] { + return [...document.querySelectorAll(".cm-blame-entry")].map((el) => el.textContent ?? ""); + } + + it("attributes each line once the column is turned on", async () => { + await openTheFile(); + + fireEvent.click(screen.getByRole("button", { name: "Blame" })); + + await waitFor(() => { + expect(entries()).toEqual(["CJ 2026-08-06", "uncommitted"]); + }); + }); + + it("does not read a blame for a file nobody asked about", async () => { + const git = stubGit({ blame: vi.fn(() => Promise.resolve(blame)) }); + await openTheFile(git); + + expect(git.blame).not.toHaveBeenCalled(); + }); + + it("takes the column away again", async () => { + await openTheFile(); + fireEvent.click(screen.getByRole("button", { name: "Blame" })); + await waitFor(() => { + expect(entries()).toHaveLength(2); + }); + + fireEvent.click(screen.getByRole("button", { name: "Blame" })); + + expect(document.querySelector(".cm-blame")).toBeNull(); + }); + + it("offers nothing to blame for an untracked file", async () => { + await openTheFile( + stubGit({ + status: () => + Promise.resolve({ + ...emptyStatus(), + files: [ + { + path: "ingress.yaml", + staged: "", + worktree: UNTRACKED, + conflicted: false, + origPath: "", + }, + ], + }), + }), + ); + + await waitFor(() => { + expect(screen.queryByRole("button", { name: "Blame" })).toBeNull(); + }); + }); + + it("reports git's refusal in git's own words", async () => { + await openTheFile( + stubGit({ + blame: () => Promise.reject(new Error("fatal: no such path in HEAD")), + }), + ); + + fireEvent.click(screen.getByRole("button", { name: "Blame" })); + + expect((await screen.findByRole("alert")).textContent).toContain("no such path in HEAD"); + }); +}); + +describe("locating the open file in the tree (#56)", () => { + const listings: Record = { + "": [{ name: "manifests", isDir: true }], + manifests: [{ name: "prod", isDir: true }], + "manifests/prod": [{ name: "ingress.yaml", isDir: false }], + }; + + function stubDirectory(): Directory { + return { + list: (_root, relPath) => Promise.resolve(listings[relPath] ?? []), + create: () => Promise.resolve(), + rename: () => Promise.resolve(), + remove: () => Promise.resolve(), + prefixes: () => Promise.resolve({}), + }; + } + + function stubFiles(): Files { + return { + read: () => + Promise.resolve( + watch.FileContent.createFrom({ + content: "kind: Ingress\n", + crlf: false, + mixedEol: false, + readOnly: false, + size: 14, + }), + ), + write: () => Promise.resolve(), + }; + } + + /** Opens the nested file, then collapses the chain so nothing shows it. */ + async function openThenHide() { + render( + , + ); + fireEvent.click(await screen.findByRole("treeitem", { name: /manifests/ })); + fireEvent.click(await screen.findByRole("treeitem", { name: /prod$/ })); + fireEvent.click(await screen.findByRole("treeitem", { name: /ingress\.yaml/ })); + await screen.findByRole("navigation", { name: "path of ingress.yaml" }); + + fireEvent.click(screen.getByRole("treeitem", { name: /manifests/ })); + expect(screen.queryByRole("treeitem", { name: /ingress\.yaml/ })).toBeNull(); + } + + it("brings the open file back on screen and selects it", async () => { + await openThenHide(); + + fireEvent.click(screen.getByRole("button", { name: "Locate" })); + + const row = await screen.findByRole("treeitem", { name: /ingress\.yaml/ }); + expect(row.getAttribute("aria-selected")).toBe("true"); + }); + + // Changed-only mode is a filter over the rows, and a file with nothing + // changed has no row in that list to select. + it("leaves changed-only mode, which would otherwise have no row to select", async () => { + await openThenHide(); + fireEvent.click(screen.getByRole("button", { name: "Changed only" })); + expect(screen.getByText("Nothing has changed in this project.")).toBeDefined(); + + fireEvent.click(screen.getByRole("button", { name: "Locate" })); + + expect(await screen.findByRole("treeitem", { name: /ingress\.yaml/ })).toBeDefined(); + }); + + it("says nothing when no file is open", async () => { + await renderWith(["infra"]); + + expect(screen.queryByRole("button", { name: "Locate" })).toBeNull(); + }); +}); diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 738e3c6..bed82bd 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -9,7 +9,7 @@ import type { Directory } from "./lib/directory"; import { wailsDirectory } from "./lib/directory"; import type { Files } from "./lib/files"; import { wailsFiles } from "./lib/files"; -import type { Git } from "./lib/git"; +import type { Git, Status } from "./lib/git"; import { wailsGit } from "./lib/git"; import type { Project, Registry } from "./lib/projects"; import { projectLabel, wailsRegistry } from "./lib/projects"; @@ -24,6 +24,7 @@ import { preferredAppearance, watchAppearance, } from "./lib/theme"; +import { NO_BLAME, useBlame } from "./lib/useBlame"; import { useEditorTabs } from "./lib/useEditorTabs"; import { useFileTree } from "./lib/useFileTree"; import { useGitOps } from "./lib/useGitOps"; @@ -32,6 +33,7 @@ import { useTerminals } from "./lib/useTerminals"; import { Breadcrumb } from "./components/Breadcrumb"; import { EditorPane } from "./components/EditorPane"; import { EditorTabs } from "./components/EditorTabs"; +import { ViewToolbar } from "./components/ViewToolbar"; const initialStatus: BuildStatus = { info: detachedBuild, attached: false }; @@ -208,6 +210,9 @@ export default function App({ @@ -292,6 +297,13 @@ function BuildLine({ build }: { readonly build: BuildStatus }) { interface EditorProps { readonly project: Project; readonly editors: ReturnType; + /** This project's git status (#8): what decides whether a file has a blame + * column to offer (#52). */ + readonly status: Status; + /** The git seam, for the blame the column shows (#52). */ + readonly git: Git; + /** Selects the open file in the tree — `FileTreeController.locate` (#56). */ + onLocate: (path: string) => void; readonly appearance: Appearance; /** What a breadcrumb segment opens in the tree (#43). */ onReveal: (dir: string) => void; @@ -305,10 +317,20 @@ interface EditorProps { * unmounted on a project switch would drop its CodeMirror view, and with it * the undo history behind whatever unsaved work the tab is holding. */ -function Editor({ project, editors, appearance, onReveal }: EditorProps) { +function Editor({ + project, + editors, + status, + git, + onLocate, + appearance, + onReveal, +}: EditorProps) { // The strip's own tabs, not every project's: the breadcrumb describes what // is on screen, and `activeKey` is per project. const active = editors.visible.find((tab) => tab.key === editors.activeKey) ?? null; + // One blame, for the file on screen. See useBlame for why it is not per tab. + const blame = useBlame(active, git); return ( <> @@ -325,6 +347,13 @@ function Editor({ project, editors, appearance, onReveal }: EditorProps) { + +
{editors.tabs.map((tab) => ( { void editors.save(key); diff --git a/frontend/src/components/Control.test.tsx b/frontend/src/components/Control.test.tsx new file mode 100644 index 0000000..bec9316 --- /dev/null +++ b/frontend/src/components/Control.test.tsx @@ -0,0 +1,109 @@ +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { Control } from "./Control"; + +afterEach(cleanup); + +const button = () => screen.getByRole("button"); + +describe("a control that acts", () => { + it("shows its icon and its label", () => { + render(); + + expect(button().textContent).toBe("File"); + expect(button().querySelector("[data-icon=plus]")).not.toBeNull(); + }); + + // The distinction the component exists for. A permanent aria-pressed="false" + // announces a toggle that happens to be off, which is a different control + // from a button that does something once. + it("reports no pressed state at all", () => { + render(); + + expect(button().hasAttribute("aria-pressed")).toBe(false); + expect(button().className).not.toContain("control--on"); + }); + + it("takes its accessible name from the text on screen", () => { + render(); + + // No aria-label to drift from the label beside it. + expect(button().hasAttribute("aria-label")).toBe(false); + expect(screen.getByRole("button", { name: "File" })).toBeDefined(); + }); + + it("calls back when pressed", () => { + const onClick = vi.fn(); + render(); + + fireEvent.click(button()); + + expect(onClick).toHaveBeenCalledTimes(1); + }); +}); + +describe("a control that toggles", () => { + it("reports off without looking pressed", () => { + render(); + + expect(button().getAttribute("aria-pressed")).toBe("false"); + expect(button().className).not.toContain("control--on"); + }); + + it("reports on and looks it", () => { + render(); + + expect(button().getAttribute("aria-pressed")).toBe("true"); + expect(button().className).toContain("control--on"); + }); + + // What the tree's dotfile toggle used to do, and must not: a control whose + // name changes with its state leaves a user unable to tell whether the words + // describe where they are or where the click leads (#54). + it("keeps one name in both states", () => { + const { rerender } = render( + , + ); + const off = button().getAttribute("aria-label") ?? button().textContent; + + rerender(); + + expect(button().getAttribute("aria-label") ?? button().textContent).toBe(off); + }); +}); + +describe("a compact control", () => { + it("drops the visible label and keeps the name", () => { + render(); + + expect(button().textContent).toBe(""); + expect(screen.getByRole("button", { name: "Show hidden" })).toBeDefined(); + }); + + it("is the same control to a screen reader as the labelled one", () => { + render(); + const compact = button().getAttribute("aria-label"); + + cleanup(); + render(); + + expect(compact).toBe(button().textContent); + }); +}); + +describe("a control whose label abbreviates its name", () => { + it("is addressed by the fuller name", () => { + render(); + + expect(screen.getByRole("button", { name: "New file" })).toBeDefined(); + }); + + // WCAG 2.5.3: a user who says what they can read has to reach the control. + // "File" is inside "New file"; a name that dropped it would not be. + it("keeps the visible label inside the spoken name", () => { + render(); + + const spoken = button().getAttribute("aria-label") ?? ""; + expect(spoken.toLowerCase()).toContain("folder"); + }); +}); diff --git a/frontend/src/components/Control.tsx b/frontend/src/components/Control.tsx new file mode 100644 index 0000000..bfcfb8f --- /dev/null +++ b/frontend/src/components/Control.tsx @@ -0,0 +1,87 @@ +import type { UiIconName } from "./Icon"; +import { UiIcon } from "./Icon"; + +export interface ControlProps { + readonly icon: UiIconName; + /** The control's name. It does not change with the control's state — see + * below. */ + readonly label: string; + /** + * Whether a toggle is on, or absent for a control that acts rather than + * toggles. + * + * Absent is not `false`. A button that performs an action has no pressed + * state, and reporting a permanent `aria-pressed="false"` tells a screen + * reader it is a toggle that happens to be off — which is a different + * control from the one that is there. + */ + readonly pressed?: boolean; + /** The hover tooltip. The label says what the control is; this is where the + * sentence explaining it goes. */ + readonly title?: string; + /** + * Hides the label visually, for a row too narrow to carry it. + * + * The tree header is the row that needs it: four labelled controls want + * 274px and the sidebar's default is 260 (`SIDEBAR_MIN` is 180). The + * accessible name is unaffected — a compact control is the same control, + * and the only thing it drops is the part a sighted user can infer from the + * icon. + */ + readonly compact?: boolean; + /** + * The accessible name, when the visible label is an abbreviation of it — + * `File` for a control that makes a new one. + * + * It must contain the visible label, or a user speaking what they can see + * cannot address it (WCAG 2.5.3). Like the label, it does not change with + * the control's state. + */ + readonly name?: string; + onClick: () => void; +} + +/** + * One piece of chrome the user can press: the editor toolbar's toggles (#52) + * and the file tree's header controls (#54). + * + * It exists because those two rows had three looks between them — an icon + * beside a word with no hover state, a bare icon that turned accent-coloured, + * and a word with no icon at all — and a shared stylesheet class alone would + * not have fixed the part that actually misleads: what the control is called. + * + * A control's name is fixed. The tree's dotfile toggle used to rename itself + * from `show dotfiles` to `hide dotfiles`, which leaves a user unable to tell + * whether the words describe the state they are in or the one the click leads + * to — and leaves a screen reader announcing a different control each time it + * is pressed. State is `aria-pressed` and a filled box; the name stays put. + */ +export function Control({ + icon, + label, + pressed, + title, + compact = false, + name, + onClick, +}: ControlProps) { + // Left off entirely when the visible label is already the whole name, so the + // accessible name comes from the text on screen rather than from a copy of + // it that could drift. + const spoken = compact || name !== undefined ? (name ?? label) : undefined; + + return ( + + ); +} diff --git a/frontend/src/components/EditorPane.test.tsx b/frontend/src/components/EditorPane.test.tsx index 2a4afdb..6906437 100644 --- a/frontend/src/components/EditorPane.test.tsx +++ b/frontend/src/components/EditorPane.test.tsx @@ -4,6 +4,8 @@ import type { MountedEditor } from "../lib/codemirror"; import type { EditorTab, EditorTabKind } from "../lib/editorTabs"; import { newTab, withExternalChange, withLoaded } from "../lib/editorTabs"; import type { FileContent } from "../lib/files"; +import type { Blame } from "../lib/git"; +import { NO_BLAME } from "../lib/useBlame"; import { EditorPane } from "./EditorPane"; afterEach(cleanup); @@ -26,11 +28,21 @@ function fakeEditor(): MountedEditor { setContent: vi.fn(), setTheme: vi.fn(), setReadOnly: vi.fn(), + setBlame: vi.fn(), focus: vi.fn(), dispose: vi.fn(), }; } +/** A one-line blame, for the pane's own wiring — what it says is + * `blame.test.ts`'s subject, not this file's. */ +const someBlame: Blame = { + commits: [ + { sha: "a1b2c3d", author: "Craig Johnston", authorTime: 1, summary: "x", uncommitted: false }, + ], + lines: [0], +}; + function renderPane(tab: EditorTab, over: Partial[0]> = {}) { const editor = fakeEditor(); const mount = vi.fn().mockReturnValue(editor); @@ -38,6 +50,7 @@ function renderPane(tab: EditorTab, over: Partial[ tab, active: true, appearance: "dark" as const, + blame: NO_BLAME, onChange: vi.fn(), onSave: vi.fn(), onKeepMine: vi.fn(), @@ -45,11 +58,12 @@ function renderPane(tab: EditorTab, over: Partial[ mount, ...over, }; - const { rerender } = render(); + const { rerender, container } = render(); return { props, editor, mount, + container, rerender: (next: Partial[0]>) => { rerender(); }, @@ -115,6 +129,47 @@ describe("mounting an editor", () => { expect(editor.setReadOnly).toHaveBeenCalledWith(true); }); + it("pushes the blame column into the existing view rather than rebuilding it", () => { + const tab = ready(); + const { mount, editor, rerender } = renderPane(tab); + + rerender({ tab: { ...tab, blame: true }, blame: { blame: someBlame, error: null } }); + + expect(mount).toHaveBeenCalledTimes(1); + expect(editor.setBlame).toHaveBeenLastCalledWith(true, someBlame); + }); + + // The dirty case (#52): the toggle stays on and the entries go, because the + // line numbers they are stated in are no longer the buffer's. + it("keeps the column shown with no entries when the blame goes stale", () => { + const tab = { ...ready(), blame: true }; + const { editor, rerender } = renderPane(tab, { + blame: { blame: someBlame, error: null }, + }); + + rerender({ blame: NO_BLAME }); + + expect(editor.setBlame).toHaveBeenLastCalledWith(true, null); + }); + + it("shows git's own words when a blame fails", () => { + renderPane(ready(), { + blame: { blame: null, error: "fatal: no such path 'x.yaml' in HEAD" }, + }); + + expect(screen.getByRole("alert").textContent).toContain("no such path"); + }); + + // The blame failed, not the file. Replacing the buffer with a message would + // cost the user the editor over a column they can turn off. + it("keeps the file on screen when its blame fails", () => { + const { container } = renderPane(ready("a: 1\n"), { + blame: { blame: null, error: "fatal: nope" }, + }); + + expect(container.querySelector("[data-testid=codemirror-host]")).not.toBeNull(); + }); + // A stale handler would report a later tab's keystrokes against the key the // view was mounted with. it("reports edits against the current tab key after a re-render", () => { diff --git a/frontend/src/components/EditorPane.tsx b/frontend/src/components/EditorPane.tsx index ad32c58..111422e 100644 --- a/frontend/src/components/EditorPane.tsx +++ b/frontend/src/components/EditorPane.tsx @@ -3,6 +3,7 @@ import type { MountedEditor } from "../lib/codemirror"; import { mountEditor } from "../lib/codemirror"; import type { EditorTab } from "../lib/editorTabs"; import { readOnlyNotice } from "../lib/editorTabs"; +import type { BlameState } from "../lib/useBlame"; import type { Appearance } from "../lib/theme"; import { MarkdownPreview } from "./MarkdownPreview"; @@ -10,6 +11,9 @@ export interface EditorPaneProps { readonly tab: EditorTab; readonly active: boolean; readonly appearance: Appearance; + /** This pane's blame column (#52). Only the active tab's is ever read, so + * every other pane is handed the empty state. */ + readonly blame: BlameState; onChange: (key: string, content: string) => void; onSave: (key: string) => void; onKeepMine: (key: string) => void; @@ -31,6 +35,7 @@ export function EditorPane({ tab, active, appearance, + blame, onChange, onSave, onKeepMine, @@ -57,11 +62,12 @@ export function EditorPane({

)} - {tab.error !== null && ( -

- {tab.error} -

- )} + + + {/* git's own words, not a translation of them (DESIGN.md §7). It is a + separate line from the tab's error because they are separate + failures: this one costs the column, not the file. */} + {tab.status === "loading" &&

Opening {tab.title}…

} @@ -72,6 +78,7 @@ export function EditorPane({ + {message} +

+ ); +} + interface ConflictBarProps { readonly tab: EditorTab; onKeepMine: (key: string) => void; @@ -126,6 +147,7 @@ function ConflictBar({ tab, onKeepMine, onTakeDisk }: ConflictBarProps) { interface CodeMirrorHostProps { readonly tab: EditorTab; readonly appearance: Appearance; + readonly blame: BlameState; onChange: (key: string, content: string) => void; onSave: (key: string) => void; readonly mount: typeof mountEditor; @@ -142,6 +164,7 @@ interface CodeMirrorHostProps { function CodeMirrorHost({ tab, appearance, + blame, onChange, onSave, mount, @@ -201,5 +224,12 @@ function CodeMirrorHost({ editor.current?.setReadOnly(tab.readOnly); }, [tab.readOnly]); + // Two arguments, because the column being on and the column having entries + // are two states: an unsaved edit invalidates the line numbers a blame is + // stated in, so the entries go and the column stays (#52). + useEffect(() => { + editor.current?.setBlame(tab.blame, blame.blame); + }, [tab.blame, blame.blame]); + return
; } diff --git a/frontend/src/components/FileTree.test.tsx b/frontend/src/components/FileTree.test.tsx index 8b9b046..22522da 100644 --- a/frontend/src/components/FileTree.test.tsx +++ b/frontend/src/components/FileTree.test.tsx @@ -92,7 +92,7 @@ function toneOf(row: HTMLElement): string | null { } function showChangedOnly(): void { - fireEvent.click(screen.getByRole("button", { name: "show changed files only" })); + fireEvent.click(screen.getByRole("button", { name: "Changed only" })); } function loadedRoot(): TreeState { @@ -123,6 +123,7 @@ function fakeController( collapse: vi.fn(), select: vi.fn(), reveal: vi.fn(), + locate: vi.fn(), toggleHidden: vi.fn(), toggleChangedOnly: vi.fn(), createEntry: vi.fn().mockResolvedValue(null), @@ -271,22 +272,114 @@ describe("keyboard navigation", () => { }); }); +describe("scrolling the selected row (#56)", () => { + /** + * The tree without the changed-only Harness above. + * + * These re-render with a new state, and the Harness pins its own copy at + * mount — so through it the component would never see the second state and + * every assertion here would pass over a tree that had not moved. + */ + function renderPlain(state: TreeState) { + const { rerender } = render( + , + ); + return (next: TreeState) => { + rerender( + , + ); + }; + } + + const at = (path: string | null, locateRequest: number): TreeState => ({ + ...loadedManifests(loadedRoot()), + selected: path, + locateRequest, + }); + + /** The alignment each scrollIntoView call asked for, in order. */ + const blocks = (spy: ReturnType) => + spy.mock.calls.map(([options]) => (options as ScrollIntoViewOptions).block); + + it("puts a located row in the middle of the pane", () => { + const spy = vi.spyOn(Element.prototype, "scrollIntoView"); + const rerender = renderPlain(at(null, 0)); + spy.mockClear(); + + rerender(at("manifests/prod.yaml", 1)); + + expect(blocks(spy)).toContain("center"); + }); + + // Centring every selection would drag the tree out from under someone + // arrowing through it. + it("moves an ordinarily selected row only as far as it must", () => { + const spy = vi.spyOn(Element.prototype, "scrollIntoView"); + const rerender = renderPlain(at("manifests", 0)); + spy.mockClear(); + + rerender(at("manifests/prod.yaml", 0)); + + expect(blocks(spy)).toContain("nearest"); + expect(blocks(spy)).not.toContain("center"); + }); + + // Pressing Locate on the file already selected must still centre it: the + // selection does not change, so only the request tells the two apart. + it("centres again when the same file is located twice", () => { + const spy = vi.spyOn(Element.prototype, "scrollIntoView"); + const rerender = renderPlain(at("manifests/prod.yaml", 1)); + spy.mockClear(); + + rerender(at("manifests/prod.yaml", 2)); + + expect(blocks(spy)).toContain("center"); + }); + + // The row a locate asks for often does not exist yet: its directory is + // still being listed. The centring has to survive until it does, or the + // file arrives on screen pinned to whichever edge it came in on. + it("centres the row when it arrives after the locate", () => { + const spy = vi.spyOn(Element.prototype, "scrollIntoView"); + const rerender = renderPlain(at(null, 0)); + spy.mockClear(); + + // The locate lands first: the file is selected, but the directory holding + // it has not been listed, so it has no row to scroll to. + rerender({ ...loadedRoot(), selected: "manifests/prod.yaml", locateRequest: 1 }); + expect(blocks(spy)).toEqual([]); + + // The listing arrives. + rerender(at("manifests/prod.yaml", 1)); + + expect(blocks(spy)).toContain("center"); + }); +}); + describe("the hidden-files toggle", () => { - it("calls toggleHidden and reflects the current state", () => { + const toggle = () => screen.getByRole("button", { name: "Show hidden" }); + + it("calls toggleHidden", () => { const tree = fakeController(loadedRoot()); renderTree({ tree }); - const toggle = screen.getByRole("button", { name: "show dotfiles" }); - fireEvent.click(toggle); + fireEvent.click(toggle()); expect(tree.toggleHidden).toHaveBeenCalled(); }); - it("labels the button by whether hidden files are already shown", () => { - const shown = { ...loadedRoot(), showHidden: true }; - renderTree({ tree: fakeController(shown) }); + // It used to rename itself — `show dotfiles`, then `hide dotfiles` — which + // leaves a user unable to tell whether the words describe the state they are + // in or the one the click leads to, and a screen reader announcing a + // different control each time it is pressed (#54). + it("keeps its name in both states and reports the state as pressed", () => { + renderTree({ tree: fakeController(loadedRoot()) }); + expect(toggle().getAttribute("aria-pressed")).toBe("false"); + + cleanup(); + renderTree({ tree: fakeController({ ...loadedRoot(), showHidden: true }) }); - expect(screen.getByRole("button", { name: "hide dotfiles" })).toBeDefined(); + expect(toggle().getAttribute("aria-pressed")).toBe("true"); }); }); @@ -295,7 +388,7 @@ describe("creating an entry", () => { const tree = fakeController(withListing(initialTree(), ROOT, [])); renderTree({ tree }); - fireEvent.click(screen.getByRole("button", { name: "new file" })); + fireEvent.click(screen.getByRole("button", { name: "New file" })); const field = screen.getByRole("textbox", { name: "new file name" }); fireEvent.change(field, { target: { value: "values.yaml" } }); fireEvent.keyDown(field, { key: "Enter" }); @@ -309,7 +402,7 @@ describe("creating an entry", () => { }); renderTree({ tree }); - fireEvent.click(screen.getByRole("button", { name: "new file" })); + fireEvent.click(screen.getByRole("button", { name: "New file" })); const field = screen.getByRole("textbox", { name: "new file name" }); fireEvent.change(field, { target: { value: "deploy.yaml" } }); fireEvent.keyDown(field, { key: "Enter" }); @@ -322,7 +415,7 @@ describe("creating an entry", () => { const tree = fakeController(withListing(initialTree(), ROOT, [])); renderTree({ tree }); - fireEvent.click(screen.getByRole("button", { name: "new folder" })); + fireEvent.click(screen.getByRole("button", { name: "New folder" })); const field = screen.getByRole("textbox", { name: "new folder name" }); fireEvent.keyDown(field, { key: "Escape" }); @@ -647,7 +740,7 @@ describe("the changed-only mode (#40)", () => { renderTree({ tree: fakeController(loadedRoot()), status: deep }); showChangedOnly(); - fireEvent.click(screen.getByRole("button", { name: "show all files" })); + fireEvent.click(screen.getByRole("button", { name: "Changed only" })); expect(screen.getAllByRole("treeitem").map((el) => el.textContent)).toEqual([ "manifests•", diff --git a/frontend/src/components/FileTree.tsx b/frontend/src/components/FileTree.tsx index 67eed70..6f40c9a 100644 --- a/frontend/src/components/FileTree.tsx +++ b/frontend/src/components/FileTree.tsx @@ -5,7 +5,7 @@ import { ROOT, parentPath, visibleRows } from "../lib/tree"; import type { Status } from "../lib/git"; import type { Badges } from "../lib/gitStatus"; import { badgeAt, badgeTone, badgesFor, changedRows } from "../lib/gitStatus"; -import { UiIcon } from "./Icon"; +import { Control } from "./Control"; import { RowView } from "./FileTreeRow"; import { CreateRow } from "./InlineField"; @@ -85,11 +85,29 @@ export function FileTree({ tree, status, onOpenFile }: FileTreeProps) { // lands, which is what makes a row that did not exist yet still arrive on // screen. const selectedIndex = rows.findIndex((row) => row.path === tree.state.selected); + const { locateRequest } = tree.state; + // The last locate this pane has already scrolled for. A ref rather than + // state: acting on it must not cause the render that would act on it again. + const centredFor = useRef(locateRequest); useEffect(() => { - if (selectedIndex >= 0) { - rowRefs.current[selectedIndex]?.scrollIntoView({ block: "nearest" }); + if (selectedIndex < 0) { + return; } - }, [selectedIndex]); + // A locate puts the row in the middle of the pane; an ordinary selection + // moves it just far enough to be visible. Centring every selection would + // drag the tree out from under someone arrowing through it, and scrolling + // a locate only into view leaves the file the user asked for pinned to + // whichever edge it came in on, with no context above or below it. + // + // `center` is a request, not a guarantee: a row near either end of the + // list stops at the scroll extreme, which is as close to the middle as it + // can be. + const centring = locateRequest !== centredFor.current; + centredFor.current = locateRequest; + rowRefs.current[selectedIndex]?.scrollIntoView({ + block: centring ? "center" : "nearest", + }); + }, [selectedIndex, locateRequest]); // Roving tabindex: only move DOM focus when the tree itself already has // it, so a background listing refresh (an /events update) never steals @@ -157,38 +175,55 @@ export function FileTree({ tree, status, onOpenFile }: FileTreeProps) { return (
+ {/* One control shape for the whole row (#54), and one name per control + whatever state it is in — see components/Control.tsx. + + The toggles are compact and the actions are not, which is a width + decision rather than a taste one: four labelled controls measure + 274px and the sidebar's default is 260, its minimum 180. This row + measures 180 exactly, so it survives a drag to the minimum — and any + control added to it has to be compact or the row stops fitting. */}
- - - - + />
>; diff --git a/frontend/src/components/ViewToolbar.test.tsx b/frontend/src/components/ViewToolbar.test.tsx new file mode 100644 index 0000000..c15d835 --- /dev/null +++ b/frontend/src/components/ViewToolbar.test.tsx @@ -0,0 +1,175 @@ +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { EditorTab, EditorTabKind } from "../lib/editorTabs"; +import { newTab, withBlame, withLoaded, withMode } from "../lib/editorTabs"; +import type { FileStatus, Status } from "../lib/git"; +import { NOT_A_REPOSITORY, NO_GIT, UNTRACKED, emptyStatus } from "../lib/git"; +import { ViewToolbar } from "./ViewToolbar"; + +afterEach(cleanup); + +function tab(path = "deploy.yaml", kind: EditorTabKind = "yaml"): EditorTab { + return withLoaded(newTab("k0", "infra", "/w/infra", path, kind), { + content: "a: 1\n", + crlf: false, + mixedEol: false, + readOnly: false, + size: 5, + }); +} + +function statusOf(files: FileStatus[] = []): Status { + return { ...emptyStatus(), files }; +} + +function untracked(path: string): FileStatus { + return { path, staged: "", worktree: UNTRACKED, conflicted: false, origPath: "" }; +} + +function renderBar(open: EditorTab | null, status: Status = statusOf()) { + const onToggleBlame = vi.fn(); + const onLocate = vi.fn(); + render( + , + ); + return { onToggleBlame, onLocate }; +} + +const blameButton = () => screen.queryByRole("button", { name: "Blame" }); +const locateButton = () => screen.queryByRole("button", { name: "Locate" }); + +describe("the locate control (#56)", () => { + it("is offered for any open file", () => { + renderBar(tab()); + + expect(locateButton()).not.toBeNull(); + }); + + // It is not a git control, so nothing about the repository takes it away. + it("is offered for a file git is not tracking", () => { + renderBar(tab(), statusOf([untracked("deploy.yaml")])); + + expect(locateButton()).not.toBeNull(); + expect(blameButton()).toBeNull(); + }); + + it("is offered when there is no git at all", () => { + renderBar(tab(), { ...emptyStatus(), availability: NO_GIT }); + + expect(locateButton()).not.toBeNull(); + }); + + it("is offered for a markdown file in preview, which has no gutter", () => { + renderBar(tab("README.md", "markdown")); + + expect(locateButton()).not.toBeNull(); + expect(blameButton()).toBeNull(); + }); + + it("asks for the open file by its own path", () => { + const { onLocate } = renderBar(tab("deploy/base/svc.yaml")); + + fireEvent.click(locateButton() as HTMLElement); + + expect(onLocate).toHaveBeenCalledWith("deploy/base/svc.yaml"); + }); + + it("acts rather than toggles, so it reports no pressed state", () => { + renderBar(tab()); + + expect(locateButton()?.hasAttribute("aria-pressed")).toBe(false); + }); +}); + +describe("when the toolbar appears", () => { + it("offers blame for a file git is tracking", () => { + renderBar(tab()); + + expect(blameButton()).not.toBeNull(); + }); + + // A tracked file with no local changes is absent from the status entirely — + // git emits a record only for a path that differs — so "not mentioned" has + // to mean tracked. + it("offers blame for a tracked file that has not been touched", () => { + renderBar(tab(), statusOf([untracked("other.yaml")])); + + expect(blameButton()).not.toBeNull(); + }); + + it("drops blame for an untracked file, keeping the row", () => { + renderBar(tab(), statusOf([untracked("deploy.yaml")])); + + expect(blameButton()).toBeNull(); + expect(screen.getByRole("toolbar")).toBeDefined(); + }); + + it("drops blame when git is not installed", () => { + renderBar(tab(), { ...emptyStatus(), availability: NO_GIT }); + + expect(blameButton()).toBeNull(); + }); + + it("drops blame when the project is not a repository", () => { + renderBar(tab(), { ...emptyStatus(), availability: NOT_A_REPOSITORY }); + + expect(blameButton()).toBeNull(); + }); + + // The row goes when it would hold nothing, which is when no file is open — + // not when one particular control does not apply. + it("is absent when no file is open", () => { + renderBar(null); + + expect(screen.queryByRole("toolbar")).toBeNull(); + }); + + // The column is a CodeMirror gutter, and a rendered markdown preview has no + // gutter to put it in. A toggle there would visibly do nothing. + it("drops blame for a markdown file showing its preview", () => { + renderBar(tab("README.md", "markdown")); + + expect(blameButton()).toBeNull(); + }); + + it("offers blame for a markdown file being edited", () => { + renderBar(withMode(tab("README.md", "markdown"), "edit")); + + expect(blameButton()).not.toBeNull(); + }); +}); + +describe("the blame toggle", () => { + it("reports whether the column is on", () => { + renderBar(tab()); + + expect(blameButton()?.getAttribute("aria-pressed")).toBe("false"); + }); + + it("reports a column that is on", () => { + renderBar(withBlame(tab(), true)); + + expect(blameButton()?.getAttribute("aria-pressed")).toBe("true"); + }); + + it("asks for the column when it is off", () => { + const { onToggleBlame } = renderBar(tab()); + + fireEvent.click(blameButton() as HTMLElement); + + expect(onToggleBlame).toHaveBeenCalledWith("k0", true); + }); + + it("asks to take the column away when it is on", () => { + const { onToggleBlame } = renderBar(withBlame(tab(), true)); + + fireEvent.click(blameButton() as HTMLElement); + + expect(onToggleBlame).toHaveBeenCalledWith("k0", false); + }); +}); diff --git a/frontend/src/components/ViewToolbar.tsx b/frontend/src/components/ViewToolbar.tsx new file mode 100644 index 0000000..6694110 --- /dev/null +++ b/frontend/src/components/ViewToolbar.tsx @@ -0,0 +1,82 @@ +import type { EditorTab } from "../lib/editorTabs"; +import type { Status } from "../lib/git"; +import { isTracked } from "../lib/gitStatus"; +import { Control } from "./Control"; + +export interface ViewToolbarProps { + /** The file the editor is showing, or null when nothing is open. */ + readonly tab: EditorTab | null; + /** This project's git status (#8) — what says whether the file has history + * to show. */ + readonly status: Status; + /** Selects this file in the tree, expanding whatever hides it (#56). */ + onLocate: (path: string) => void; + onToggleBlame: (key: string, blame: boolean) => void; +} + +/** + * The active file's toolbar, under the breadcrumb (#52). + * + * It is a row of its own rather than more buttons on the tab. A control here + * belongs to the file on screen, not to the strip: putting it on the tab would + * repeat it once per open file and grow every tab by the number of controls + * the editor grows. + * + * Blame is the first thing in it, not the point of it. The row is where a + * per-file control goes — #35's diff toggle next, and whatever else the editor + * grows that acts on the open file rather than on the project — so a control + * added here should take its own visibility rule the way `blameable` does + * rather than assume the row is about git. + * + * The row is absent, not empty, when it would hold nothing. An untracked file + * has no blame to show, and a bar of disabled controls says less about why + * than no bar at all does. + */ +export function ViewToolbar({ tab, status, onLocate, onToggleBlame }: ViewToolbarProps) { + // The row exists when any control in it does, not when a particular one + // does. Locate applies to every open file, so today that means "whenever a + // file is open" — but the test is written the way it is because the next + // control to land here (#35's diff toggle) will not apply to every file + // either, and the row must not turn on the wrong condition. + if (tab === null) { + return null; + } + + return ( +
+ { + onLocate(tab.path); + }} + /> + {blameable(tab, status) && ( + { + onToggleBlame(tab.key, !tab.blame); + }} + /> + )} +
+ ); +} + +/** + * Whether this tab can show a blame column. + * + * Tracked is the git half. The other half is that the column is a CodeMirror + * gutter: a markdown tab showing its rendered preview has no gutter to put it + * in, so the toggle would be a button that visibly does nothing. + */ +function blameable(tab: EditorTab, status: Status): boolean { + if (tab.kind === "markdown" && tab.mode === "preview") { + return false; + } + return isTracked(status, tab.path); +} diff --git a/frontend/src/lib/blame.test.ts b/frontend/src/lib/blame.test.ts new file mode 100644 index 0000000..eaf865f --- /dev/null +++ b/frontend/src/lib/blame.test.ts @@ -0,0 +1,166 @@ +import { describe, expect, it } from "vitest"; +import { + BLAME_LABEL_WIDTH, + UNCOMMITTED_LABEL, + blameLabel, + blameTooltip, + commitAt, + formatDay, + formatMoment, + initials, +} from "./blame"; +import type { Blame, BlameCommit } from "./git"; + +const commit = (over: Partial = {}): BlameCommit => ({ + sha: "a1b2c3d4e5f60718293a4b5c6d7e8f9012345678", + author: "Craig Johnston", + // 2026-08-06 09:30 local, whatever zone the test runs in. + authorTime: Math.floor(new Date(2026, 7, 6, 9, 30).getTime() / 1000), + summary: "Give editor tabs a context menu", + uncommitted: false, + ...over, +}); + +const blame = (commits: BlameCommit[], lines: number[]): Blame => ({ commits, lines }); + +/** A string's length in glyphs, which is what a monospace column is measured + * in — `.length` counts a surrogate pair twice. */ +const glyphs = (text: string): number => [...text].length; + +describe("commitAt", () => { + const two = blame([commit({ author: "First" }), commit({ author: "Second" })], [0, 1, 0]); + + it("resolves a line to the commit its index names", () => { + expect(commitAt(two, 1)?.author).toBe("First"); + expect(commitAt(two, 2)?.author).toBe("Second"); + expect(commitAt(two, 3)?.author).toBe("First"); + }); + + it("has nothing for a line past the end of the blame", () => { + // The buffer growing under a blame read before the edit — the case that + // makes this a guard rather than a formality. + expect(commitAt(two, 4)).toBeNull(); + }); + + it("has nothing for a line before the first", () => { + expect(commitAt(two, 0)).toBeNull(); + }); + + it("has nothing for a line the blame did not cover", () => { + expect(commitAt(blame([commit()], [-1]), 1)).toBeNull(); + }); + + it("has nothing for an index that names no commit", () => { + expect(commitAt(blame([commit()], [7]), 1)).toBeNull(); + }); +}); + +describe("initials", () => { + it("takes the first and last of a full name", () => { + expect(initials("Craig Johnston")).toBe("CJ"); + expect(initials("Ada Byron King")).toBe("AK"); + }); + + it("takes two letters from a single-word name", () => { + expect(initials("cjimti")).toBe("CJ"); + }); + + it("takes what there is of a one-letter name", () => { + expect(initials("x")).toBe("X"); + }); + + it("answers something for a name git reported as empty", () => { + expect(initials("")).toBe("??"); + expect(initials(" ")).toBe("??"); + }); + + it("splits on any run of whitespace", () => { + expect(initials(" Craig Johnston ")).toBe("CJ"); + }); + + it("takes whole code points, not halves of them", () => { + // A name whose first character is outside the BMP. charAt would return a + // lone surrogate here, which renders as a replacement glyph. + expect(initials("𝒜da Lovelace")).toBe("𝒜L"); + }); +}); + +describe("blameLabel", () => { + it("is the author's initials and the date they wrote it", () => { + expect(blameLabel(commit())).toBe("CJ 2026-08-06"); + }); + + it("names nobody for a line that is in no commit", () => { + expect(blameLabel(commit({ uncommitted: true, author: "Not Committed Yet" }))).toBe( + UNCOMMITTED_LABEL, + ); + }); + + // The column is sized from BLAME_LABEL_WIDTH, in a monospace gutter, so the + // label has to actually be that many characters — a longer one is truncated + // to an ellipsis rather than wrapped, which is how the date loses its day. + // + // Counted in code points rather than in `.length`: an astral initial is one + // glyph in the column and two UTF-16 units in the string, and the column is + // measured in glyphs. + it("is exactly the width the column is sized for", () => { + for (const author of ["Craig Johnston", "Ada Byron King", "cjimti", ""]) { + expect(glyphs(blameLabel(commit({ author })))).toBe(BLAME_LABEL_WIDTH); + } + }); + + it("never exceeds that width, whatever git reported", () => { + for (const author of ["Wolfgang Amadeus Mozart", "𝒜da Lovelace", "x", " "]) { + expect(glyphs(blameLabel(commit({ author })))).toBeLessThanOrEqual(BLAME_LABEL_WIDTH); + } + expect(glyphs(UNCOMMITTED_LABEL)).toBeLessThanOrEqual(BLAME_LABEL_WIDTH); + }); + + it("drops the date when git reported no readable time", () => { + expect(blameLabel(commit({ authorTime: 0 }))).toBe("CJ"); + }); +}); + +describe("blameTooltip", () => { + it("carries the name, the moment, the short sha and the subject", () => { + expect(blameTooltip(commit())).toBe( + "Craig Johnston · 2026-08-06 09:30 · a1b2c3d · Give editor tabs a context menu", + ); + }); + + it("omits a subject git did not report rather than leaving a gap", () => { + expect(blameTooltip(commit({ summary: "" }))).toBe( + "Craig Johnston · 2026-08-06 09:30 · a1b2c3d", + ); + }); + + it("says what an uncommitted line is instead of naming a commit", () => { + const tip = blameTooltip(commit({ uncommitted: true })); + + expect(tip).toContain("Not committed yet"); + expect(tip).not.toContain("a1b2c3d"); + }); +}); + +describe("formatDay", () => { + it("is the date's own local fields, zero-padded", () => { + expect(formatDay(new Date(2026, 7, 6))).toBe("2026-08-06"); + expect(formatDay(new Date(2026, 0, 1))).toBe("2026-01-01"); + expect(formatDay(new Date(1999, 11, 31))).toBe("1999-12-31"); + }); + + it("reports the day the author saw, not UTC's", () => { + // 11pm local on the 6th is the 7th in UTC east of Greenwich. toISOString + // would report the wrong day for half the world; this must not. + const late = new Date(2026, 7, 6, 23, 30); + + expect(formatDay(late)).toBe("2026-08-06"); + }); +}); + +describe("formatMoment", () => { + it("adds a zero-padded 24-hour time to the day", () => { + expect(formatMoment(new Date(2026, 7, 6, 9, 5))).toBe("2026-08-06 09:05"); + expect(formatMoment(new Date(2026, 7, 6, 23, 59))).toBe("2026-08-06 23:59"); + }); +}); diff --git a/frontend/src/lib/blame.ts b/frontend/src/lib/blame.ts new file mode 100644 index 0000000..3633930 --- /dev/null +++ b/frontend/src/lib/blame.ts @@ -0,0 +1,154 @@ +import type { Blame, BlameCommit } from "./git"; + +/** + * The blame column's model (#52): turning what git said about a file into the + * two strings a gutter entry shows — a label narrow enough to sit beside the + * code, and a tooltip carrying everything the label had to leave out. + * + * Everything here is pure. The hook (`useBlame`) owns the backend call and the + * rules about when a blame is stale; the gutter (`blameExtension`) owns the + * CodeMirror side; this owns what an entry says. It is the same split + * `editorTabs.ts` already uses, and the reason a date format can be tested + * without a repository or a DOM. + */ + +/** + * The widest label the column has to hold, in characters: `XX YYYY-MM-DD`. + * + * It is a constant rather than a number in the stylesheet because the two are + * one decision. Initials are capped at two and the date is fixed-width, so + * every committed entry is exactly this long — the column is sized from here, + * and a label that outgrew it would be truncated rather than wrapped. + */ +export const BLAME_LABEL_WIDTH = 13; + +/** What the column shows for a line git attributes to no commit. It replaces + * the initials rather than sitting beside them: there is nobody to initial. */ +export const UNCOMMITTED_LABEL = "uncommitted"; + +/** The initials shown for an author whose name produced none. */ +const UNKNOWN_INITIALS = "??"; + +/** + * The commit a 1-based line number is attributed to, or null. + * + * Null covers three real cases, and the caller treats them alike because there + * is nothing to say about any of them: a line past the end of the blame (the + * buffer has grown since it was read), a line the blame did not cover + * (`internal/git`'s -1), and an index that does not name a commit. + */ +export function commitAt(blame: Blame, line: number): BlameCommit | null { + if (line < 1 || line > blame.lines.length) { + return null; + } + const index = blame.lines[line - 1]; + if (index < 0 || index >= blame.commits.length) { + return null; + } + return blame.commits[index]; +} + +/** + * The label for one entry: the author's initials and the date they wrote it. + * + * The date is `YYYY-MM-DD` rather than a locale format or a relative phrase. + * It is the same width for every line, which is what lets the column hold a + * fixed size, and it sorts by eye — the question the column answers is "which + * of these lines is newer than the others". + */ +export function blameLabel(commit: BlameCommit): string { + if (commit.uncommitted) { + return UNCOMMITTED_LABEL; + } + // Trimmed, because a commit whose timestamp git could not print leaves the + // date empty and a label ending in a space is a label with a rendering fault. + return `${initials(commit.author)} ${blameDay(commit)}`.trimEnd(); +} + +/** + * The entry's tooltip: everything the label had no room for. + * + * The abbreviated SHA is here rather than in the label because it is what a + * user takes to the terminal — `git show ` in the pane below is the next + * step after finding the line that surprised them. + */ +export function blameTooltip(commit: BlameCommit): string { + if (commit.uncommitted) { + return "Not committed yet — this line is only in the working tree"; + } + const parts = [commit.author, blameMoment(commit), abbreviate(commit.sha)]; + if (commit.summary !== "") { + parts.push(commit.summary); + } + return parts.filter((part) => part !== "").join(" · "); +} + +/** + * A person's initials, at most two. + * + * A one-word name gives its first two letters — `cjimti` is `CJ` — because a + * single letter in a two-character column reads as a rendering fault rather + * than as a name. More than two words gives the first and the last, which is + * the pair a reader recognises in `Ada Byron King`. + * + * Iterated by code point, not by index: a name beginning with an astral + * character sliced by `charAt` would produce half a surrogate pair. + */ +export function initials(author: string): string { + const words = author.split(/\s+/u).filter((word) => word !== ""); + if (words.length === 0) { + return UNKNOWN_INITIALS; + } + if (words.length === 1) { + return firstLetters(words[0], 2); + } + return firstLetters(words[0], 1) + firstLetters(words[words.length - 1], 1); +} + +/** The first `count` code points of a word, upper-cased. */ +function firstLetters(word: string, count: number): string { + return Array.from(word).slice(0, count).join("").toLocaleUpperCase(); +} + +/** A commit's date, in the reader's own zone. */ +function blameDay(commit: BlameCommit): string { + return commit.authorTime === 0 ? "" : formatDay(momentOf(commit)); +} + +/** A commit's date and time, in the reader's own zone. */ +function blameMoment(commit: BlameCommit): string { + return commit.authorTime === 0 ? "" : formatMoment(momentOf(commit)); +} + +function momentOf(commit: BlameCommit): Date { + return new Date(commit.authorTime * 1000); +} + +/** + * `YYYY-MM-DD`, from the date's local fields. + * + * Local rather than UTC: a commit made at 11pm should carry the day its author + * made it, not tomorrow's. `toISOString` would give the second, and a locale + * format would give a different width per reader. + */ +export function formatDay(date: Date): string { + return [ + String(date.getFullYear()).padStart(4, "0"), + pad(date.getMonth() + 1), + pad(date.getDate()), + ].join("-"); +} + +/** `YYYY-MM-DD HH:MM`, for the tooltip. */ +export function formatMoment(date: Date): string { + return `${formatDay(date)} ${pad(date.getHours())}:${pad(date.getMinutes())}`; +} + +function pad(value: number): string { + return String(value).padStart(2, "0"); +} + +/** The short object name git itself would print. */ +function abbreviate(sha: string): string { + return sha.slice(0, 7); +} diff --git a/frontend/src/lib/codemirror.test.ts b/frontend/src/lib/codemirror.test.ts index decb340..451086a 100644 --- a/frontend/src/lib/codemirror.test.ts +++ b/frontend/src/lib/codemirror.test.ts @@ -1,5 +1,8 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { languageFor, mountEditor, readOnlyExtension } from "./codemirror"; +import { undo } from "@codemirror/commands"; +import { EditorView } from "@codemirror/view"; +import { blameExtension, languageFor, mountEditor, readOnlyExtension } from "./codemirror"; +import type { Blame } from "./git"; const mounted: { dispose: () => void }[] = []; @@ -37,6 +40,16 @@ function mount(over: Seed = {}) { return { editor, options: { onChange, onSave }, container }; } +/** The live view behind a mounted editor, for the few assertions that are + * about editor state rather than about what is on screen. */ +function viewIn(container: HTMLElement): EditorView { + const view = EditorView.findFromDOM(container); + if (view === null) { + throw new Error("no CodeMirror view in this container"); + } + return view; +} + /** The document as CodeMirror currently holds it, read off the DOM so the * assertion goes through the same rendering the user sees. */ function textOf(container: HTMLElement): string { @@ -154,6 +167,132 @@ describe("the read-only extension", () => { }); }); +/** A blame attributing every line to one of two commits, by index. */ +function blameOver(lines: number[]): Blame { + return { + commits: [ + { + sha: "a1b2c3d4e5f60718293a4b5c6d7e8f9012345678", + author: "Craig Johnston", + authorTime: Math.floor(new Date(2026, 7, 6, 9, 30).getTime() / 1000), + summary: "Give editor tabs a context menu", + uncommitted: false, + }, + { + sha: "0000000000000000000000000000000000000000", + author: "Not Committed Yet", + authorTime: 0, + summary: "", + uncommitted: true, + }, + ], + lines, + }; +} + +/** The blame column's rendered entries, in line order. */ +function entriesIn(container: HTMLElement): HTMLElement[] { + return [...container.querySelectorAll(".cm-blame .cm-blame-entry")]; +} + +describe("the blame column", () => { + it("is absent until it is asked for", () => { + const { container } = mount({ content: "a: 1\nb: 2\n" }); + + expect(container.querySelector(".cm-blame")).toBeNull(); + }); + + it("shows an entry per line, attributed to that line's commit", () => { + const { editor, container } = mount({ content: "a: 1\nb: 2\n" }); + + editor.setBlame(true, blameOver([0, 1])); + + const entries = entriesIn(container); + expect(entries).toHaveLength(2); + expect(entries[0].textContent).toBe("CJ 2026-08-06"); + expect(entries[0].title).toContain("Give editor tabs a context menu"); + expect(entries[1].textContent).toBe("uncommitted"); + }); + + it("marks an uncommitted entry, so it is not read as a name", () => { + const { editor, container } = mount({ content: "a: 1\nb: 2\n" }); + + editor.setBlame(true, blameOver([0, 1])); + + const entries = entriesIn(container); + expect(entries[0].className).not.toContain("uncommitted"); + expect(entries[1].className).toContain("cm-blame-entry--uncommitted"); + }); + + // The dirty-buffer case (#52): the toggle is still on, the line numbers the + // blame was stated in are no longer the buffer's, and the column holds its + // place so the code does not shift when the entries go. + it("keeps the column and drops the entries when there is no blame to show", () => { + const { editor, container } = mount({ content: "a: 1\nb: 2\n" }); + editor.setBlame(true, blameOver([0, 1])); + + editor.setBlame(true, null); + + expect(container.querySelector(".cm-blame")).not.toBeNull(); + expect(entriesIn(container)).toHaveLength(0); + }); + + it("takes the column away when the toggle goes off", () => { + const { editor, container } = mount({ content: "a: 1\n" }); + editor.setBlame(true, blameOver([0])); + + editor.setBlame(false, null); + + expect(container.querySelector(".cm-blame")).toBeNull(); + }); + + it("leaves a line the blame does not reach without an entry", () => { + // A buffer longer than the blame — what an external change produces + // between the reload and the re-read. + const { editor, container } = mount({ content: "a: 1\nb: 2\nc: 3\n" }); + + editor.setBlame(true, blameOver([0, 0])); + + expect(entriesIn(container)).toHaveLength(2); + }); + + it("does not touch the document, the way every other setter here does not", () => { + const { editor, container } = mount({ content: "a: 1\n" }); + + editor.setBlame(true, blameOver([0])); + + expect(textOf(container)).toContain("a: 1"); + }); + + // The column is a view, not an edit. Turning it on must cost the user + // nothing they were in the middle of — a reconfiguration that moved the + // caret or emptied the undo stack would make the toggle unusable while + // working, which is the only time anyone reaches for it. + it("leaves the selection and the undo history alone", () => { + const { editor, container } = mount({ content: "a: 1\nb: 2\n" }); + editor.setContent("a: 1\nb: 2\nc: 3\n"); + const view = viewIn(container); + view.dispatch({ selection: { anchor: 3 } }); + + editor.setBlame(true, blameOver([0, 0, 0])); + editor.setBlame(false, null); + + expect(view.state.selection.main.anchor).toBe(3); + // The edit above is still undoable: a lost history would leave the + // document as it is. + undo(view); + expect(view.state.doc.toString()).toBe("a: 1\nb: 2\n"); + }); +}); + +describe("the blame extension", () => { + it("adds nothing at all when the column is off", () => { + // Not merely an empty gutter: a file nobody asked to blame should carry no + // per-line callback for CodeMirror to run on every update. + expect(blameExtension(false, blameOver([0]))).toEqual([]); + }); +}); + describe("disposal", () => { it("tears the view out of the DOM", () => { const { editor, container } = mount(); diff --git a/frontend/src/lib/codemirror.ts b/frontend/src/lib/codemirror.ts index e3d93eb..15039f2 100644 --- a/frontend/src/lib/codemirror.ts +++ b/frontend/src/lib/codemirror.ts @@ -14,14 +14,18 @@ import { highlightSelectionMatches, search, searchKeymap } from "@codemirror/sea import { Compartment, EditorState, type Extension } from "@codemirror/state"; import { EditorView, + GutterMarker, drawSelection, + gutter, highlightActiveLine, highlightActiveLineGutter, keymap, lineNumbers, } from "@codemirror/view"; import { tags } from "@lezer/highlight"; +import { BLAME_LABEL_WIDTH, blameLabel, blameTooltip, commitAt } from "./blame"; import type { EditorTabKind } from "./editorTabs"; +import type { Blame, BlameCommit } from "./git"; import type { Appearance } from "./theme"; /** @@ -38,12 +42,34 @@ import type { Appearance } from "./theme"; * diagnostics are #13's, and this file should not grow them by accident. */ +/** + * The editor's monospace stack. + * + * Named because two places need it and they must not drift: the content, and + * the blame column beside it. CodeMirror's gutters inherit the app's UI font + * unless told otherwise, and a proportional gutter makes `ch` mean something + * other than a character — which is how a column sized to hold + * `XX YYYY-MM-DD` ends up truncating it. + */ +const MONO_FAMILY = + 'ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, "Liberation Mono", monospace'; + /** A mounted editor: what the pane drives. */ export interface MountedEditor { /** Replaces the document, unless it already holds exactly this text. */ setContent(content: string): void; setTheme(appearance: Appearance): void; setReadOnly(readOnly: boolean): void; + /** + * Shows, hides or refreshes the blame column (#52). + * + * `shown` and `blame` are separate arguments because they are separate + * states. A column that is shown with no blame to put in it is the tab's + * dirty case: the toggle is still on, the entries are not trustworthy while + * the buffer has an unsaved line in it, and the column keeps its width so + * that typing does not shift the code sideways. + */ + setBlame(shown: boolean, blame: Blame | null): void; focus(): void; dispose(): void; } @@ -70,12 +96,17 @@ export function mountEditor( ): MountedEditor { const themeSlot = new Compartment(); const readOnlySlot = new Compartment(); + const blameSlot = new Compartment(); const view = new EditorView({ parent: container, state: EditorState.create({ doc: options.content, extensions: [ + // Ahead of baseExtensions, so the blame column sits to the left of the + // line numbers rather than between them and the code. Gutters are laid + // out in the order their extensions appear. + blameSlot.of([]), ...baseExtensions(options.onSave), ...languageFor(options.kind), themeSlot.of(editorTheme(options.appearance)), @@ -112,6 +143,11 @@ export function mountEditor( effects: readOnlySlot.reconfigure(readOnlyExtension(readOnly)), }); }, + setBlame: (shown, blame) => { + view.dispatch({ + effects: blameSlot.reconfigure(blameExtension(shown, blame)), + }); + }, focus: () => { view.focus(); }, @@ -172,6 +208,67 @@ export function languageFor(kind: EditorTabKind): Extension[] { return []; } +/** + * The blame column (#52), as an extension that can be swapped in place. + * + * Nothing is added when the column is off, so a file nobody asked to blame + * carries no gutter and no per-line callback at all. When it is on with no + * blame behind it every line's marker is null, which leaves the gutter present + * and empty — the width comes from CSS, not from its contents, so the code + * does not move when the entries come and go. + */ +export function blameExtension(shown: boolean, blame: Blame | null): Extension { + if (!shown) { + return []; + } + return gutter({ + class: "cm-blame", + lineMarker: (view, line) => { + if (blame === null) { + return null; + } + const commit = commitAt(blame, view.state.doc.lineAt(line.from).number); + return commit === null ? null : new BlameEntry(commit); + }, + }); +} + +/** One line's entry in the blame column. */ +class BlameEntry extends GutterMarker { + private readonly label: string; + private readonly tooltip: string; + private readonly uncommitted: boolean; + + constructor(commit: BlameCommit) { + super(); + this.label = blameLabel(commit); + this.tooltip = blameTooltip(commit); + this.uncommitted = commit.uncommitted; + } + + // CodeMirror keeps a marker's DOM when the new marker for a line compares + // equal, so this is what stops every entry in the file being rebuilt on a + // keystroke. Comparing the rendered strings rather than the commit is + // deliberate: two entries that say the same thing are the same entry. + override eq(other: GutterMarker): boolean { + return ( + other instanceof BlameEntry && + other.label === this.label && + other.tooltip === this.tooltip + ); + } + + override toDOM(): Node { + const entry = document.createElement("span"); + entry.className = this.uncommitted + ? "cm-blame-entry cm-blame-entry--uncommitted" + : "cm-blame-entry"; + entry.textContent = this.label; + entry.title = this.tooltip; + return entry; + } +} + /** The read-only state, as an extension that can be swapped in place. */ export function readOnlyExtension(readOnly: boolean): Extension { return [EditorState.readOnly.of(readOnly), EditorView.editable.of(!readOnly)]; @@ -192,11 +289,7 @@ export function editorTheme(appearance: Appearance): Extension { EditorView.theme( { "&": { color: c.fg, backgroundColor: c.bg, height: "100%" }, - ".cm-content": { - caretColor: c.caret, - fontFamily: - 'ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, "Liberation Mono", monospace', - }, + ".cm-content": { caretColor: c.caret, fontFamily: MONO_FAMILY }, ".cm-cursor, .cm-dropCursor": { borderLeftColor: c.caret }, "&.cm-focused .cm-selectionBackground, .cm-selectionBackground, .cm-content ::selection": { backgroundColor: c.selection }, @@ -207,6 +300,42 @@ export function editorTheme(appearance: Appearance): Extension { border: "none", }, ".cm-activeLineGutter": { backgroundColor: c.activeLine, color: c.fg }, + // A fixed width, so the column is the same size whether it holds + // entries or is waiting on a save to get them back — the code to its + // right must not move when the user starts typing. The label is sized + // to `XX YYYY-MM-DD`, and anything longer is clipped rather than + // allowed to widen it. + ".cm-blame": { + // Monospace and a width in `ch`, so the width is stated in the unit + // the label is measured in: every committed entry is exactly + // `XX YYYY-MM-DD`, and one extra character of slack keeps a subpixel + // rounding from truncating the last digit of the date. + // + // content-box is the load-bearing part. Everything in a CodeMirror + // view is border-box, under which this width would be the padded box + // and the text would get whatever was left — about a character less + // than the label needs, which truncates the date to `2026-01…`. The + // width has to mean the text, so the padding goes outside it. + boxSizing: "content-box", + width: `${String(BLAME_LABEL_WIDTH + 1)}ch`, + padding: "0 0.5rem 0 0.35rem", + color: c.gutterFg, + borderRight: `1px solid ${c.rule}`, + fontFamily: MONO_FAMILY, + fontSize: "0.85em", + // Both spellings, for the reason style.css's `.shell` rule gives at + // length: unprefixed `user-select` reached WebKit only in Safari 17, + // and this app's window is a WebKit one. + WebkitUserSelect: "none", + userSelect: "none", + }, + ".cm-blame-entry": { + display: "block", + overflow: "hidden", + whiteSpace: "nowrap", + textOverflow: "ellipsis", + }, + ".cm-blame-entry--uncommitted": { fontStyle: "italic", opacity: "0.7" }, ".cm-panels": { backgroundColor: c.gutterBg, color: c.fg }, ".cm-searchMatch": { backgroundColor: c.searchMatch }, ".cm-searchMatch.cm-searchMatch-selected": { @@ -233,6 +362,8 @@ interface EditorPalette { readonly activeLine: string; readonly gutterBg: string; readonly gutterFg: string; + /** A hairline between one gutter and the next — the blame column's edge. */ + readonly rule: string; readonly searchMatch: string; readonly searchMatchActive: string; readonly key: string; @@ -252,6 +383,7 @@ const DARK: EditorPalette = { activeLine: "#1c1f26", gutterBg: "#16181d", gutterFg: "#5c6370", + rule: "#262a33", searchMatch: "#3a4a6b", searchMatchActive: "#61afef", key: "#61afef", @@ -271,6 +403,7 @@ const LIGHT: EditorPalette = { activeLine: "#f0f1f5", gutterBg: "#fbfbfd", gutterFg: "#6b7280", + rule: "#e3e5ea", searchMatch: "#d6e4fb", searchMatchActive: "#2f6fd0", key: "#2f6fd0", diff --git a/frontend/src/lib/editorTabs.test.ts b/frontend/src/lib/editorTabs.test.ts index f4310ca..36c778a 100644 --- a/frontend/src/lib/editorTabs.test.ts +++ b/frontend/src/lib/editorTabs.test.ts @@ -18,6 +18,8 @@ import { resolveTakeDisk, selectionAfterClose, tabsForProject, + blameIsCurrent, + withBlame, withEdit, withError, withExternalChange, @@ -354,6 +356,41 @@ describe("the strip", () => { }); }); +describe("the blame column (#52)", () => { + it("is off on a tab nobody has asked about", () => { + expect(blank().blame).toBe(false); + }); + + it("turns on and off for one tab", () => { + const on = withBlame(ready("a: 1\n"), true); + + expect(on.blame).toBe(true); + expect(withBlame(on, false).blame).toBe(false); + }); + + // The whole rule the column rests on: a blame is stated in the line numbers + // of the file git measured, and an unsaved insertion moves every line below + // it. `baseline` is the disk content by definition, so matching it is the + // test — nothing else needs asking. + it("describes the buffer only while it matches disk", () => { + const clean = ready("a: 1\n"); + + expect(blameIsCurrent(clean)).toBe(true); + expect(blameIsCurrent(withEdit(clean, "inserted\na: 1\n"))).toBe(false); + }); + + it("describes a tab whose edit was undone back to disk", () => { + const clean = ready("a: 1\n"); + + expect(blameIsCurrent(withEdit(withEdit(clean, "a: 2\n"), "a: 1\n"))).toBe(true); + }); + + it("describes nothing about a tab that has not loaded", () => { + expect(blameIsCurrent(blank())).toBe(false); + expect(blameIsCurrent(withError(blank(), "gone"))).toBe(false); + }); +}); + describe("the paths a tab can copy (#42)", () => { /** A tab for `path` under `root`. */ const at = (root: string, path: string): EditorTab => diff --git a/frontend/src/lib/editorTabs.ts b/frontend/src/lib/editorTabs.ts index 100e1ae..e24d6a8 100644 --- a/frontend/src/lib/editorTabs.ts +++ b/frontend/src/lib/editorTabs.ts @@ -38,6 +38,11 @@ export interface EditorTab { readonly title: string; readonly kind: EditorTabKind; readonly mode: EditorMode; + /** Whether the blame column is turned on for this tab (#52). It is per tab + * rather than per strip: blame answers a question about one file, and a + * user who turned it on to read a manifest should not find it on the + * README they open next. */ + readonly blame: boolean; readonly status: EditorTabStatus; /** The buffer as the user is editing it, LF-normalized. */ readonly content: string; @@ -190,6 +195,7 @@ export function newTab( // Markdown opens in rendered preview by default (the issue's own // wording); yaml and text have no preview to default away from. mode: kind === "markdown" ? "preview" : "edit", + blame: false, status: "loading", content: "", baseline: "", @@ -343,6 +349,24 @@ export function withMode(tab: EditorTab, mode: EditorMode): EditorTab { return { ...tab, mode }; } +/** Turns this tab's blame column on or off (#52). */ +export function withBlame(tab: EditorTab, blame: boolean): EditorTab { + return { ...tab, blame }; +} + +/** + * Whether a tab's blame entries describe the buffer on screen. + * + * `git blame` measures the file on disk and answers in line numbers. One + * unsaved insertion moves every line below it, so a blame read before that + * edit now names the wrong author for most of the file. Dirty is the whole + * test: `baseline` is the disk content by definition, so a tab that matches it + * is a tab git measured. + */ +export function blameIsCurrent(tab: EditorTab): boolean { + return tab.status === "ready" && !isDirty(tab); +} + /** * Reconciles a tab against a file that a `tree` event says may have changed * on disk, given its freshly re-read content. diff --git a/frontend/src/lib/git.ts b/frontend/src/lib/git.ts index 83c0a73..7e7de3f 100644 --- a/frontend/src/lib/git.ts +++ b/frontend/src/lib/git.ts @@ -1,4 +1,5 @@ import { + GitBlame, GitBranches, GitCheckout, GitPull, @@ -47,6 +48,34 @@ export interface Status { readonly files: readonly FileStatus[]; } +/** One commit a file's lines are attributed to (#52), matching + * `internal/git.BlameCommit`. */ +export interface BlameCommit { + readonly sha: string; + readonly author: string; + /** When the author wrote it, in Unix seconds; 0 when git reported no + * readable time. */ + readonly authorTime: number; + /** The commit's subject line. */ + readonly summary: string; + /** git's all-zero SHA: work in the working tree and in no commit. Such a + * commit has no author or date worth showing. */ + readonly uncommitted: boolean; +} + +/** + * One file's per-line attribution, in the shape git's porcelain format states + * it: each commit once, and a line-by-line reference into them. + * + * `lines` holds one index into `commits` per line of the file, line 1 first. + * An index outside `commits` means the blame did not cover that line — see + * `commitAt` in `blame.ts`, which is the only thing that should read this. + */ +export interface Blame { + readonly commits: readonly BlameCommit[]; + readonly lines: readonly number[]; +} + /** * Why a project has no readable git state, matching * `internal/git.Availability`. @@ -87,6 +116,10 @@ export const UNTRACKED = "untracked"; */ export interface Git { status: (root: string) => Promise; + /** One file's per-line attribution (#52). `path` is root-relative and + * slash-separated; a path git will not blame rejects with git's own words + * rather than resolving to an empty blame. */ + blame: (root: string, path: string) => Promise; pull: (root: string) => Promise; /** `remote` takes effect only when `setUpstream` is true; otherwise the * repository's own push configuration decides where the branch goes. */ @@ -99,6 +132,7 @@ export interface Git { /** The git seam backed by the generated Wails bindings. */ export const wailsGit: Git = { status: (root) => GitStatus(root), + blame: (root, path) => GitBlame(root, path), pull: (root) => GitPull(root), push: (root, remote, setUpstream) => GitPush(root, remote, setUpstream), checkout: (root, branch) => GitCheckout(root, branch), @@ -106,6 +140,12 @@ export const wailsGit: Git = { remotes: (root) => GitRemotes(root), }; +/** A blame with nothing in it — a file of no lines. Every consumer already + * handles a line the blame does not cover, so this needs no special case. */ +export function emptyBlame(): Blame { + return { commits: [], lines: [] }; +} + /** A status for a project nothing has been read for yet: available-shaped and * empty, so every consumer renders it without a null check. */ export function emptyStatus(): Status { diff --git a/frontend/src/lib/gitStatus.test.ts b/frontend/src/lib/gitStatus.test.ts index 66bfa4b..20ec068 100644 --- a/frontend/src/lib/gitStatus.test.ts +++ b/frontend/src/lib/gitStatus.test.ts @@ -21,6 +21,7 @@ import { changedCount, changedRows, fileBadge, + isTracked, } from "./gitStatus"; /** One changed path, defaulting every field the case under test is not @@ -295,6 +296,48 @@ describe("the changed-only rows (#40)", () => { }); }); +describe("whether a path is tracked (#52)", () => { + // git emits a record only for a path that differs, so a path it did not + // mention is a tracked path with nothing to report. Reading the absence the + // other way would take the blame column off every unmodified file in the + // repository — which is most of them. + it("counts a path git did not mention as tracked", () => { + expect(isTracked(statusOf([file("other.yaml", { worktree: MODIFIED })]), "deploy.yaml")).toBe( + true, + ); + }); + + it("counts a changed path as tracked", () => { + expect(isTracked(statusOf([file("deploy.yaml", { worktree: MODIFIED })]), "deploy.yaml")).toBe( + true, + ); + }); + + it("counts a staged new path as tracked", () => { + // `git add` on a new file puts it in the index, and blame answers for it: + // every line comes back uncommitted. + expect(isTracked(statusOf([file("new.yaml", { staged: ADDED })]), "new.yaml")).toBe(true); + }); + + it("does not count an untracked path", () => { + expect(isTracked(statusOf([file("scratch.yaml", { worktree: UNTRACKED })]), "scratch.yaml")).toBe( + false, + ); + }); + + it("counts nothing when git is missing or the project is not a repository", () => { + for (const availability of [NO_GIT, NOT_A_REPOSITORY]) { + expect(isTracked({ ...emptyStatus(), availability }, "deploy.yaml")).toBe(false); + } + }); + + it("matches the whole path, not a suffix of it", () => { + const status = statusOf([file("deploy/deploy.yaml", { worktree: UNTRACKED })]); + + expect(isTracked(status, "deploy.yaml")).toBe(true); + }); +}); + describe("the status bar's branch line", () => { it("shows the branch, its counts and the change total", () => { const status = statusOf([file("a.yaml", { worktree: MODIFIED }), file("b.yaml", { worktree: MODIFIED })], { diff --git a/frontend/src/lib/gitStatus.ts b/frontend/src/lib/gitStatus.ts index 6307e72..24835a7 100644 --- a/frontend/src/lib/gitStatus.ts +++ b/frontend/src/lib/gitStatus.ts @@ -1,6 +1,7 @@ import type { FileStatus, Status } from "./git"; import { ADDED, + AVAILABLE, COPIED, DELETED, MODIFIED, @@ -281,6 +282,24 @@ function byTreeOrder(a: TreeEntry, b: TreeEntry): number { return x > y ? 1 : 0; } +/** + * Whether git is tracking a path, which is what decides that the blame toggle + * exists for it (#52). + * + * It needs no call of its own. porcelain v2 emits a record for a path that + * differs and nothing for one that does not, so a path git did not mention is + * a tracked, unmodified path — and the only records that mean "not tracked" + * are the untracked ones. The two unavailable states are false for the reason + * they exist: with no git, or outside a repository, there is nothing to ask. + */ +export function isTracked(status: Status, path: string): boolean { + if (status.availability !== AVAILABLE) { + return false; + } + const entry = status.files.find((file) => file.path === path); + return entry === undefined || entry.worktree !== UNTRACKED; +} + /** How many paths the status bar reports as changed. */ export function changedCount(status: Status): number { return status.files.length; diff --git a/frontend/src/lib/tree.test.ts b/frontend/src/lib/tree.test.ts index 616278d..f2994c7 100644 --- a/frontend/src/lib/tree.test.ts +++ b/frontend/src/lib/tree.test.ts @@ -14,6 +14,7 @@ import { looksLikeManifest, parentPath, resolveIconKind, + locate, reveal, select, toggleChangedOnly, @@ -346,6 +347,81 @@ describe("the ancestor chain (#43)", () => { }); }); +describe("locating a file (#56)", () => { + it("expands the directories above it and selects the file", () => { + const state = locate(initialTree(), "manifests/prod/ingress.yaml"); + + expect([...state.expanded].sort()).toEqual([ROOT, "manifests", "manifests/prod"]); + expect(state.selected).toBe("manifests/prod/ingress.yaml"); + }); + + // The difference from `reveal`, and the reason this is not just a call to + // it: a file in the expanded set is a directory listing nobody will fetch, + // and the hook would ask the backend to list a file. + it("does not expand the file itself", () => { + const state = locate(initialTree(), "manifests/prod/ingress.yaml"); + + expect(state.expanded.has("manifests/prod/ingress.yaml")).toBe(false); + }); + + it("selects a file at the root without expanding anything new", () => { + const state = locate(initialTree(), "README.md"); + + expect(state.selected).toBe("README.md"); + expect([...state.expanded]).toEqual([ROOT]); + }); + + it("leaves changed-only mode, which would otherwise hide the file", () => { + const filtered = toggleChangedOnly(initialTree()); + + expect(locate(filtered, "manifests/ingress.yaml").changedOnly).toBe(false); + }); + + // The case a parent-only hidden check gets wrong: this file has no hidden + // ancestor, so revealing its parent alone would leave the filter on and the + // locate would do nothing at all. + it("shows hidden files for a dotfile at the root", () => { + expect(locate(initialTree(), ".gitignore").showHidden).toBe(true); + }); + + it("shows hidden files for a file under a hidden directory", () => { + const state = locate(initialTree(), ".github/workflows/ci.yml"); + + expect(state.showHidden).toBe(true); + expect(state.expanded.has(".github/workflows")).toBe(true); + }); + + it("leaves the filter alone for an ordinary file", () => { + expect(locate(initialTree(), "manifests/ingress.yaml").showHidden).toBe(false); + }); + + // The view scrolls a located row to the middle of the pane and an ordinary + // selection only into view, and it cannot tell the two apart from `selected` + // alone — locating a file that is already selected changes nothing else. + it("records a request every time, including for the file already selected", () => { + const first = locate(initialTree(), "manifests/ingress.yaml"); + const again = locate(first, "manifests/ingress.yaml"); + + expect(first.locateRequest).toBe(initialTree().locateRequest + 1); + expect(again.locateRequest).toBe(first.locateRequest + 1); + expect(again.selected).toBe(first.selected); + }); + + it("is the only thing that records one", () => { + const located = locate(initialTree(), "manifests/ingress.yaml"); + + for (const state of [ + select(located, "other.yaml"), + reveal(located, "manifests"), + expand(located, "manifests"), + toggleHidden(located), + toggleChangedOnly(located), + ]) { + expect(state.locateRequest).toBe(located.locateRequest); + } + }); +}); + describe("revealing a directory (#43)", () => { it("expands the directory and every directory above it", () => { const state = reveal(initialTree(), "manifests/prod"); diff --git a/frontend/src/lib/tree.ts b/frontend/src/lib/tree.ts index 6e1f155..1f4643a 100644 --- a/frontend/src/lib/tree.ts +++ b/frontend/src/lib/tree.ts @@ -63,6 +63,21 @@ export interface TreeState { * the classification happen once per file for the life of the tree. */ readonly manifests: ReadonlyMap; + /** + * How many times a locate has been asked for (#56). + * + * A counter rather than a flag, and it exists because "the selection + * changed" and "the user asked to be shown this file" need different + * answers from the tree: an ordinary selection scrolls the row just far + * enough to be visible, and a locate puts it in the middle of the pane. The + * two are indistinguishable from `selected` alone — pressing Locate on a + * file that is already selected changes nothing about it. + * + * Nothing reads the value. What matters is that it differs from the last + * one the view acted on, which is also what makes a second locate of the + * same file work. + */ + readonly locateRequest: number; } /** A tree with nothing loaded yet, root pre-expanded — the top level of any @@ -75,6 +90,7 @@ export function initialTree(): TreeState { showHidden: false, changedOnly: false, manifests: new Map(), + locateRequest: 0, }; } @@ -414,6 +430,32 @@ export function reveal(state: TreeState, dir: string): TreeState { }; } +/** + * Brings a file on screen: every directory above it expanded, the file itself + * selected, and every filter that would have hidden it cleared (#56). + * + * It is not `reveal` with a file path. `reveal` expands what it is given, and + * a file in the expanded set is a directory listing nobody will ever fetch — + * `useFileTree.reveal` would ask the backend to list a file. This expands the + * chain above the file and stops there. + * + * The hidden check runs over the file's own chain rather than its parent's, + * which is the case a parent-only check gets wrong: `.gitignore` at the root + * has no hidden ancestor, so revealing its parent would leave `showHidden` + * off and the tree would answer the locate by doing nothing at all. + */ +export function locate(state: TreeState, path: string): TreeState { + const revealed = reveal(state, parentPath(path)); + return { + ...revealed, + selected: path, + locateRequest: state.locateRequest + 1, + showHidden: + revealed.showHidden || + ancestry(path).some((step) => isHidden({ name: baseName(step) })), + }; +} + export function select(state: TreeState, path: string | null): TreeState { return { ...state, selected: path }; } diff --git a/frontend/src/lib/useBlame.test.ts b/frontend/src/lib/useBlame.test.ts new file mode 100644 index 0000000..8b72db0 --- /dev/null +++ b/frontend/src/lib/useBlame.test.ts @@ -0,0 +1,199 @@ +import { renderHook, waitFor } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; +import type { EditorTab } from "./editorTabs"; +import { newTab, withBlame, withEdit, withLoaded } from "./editorTabs"; +import type { Blame } from "./git"; +import { emptyBlame } from "./git"; +import { useBlame } from "./useBlame"; + +const blame: Blame = { + commits: [ + { + sha: "a1b2c3d", + author: "Craig Johnston", + authorTime: 1_754_400_000, + summary: "first", + uncommitted: false, + }, + ], + lines: [0], +}; + +/** A ready tab holding `content`, with the blame column on unless told + * otherwise. */ +function tab(content = "a: 1\n", on = true): EditorTab { + const loaded = withLoaded(newTab("k0", "infra", "/w/infra", "deploy.yaml", "yaml"), { + content, + crlf: false, + mixedEol: false, + readOnly: false, + size: content.length, + }); + return withBlame(loaded, on); +} + +function fakeGit(answer: Blame = blame) { + return { blame: vi.fn(() => Promise.resolve(answer)) }; +} + +describe("reading a blame", () => { + it("reads the active tab's file when the column is on", async () => { + const git = fakeGit(); + const { result } = renderHook(() => useBlame(tab(), git)); + + await waitFor(() => { + expect(result.current.blame).toEqual(blame); + }); + expect(git.blame).toHaveBeenCalledWith("/w/infra", "deploy.yaml"); + expect(result.current.error).toBeNull(); + }); + + it("reads nothing while the column is off", () => { + const git = fakeGit(); + + const { result } = renderHook(() => useBlame(tab("a: 1\n", false), git)); + + expect(git.blame).not.toHaveBeenCalled(); + expect(result.current.blame).toBeNull(); + }); + + it("reads nothing when there is no file open", () => { + const git = fakeGit(); + + renderHook(() => useBlame(null, git)); + + expect(git.blame).not.toHaveBeenCalled(); + }); + + // The core rule of #52: a blame is stated in the line numbers of the file on + // disk, and an unsaved insertion moves every line below it. Attributing the + // buffer with a blame of the saved file would name the wrong author for most + // of it. + it("drops the entries while the buffer has unsaved edits", async () => { + const git = fakeGit(); + const clean = tab("a: 1\n"); + const { result, rerender } = renderHook(({ open }) => useBlame(open, git), { + initialProps: { open: clean }, + }); + await waitFor(() => { + expect(result.current.blame).not.toBeNull(); + }); + + rerender({ open: withEdit(clean, "inserted\na: 1\n") }); + + expect(result.current.blame).toBeNull(); + }); + + // The other half of the same rule: a save moves the baseline, and the lines + // the user just wrote come back attributed to nobody, which is what they are. + it("reads again when the file is saved", async () => { + const git = fakeGit(); + const first = tab("a: 1\n"); + const { rerender } = renderHook(({ open }) => useBlame(open, git), { + initialProps: { open: first }, + }); + await waitFor(() => { + expect(git.blame).toHaveBeenCalledTimes(1); + }); + + rerender({ open: { ...first, content: "a: 2\n", baseline: "a: 2\n" } }); + + await waitFor(() => { + expect(git.blame).toHaveBeenCalledTimes(2); + }); + }); + + // A tab is a new object on every keystroke. An effect that depended on the + // object rather than on what it holds would spend a subprocess per character. + it("does not read again when nothing it depends on changed", async () => { + const git = fakeGit(); + const open = tab("a: 1\n"); + const { rerender } = renderHook(({ current }) => useBlame(current, git), { + initialProps: { current: open }, + }); + await waitFor(() => { + expect(git.blame).toHaveBeenCalledTimes(1); + }); + + rerender({ current: { ...open } }); + + expect(git.blame).toHaveBeenCalledTimes(1); + }); + + it("reads the new file when the user switches tabs", async () => { + const git = fakeGit(); + const { rerender } = renderHook(({ open }) => useBlame(open, git), { + initialProps: { open: tab() }, + }); + await waitFor(() => { + expect(git.blame).toHaveBeenCalledTimes(1); + }); + + rerender({ open: { ...tab(), key: "k1", path: "other.yaml" } }); + + await waitFor(() => { + expect(git.blame).toHaveBeenLastCalledWith("/w/infra", "other.yaml"); + }); + }); + + it("keeps an empty blame rather than treating it as a failure", async () => { + const git = fakeGit(emptyBlame()); + const { result } = renderHook(() => useBlame(tab(), git)); + + await waitFor(() => { + expect(result.current.blame).toEqual(emptyBlame()); + }); + expect(result.current.error).toBeNull(); + }); +}); + +describe("when git refuses", () => { + it("keeps git's own words rather than translating them", async () => { + const git = { + blame: vi.fn(() => + Promise.reject(new Error("fatal: no such path 'x.yaml' in HEAD")), + ), + }; + const { result } = renderHook(() => useBlame(tab(), git)); + + await waitFor(() => { + expect(result.current.error).toBe("fatal: no such path 'x.yaml' in HEAD"); + }); + expect(result.current.blame).toBeNull(); + }); + + // The generated binding throws synchronously with no Wails runtime behind + // it, and it throws a string rather than an Error. + it("says something for a rejection that is not an Error", async () => { + const git = { blame: vi.fn(() => Promise.reject("no bridge")) }; + const { result } = renderHook(() => useBlame(tab(), git)); + + await waitFor(() => { + expect(result.current.error).toBe("no bridge"); + }); + }); + + it("says something for a rejection with no message at all", async () => { + const git = { blame: vi.fn(() => Promise.reject(undefined)) }; + const { result } = renderHook(() => useBlame(tab(), git)); + + await waitFor(() => { + expect(result.current.error).toBe("the git backend is not reachable"); + }); + }); + + it("clears the failure when the column is turned off", async () => { + const git = { blame: vi.fn(() => Promise.reject(new Error("fatal: nope"))) }; + const open = tab(); + const { result, rerender } = renderHook(({ current }) => useBlame(current, git), { + initialProps: { current: open }, + }); + await waitFor(() => { + expect(result.current.error).not.toBeNull(); + }); + + rerender({ current: withBlame(open, false) }); + + expect(result.current.error).toBeNull(); + }); +}); diff --git a/frontend/src/lib/useBlame.ts b/frontend/src/lib/useBlame.ts new file mode 100644 index 0000000..ad89650 --- /dev/null +++ b/frontend/src/lib/useBlame.ts @@ -0,0 +1,99 @@ +import { useEffect, useRef, useState } from "react"; +import type { EditorTab } from "./editorTabs"; +import { blameIsCurrent } from "./editorTabs"; +import type { Blame, Git } from "./git"; +import { wailsGit } from "./git"; + +/** The reading half of the git seam this hook uses — the only call it makes. */ +type BlameReader = Pick; + +/** What the editor pane knows about its blame: the data, or why there is none. */ +export interface BlameState { + readonly blame: Blame | null; + /** git's own words when it refused (DESIGN.md §7), null otherwise. */ + readonly error: string | null; +} + +/** No blame and no failure: what a pane whose column is off is handed, and + * what a read resets to. */ +export const NO_BLAME: BlameState = { blame: null, error: null }; + +/** + * The active tab's blame (#52), read when the column is on and the buffer + * matches disk. + * + * Only one tab's blame is ever held. Every pane stays mounted for the reason + * `EditorPane` documents, but only one of them is on screen, and a blame is a + * subprocess per file — reading one for a tab nobody is looking at would spend + * a `git blame` on every open file every time one of them was saved. + * + * The read is keyed on the tab's `baseline`, which is the disk content by + * definition. That is what makes a save refresh the column: the write moves + * the baseline, this asks git again, and the lines the user just wrote come + * back attributed to nobody, which is what they are. + */ +export function useBlame(tab: EditorTab | null, git: BlameReader = wailsGit): BlameState { + const [state, setState] = useState(NO_BLAME); + + // The seam behind a ref, for the reason `useGitStatus` documents at length: + // a caller passing an inline object would otherwise rebuild it every render + // and re-run the read effect forever. + const seam = useRef(git); + useEffect(() => { + seam.current = git; + }, [git]); + + // Read out of the tab rather than depending on the object: a tab is replaced + // on every keystroke, and an effect that depended on it would re-read the + // blame between one character and the next. + const wanted = tab !== null && tab.blame && blameIsCurrent(tab); + const root = tab?.root ?? ""; + const path = tab?.path ?? ""; + const baseline = tab?.baseline ?? ""; + + useEffect(() => { + if (!wanted) { + setState(NO_BLAME); + return; + } + // A blame that is still in flight when the user switches tabs or saves + // again is dropped rather than applied: it describes a file that is no + // longer the one on screen, and its line numbers would land on the wrong + // text. + let live = true; + void (async () => { + const result = await readBlame(seam.current, root, path); + if (live) { + setState(result); + } + })(); + return () => { + live = false; + }; + }, [wanted, root, path, baseline]); + + return state; +} + +/** Reads one blame, turning a rejection into a message rather than a throw — + * including the synchronous throw the generated binding produces when there is + * no Wails runtime behind it. */ +async function readBlame( + git: BlameReader, + root: string, + path: string, +): Promise { + try { + return { blame: await git.blame(root, path), error: null }; + } catch (failure: unknown) { + return { blame: null, error: describeError(failure) }; + } +} + +/** Renders a rejected binding call as a sentence the pane can show. */ +function describeError(error: unknown): string { + if (error instanceof Error) { + return error.message; + } + return typeof error === "string" ? error : "the git backend is not reachable"; +} diff --git a/frontend/src/lib/useEditorTabs.test.ts b/frontend/src/lib/useEditorTabs.test.ts index 5f39895..e50bf79 100644 --- a/frontend/src/lib/useEditorTabs.test.ts +++ b/frontend/src/lib/useEditorTabs.test.ts @@ -480,6 +480,55 @@ describe("the strip across projects", () => { }); }); +describe("the blame toggle (#52)", () => { + it("turns one tab's column on without touching another's", async () => { + const files = fakeFiles({ "a.yaml": content("a: 1\n"), "b.yaml": content("b: 2\n") }); + const { result } = renderHook(() => useEditorTabs("infra", null, files)); + + act(() => { + result.current.open("infra", "/w/infra", "a.yaml"); + }); + await waitFor(() => { + expect(result.current.visible).toHaveLength(1); + }); + act(() => { + result.current.open("infra", "/w/infra", "b.yaml"); + }); + await waitFor(() => { + expect(result.current.visible).toHaveLength(2); + }); + + act(() => { + result.current.setBlame(result.current.visible[0].key, true); + }); + + expect(result.current.visible[0].blame).toBe(true); + expect(result.current.visible[1].blame).toBe(false); + }); + + it("turns it off again", async () => { + const files = fakeFiles({ "a.yaml": content("a: 1\n") }); + const { result } = renderHook(() => useEditorTabs("infra", null, files)); + + act(() => { + result.current.open("infra", "/w/infra", "a.yaml"); + }); + await waitFor(() => { + expect(result.current.visible).toHaveLength(1); + }); + const key = result.current.visible[0].key; + + act(() => { + result.current.setBlame(key, true); + }); + act(() => { + result.current.setBlame(key, false); + }); + + expect(result.current.visible[0].blame).toBe(false); + }); +}); + describe("tabsInChangedDirs", () => { const tab = (path: string, root = "/w/infra") => newTab(`k-${path}`, "infra", root, path, "yaml"); diff --git a/frontend/src/lib/useEditorTabs.ts b/frontend/src/lib/useEditorTabs.ts index 3c1cfcc..c8ca668 100644 --- a/frontend/src/lib/useEditorTabs.ts +++ b/frontend/src/lib/useEditorTabs.ts @@ -11,6 +11,7 @@ import { resolveTakeDisk, selectionAfterClose, tabsForProject, + withBlame, withEdit, withError, withExternalChange, @@ -55,6 +56,8 @@ export interface EditorTabs { readonly close: (key: string) => void; readonly closeProject: (project: string) => void; readonly setMode: (key: string, mode: EditorMode) => void; + /** Turns one tab's blame column on or off (#52). */ + readonly setBlame: (key: string, blame: boolean) => void; readonly keepMine: (key: string) => void; readonly takeDisk: (key: string) => void; } @@ -194,6 +197,10 @@ export function useEditorTabs( setTabs((current) => mapTab(current, key, (tab) => withMode(tab, mode))); }, []); + const setBlame = useCallback((key: string, blame: boolean) => { + setTabs((current) => mapTab(current, key, (tab) => withBlame(tab, blame))); + }, []); + const keepMine = useCallback((key: string) => { setTabs((current) => mapTab(current, key, resolveKeepMine)); }, []); @@ -261,6 +268,7 @@ export function useEditorTabs( close, closeProject, setMode, + setBlame, keepMine, takeDisk, }; diff --git a/frontend/src/lib/useFileTree.test.ts b/frontend/src/lib/useFileTree.test.ts index 251c2c3..92f4bac 100644 --- a/frontend/src/lib/useFileTree.test.ts +++ b/frontend/src/lib/useFileTree.test.ts @@ -262,6 +262,54 @@ describe("expanding a directory", () => { }); }); +describe("locating a file (#56)", () => { + function nested() { + return fakeDirectory({ + [ROOT]: [{ name: "manifests", isDir: true }], + manifests: [{ name: "prod", isDir: true }], + "manifests/prod": [{ name: "ingress.yaml", isDir: false }], + }); + } + + it("opens the directories above the file and selects it", async () => { + const directory = nested(); + const { result } = renderHook(() => useFileTree("/w/infra", null, directory)); + await waitFor(() => { + expect(result.current.state.dirs[ROOT]?.status).toBe("loaded"); + }); + + act(() => { + result.current.locate("manifests/prod/ingress.yaml"); + }); + + expect(result.current.state.expanded.has("manifests/prod")).toBe(true); + expect(result.current.state.selected).toBe("manifests/prod/ingress.yaml"); + await waitFor(() => { + expect(result.current.state.dirs["manifests/prod"]?.status).toBe("loaded"); + }); + }); + + // Listing a file is an error from the backend, and the row appears as soon + // as its own directory is listed — so the file must never be asked for. + it("never asks the backend to list the file", async () => { + const directory = nested(); + const { result } = renderHook(() => useFileTree("/w/infra", null, directory)); + await waitFor(() => { + expect(result.current.state.dirs[ROOT]?.status).toBe("loaded"); + }); + + act(() => { + result.current.locate("manifests/prod/ingress.yaml"); + }); + await waitFor(() => { + expect(result.current.state.dirs["manifests/prod"]?.status).toBe("loaded"); + }); + + const asked = directory.list.mock.calls.map(([, path]) => path); + expect(asked).not.toContain("manifests/prod/ingress.yaml"); + }); +}); + describe("revealing a directory (#43)", () => { /** A project three levels deep, with only its root listed so far. */ function nested() { diff --git a/frontend/src/lib/useFileTree.ts b/frontend/src/lib/useFileTree.ts index d8ea55b..486121c 100644 --- a/frontend/src/lib/useFileTree.ts +++ b/frontend/src/lib/useFileTree.ts @@ -14,6 +14,7 @@ import { initialTree, joinPath, parentPath, + locate, reveal, select, toggleChangedOnly, @@ -43,6 +44,10 @@ export interface FileTreeController { /** Opens a directory and everything above it, and selects it — what a * breadcrumb segment click does (#43). */ readonly reveal: (dir: string) => void; + /** Brings one file on screen and selects it (#56): the directories above it + * expand, the filters that would have hidden it clear, and the row scrolls + * into view once its listing lands. */ + readonly locate: (path: string) => void; readonly toggleHidden: () => void; readonly toggleChangedOnly: () => void; /** Resolves to an error message on failure, or null on success. */ @@ -183,6 +188,20 @@ export function useFileTree( [list], ); + const locateFile = useCallback( + (path: string) => { + setState((current) => locate(current, path)); + // The chain above the file, not the file: listing a file is a backend + // error, and the row appears as soon as its own directory is listed. + for (const dir of ancestry(parentPath(path))) { + if (!(dir in stateRef.current.dirs)) { + list(dir); + } + } + }, + [list], + ); + // /events: a coalesced batch names directories that may have changed // (PROTOCOL.md §5). Only directories this tree has already loaded are // worth re-fetching — see affectedTrackedDirs for why. @@ -267,6 +286,7 @@ export function useFileTree( setState((current) => select(current, path)); }, []), reveal: revealDir, + locate: locateFile, toggleHidden: useCallback(() => { setState((current) => toggleHidden(current)); }, []), diff --git a/frontend/src/lib/useGitOps.test.ts b/frontend/src/lib/useGitOps.test.ts index 093df58..b81b304 100644 --- a/frontend/src/lib/useGitOps.test.ts +++ b/frontend/src/lib/useGitOps.test.ts @@ -2,7 +2,7 @@ import { act, renderHook, waitFor } from "@testing-library/react"; import type { Mock } from "vitest"; import { describe, expect, it, vi } from "vitest"; import type { Git } from "./git"; -import { emptyStatus } from "./git"; +import { emptyBlame, emptyStatus } from "./git"; import { useGitOps } from "./useGitOps"; /** The seam with every member a spy, so a test can both assert on a call and @@ -20,6 +20,7 @@ type SpiedGit = { [K in keyof Git]: Mock }; function fakeGit(overrides: Partial = {}): SpiedGit { const seam: SpiedGit = { status: vi.fn(() => Promise.resolve(emptyStatus())), + blame: vi.fn(() => Promise.resolve(emptyBlame())), pull: vi.fn(() => Promise.resolve()), push: vi.fn(() => Promise.resolve()), checkout: vi.fn(() => Promise.resolve()), diff --git a/frontend/src/style.css b/frontend/src/style.css index b8baf4d..cd0fd9a 100644 --- a/frontend/src/style.css +++ b/frontend/src/style.css @@ -151,6 +151,36 @@ body { height: 100%; background: var(--m6t-bg); color: var(--m6t-fg); + /* Chrome is not a document: a tab, a tree row, a breadcrumb and a toolbar + label are controls that happen to be made of words, and none of them + should take a selection. WebKit selects the word under the pointer when a + context menu opens, so without this a right-click on a project tab + highlights its name. + + App-wide rather than per-component, because it was per-component and that + is how the project strip ended up as the one row nobody had covered. + Content opts back in below — that list is short and reviewable, which the + inverse never was. + + The prefix is load-bearing, not decoration. Unprefixed `user-select` only + reached WebKit in Safari 17, and m6t's window is a WKWebView on macOS and + a WebKitGTK one on Linux: on anything older the unprefixed property is + dropped and every rule here silently does nothing — while testing clean in + a Chromium browser, which supports both. */ + -webkit-user-select: none; + user-select: none; +} + +/* The three places that hold a document rather than chrome, plus the fields + the user types into. Selecting a line of YAML, a paragraph of rendered + markdown, or terminal output is the point of those panes. */ +.cm-editor, +.markdown, +.xterm, +input, +textarea { + -webkit-user-select: text; + user-select: text; } .shell__error { @@ -183,9 +213,6 @@ body { padding: 0; list-style: none; overflow-x: auto; - /* Dragging a tab across the strip would otherwise select the labels it - passes over, the same reason the terminal strip carries this. */ - user-select: none; } /* Square, edge-to-edge tabs marked by an accent rule rather than a rounded @@ -500,38 +527,22 @@ body { align-items: center; gap: var(--m6t-space-2); height: var(--m6t-row); - padding: 0 var(--m6t-space-3); + /* Half the inset the row used to carry: the controls now bring their own + padding, and the two together put the first icon back on the 8px the + header has always started its content at. It is also what keeps the row + inside the 180px sidebar minimum. */ + padding: 0 var(--m6t-space-2); border-bottom: 1px solid var(--m6t-border); color: var(--m6t-muted); font-size: var(--m6t-font-size-sm); } -.tree__header button { - height: 100%; - padding: 0 var(--m6t-space-2); - border: none; - border-radius: 0; - background: none; - color: inherit; - font: inherit; - font-size: var(--m6t-font-size-sm); -} - -/* The mode toggle is an icon and the dotfile toggle is words, so the icon - takes the square the other buttons' glyphs take rather than the width of a - label the header has no room for. */ -.tree__mode-toggle { - display: flex; - align-items: center; - margin-left: auto; -} - -.tree__mode-toggle[aria-pressed="true"] { - color: var(--m6t-accent); -} - -.tree__hidden-toggle { - white-space: nowrap; +/* The two act-once controls sit left, the two toggles right. A spacer rather + than `margin-left: auto` on the first toggle: the gap belongs to the row's + arrangement, and a control that carried it could not be reordered without + taking the layout with it. */ +.tree__header-gap { + flex: 1; } .tree__rows { @@ -560,7 +571,6 @@ body { padding-right: var(--m6t-space-2); padding-left: calc(var(--m6t-space-3) + var(--m6t-indent) * var(--depth, 0)); cursor: pointer; - user-select: none; white-space: nowrap; } @@ -700,7 +710,6 @@ button:hover { border-bottom: 1px solid var(--m6t-border); background: var(--m6t-panel); overflow-x: auto; - user-select: none; } .tab { @@ -1089,7 +1098,6 @@ button:hover { editor's top edge. */ overflow-x: auto; white-space: nowrap; - user-select: none; } .breadcrumb__crumb { @@ -1123,6 +1131,78 @@ button:hover { color: var(--m6t-muted); } +/* The active file's toolbar (#52), under the breadcrumb. It carries no border + of its own: the breadcrumb's bottom rule is directly above it, and a second + hairline one row down reads as a boxed strip rather than as chrome. + + Nothing insets the first control from the left. The row it has to agree with + is the editor below, not the breadcrumb above: the blame column starts hard + against the pane's left edge, and a control floating a few pixels in from it + reads as misplaced however carefully those pixels were chosen. The trailing + padding stays, for a control that ends up right-aligned in this row. */ +.view-toolbar { + display: flex; + flex: none; + align-items: center; + gap: var(--m6t-space-2); + height: var(--m6t-row); + padding: 0 var(--m6t-space-3) 0 0; + border-bottom: 1px solid var(--m6t-border); +} + +/* The first control's box starts on the pane's edge and its own padding insets + the glyph. Pulling the box further left instead would put the pressed fill + outside the pane it belongs to. */ + +/* ============================================================================= + Controls (#52, #54) + ============================================================================= + + The one shape for a piece of chrome the user presses: the editor toolbar's + toggles and the file tree's header. Defined once because the two rows sit + four pixels apart in the same window and had three looks between them before + this existed. See components/Control.tsx for the naming rule that goes with + it — the look is only half of what made those rows inconsistent. + + The padding is the control's own, not the row's. It is what the pressed box + is drawn around — a filled rectangle flush against the glyphs it contains + reads as a rendering fault rather than as a state — and it doubles the hit + target of an icon-only toggle. The row cancels it on the first control so + that the icon still starts on the row's own left edge; see `.view-toolbar`. + + No border, though: a border would move the label by a pixel when a toggle + goes on. The pressed outline is drawn inside instead. + ========================================================================== */ +.control { + display: flex; + align-items: center; + gap: var(--m6t-space-2); + height: var(--m6t-row); + padding: 0 var(--m6t-space-2); + border: none; + border-radius: var(--m6t-radius); + background: none; + color: var(--m6t-muted); + font: inherit; + font-size: var(--m6t-font-size-sm); + white-space: nowrap; + cursor: pointer; +} + +.control:hover, +.control:focus-visible { + color: var(--m6t-fg); +} + +/* A pressed toggle is a state, not a hover: it keeps its fill while the + pointer is elsewhere, which is what tells a user the column they are looking + at is one they turned on. */ +.control--on { + box-shadow: inset 0 0 0 1px var(--m6t-border); + background: var(--m6t-panel-alt); + color: var(--m6t-fg); +} + /* ============================================================================= Markdown preview ============================================================================= diff --git a/frontend/src/style.test.ts b/frontend/src/style.test.ts index 31ff5a2..dcc19f4 100644 --- a/frontend/src/style.test.ts +++ b/frontend/src/style.test.ts @@ -182,6 +182,24 @@ describe("the style tokens", () => { ).toEqual([]); }); + // A property whose unprefixed form WebKit only learned in Safari 17. Every + // `user-select` in this file was unprefixed, which meant the whole app's + // chrome was selectable in the window it actually ships in while testing + // clean in a Chromium browser — the failure mode a gate is for. + it("pairs every user-select with its WebKit prefix", () => { + const plain = (css.match(/(^|[;{])\s*user-select\s*:/g) ?? []).length; + const prefixed = (css.match(/-webkit-user-select\s*:/g) ?? []).length; + + expect( + plain, + "every `user-select` needs a `-webkit-user-select` beside it: the window " + + "is a WKWebView on macOS and a WebKitGTK one on Linux, and neither " + + "honours the unprefixed property before Safari 17.", + ).toBe(prefixed); + // The premise: if the declarations vanish, the equality above is 0 === 0. + expect(plain).toBeGreaterThan(0); + }); + // The gate is worthless if its matcher does not fire. These are the exact // shapes the old stylesheet was full of. it("catches the shapes it was written for", () => { diff --git a/frontend/wailsjs/go/app/App.d.ts b/frontend/wailsjs/go/app/App.d.ts index 5c18f4b..94927f9 100755 --- a/frontend/wailsjs/go/app/App.d.ts +++ b/frontend/wailsjs/go/app/App.d.ts @@ -14,6 +14,8 @@ export function CreateEntry(arg1:string,arg2:string,arg3:boolean):Promise; export function DeleteEntry(arg1:string,arg2:string):Promise; +export function GitBlame(arg1:string,arg2:string):Promise; + export function GitBranches(arg1:string):Promise>; export function GitCheckout(arg1:string,arg2:string):Promise; diff --git a/frontend/wailsjs/go/app/App.js b/frontend/wailsjs/go/app/App.js index c216596..88d5afb 100755 --- a/frontend/wailsjs/go/app/App.js +++ b/frontend/wailsjs/go/app/App.js @@ -18,6 +18,10 @@ export function DeleteEntry(arg1, arg2) { return window['go']['app']['App']['DeleteEntry'](arg1, arg2); } +export function GitBlame(arg1, arg2) { + return window['go']['app']['App']['GitBlame'](arg1, arg2); +} + export function GitBranches(arg1) { return window['go']['app']['App']['GitBranches'](arg1); } diff --git a/frontend/wailsjs/go/models.ts b/frontend/wailsjs/go/models.ts index 687f04e..96496c3 100755 --- a/frontend/wailsjs/go/models.ts +++ b/frontend/wailsjs/go/models.ts @@ -21,6 +21,59 @@ export namespace buildinfo { export namespace git { + export class BlameCommit { + sha: string; + author: string; + authorTime: number; + summary: string; + uncommitted: boolean; + + static createFrom(source: any = {}) { + return new BlameCommit(source); + } + + constructor(source: any = {}) { + if ('string' === typeof source) source = JSON.parse(source); + this.sha = source["sha"]; + this.author = source["author"]; + this.authorTime = source["authorTime"]; + this.summary = source["summary"]; + this.uncommitted = source["uncommitted"]; + } + } + export class Blame { + commits: BlameCommit[]; + lines: number[]; + + static createFrom(source: any = {}) { + return new Blame(source); + } + + constructor(source: any = {}) { + if ('string' === typeof source) source = JSON.parse(source); + this.commits = this.convertValues(source["commits"], BlameCommit); + this.lines = source["lines"]; + } + + convertValues(a: any, classs: any, asMap: boolean = false): any { + if (!a) { + return a; + } + if (a.slice && a.map) { + return (a as any[]).map(elem => this.convertValues(elem, classs)); + } else if ("object" === typeof a) { + if (asMap) { + for (const key of Object.keys(a)) { + a[key] = new classs(a[key]); + } + return a; + } + return new classs(a); + } + return a; + } + } + export class Branch { name: string; upstream: string; diff --git a/godobject_budget_test.go b/godobject_budget_test.go index ef5c224..11cbd64 100644 --- a/godobject_budget_test.go +++ b/godobject_budget_test.go @@ -183,7 +183,21 @@ const ( // frontend rewrite every path and kube binding in the same call; this one // takes names, must name exactly the registered set, and cannot change // anything else about a project. - maxAppMethods = 22 + // + // 22 -> 23 in #52. One binding, GitBlame. It passes the #9 test — the + // editor asks about one file when the user turns the column on, gets its + // attribution back, and is done — and it takes no field, because + // internal/git still keeps nothing between calls. + // + // What is worth stating is why it is not part of GitStatus, which is the + // binding it most resembles. A status is read for a whole project on every + // filesystem event the watcher publishes; a blame is one subprocess per + // file, wanted only while a column is on. Folding the blame into the + // status would run `git blame` on every open file every time any of them + // was saved, for a column nobody asked to see — the same cost the #38 + // paragraph above refused to fold into ListDirectory, arriving from the + // other direction. + maxAppMethods = 23 // appCoordinatorType is the struct these ceilings bound. appCoordinatorType = "App" diff --git a/internal/app/git.go b/internal/app/git.go index 7831513..b746915 100644 --- a/internal/app/git.go +++ b/internal/app/git.go @@ -30,6 +30,23 @@ func (*App) GitStatus(root string) (git.Status, error) { return status, nil } +// GitBlame attributes each line of one file to the commit that last touched +// it (#52), for the editor's blame column. relPath is root-relative and +// slash-separated, the form ReadFile and the file tree already use. +// +// It is a second reader beside GitStatus rather than part of it: a status is +// read for a whole project on every filesystem event, and a blame is read for +// one file only while a user has the column turned on. Folding a per-file +// subprocess into the event-driven call would run `git blame` on every save of +// every file, for a column nobody asked to see. +func (*App) GitBlame(root, relPath string) (git.Blame, error) { + blame, err := git.LoadBlame(root, relPath) + if err != nil { + return git.Blame{}, fmt.Errorf("blaming %s in %s: %w", relPath, root, err) + } + return blame, nil +} + // The mutating half of DESIGN.md §7. Each of these is one operation the // branch bar puts a control on, and each is a thin pass through to // internal/git — the binding layer composes services, it does not implement diff --git a/internal/app/git_test.go b/internal/app/git_test.go index 1ffd88d..d99f31f 100644 --- a/internal/app/git_test.go +++ b/internal/app/git_test.go @@ -140,6 +140,40 @@ func TestGitRemotesReportsWhatIsConfigured(t *testing.T) { } } +func TestGitBlameAttributesLinesThroughTheBinding(t *testing.T) { + a := testApp(t) + dir := gitRepoDir(t) + writeManifest(t, dir, "deploy.yaml", "kind: Deployment\n") + runRepoGit(t, dir, "add", "-A") + runRepoGit(t, dir, "commit", "-qm", "first") + + blame, err := a.GitBlame(dir, "deploy.yaml") + if err != nil { + t.Fatalf("GitBlame: %v", err) + } + if len(blame.Lines) != 1 { + t.Fatalf("lines = %v, want one", blame.Lines) + } + if got := blame.Commits[blame.Lines[0]].Author; got != "m6t tests" { + t.Errorf("author = %q, want the fixture's committer", got) + } +} + +// A path the file tree would never emit is refused before git runs, and the +// refusal names the project it was refused in like every other binding here. +func TestGitBlameRefusesAPathOutsideTheProject(t *testing.T) { + a := testApp(t) + dir := gitRepoDir(t) + + _, err := a.GitBlame(dir, "../escape.yaml") + if !errors.Is(err, git.ErrInvalidPath) { + t.Fatalf("GitBlame(../escape.yaml) = %v, want ErrInvalidPath", err) + } + if !strings.Contains(err.Error(), dir) { + t.Errorf("error = %q, want it to name the project path", err) + } +} + // A failing operation reaches the frontend with git's own words in it. The // binding wraps; it does not summarize (DESIGN.md §7). func TestGitOperationsSurfaceGitsOwnWords(t *testing.T) { diff --git a/internal/git/blame.go b/internal/git/blame.go new file mode 100644 index 0000000..2f24449 --- /dev/null +++ b/internal/git/blame.go @@ -0,0 +1,286 @@ +package git + +import ( + "errors" + "fmt" + "path/filepath" + "slices" + "strconv" + "strings" +) + +// Per-line attribution (#52): who last touched each line of one file, and +// when. It is a reader like status.go, and for the same reason it is safe to +// run whenever the editor asks — `git blame` writes nothing. +// +// What it reads is the file *on disk*, not the editor's buffer. That is not a +// limitation to work around: blame's answer is a line number, and a buffer +// with an unsaved insertion in it has different line numbers from the file +// git measured. The UI clears the column rather than shifting it (#52), and +// this package is the reason it can — it reports what git said, and nothing +// here guesses at what an unsaved edit would have done to it. + +// ErrInvalidPath is a path that would address something outside the project's +// worktree. +// +// Like ErrInvalidRef it is a rejection rather than a git failure: no +// subprocess runs. It exists because the bound surface is a public API +// (CLAUDE.md), and a caller that is not the file tree can hand this package a +// path the file tree would never produce. +var ErrInvalidPath = errors.New("not a path inside the project") + +// The porcelain header keys this package reads. The format carries the +// committer's name and time as well, and neither is read: a blame column +// answers "who wrote this line", and a rebase or a patch applied on someone's +// behalf makes the committer a different person from the one who did. +const ( + blameAuthor = "author" + blameAuthorTime = "author-time" + blameSummary = "summary" +) + +// blameHeaderFields is the shortest group header porcelain emits: +// ` `. The first line of a group carries a fourth +// field, the number of lines in it, which this package does not need — every +// line in the group gets its own header anyway. +const blameHeaderFields = 3 + +// How a porcelain timestamp is read: base ten, into the width AuthorTime is +// declared at. They are named because revive counts a bare `10, 64` as two +// magic numbers, and it is right that a base and a bit width read alike at a +// call site. Sixty-four rather than the platform int: a 32-bit build would +// otherwise stop parsing author times in 2038. +const ( + decimalBase = 10 + timestampBits = 64 +) + +// unattributed is the commit index of a line the blame did not cover. See +// (*blameParser).close for when that can happen, which is: not through git. +const unattributed = -1 + +// BlameCommit is one commit that a file's lines are attributed to. +type BlameCommit struct { + // SHA is the full object name, as git printed it. + SHA string `json:"sha"` + + // Author is the name on the commit, "" when git reported none. + Author string `json:"author"` + + // AuthorTime is when the author wrote it, in Unix seconds. The zone git + // also reports is dropped: the column shows a date, and a date rendered in + // the reader's own zone is the one they can compare against today. + AuthorTime int64 `json:"authorTime"` + + // Summary is the commit's subject line. + Summary string `json:"summary"` + + // Uncommitted marks git's all-zero SHA — the one it attributes work that + // is in the working tree and in no commit. Such a "commit" has no author + // and no date worth showing, so the UI marks the line instead of naming + // someone. + Uncommitted bool `json:"uncommitted"` +} + +// Blame is one file's attribution, in the shape git's porcelain format states +// it: the commits once each, and a line-by-line reference into them. +type Blame struct { + // Commits is each distinct commit, in the order its first line appeared. + // Never nil: it crosses the bridge as JSON. + Commits []BlameCommit `json:"commits"` + + // Lines is one index into Commits per line of the file, line 1 first. A + // line the blame did not cover holds -1, which is not reachable through + // git — see (*blameParser).close. + Lines []int `json:"lines"` +} + +// LoadBlame attributes each line of a file to the commit that last touched it. +// +// path is root-relative and slash-separated, the form the file tree and the +// editor already use. A path git cannot blame — one that has never been +// committed, one it is ignoring — is an error carrying git's own stderr, not +// an empty blame: "no attribution" and "this file has no history" read +// identically as a blank column, and only one of them is worth showing the +// user a reason for. +func LoadBlame(root, path string) (Blame, error) { + if err := validatePath(path); err != nil { + return Blame{}, err + } + // The `--` is what makes the argument a path rather than a revision. It is + // also why a file whose name begins with a dash needs no rejection below: + // after the separator, `-f.yaml` is a file. + out, err := runGit(root, "blame", "--porcelain", "--", path) + if err != nil { + return Blame{}, err + } + return parseBlame(out), nil +} + +// validatePath rejects a path that would address something outside the +// project's worktree. +// +// git runs with `-C root` and would refuse an escaping pathspec itself, with a +// message about being outside the repository. This runs first anyway: the +// check is one string scan against a subprocess, and "git happened to say no" +// is a weaker guarantee than "we never asked it". +// +// A backslash is treated as a separator on every platform, so `..\..\etc` is +// refused on Linux too. That costs the ability to blame a file with a +// backslash in its name on a Unix filesystem, which is a name no manifest +// repository has and not a trade worth reversing. +func validatePath(path string) error { + if path == "" || strings.ContainsRune(path, 0) { + return fmt.Errorf(rejectedFormat, path, ErrInvalidPath) + } + if strings.HasPrefix(path, "/") || filepath.IsAbs(path) { + return fmt.Errorf(rejectedFormat, path, ErrInvalidPath) + } + if slices.Contains(strings.FieldsFunc(path, isPathSeparator), "..") { + return fmt.Errorf(rejectedFormat, path, ErrInvalidPath) + } + return nil +} + +func isPathSeparator(r rune) bool { + return r == '/' || r == '\\' +} + +// parseBlame reads `git blame --porcelain`. +// +// The format states a commit's details once, on the first line attributed to +// it, and refers back to it by SHA on every line after. This keeps that shape +// rather than flattening it: a 5,000-line file that came from one commit is +// one commit record and 5,000 indices, not 5,000 copies of an author and a +// timestamp crossing the bridge. +// +// A malformed record is skipped rather than failing the read, the same rule +// status.go's parse documents: a column is a view, and one line this version +// cannot read should cost that line's entry, not the file's. +func parseBlame(out string) Blame { + parser := blameParser{ + index: map[string]int{}, + commits: []BlameCommit{}, + lines: []int{}, + // The file cannot have more lines than the output does: every line of + // it costs at least the tab-prefixed record carrying its content. That + // makes this a bound derived from the input rather than a number + // somebody picked, and it is what stops a line number the parser reads + // out of a malformed record from sizing an allocation. See close. + limit: strings.Count(out, "\n") + 1, + } + for line := range strings.SplitSeq(out, "\n") { + parser.read(line) + } + return Blame{Commits: parser.commits, Lines: parser.lines} +} + +// blameParser accumulates one file's blame across the format's three kinds of +// line: a group header that opens an entry, extended headers describing its +// commit, and the file's own content, tab-prefixed, that closes it. +type blameParser struct { + index map[string]int + commits []BlameCommit + lines []int + + // limit is the highest line number this blame can be about, derived in + // parseBlame from the size of the output. + limit int + + // sha and line are the entry currently open; inEntry says whether one is, + // which is what tells a `summary ...` header apart from the group header + // that opens the next entry — both are ` `. + sha string + line int + inEntry bool +} + +func (p *blameParser) read(raw string) { + switch { + case strings.HasPrefix(raw, "\t"): + p.close() + case !p.inEntry: + p.open(raw) + default: + p.header(raw) + } +} + +// open reads a group header, ` []`, and +// registers its commit the first time that SHA appears. +func (p *blameParser) open(raw string) { + fields := strings.Fields(raw) + if len(fields) < blameHeaderFields { + return + } + final, err := strconv.Atoi(fields[2]) + if err != nil || final < 1 { + return + } + p.sha, p.line, p.inEntry = fields[0], final, true + if _, seen := p.index[p.sha]; !seen { + p.index[p.sha] = len(p.commits) + p.commits = append(p.commits, BlameCommit{ + SHA: p.sha, + Uncommitted: uncommitted(p.sha), + }) + } +} + +// header folds one `key value` record into the open entry's commit. +func (p *blameParser) header(raw string) { + key, value, ok := strings.Cut(raw, fieldSeparator) + if !ok { + return + } + commit := &p.commits[p.index[p.sha]] + switch key { + case blameAuthor: + commit.Author = value + case blameAuthorTime: + // A timestamp git could not print as a number leaves the zero value, + // which the UI shows as no date. The author and the summary are still + // worth showing without it. + if seconds, err := strconv.ParseInt(value, decimalBase, timestampBits); err == nil { + commit.AuthorTime = seconds + } + case blameSummary: + commit.Summary = value + } +} + +// close records the attribution of the line whose content just arrived. +// +// The entry is placed at the line number git gave it rather than appended, and +// any line the output skipped keeps `unattributed`. Neither case is reachable +// through git — porcelain emits every line of the file, in order — and that is +// exactly why it is worth placing rather than appending: if the output ever +// does skip a line, one blank entry is a better failure than every attribution +// below it being off by one. +func (p *blameParser) close() { + if !p.inEntry { + return + } + // A line number past what the output could be describing is a malformed + // record, and the record is dropped rather than believed: the alternative + // is growing the slice to whatever number was parsed, which turns one bad + // digit into an allocation the size of that digit. + if p.line > p.limit { + p.inEntry = false + return + } + for len(p.lines) < p.line { + p.lines = append(p.lines, unattributed) + } + p.lines[p.line-1] = p.index[p.sha] + p.inEntry = false +} + +// uncommitted reports git's all-zero SHA, which it attributes lines that are +// in the working tree and in no commit. +// +// It is matched by shape rather than against a constant of forty zeros because +// a repository created with SHA-256 object names writes sixty-four of them. +func uncommitted(sha string) bool { + return sha != "" && strings.Trim(sha, "0") == "" +} diff --git a/internal/git/blame_test.go b/internal/git/blame_test.go new file mode 100644 index 0000000..c7a204e --- /dev/null +++ b/internal/git/blame_test.go @@ -0,0 +1,330 @@ +package git + +import ( + "errors" + "strconv" + "strings" + "testing" +) + +// The fixture tests here drive the real git binary, for the reason +// run_test.go states: what is under test is whether the argv this package +// builds is one git accepts and answers. A stub would agree with a wrong flag. +// The parser tests below them use captured output, because the shapes they +// cover — a truncated record, a SHA-256 zero SHA — are ones a fixture cannot +// be made to produce on demand. + +// blameOf reads the blame these tests assert against, failing the test rather +// than returning an error nobody would check. +func blameOf(t *testing.T, dir, path string) Blame { + t.Helper() + blame, err := LoadBlame(dir, path) + if err != nil { + t.Fatalf("LoadBlame(%q): %v", path, err) + } + return blame +} + +// commitAt is the commit a line is attributed to, by 1-based line number. +func commitAt(t *testing.T, blame Blame, line int) BlameCommit { + t.Helper() + if line < 1 || line > len(blame.Lines) { + t.Fatalf("line %d is outside the blame's %d lines", line, len(blame.Lines)) + } + index := blame.Lines[line-1] + if index < 0 || index >= len(blame.Commits) { + t.Fatalf("line %d has commit index %d, outside the %d commits", line, index, len(blame.Commits)) + } + return blame.Commits[index] +} + +// The acceptance criterion: each line goes to the author of the commit that +// last touched it, not to the author of the file. +func TestLoadBlameAttributesLinesToTheAuthorWhoLastTouchedThem(t *testing.T) { + dir := initRepo(t) + writeFixtureFile(t, dir, "a.yaml", "first\nsecond\nthird\n") + runFixtureGit(t, dir, "add", "-A") + runFixtureGit(t, dir, "commit", "-qm", "one") + + writeFixtureFile(t, dir, "a.yaml", "first\nchanged\nthird\n") + runFixtureGit(t, dir, "add", "-A") + runFixtureGit(t, dir, + "-c", "user.name=Second Author", "-c", "user.email=second@example.invalid", + "commit", "-qm", "two") + + blame := blameOf(t, dir, "a.yaml") + + if len(blame.Lines) != 3 { + t.Fatalf("lines = %d, want 3", len(blame.Lines)) + } + if got := commitAt(t, blame, 1).Author; got != "m6t tests" { + t.Errorf("line 1 author = %q, want the first commit's", got) + } + if got := commitAt(t, blame, 2).Author; got != "Second Author" { + t.Errorf("line 2 author = %q, want the second commit's", got) + } + if got := commitAt(t, blame, 3).Author; got != "m6t tests" { + t.Errorf("line 3 author = %q, want the first commit's", got) + } + if got := commitAt(t, blame, 2).Summary; got != "two" { + t.Errorf("line 2 summary = %q, want %q", got, "two") + } + if commitAt(t, blame, 2).AuthorTime <= 0 { + t.Error("line 2 has no author time") + } +} + +// A line edited on disk but not committed belongs to nobody. Attributing it to +// whoever wrote the line it replaced is the failure this guards: it would name +// a person for text they never wrote. +func TestLoadBlameMarksUncommittedLines(t *testing.T) { + dir := commitFixture(t) + writeFixtureFile(t, dir, "a.yaml", "one\nadded here\n") + + blame := blameOf(t, dir, "a.yaml") + + if got := commitAt(t, blame, 1); got.Uncommitted { + t.Errorf("line 1 = %+v, want the committed line attributed", got) + } + second := commitAt(t, blame, 2) + if !second.Uncommitted { + t.Errorf("line 2 = %+v, want it marked uncommitted", second) + } + if strings.Trim(second.SHA, "0") != "" { + t.Errorf("uncommitted SHA = %q, want git's all-zero name", second.SHA) + } +} + +// The wire form's whole point: one commit record however many lines it wrote. +func TestLoadBlameStatesEachCommitOnce(t *testing.T) { + dir := initRepo(t) + writeFixtureFile(t, dir, "a.yaml", strings.Repeat("line\n", 50)) + runFixtureGit(t, dir, "add", "-A") + runFixtureGit(t, dir, "commit", "-qm", "one") + + blame := blameOf(t, dir, "a.yaml") + + if len(blame.Lines) != 50 { + t.Fatalf("lines = %d, want 50", len(blame.Lines)) + } + if len(blame.Commits) != 1 { + t.Errorf("commits = %d, want 1 for a file from one commit", len(blame.Commits)) + } +} + +// A path git will not blame has to say why. An empty blame would render as a +// blank column, which is also what a file with no history looks like. +func TestLoadBlameReportsGitsRefusal(t *testing.T) { + dir := commitFixture(t) + writeFixtureFile(t, dir, "untracked.yaml", "one\n") + + _, err := LoadBlame(dir, "untracked.yaml") + if err == nil { + t.Fatal("LoadBlame on an untracked path succeeded; want git's refusal") + } + if !strings.Contains(err.Error(), "untracked.yaml") { + t.Errorf("error = %v, want git's own message about the path", err) + } +} + +// An empty file has no lines to attribute, and the slices still have to be +// slices: they cross the bridge as JSON, where nil is null. +func TestLoadBlameOnAnEmptyFileIsEmptyRatherThanNil(t *testing.T) { + dir := initRepo(t) + writeFixtureFile(t, dir, "empty.yaml", "") + runFixtureGit(t, dir, "add", "-A") + runFixtureGit(t, dir, "commit", "-qm", "one") + + blame := blameOf(t, dir, "empty.yaml") + + if blame.Lines == nil || blame.Commits == nil { + t.Fatalf("blame = %+v; nil marshals to null rather than []", blame) + } + if len(blame.Lines) != 0 { + t.Errorf("lines = %d, want none", len(blame.Lines)) + } +} + +func TestLoadBlameRejectsPathsOutsideTheProject(t *testing.T) { + dir := commitFixture(t) + + for _, path := range []string{ + "", + "../escape.yaml", + "nested/../../escape.yaml", + "..\\escape.yaml", + "/etc/passwd", + "a\x00.yaml", + } { + if _, err := LoadBlame(dir, path); !errors.Is(err, ErrInvalidPath) { + t.Errorf("LoadBlame(%q) = %v, want ErrInvalidPath", path, err) + } + } +} + +// A leading dash is a file name, not an option: LoadBlame puts `--` in front +// of the path. Rejecting it would refuse a file git is perfectly able to blame. +func TestLoadBlameBlamesAFileNamedLikeAnOption(t *testing.T) { + dir := initRepo(t) + writeFixtureFile(t, dir, "-f.yaml", "one\n") + runFixtureGit(t, dir, "add", "-A") + runFixtureGit(t, dir, "commit", "-qm", "one") + + blame := blameOf(t, dir, "-f.yaml") + + if len(blame.Lines) != 1 { + t.Fatalf("lines = %d, want 1", len(blame.Lines)) + } + if commitAt(t, blame, 1).Summary != "one" { + t.Errorf("summary = %q, want %q", commitAt(t, blame, 1).Summary, "one") + } +} + +// A subdirectory path arrives slash-separated from the tree and has to survive +// the separator check that rejects an escape. +func TestLoadBlameBlamesAPathInASubdirectory(t *testing.T) { + dir := initRepo(t) + writeFixtureFile(t, dir, "deploy/base/svc.yaml", "kind: Service\n") + runFixtureGit(t, dir, "add", "-A") + runFixtureGit(t, dir, "commit", "-qm", "one") + + blame := blameOf(t, dir, "deploy/base/svc.yaml") + + if len(blame.Lines) != 1 { + t.Fatalf("lines = %d, want 1", len(blame.Lines)) + } +} + +// A SHA-256 repository writes sixty-four zeros for an uncommitted line, so a +// forty-zero constant would attribute those lines to a commit named entirely +// with zeros. +func TestParseBlameMarksASha256ZeroName(t *testing.T) { + sha := strings.Repeat("0", 64) + blame := parseBlame(sha + " 1 1 1\nauthor Not Committed Yet\nsummary x\n\tone\n") + + if len(blame.Commits) != 1 { + t.Fatalf("commits = %d, want 1", len(blame.Commits)) + } + if !blame.Commits[0].Uncommitted { + t.Errorf("commit = %+v, want it marked uncommitted", blame.Commits[0]) + } +} + +// A real SHA is not a zero SHA, however few non-zero digits it has. +func TestParseBlameKeepsACommitWithLeadingZeros(t *testing.T) { + sha := strings.Repeat("0", 39) + "1" + blame := parseBlame(sha + " 1 1 1\nauthor Someone\n\tone\n") + + if blame.Commits[0].Uncommitted { + t.Errorf("commit = %+v, want it treated as a real commit", blame.Commits[0]) + } +} + +// A record this parser cannot read costs its own line, not the file's — the +// rule status.go's parse already documents. +func TestParseBlameSkipsUnreadableRecords(t *testing.T) { + blame := parseBlame("truncated\nabc 1 1 1\nauthor Someone\n\tone\n") + + if len(blame.Lines) != 1 { + t.Fatalf("lines = %v, want the one readable entry", blame.Lines) + } + if blame.Commits[blame.Lines[0]].Author != "Someone" { + t.Errorf("author = %q, want the readable record's", blame.Commits[blame.Lines[0]].Author) + } +} + +// A line number git skipped leaves one blank entry. The alternative — appending +// in arrival order — would shift every attribution below the gap by one, which +// is a wrong answer rather than a missing one. +func TestParseBlameLeavesASkippedLineUnattributed(t *testing.T) { + blame := parseBlame("abc 1 1 1\nauthor Someone\n\tone\nabc 3 3 1\n\tthree\n") + + if len(blame.Lines) != 3 { + t.Fatalf("lines = %v, want three entries", blame.Lines) + } + if blame.Lines[1] != unattributed { + t.Errorf("line 2 = %d, want %d", blame.Lines[1], unattributed) + } + if blame.Lines[0] != blame.Lines[2] { + t.Errorf("lines = %v, want line 3 attributed to the same commit as line 1", blame.Lines) + } +} + +// A group header whose line number is not one is a malformed record, and it +// opens no entry: believing it would put an entry at a line the file has no +// content for. +func TestParseBlameSkipsAHeaderWithoutAUsableLineNumber(t *testing.T) { + for _, header := range []string{"abc 1 zero 1", "abc 1 0 1", "abc 1 -2 1"} { + blame := parseBlame(header + "\nauthor Someone\n\tone\n") + + if len(blame.Lines) != 0 { + t.Errorf("parseBlame(%q) lines = %v, want none", header, blame.Lines) + } + } +} + +// A content line arriving with no entry open is a record out of order. It is +// dropped rather than attributed to whatever entry closed before it. +func TestParseBlameIgnoresContentWithNoEntryOpen(t *testing.T) { + blame := parseBlame("\tstray\nabc 1 1 1\nauthor Someone\n\tone\n") + + if len(blame.Lines) != 1 { + t.Fatalf("lines = %v, want the one real entry", blame.Lines) + } + if blame.Commits[blame.Lines[0]].Author != "Someone" { + t.Errorf("author = %q, want the real entry's", blame.Commits[blame.Lines[0]].Author) + } +} + +// A line number the output could not be describing is a malformed record. It +// is dropped rather than believed: growing the slice to whatever number was +// parsed turns one bad digit into an allocation the size of that digit. +func TestParseBlameDropsALineNumberBeyondTheOutput(t *testing.T) { + blame := parseBlame("abc 1 2147483000 1\nauthor Someone\n\tone\n") + + if len(blame.Lines) != 0 { + t.Fatalf("lines = %d, want none for a record naming an impossible line", len(blame.Lines)) + } +} + +// The bound must not reject a real blame. One record per line plus the headers +// leaves it far above any line number git will print for the file. +func TestParseBlameKeepsEveryLineOfARealisticBlame(t *testing.T) { + var out strings.Builder + const lines = 200 + for i := 1; i <= lines; i++ { + out.WriteString("abc " + strconv.Itoa(i) + " " + strconv.Itoa(i) + " 1\n\tcontent\n") + } + + blame := parseBlame(out.String()) + + if len(blame.Lines) != lines { + t.Fatalf("lines = %d, want %d", len(blame.Lines), lines) + } +} + +// A timestamp git could not print as a number costs the date, not the entry. +func TestParseBlameKeepsACommitWithAnUnreadableTime(t *testing.T) { + blame := parseBlame("abc 1 1 1\nauthor Someone\nauthor-time later\nsummary x\n\tone\n") + + commit := blame.Commits[0] + if commit.AuthorTime != 0 { + t.Errorf("authorTime = %d, want 0", commit.AuthorTime) + } + if commit.Author != "Someone" || commit.Summary != "x" { + t.Errorf("commit = %+v, want the readable fields kept", commit) + } +} + +// A summary is a sentence, and its first word is not a header key. This is why +// the parser tracks whether an entry is open rather than matching on shape. +func TestParseBlameKeepsAWholeSummaryLine(t *testing.T) { + blame := parseBlame("abc 1 1 1\nsummary author of the change\n\tone\n") + + if got := blame.Commits[0].Summary; got != "author of the change" { + t.Errorf("summary = %q, want the whole line", got) + } + if got := blame.Commits[0].Author; got != "" { + t.Errorf("author = %q, want none — the summary is not an author header", got) + } +} diff --git a/package_budget_test.go b/package_budget_test.go index 01a0c78..433247a 100644 --- a/package_budget_test.go +++ b/package_budget_test.go @@ -55,8 +55,9 @@ var structuralPins = map[string]packagePin{ // they are unreachable code. Both figures ratchet down rather than // standing still, because a ceiling left where a package used to be // is room for the next thing to move in unnoticed. - loc: 900, exported: 21, - why: "git service: both halves of DESIGN.md §7 over the system git — porcelain v2 status with its two degraded states reported as values rather than errors, and the writes the terminal is a bad place for (pull, push, branch switch)", + // 900 -> 1100 and 21 -> 25 in #52. See locCeilingNote. + loc: 1100, exported: 25, + why: "git service: DESIGN.md §7 over the system git — porcelain v2 status with its two degraded states reported as values rather than errors, porcelain blame for the editor's per-line attribution, and the writes the terminal is a bad place for (pull, push, branch switch)", }, "internal/buildinfo": { loc: 150, exported: 2, @@ -235,6 +236,37 @@ var structuralPins = map[string]packagePin{ // match with errors.Is — the bound surface is a public API, so its refusals // are part of the contract rather than strings to compare. // +// 900 -> 1100 and 21 -> 25 in #52, and the paragraph above named this exact +// case as one that should go the other way, so it has to be answered rather +// than quietly overruled. What it said is that a reader parsing a different +// format for a different consumer, sharing only the binary, should land as +// its own package. blame.go is that reader — porcelain blame, for the +// editor's gutter rather than the tree's badges — and it did not land as one. +// +// The reason is the sentence before it, which turns out to bind harder than +// the sentence after: a sibling package cannot reach runGit, so +// internal/blame only compiles by extracting the runner into a second +// dependency-root package beside internal/buildinfo. The runner is not +// incidental here. It is what pins LC_ALL so the not-a-repository match keeps +// working, what passes --no-optional-locks so a read does not publish a +// change event that triggers another read, what bounds a call against a +// hung network mount, and what carries git's stderr out verbatim. A blame +// package that duplicated any of that would be a second answer to a question +// this repository has already answered once; one that imported it would need +// the layering rule loosened for a package with a single consumer. +// +// So the split #35 was told to make is real, and the seam is not where that +// paragraph put it. It is the runner, not the parser — and extracting it is a +// refactor of #8 and #9's code with its own blast radius, which is not +// something to do inside a ticket about a column in the editor. It is #53, +// which blocks #35, rather than something done here. +// +// 1072 is today's actual and 1100 is the same slim follow-up room the rest of +// this table carries — deliberately not enough for #35 to move in under. The +// surface goes to 25 with the usual zero slack: Blame and BlameCommit cross +// the bridge, LoadBlame is the operation, and ErrInvalidPath is a refusal a +// caller matches with errors.Is, the same reason ErrInvalidRef is exported. +// // 620 -> 700 in #9. git.go gains the mutating half of the git service: eight // delegating bindings, each four lines because each names its own operation // and project in front of what internal/git returned, the way GitStatus