Keep showing a change after it's approved

cli/src/project.ts

@@ -6,7 +6,7 @@ import path from "node:path";
 import { FilesystemBlobStorage } from "selfie-store/adapters/filesystem-blob";
 import { sharpCodec } from "selfie-store/adapters/sharp-codec";
 import { SqliteAdapter } from "selfie-store/adapters/sqlite";
-import { Blobs, initStoreSchema, RenderBatches, Sql, Store, BuiltStorybooks, StoredDiffs, type CompareOptions, type Comparison, type RenderBatch } from "selfie-store";
+import { Blobs, initStoreSchema, RenderBatches, Sql, Store, BuiltStorybooks, StoredDiffs, type ApprovedSince, type CompareOptions, type Comparison, type RenderBatch } from "selfie-store";
 import type { PixelDiff } from "./diff.ts";
 import { SelfieError } from "./errors.ts";
 import { ancestorsOf, defaultBranchOf, earliestAuthoredSince, headSha, openRepo, type Repo } from "./git.ts";
@@ -121,6 +121,18 @@ export async function compareAsReviewed(project: Project, batchId: string, optio
   return project.batches.compareFromHistory(batchId, decisions, options);
 }
 
+/** See `docs/specs/renders.md` -> Approved since a base. With no default branch to tell one from, there's no saying. */
+export async function approvedSinceOf(project: Project, batchId: string): Promise<ApprovedSince> {
+  const batch = await requireBatch(project, batchId);
+  const { batches, repo } = project;
+  const { defaultBranch } = await defaultBranchOf(repo, batch.gitSha);
+  if (defaultBranch === null) return { base: null, renders: [] };
+  const commits = await reachableCommits(project, batch);
+  const base = await batches.baseAmong(defaultBranch, batch, commits);
+  const [decisions, baseDecisions] = await Promise.all([batches.decisionsAt(batch.environment, commits), base === null ? [] : batches.decisionsAt(base.environment, await ancestorsOf(repo, base.gitSha))]);
+  return { base, renders: await batches.approvedSinceFromHistory(batchId, decisions, baseDecisions) };
+}
+
 /** A batch someone named, which must be well formed and in this project's store. */
 export async function requireBatch(project: Project, batchId: string): Promise<RenderBatch> {
   assertBatchId(batchId);

cli/src/ui.ts

@@ -16,7 +16,7 @@ import { currentBranch, dirtyPaths, gitUserEmail, headMovedAt, headSha } from ".
 import { isOpenNote } from "selfie-ui/notes";
 import { advanceRenderProgress } from "selfie-ui/render-progress";
 import { sentNotes, textForAgent } from "./notes.ts";
-import { batchesAtHead, compareAsReviewed, reachableCommits, readReviewCache, requireBatch, type Project } from "./project.ts";
+import { approvedSinceOf, batchesAtHead, compareAsReviewed, reachableCommits, readReviewCache, requireBatch, type Project } from "./project.ts";
 
 /** Each is about 20 KB of PNG. This is a reviewer going back and forth, not a cache of a batch. */
 const DIFFS_KEPT = 32;
@@ -75,7 +75,7 @@ export function localReviewSource(
     );
   };
 
-  const { toRows, viewOf } = batchViews({
+  const { toRows, viewOf, approvedSinceView } = batchViews({
     batches,
     storybooks: project.storybooks,
     keptStorybookUrl,
@@ -269,6 +269,8 @@ export function localReviewSource(
       );
     },
 
+    approvedSince: async (batchId) => approvedSinceView(await approvedSinceOf(project, batchId)),
+
     async decide(batchId: string, decision: DecisionKind, targets: readonly DecisionTarget[]) {
       const actor = await requireActor("no decision was recorded");
       const { comparison } = await decide(project, { batchId, decision, actor, stories: { targets } });

cli/test/approvals.test.ts

@@ -3,6 +3,7 @@
 import { mkdir, writeFile } from "node:fs/promises";
 import path from "node:path";
 import { afterEach, beforeEach, describe, expect, test } from "vitest";
+import { approvedSinceOf, openProject } from "../src/project.ts";
 import { BLUE, commit, createHarness, git, GREEN, TEST_CONFIG, type Harness } from "./support.ts";
 
 let h: Harness;
@@ -140,6 +141,30 @@ describe("across branches", () => {
     expect(states).toEqual({});
   });
 
+  test("a merge reads as unchanged, and can still say what it brought and what that replaced", async () => {
+    const onMain = await review();
+    await h.cli("approve", onMain.batchId, "--all");
+    git(h.repo, "checkout", "-b", "feature");
+    await commit(h.repo, "changes one");
+    h.stories = [{ id: "a--one", color: BLUE }, { id: "a--two" }];
+    const onFeature = await review();
+    await h.cli("approve", onFeature.batchId, "--all");
+    git(h.repo, "checkout", "-");
+    git(h.repo, "merge", "--no-ff", "--no-edit", "feature");
+    const merged = await review();
+    expect(merged.states).toEqual({});
+
+    const project = await openProject({ cwd: h.repo, env: { SELFIE_HOME: h.home } });
+    try {
+      const { base, renders } = await approvedSinceOf(project, merged.batchId);
+      expect(base?.id).toBe(onMain.batchId);
+      expect(renders.map(({ story, state, approvedIn }) => [story, state, approvedIn])).toEqual([["a--one", "changed", onFeature.batchId]]);
+      expect(renders[0]!.goldenHash).not.toBe(renders[0]!.imageHash);
+    } finally {
+      project.close();
+    }
+  });
+
   test("a branch's approvals count nowhere its history doesn't go", async () => {
     await approvedOn("feature");
     git(h.repo, "checkout", "-");

cli/test/sync-routes.test.ts

@@ -80,7 +80,7 @@ beforeEach(async () => {
     // With no token, it's a public repository's anonymous viewer.
     open: async (project, token) => {
       const access = project === "42" ? ACCESS[token ?? "reader"] : undefined;
-      return access === undefined ? null : { access, batches: cloud.batches, store: cloud.store, storybooks: cloud.storybooks, diffs: cloud.diffs, hasCommit: async (sha) => PUSHED.has(sha) };
+      return access === undefined ? null : { access, defaultBranch: "main", batches: cloud.batches, store: cloud.store, storybooks: cloud.storybooks, diffs: cloud.diffs, hasCommit: async (sha) => PUSHED.has(sha) };
     },
     findProject: async (owner, name) => (owner === "diffplug" && name === "selfie" ? "42" : null),
   });

cli/test/sync.test.ts

@@ -12,7 +12,7 @@ import { runCli } from "../src/cli.ts";
 import { encodePng } from "../src/png.ts";
 import { openProject, type Project } from "../src/project.ts";
 import { localReviewSource } from "../src/ui.ts";
-import { commit, createHarness, fakeRender, git, GREEN, RED, solid, type FakeStory, type Harness, writeTestConfig } from "./support.ts";
+import { BLUE, commit, createHarness, fakeRender, git, GREEN, RED, solid, type FakeStory, type Harness, writeTestConfig } from "./support.ts";
 
 const REMOTE = "http://127.0.0.1:9";
 /** What the cloud would answer, in the shape the client calls it. */
@@ -100,7 +100,7 @@ beforeEach(async () => {
   app = createSyncApi({
     open: async (project, token) => {
       const access = project === "42" && token !== null ? ACCESS[token] : undefined;
-      return access === undefined ? null : { access, batches: cloud.batches, store: cloud.store, storybooks: cloud.storybooks, diffs: cloud.diffs, hasCommit: async (sha) => pushedCommits === null || pushedCommits.has(sha) };
+      return access === undefined ? null : { access, defaultBranch: "main", batches: cloud.batches, store: cloud.store, storybooks: cloud.storybooks, diffs: cloud.diffs, hasCommit: async (sha) => pushedCommits === null || pushedCommits.has(sha) };
     },
     findProject: async (owner, name) => (owner === "diffplug" && name === "selfie" ? "42" : null),
   });
@@ -124,6 +124,23 @@ describe("pushing", () => {
     expect(await cloud.batches.decisionsAt(held!.environment, [head])).toHaveLength(2);
   });
 
+  test("keeps the comparison of what a branch approved with what its base had, which no one commit compared", async () => {
+    const before = (await imagesOf(await approved()))[0]!;
+    await laptop("writer", "push");
+    git(h.repo, "checkout", "-b", "feature");
+    for (const color of [BLUE, [1, 2, 3]] as const) {
+      await commit(h.repo, `makes it ${color}`);
+      stories = [{ id: "a", color }, { id: "b", color: GREEN }];
+      await approved();
+      await laptop("writer", "push");
+    }
+    await commit(h.repo, "changes nothing");
+    const after = (await imagesOf(await record()))[0]!;
+    expect(await cloud.diffs.has(after, before)).toBe(false);
+    await laptop("writer", "push");
+    expect(await cloud.diffs.has(after, before)).toBe(true);
+  });
+
   test("names who pushed as a decision's author, not what the laptop says", async () => {
     const batchId = await approved();
     await laptop("writer", "push");

cli/test/ui-decisions.test.ts

@@ -6,6 +6,37 @@ import { UNKNOWN_HASH, h, project, review, get, change, decideOn, viewOf, approv
 
 const ON = { batchId: "00000000-0000-4000-8000-000000000001", gitSha: "e".repeat(40) };
 
+describe("what's been approved since a batch's base", () => {
+  test("says what a merge brought, and refuses a decision on it", async () => {
+    h.stories = [{ id: "a" }, { id: "b" }];
+    const onMain = await approveAll();
+    git(h.repo, "checkout", "-b", "feature");
+    await commit(h.repo, "on a branch");
+    h.stories = [{ id: "a", color: BLUE }, { id: "b" }];
+    const onBranch = (await review()).json.batchId as string;
+    await h.cli("approve", onBranch, "--all");
+    git(h.repo, "checkout", "-");
+    git(h.repo, "merge", "--no-ff", "--no-edit", "feature");
+    const { batchId } = (await review()).json;
+
+    expect((await viewOf(batchId)).rows).toEqual([]);
+    const { base, rows } = (await (await get(`/api/batches/${batchId}/approved-since`)).json()) as any;
+    expect(base.id).toBe(onMain);
+    expect(rows.map(({ story, state, approvedIn, pixelDiff, decision }: any) => [story, state, approvedIn, pixelDiff, decision.decision])).toEqual([["a", "changed", onBranch, null, "approved"]]);
+    expect(rows[0].goldenHash).not.toBe(rows[0].imageHash);
+
+    expect((await decideOn(batchId, "rejected", [{ story: "a", imageHash: rows[0].imageHash }])).status).toBe(409);
+    expect((await viewOf(batchId)).blockers).toEqual([]);
+  });
+
+  test("has nothing to say for a batch that decided on its own changes", async () => {
+    h.stories = [{ id: "a" }];
+    const onMain = await approveAll();
+    const again = (await (await get(`/api/batches/${onMain}/approved-since`)).json()) as any;
+    expect(again).toEqual({ base: null, rows: [] });
+  });
+});
+
 describe("approve and reject", () => {
   let batchId: string;
   let rows: any[];

docs/specs/chat.md

@@ -15,7 +15,7 @@ Source of truth: `ui/src/worker/chat.ts`, `compare` in `ui/src/worker/github.ts`
 - **Must keep `pageHeaders` and `sameOriginWrites` on it**, as on any review route (`docs/specs/cloud.md` → Pages).
 - **Must take at most 10 questions a minute from one address**, by the `CHAT_LIMIT` binding, and answer 429 `too-many-questions` past that (rationale).
 - **Must take at most 40 messages, with at most 4,000 characters in a question and 40,000 in an answer, in at most 1 MB**, and answer 400 `bad-request` otherwise.
-- **Must offer it only on a hosted story that isn't unchanged.** `selfie ui` has no chat. Any other story is 404 `unknown-story`.
+- **Must offer it only on a hosted story that isn't unchanged.** `selfie ui` has no chat. Any other story is 404 `unknown-story`, which includes one shown as approved earlier (`docs/specs/ui.md` → Screens), since it's unchanged in that batch.
 
 ## What the model reads
 

docs/specs/glossary.md

@@ -17,6 +17,7 @@
 | **Session** | Whose notes these are: a local Git worktree's top folder (`docs/specs/feedback.md`). |
 | **Approval** | A person's or agent's acceptance of a render, or of a story being gone, at the commit of the batch it was made on. It counts wherever that commit's history reaches, and may be made before the change is pushed (`docs/specs/renders.md` → Approvals). |
 | **Rejection** | A person's or agent's refusal of a render change. |
+| **Base** | The batch of the default branch that a batch's changes approved earlier are told apart from (`docs/specs/renders.md` → Approved since a base). |
 | **Decision** | An approval or a rejection, recorded against a render name, an image hash or a removal, and a commit (`docs/specs/renders.md`). |
 | **Render name** | What identifies a render across batches and stores: its story ID, with any render variant in it, and its environment ID. |
 | **Render state** | How a batch's render compares with its reference: unchanged, changed, added, removed, or failed (`docs/specs/renders.md`). |

docs/specs/renders.md

@@ -99,6 +99,17 @@ Source of truth: `compareWithApprovals` and `RenderBatches.compareFromHistory` i
 - **Must let a batch recorded on a dirty tree take approvals**, which count from its commit.
 - **Must read one commit list as one statement**, however long, so that history isn't a statement per commit (rationale).
 
+## Approved since a base
+
+Source of truth: `approvedSinceBase`, `RenderBatches.approvedSinceReached`, `baseAmong`, and `approvedSinceFromHistory` in `store/src/core/render-batches.ts`; `approvedSinceOf` in `cli/src/project.ts`.
+
+Once it's approved, a change is what later batches compare with, so they read it as unchanged. This is how they can still show it.
+
+- **Must take a batch's base to be the newest finished batch of the default branch, in its environment, at a commit it reaches other than its own**, by when they started (rationale). A batch that reaches none has no base, and every approval it reaches counts. With no default branch to go by, there's nothing to say.
+- **Must list each story that doesn't differ now and would by the base's approvals, as those compare it**, so `goldenHash` is what it replaced, with the newest approval's `decision` and the batch it was made on as `approvedIn`. That takes in what's been added, and a removal that's been approved.
+- **Must leave out what the batch still lists**, which is for it to decide on, including what was approved on this batch, which stays there as [Approvals](#approvals) says.
+- **Never take a decision on one of these.** `decide` refuses it as it does any unchanged image (`stale-decision`).
+
 ## Schema
 
 Source of truth: `CREATE_BATCH_TABLES` in `store/src/core/render-batches.ts`, `initStoreSchema` in `store/src/core/store.ts`.

docs/specs/renders.rationale.md

@@ -62,6 +62,14 @@
 
 **No derived table, locally.** Measured on local SQLite at Dormouse's 568 stories, reaching 1,000 commits: reading 11,360 decisions takes about 0.4 s, 113,600 about 1.7 s, and 568,000 about 4 s. Those have every story approved at every commit, which is much worse than a commit that changes a few stories. A compare reads them once, and an approval twice.
 
+## Approved since a base
+
+**Why a batch of the default branch.** It's what a merge commit's first parent and a branch's starting point have in common, and it needs no parent order, which `commit_parents` doesn't keep, and no merge base, which a store with no Git can't ask for. The default branch filter is what keeps a branch's own batches from being the base, so all a branch approved stays in the list of each of its later commits, and of its merge. Once a batch of the default branch has it, later batches take that as their base, and stop listing it, so a merge shows only what it brought. If a run renders an old commit again, it's the newest by when it started, and the list reaches further back than it needs to, which only shows more.
+
+**Why not keep what an approval replaced.** It'd need a schema change, and existing decisions wouldn't have it, while this reads what's already there, and works on every batch already recorded.
+
+**Why a list of its own, and not more rows.** A batch's `rows` are what it blocks on and what can be decided, so these would need leaving out everywhere those are used. Most of the time nobody opens them.
+
 ## Schema
 
 **Foreign keys, before there's data.** Nothing in the store deletes yet, and every write already goes through a path that writes parents first, so these catch only bugs for now. What they're for is retention: once a batch can be deleted, they're what stop one going while something points at it. SQLite can't add a foreign key to an existing table, so waiting would mean rebuilding tables in a migration, where today it's an edit to their `CREATE` statements. Inserting 10,000 renders took 9 ms with checks and 8 ms without, in one local run. They haven't run on real D1, only on its engine in the Worker tests.

docs/specs/sync.md

@@ -34,7 +34,7 @@ To mount under `/api`. `GET /repositories/:owner/:name` finds a repository's pro
 | `PUT /storybooks/files/:hash` | A file's bytes, which must hash to that | Upload |
 | `PUT /storybooks/:hash` | `{ files }`, each `{ path, fileHash }`, once all are here, or `missing-files` | Upload |
 | `POST /batches/:id/storybook` | `{ build }`, which built Storybook rendered this batch | Upload |
-| `GET /batches/:id/diffs/needed` | `{ pairs }`, each `{ current, golden }`, the changed pairs with no diff here | Upload |
+| `GET /batches/:id/diffs/needed` | `{ pairs }`, each `{ current, golden }`, the changed pairs with no diff here, including those of what's been approved since its base (`docs/specs/renders.md` → Approved since a base) | Upload |
 | `PUT /diffs/:current/:golden/image` | The diff image's bytes, sent first | Upload |
 | `PUT /diffs/:current/:golden` | Its `PixelDiff`, which says both are here | Upload |
 | `POST /decisions` | `{ environment, decisions }`, at most 1,000, each with its `gitSha` and `batchId`, and a null `imageHash` for a removal | Decide |