diff --git a/packages/overlay/src/app.ts b/packages/overlay/src/app.ts index a3341bb..2a1bfdf 100644 --- a/packages/overlay/src/app.ts +++ b/packages/overlay/src/app.ts @@ -7,6 +7,7 @@ import { type CreateJobRequest, type Editor, type ElementContext, + type GitHealth, type ImageInput, type JobDiffBundle, type JobHistorySummary, @@ -631,6 +632,14 @@ export class AirshipApp { private redoBtn!: HTMLButtonElement; private tooltips: Tooltips | null = null; + /** + * Whether the daemon's git works, so the turn menu can grey its git verbs + * with the reason attached. Undefined until the first `git:health` arrives, + * which reads as healthy — an older daemon never sends one, and hiding a + * working Commit is worse than offering one that might fail. + */ + private gitHealth: GitHealth | undefined; + private selected: Selection | null = null; private images: ImageInput[] = []; private activeJobId: string | null = null; @@ -4027,6 +4036,12 @@ export class AirshipApp { this.previewKey = ""; this.schedulePreview(); break; + case "git:health": + // Stored, not rendered: the turn menu is built on open, so the next + // time it opens it reads this. Nothing on screen depends on it until + // then, which is why there is no repaint here. + this.gitHealth = ev.health; + break; case "job:created": if (this.awaiting && !this.activeJobId) { this.activeJobId = ev.job.jobId; @@ -4238,6 +4253,7 @@ export class AirshipApp { private assistantActions(bundle: JobDiffBundle): AssistantActions { const canRevert = bundle.status === "done" && bundle.diffs.length > 0; return { + git: this.gitHealth, onBranch: bundle.status === "done" ? () => this.branch(bundle.jobId) : undefined, onComment: canRevert diff --git a/packages/overlay/src/chat/transcript.ts b/packages/overlay/src/chat/transcript.ts index c06458e..a0252dc 100644 --- a/packages/overlay/src/chat/transcript.ts +++ b/packages/overlay/src/chat/transcript.ts @@ -1,4 +1,4 @@ -import type { Editor, JobDiffBundle } from "@airship/protocol"; +import type { Editor, GitHealth, JobDiffBundle } from "@airship/protocol"; import { firstHunkLine, renderDiff, selectedLineRange } from "../diff-view"; import { clear, cls, el } from "../dom"; import { icon } from "../icons"; @@ -9,6 +9,16 @@ import { type TimelineView, timelineView } from "./timeline"; /** Callbacks a finished assistant turn can offer. Omit any to hide its entry. */ export interface AssistantActions { + /** + * Whether git works, from the daemon. + * + * Present for the git-backed rows, which are shown greyed with the reason + * rather than hidden: a Commit that is simply missing reads as a bug, and a + * Commit that fails after the click has already cost the user the click. + * Absent is treated as healthy, which is what an older daemon that never + * sends `git:health` should look like. + */ + git?: GitHealth; onBranch?: () => void; onComment?: (file: string, body: HTMLElement) => void; onCommit?: (push: boolean) => void; @@ -334,18 +344,30 @@ function turnMenuButton( return btn; } -/** The turn kebab's contents, grouped. Exported for the same reason it is - * separate: the shape of this menu is a product decision worth reading. */ -export function turnMenu( +/** + * The "This change" rows: revert, commit, and open a pull request. + * + * Separate from `turnMenu` because all three carry an availability gate, and + * there are two different gates. Revert does not need git at all — it rewrites + * files from the before-state the turn captured — so it is greyed only when + * this turn has files whose baseline was never captured, which is a fact the + * bundle already carries. Commit and Create pull request do need git, equally + * for every turn, so they read the daemon's health report. + * + * Greyed with a reason rather than hidden: a Commit that is simply missing + * reads as a bug, and one that fails after the click has already cost the click. + */ +function changeRows( bundle: JobDiffBundle, actions: AssistantActions, anchor: HTMLElement ): MenuEntry[] { - const out: MenuEntry[] = []; - const file = bundle.diffs?.[0]?.file ?? bundle.target?.source?.file; - const line = bundle.diffs?.[0] - ? firstHunkLine(bundle.diffs[0].patch) - : (bundle.target?.source?.line ?? undefined); + // The reason only, never the hint: `GitStatus.hint` is a command to run, sized + // for a terminal, and a tooltip clamps at three lines. The banner and `airship + // doctor` are where the fix gets spelled out. + const gitBroken = actions.git && !actions.git.ok; + const gitTip = gitBroken ? actions.git?.reason : undefined; + const unrestorable = bundle.diffs?.some((d) => d.noBaseline) ?? false; const change: MenuEntry[] = []; if (actions.onUndo) { @@ -358,23 +380,29 @@ export function turnMenu( // direct-manipulation stack. `app.ts` says in as many words that the two // must never be wired together — and this menu was telling the user they // were. There is no chord for this, so it advertises none. + disabled: unrestorable, icon: "rotate-ccw", label: "Revert this change", run: actions.onUndo, + tip: unrestorable ? "No previous content to restore from" : undefined, }); } const { onCommit } = actions; if (onCommit) { change.push( { + disabled: gitBroken, icon: "version-current", label: "Commit to git", run: () => onCommit(false), + tip: gitTip, }, { + disabled: gitBroken, icon: "version-merged", label: "Commit & push", run: () => onCommit(true), + tip: gitTip, } ); } @@ -382,6 +410,7 @@ export function turnMenu( if (onCreatePr) { const files = bundle.diffs?.length ?? 0; change.push({ + disabled: gitBroken, icon: "version-branch", label: "Create pull request…", // Pushing is the only thing in this application that cannot be taken @@ -397,8 +426,26 @@ export function turnMenu( run: onCreatePr, }, ]).open(anchor, "above"), + tip: gitTip, }); } + return change; +} + +/** The turn kebab's contents, grouped. Exported for the same reason it is + * separate: the shape of this menu is a product decision worth reading. */ +export function turnMenu( + bundle: JobDiffBundle, + actions: AssistantActions, + anchor: HTMLElement +): MenuEntry[] { + const out: MenuEntry[] = []; + const file = bundle.diffs?.[0]?.file ?? bundle.target?.source?.file; + const line = bundle.diffs?.[0] + ? firstHunkLine(bundle.diffs[0].patch) + : (bundle.target?.source?.line ?? undefined); + + const change = changeRows(bundle, actions, anchor); if (change.length) { out.push({ header: "This change" }, ...change); } diff --git a/packages/overlay/src/chat/turn-menu.test.ts b/packages/overlay/src/chat/turn-menu.test.ts new file mode 100644 index 0000000..a4101e0 --- /dev/null +++ b/packages/overlay/src/chat/turn-menu.test.ts @@ -0,0 +1,125 @@ +/** + * The turn kebab's git verbs, and when they are offered. + * + * Greyed with the reason rather than hidden. A Commit that is simply missing + * reads as a bug in the editor; a Commit that is offered and then fails has + * already cost the user the click and told them nothing they can act on. + * + * Two gates, because the two failures are unrelated. Revert rewrites files from + * the before-state the turn captured and needs no git at all, so it is greyed + * only when *this* turn has files whose baseline was never captured. Commit and + * Create pull request need git for every turn equally. + */ + +import type { JobDiffBundle } from "@airship/protocol"; +import { describe, expect, it, vi } from "vitest"; +import { el } from "../dom"; +import { isMenuItem, type MenuItem } from "../popover-host"; +import { type AssistantActions, turnMenu } from "./transcript"; + +function bundleWith(noBaseline: boolean): JobDiffBundle { + return { + agent: "codex", + createdAt: 0, + diffs: [ + { + additions: 1, + deletions: 0, + file: "src/app.ts", + isDeleted: false, + isNew: false, + ...(noBaseline ? { noBaseline: true } : {}), + patch: "", + }, + ], + filesChanged: 1, + jobId: "job-1", + prompt: "make it blue", + promptPreview: "make it blue", + status: "done", + target: {}, + } as JobDiffBundle; +} + +const ALL_ACTIONS: AssistantActions = { + onBranch: vi.fn(), + onCommit: vi.fn(), + onCreatePr: vi.fn(), + onUndo: vi.fn(), +}; + +function rowsFor( + actions: Partial, + noBaseline = false +): MenuItem[] { + return turnMenu( + bundleWith(noBaseline), + { ...ALL_ACTIONS, ...actions }, + el("button") + ).filter(isMenuItem); +} + +const row = (rows: MenuItem[], label: string): MenuItem => + rows.find((r) => r.label === label) as MenuItem; + +const GIT_ROWS = ["Commit to git", "Commit & push", "Create pull request…"]; + +describe("turnMenu git verbs", () => { + it("offers every git row when git is healthy", () => { + const rows = rowsFor({ git: { ok: true } }); + for (const label of GIT_ROWS) { + expect(row(rows, label).disabled).toBeFalsy(); + expect(row(rows, label).tip).toBeUndefined(); + } + }); + + it("greys every git row with the reason when git is broken", () => { + const rows = rowsFor({ + git: { + hint: "Run `git init` there.", + ok: false, + reason: "not a git repository", + }, + }); + for (const label of GIT_ROWS) { + expect(row(rows, label).disabled).toBe(true); + expect(row(rows, label).tip).toBe("not a git repository"); + } + }); + + it("keeps the hint out of the tooltip", () => { + // `GitStatus.hint` is a command to run, sized for a terminal. The tooltip + // clamps at three lines, so the banner and `airship doctor` carry the fix. + const rows = rowsFor({ + git: { + hint: "Run `git init` there.", + ok: false, + reason: "not a git repository", + }, + }); + expect(row(rows, "Commit to git").tip).not.toContain("git init"); + }); + + it("treats an absent health report as healthy", () => { + // An older daemon never sends `git:health`. Hiding a working Commit is + // worse than offering one that might fail. + expect(row(rowsFor({}), "Commit to git").disabled).toBeFalsy(); + }); + + it("leaves Revert alone when only git is broken", () => { + // Revert is pure filesystem. On the Claude path it works with no git at all. + const rows = rowsFor({ + git: { ok: false, reason: "not a git repository" }, + }); + expect(row(rows, "Revert this change").disabled).toBeFalsy(); + }); + + it("greys Revert when the turn captured no baseline", () => { + const rows = rowsFor({ git: { ok: true } }, true); + const revert = row(rows, "Revert this change"); + expect(revert.disabled).toBe(true); + expect(revert.tip).toBe("No previous content to restore from"); + // The git rows are unaffected: git works, so committing still does. + expect(row(rows, "Commit to git").disabled).toBeFalsy(); + }); +}); diff --git a/packages/overlay/src/popover-host.test.ts b/packages/overlay/src/popover-host.test.ts index 7e823d7..9ee6af1 100644 --- a/packages/overlay/src/popover-host.test.ts +++ b/packages/overlay/src/popover-host.test.ts @@ -5,6 +5,7 @@ import { closeOpenPopover, createMenu, type MenuGroup, + type MenuItem, mountPopoverHost, openPopover, type PopoverHandle, @@ -528,6 +529,66 @@ describe("a menu with collapsible groups", () => { expect(shells()[0].querySelector(".custom-form")).toBe(custom); }); + + /* + * A greyed row that carries a reason has to stay hoverable. + * + * Browsers do not dispatch pointer events from a `disabled` form control, and + * `Tooltips` matches on `pointerover`, so `data-tip` on one is never read. The + * greying comes from `[aria-disabled="true"]` in `pop.css.ts` rather than from + * `:disabled`, which is what makes dropping the attribute free. + */ + describe("a disabled row", () => { + const rowFor = (item: MenuItem): HTMLElement => { + createMenu([item]).open(anchor(), "below"); + return shells()[0].querySelector( + `.${cls("pop-item")}` + ) as HTMLElement; + }; + + it("stays hoverable, and out of both cursors, when it has a tip", () => { + const row = rowFor({ + disabled: true, + label: "Commit to git", + run: () => undefined, + tip: "not a git repository", + }); + + expect(row.getAttribute("aria-disabled")).toBe("true"); + expect(row.hasAttribute("disabled")).toBe(false); + expect(row.dataset.tip).toBe("not a git repository"); + // Neither the roving cursor nor Tab reaches it. + expect(row.hasAttribute("data-pop-item")).toBe(false); + expect(row.getAttribute("tabindex")).toBe("-1"); + // The reason rides the accessible name too: `aria-disabled` alone + // announces that the row is unavailable and never why. + expect(row.getAttribute("aria-label")).toBe( + "Commit to git. not a git repository" + ); + }); + + it("keeps the plain disabled attribute when it has no reason to give", () => { + const row = rowFor({ disabled: true, label: "Nothing", run: vi.fn() }); + + expect(row.hasAttribute("disabled")).toBe(true); + expect(row.hasAttribute("data-tip")).toBe(false); + expect(row.hasAttribute("aria-label")).toBe(false); + }); + + it("does not run when clicked", () => { + const run = vi.fn(); + rowFor({ + disabled: true, + label: "Commit to git", + run, + tip: "not a git repository", + }).click(); + + expect(run).not.toHaveBeenCalled(); + // And the menu stays open, so the tip can still be read. + expect(shells()).toHaveLength(1); + }); + }); }); /* diff --git a/packages/overlay/src/popover-host.ts b/packages/overlay/src/popover-host.ts index 2149851..45cec65 100644 --- a/packages/overlay/src/popover-host.ts +++ b/packages/overlay/src/popover-host.ts @@ -756,6 +756,14 @@ export interface MenuItem { label: string; on?: boolean; run: () => void; + /** + * Why this row is greyed, shown on hover. + * + * A disabled row that does not say why is a dead end: the user can see that + * Commit is unavailable and has nothing to act on. Pair it with `disabled` + * whenever the reason is something they can fix. + */ + tip?: string; } /** @@ -1002,16 +1010,35 @@ export function createMenu(entries: MenuEntry[]): MenuHandle { const buildRow = (entry: MenuItem): HTMLElement => { const disabled = Boolean(entry.disabled); + /* + * A disabled row that carries a reason keeps `aria-disabled` and loses the + * `disabled` *attribute*. + * + * Browsers do not dispatch pointer events from a disabled form control, so + * `data-tip` on one is never read: `Tooltips` matches on `pointerover`, and + * the event it sees is targeted at the ancestor instead. The greying is + * driven by `[aria-disabled="true"]` in `pop.css.ts`, not by `:disabled`, so + * dropping the attribute costs nothing visually, and `tabindex="-1"` keeps + * the row out of the tab order that the attribute used to hold it out of. + * Rows with no reason to give keep the plain `disabled` and are unchanged. + */ + const hoverable = disabled && Boolean(entry.tip); const row = el( "button", { + "aria-label": + hoverable && entry.tip ? `${entry.label}. ${entry.tip}` : undefined, class: `${cls("pop-item")}${entry.on ? ` ${cls("pop-item-on")}` : ""}`, + "data-tip": entry.tip, role: "menuitem", type: "button", // Omitting the attribute is what keeps the roving cursor off it; the // arrow-key code only ever queries `[data-pop-item]`. ...(disabled - ? { "aria-disabled": "true", disabled: "" } + ? { + "aria-disabled": "true", + ...(hoverable ? { tabindex: "-1" } : { disabled: "" }), + } : { "data-pop-item": "" }), onClick: disabled ? undefined