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