diff --git a/cli/src/project.ts b/cli/src/project.ts index a287f53..dbc3109 100644 --- a/cli/src/project.ts +++ b/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 { + 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 { assertBatchId(batchId); diff --git a/cli/src/ui.ts b/cli/src/ui.ts index 58f0c38..d1ad373 100644 --- a/cli/src/ui.ts +++ b/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 } }); diff --git a/cli/test/approvals.test.ts b/cli/test/approvals.test.ts index 23616df..294f715 100644 --- a/cli/test/approvals.test.ts +++ b/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", "-"); diff --git a/cli/test/sync-routes.test.ts b/cli/test/sync-routes.test.ts index 66ddcab..0b09ce2 100644 --- a/cli/test/sync-routes.test.ts +++ b/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), }); diff --git a/cli/test/sync.test.ts b/cli/test/sync.test.ts index f7e192f..9b0a1b4 100644 --- a/cli/test/sync.test.ts +++ b/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"); diff --git a/cli/test/ui-decisions.test.ts b/cli/test/ui-decisions.test.ts index 07b2eb6..4d37d25 100644 --- a/cli/test/ui-decisions.test.ts +++ b/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[]; diff --git a/docs/specs/chat.md b/docs/specs/chat.md index 597f2e1..6477225 100644 --- a/docs/specs/chat.md +++ b/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 diff --git a/docs/specs/glossary.md b/docs/specs/glossary.md index 8bdc136..e1b4b87 100644 --- a/docs/specs/glossary.md +++ b/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`). | diff --git a/docs/specs/renders.md b/docs/specs/renders.md index a5dbc17..f29d588 100644 --- a/docs/specs/renders.md +++ b/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`. diff --git a/docs/specs/renders.rationale.md b/docs/specs/renders.rationale.md index ad90280..9236170 100644 --- a/docs/specs/renders.rationale.md +++ b/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. diff --git a/docs/specs/sync.md b/docs/specs/sync.md index 4688aca..b8be677 100644 --- a/docs/specs/sync.md +++ b/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 | diff --git a/docs/specs/ui.md b/docs/specs/ui.md index 85d099a..f85f461 100644 --- a/docs/specs/ui.md +++ b/docs/specs/ui.md @@ -47,6 +47,7 @@ Source of truth: `ReviewSource` in `ui/src/shared/api.ts`. | `renderProgress()` | How far that render has got, or null with none in flight | | `batchView(batchId)` | A `BatchView` ([Recomputing](#recomputing)) | | `unchanged(batchId)` | The batch's unchanged rows | +| `approvedSince(batchId)` | Its base, and its rows of what's been approved since that (`docs/specs/renders.md` → Approved since a base) | | `decide(batchId, decision, targets)` | The new `BatchView` | | `feedback()`, `addNote(note)`, `removeNote(id)` | This session's notes (`docs/specs/feedback.md`) | | `answerNote(id, text, batchId)`, `resolveNote(id)` | This session's notes after a person's answer, or closing one | @@ -68,6 +69,7 @@ Source of truth: `ui/src/server/app.ts`; tests in `cli/test/ui-*.test.ts`. | `GET /api/renders/progress` | `{ progress }`: `{ total, finished, startedAtMs, firstStoryStartedAtMs }`, or null with no render in flight. `startedAtMs` is when the render was asked for, and `firstStoryStartedAtMs` is null until its first story starts. Both are milliseconds since the epoch, on the clock the page shares with a local server (rationale) | | `GET /api/batches/:id` | `batchView` | | `GET /api/batches/:id/unchanged` | `{ rows }` | +| `GET /api/batches/:id/approved-since` | `{ base, rows }` | | `POST /api/batches/:id/decisions` | `{ decision, renders: [{ story, imageHash }] }` | | `GET /api/feedback` | `feedback` | | `POST /api/notes` | `{ story, batchId, imageHash, box, text }` | @@ -93,7 +95,7 @@ On a hosted Storybook's own origin (`docs/specs/access.md` → Local review serv - **Must give images, diff images, and diff stats `Cache-Control: private, max-age=31536000, immutable`**, since all are addressed by content (`docs/specs/access.md` → Local review server), and every other API answer `no-store`. - **Must decide on exactly the images a request names, or none:** the library's `decide` refuses with `stale-decision` if any named story and hash isn't a changed or added image of that batch, or a null hash a removed story of it, and takes the names exactly, never as patterns (rationale). - **Must refuse a bulk request no differently from a single one.** Nothing on the server knows "all": the client sends what it showed. -- **Must compare a batch at most twice for a decision, and once for anything else**, never reading unchanged rows unless a route asks for them. +- **Must compare a batch at most twice for a decision, and once for anything else**, never reading unchanged rows unless a route asks for them. What's approved since a base also compares it with that base's approvals, and a page reads that once. - **Must serve any page but `/assets/*` and `/api/*` as the client's one document**, with `no-store`, and a missing asset as 404. ## Recomputing @@ -134,6 +136,7 @@ Source of truth: `ui/src/client/`. - **Must group blockers by reason, in words, and offer each as a filter of the list.** - **Must keep the filter, text and state, in the URL, and nowhere else**, so back, reload, and next and previous all follow it. - **Must list unchanged stories only when asked**, by the `unchanged` filter, which is when they're fetched. +- **Must say so when a batch has changes approved earlier, and how to see them**, by the `approved-earlier` filter, with what they're compared with. Each opens as any changed, added, or removed render does, between what it replaced and this image, saying where it was approved, with a link to that, and offering no decision (rationale). **Bulk decisions:** approve or reject all, or all the current filter shows. diff --git a/docs/specs/ui.rationale.md b/docs/specs/ui.rationale.md index d379e6a..8ee7a16 100644 --- a/docs/specs/ui.rationale.md +++ b/docs/specs/ui.rationale.md @@ -30,6 +30,8 @@ Evidence and reasoning for `docs/specs/ui.md`, keyed by its headings. **Repeated changes, in the browser.** A header that grew 15 rows made all 124 stories of one batch change, and each had to be looked at for whatever else was in it. Of 146 distinct hunks there, 28 were in at least 3 stories, and those accounted for all of 83 images. Whether accepting one should outlive a batch, or be something a team shares, is what **Scope: hunks** in `docs/specs/renders.md` is for. Nothing here narrows that, since a store only ever hears of an approval of a whole image. +**Why say what was approved earlier.** A merged pull request's commit reads as nothing differing, since its approvals are what it's compared with, which is right for what blocks it and hides what it did. It's found on the page, instead of on the batch that was approved, since that's where a link from a commit lands, and nothing else leads from a later commit to an earlier batch. + **Why raw hunks, and by pixels.** A group is only a choice of what to show together, and its box holds whatever's between its hunks, so the same change could group differently beside something else. Position isn't compared, since a band that closes up sits at a different height on each page. A hash and not pixels held in memory, since a whole page's change on each of 568 stories would hold gigabytes. SHA-256, since this approves images, and a weaker hash would let a crafted image pass as a change someone accepted. **Why whole images to go through a delta's stories.** A delta's pixels are the same in every story by definition, so its crop can't tell one from another. What can be wrong is where it landed and what's around it, as when a change that's right on most pages is on top of something on one. Only one is open at a time, since 28 pairs of whole images would be a very long page. diff --git a/store/src/core/index.ts b/store/src/core/index.ts index 62c5ebb..fb1d89e 100644 --- a/store/src/core/index.ts +++ b/store/src/core/index.ts @@ -16,11 +16,13 @@ export { isBatchId, assertViewport, compareWithApprovals, + approvedSinceBase, decisionsThatCount, newestRenders, RenderBatches, BatchError, type CompareOptions, + type ApprovedSince, type ComparedRender, type Comparison, type Decision, diff --git a/store/src/core/render-batches.ts b/store/src/core/render-batches.ts index 9895f80..99049e6 100644 --- a/store/src/core/render-batches.ts +++ b/store/src/core/render-batches.ts @@ -198,18 +198,30 @@ CROSS JOIN decisions d ON d.environment_key = ${environmentKey("?1")} AND d.comm ${NAMES_OF_DECISION}`; // What a batch's commit reaches, walked by key, so that a long history's SHAs never leave the database. -const DECISIONS_REACHED = `WITH RECURSIVE reach(key) AS ( +const REACH_OF_BATCH = `WITH RECURSIVE reach(key) AS ( SELECT commit_key FROM render_batches WHERE id = ?1 UNION SELECT p.parent_key FROM reach r CROSS JOIN commit_parents p ON p.commit_key = r.key LIMIT ${MAX_REACHED_COMMITS} -) +)`; + +const DECISIONS_REACHED = `${REACH_OF_BATCH} ${PLACED_DECISION} FROM reach r CROSS JOIN decisions d ON d.environment_key = (SELECT environment_key FROM render_batches WHERE id = ?1) AND d.commit_key = r.key CROSS JOIN commits c ON c.key = d.commit_key ${NAMES_OF_DECISION}`; +/** The newest finished batch of a branch among these commits, in another's environment, and never at that one's own commit. */ +const newestOfBranchWhere = (branch: string, environment: string, commit: string, commits: string) => `SELECT ${BATCH_COLUMNS} FROM render_batches +WHERE branch = ${branch} AND finished_at IS NOT NULL AND environment_key = ${environment} AND commit_key != ${commit} AND commit_key IN (${commits}) +ORDER BY started_at DESC LIMIT 1`; + +const BASE_REACHED = `${REACH_OF_BATCH} +${newestOfBranchWhere("?2", "(SELECT environment_key FROM render_batches WHERE id = ?1)", "(SELECT commit_key FROM render_batches WHERE id = ?1)", "SELECT key FROM reach")}`; + +const BASE_AMONG = newestOfBranchWhere("?1", environmentKey("?2"), commitKey("?3"), "SELECT c.key FROM json_each(?4) j CROSS JOIN commits c ON c.sha = j.value"); + const rendersPage = (batchId: string, after: string): SqlStatement => ({ sql: READ_RENDERS_PAGE, params: [batchId, after, ROWS_PER_PAGE], idempotent: true }); const COMMITS_OF_BRANCH = `SELECT DISTINCT c.sha AS git_sha FROM render_batches r JOIN commits c ON c.key = r.commit_key @@ -352,6 +364,14 @@ export interface ComparedRender { readonly decision: Decision | null; } +/** What a batch's history has approved since its base, which reads as unchanged against what's approved now. */ +export interface ApprovedSince { + /** The default branch's batch it's told apart from, or null if its commit reaches none, and then every approval counts. */ + readonly base: RenderBatch | null; + /** Each as the base would compare it, so `goldenHash` is what it replaced, and with the batch it was approved on. */ + readonly renders: readonly (ComparedRender & { readonly approvedIn: string })[]; +} + export interface Comparison { readonly batch: RenderBatch; /** @@ -618,9 +638,46 @@ ORDER BY started_at DESC, key DESC`, } async #withApprovals(batchId: string, batchRows: readonly SqlRow[], firstRenders: readonly SqlRow[], decisions: readonly PlacedDecision[], { includeUnchanged = false }: CompareOptions = {}): Promise { + const { batch, rendered } = await this.#renderedOf(batchId, batchRows, firstRenders); + return compareWithApprovals(batch, rendered, decisions, { includeUnchanged }); + } + + async #renderedOf(batchId: string, batchRows: readonly SqlRow[], firstRenders: readonly SqlRow[]): Promise<{ readonly batch: RenderBatch; readonly rendered: readonly RenderedStory[] }> { const batch = finishedBatch(batchRows, batchId); const rows = await this.#remainingPages(firstRenders, (after) => rendersPage(batchId, after)); - return compareWithApprovals(batch, rows.map((row) => ({ story: row.story as string, imageHash: row.image_hash as string | null, failure: row.failure as string | null })).sort(byStory), decisions, { includeUnchanged }); + return { batch, rendered: rows.map((row) => ({ story: row.story as string, imageHash: row.image_hash as string | null, failure: row.failure as string | null })).sort(byStory) }; + } + + /** + * The base a batch is told apart from, as `compareReached` knows it: the newest batch of the default branch + * its commit reaches, and what was approved as of there. See `docs/specs/renders.md` -> Approved since a base. + */ + async approvedSinceReached(batchId: string, defaultBranch: string): Promise { + const [batchRows, decisionRows, baseRows, firstRenders] = (await this.#sql.batch([ + { sql: SELECT_BATCH, params: [batchId], idempotent: true }, + { sql: DECISIONS_REACHED, params: [batchId], idempotent: true }, + { sql: BASE_REACHED, params: [batchId, defaultBranch], idempotent: true }, + rendersPage(batchId, START), + ])) as [SqlRow[], SqlRow[], SqlRow[], SqlRow[]]; + const base = baseRows[0] === undefined ? null : toBatch(baseRows[0]); + const baseDecisions = base === null ? [] : await this.#sql.run({ sql: DECISIONS_REACHED, params: [base.id], idempotent: true }); + const { batch, rendered } = await this.#renderedOf(batchId, batchRows, firstRenders); + const decisions = decisionRows.map(toPlacedDecision); + const current = compareWithApprovals(batch, rendered, decisions); + return { base, current, renders: approvedSinceBase(current, rendered, decisions, baseDecisions.map(toPlacedDecision)) }; + } + + /** The newest batch of a branch at any of these commits, other than a batch's own, for a store told what's reachable. */ + async baseAmong(branch: string, { environment, gitSha }: Pick, commits: readonly string[]): Promise { + const [row] = await this.#sql.run({ sql: BASE_AMONG, params: [branch, environment, gitSha, JSON.stringify(commits)], idempotent: true }); + return row === undefined ? null : toBatch(row); + } + + /** As `approvedSinceReached`, with decisions from `decisionsAt`, for a caller with Git. */ + async approvedSinceFromHistory(batchId: string, decisions: readonly PlacedDecision[], baseDecisions: readonly PlacedDecision[]): Promise { + const [batchRows, firstRenders] = (await this.#sql.batch([{ sql: SELECT_BATCH, params: [batchId], idempotent: true }, rendersPage(batchId, START)])) as [SqlRow[], SqlRow[]]; + const { batch, rendered } = await this.#renderedOf(batchId, batchRows, firstRenders); + return approvedSinceBase(compareWithApprovals(batch, rendered, decisions), rendered, decisions, baseDecisions); } /** Every decision made at these commits. One parameter however many, so a long history is one statement. */ @@ -870,6 +927,16 @@ function decisionsByStory(decisions: readonly PlacedDecision[]): Map, story: string, batchId: string) => + decisionsOf.get(story)?.find((decision) => decision.decision === "approved" && decision.batchId !== batchId); + /** * A batch's renders against the approvals that history reaches. Pure, since this is the rule. A * story's reference is the image of its newest approval, leaving out any made on this batch. A @@ -878,13 +945,13 @@ function decisionsByStory(decisions: readonly PlacedDecision[]): Map decisionsOf.get(story)?.find(({ decision, batchId }) => decision === "approved" && batchId !== batch.id); + const referenceOf = (story: string) => newestApproval(decisionsOf, story, batch.id); /** What's been said about this image since the reference, if anything has. */ const decisionOn = (story: string, imageHash: string | null, referenceAt: number): Decision | null => { const found = decisionsOf.get(story)?.find((decision) => decision.imageHash === imageHash && decision.at >= referenceAt); @@ -913,6 +980,22 @@ export function compareWithApprovals( return { batch, renders: [...compared, ...removed], unchangedCount }; } +/** + * The stories a batch has nothing to say about now, because of an approval the base doesn't reach. Each is + * as the base's approvals compare it, which is how a change stays visible once it's what's approved. + */ +export function approvedSinceBase(current: Comparison, rendered: readonly RenderedStory[], decisions: readonly PlacedDecision[], baseDecisions: readonly PlacedDecision[]): ApprovedSince["renders"] { + const { batch } = current; + const stillDiffers = new Set(current.renders.map(({ story }) => story)); + const decisionsOf = decisionsByStory(decisions); + return compareWithApprovals(batch, rendered, baseDecisions).renders.flatMap((render) => { + const approval = newestApproval(decisionsOf, render.story, batch.id); + if (stillDiffers.has(render.story) || approval === undefined) return []; + const { decision, actor, at, batchId: approvedIn } = approval; + return [{ ...render, decision: { decision, actor, at }, approvedIn }]; + }); +} + const stateOf = (imageHash: string | null, goldenHash: string | null, failure: string | null): RenderState => failure !== null || imageHash === null ? "failed" : goldenHash === null ? "added" : imageHash === goldenHash ? "unchanged" : "changed"; diff --git a/store/test/approvals-from-history.test.ts b/store/test/approvals-from-history.test.ts index 932a826..233bf09 100644 --- a/store/test/approvals-from-history.test.ts +++ b/store/test/approvals-from-history.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { compareWithApprovals, decisionsThatCount, imageInsertStatement, RenderBatches, type PlacedDecision, type RenderBatch } from "../src/core/index.ts"; +import { approvedSinceBase, compareWithApprovals, decisionsThatCount, imageInsertStatement, RenderBatches, type PlacedDecision, type RenderBatch } from "../src/core/index.ts"; import { hashOf } from "./support/sql-params.ts"; import { createTempStore } from "./support/temp-store.ts"; @@ -68,6 +68,34 @@ describe("comparing with what history has approved", () => { }); }); +describe("what's been approved since a base", () => { + const since = (rendered: Parameters[1], decisions: readonly PlacedDecision[], baseDecisions: readonly PlacedDecision[]) => + approvedSinceBase(compareWithApprovals(batch, rendered, decisions), rendered, decisions, baseDecisions).map(({ story, state, goldenHash, approvedIn, decision }) => [story, state, goldenHash, approvedIn, decision?.decision]); + + it("shows an approved change as the base would compare it", () => { + expect(since([render("s", B)], [approved("s", A, 1), approved("s", B, 2)], [approved("s", A, 1)])).toEqual([["s", "changed", A, OTHER, "approved"]]); + }); + + it("shows an approved addition and an approved removal", () => { + expect(since([render("new", A)], [approved("new", A, 2)], [])).toEqual([["new", "added", null, OTHER, "approved"]]); + expect(since([], [approved("gone", A, 1), approved("gone", null, 2)], [approved("gone", A, 1)])).toEqual([["gone", "removed", A, OTHER, "approved"]]); + }); + + it("leaves out what the base had approved already", () => { + const decisions = [approved("s", A, 1)]; + expect(since([render("s", A)], decisions, decisions)).toEqual([]); + }); + + it("leaves out what still differs, since that's for this batch to decide on", () => { + expect(since([render("s", C)], [approved("s", A, 1), approved("s", B, 2)], [approved("s", A, 1)])).toEqual([]); + expect(since([render("s", B)], [approved("s", A, 1), approved("s", B, 2, THIS)], [approved("s", A, 1)])).toEqual([]); + }); + + it("leaves out a failure", () => { + expect(since([render("s", null)], [approved("s", A, 1), approved("s", B, 2)], [approved("s", A, 1)])).toEqual([]); + }); +}); + describe("what another store needs of a history", () => { const ats = (decisions: readonly PlacedDecision[]) => decisions.map(({ story, at }) => [story, at]); @@ -166,6 +194,72 @@ describe("on a store", () => { expect(compared.unchangedCount).toBe(1); }); + describe("what's been approved since a base", () => { + const [BASE, FEATURE, MERGE, LATER, FOLLOWUP] = ["a".repeat(40), "b".repeat(40), "c".repeat(40), "d".repeat(40), "f".repeat(40)] as const; + const on = async (gitSha: string, branch: string, stories: Record) => { + const entries = Object.entries(stories); + await temp.sql.batch(entries.map(([, image]) => imageInsertStatement(hashOf(image), image))); + const batch = await batches.beginBatch({ gitSha, environment: ENV, scale: 1, viewport: { width: 1200, height: 900 }, dirty: false, branch }); + await batches.recordRenders(batch.id, entries.map(([story, image]) => ({ story, imageHash: hashOf(image) }))); + await batches.finishBatch(batch.id, entries.length); + return batch; + }; + const listed = async (batch: RenderBatch) => { + const { base, renders } = await batches.approvedSinceReached(batch.id, "main"); + return { base: base?.id ?? null, renders: renders.map(({ story, state, imageHash, goldenHash, approvedIn }) => [story, state, imageHash, goldenHash, approvedIn]) }; + }; + + /** A base on main, a branch off it that approves a change, and that branch merged back. */ + async function merged() { + const base = await on(BASE, "main", { s: 1, t: 2 }); + await approve(base, "s", hashOf(1)); + await approve(base, "t", hashOf(2)); + const feature = await on(FEATURE, "topic", { s: 3, t: 2 }); + await approve(feature, "s", hashOf(3)); + const merge = await on(MERGE, "main", { s: 3, t: 2 }); + await batches.recordCommits([{ sha: FEATURE, parents: [BASE] }, { sha: MERGE, parents: [BASE, FEATURE] }, { sha: LATER, parents: [MERGE] }]); + return { base, feature, merge }; + } + + it("shows a merge what it brought, which it reads as unchanged", async () => { + const { base, feature, merge } = await merged(); + expect((await batches.compareReached(merge.id)).renders).toEqual([]); + expect(await listed(merge)).toEqual({ base: base.id, renders: [["s", "changed", hashOf(3), hashOf(1), feature.id]] }); + }); + + it("shows a later commit on the branch what the branch approved", async () => { + const { base, feature } = await merged(); + await batches.recordCommits([{ sha: FOLLOWUP, parents: [FEATURE] }]); + expect(await listed(await on(FOLLOWUP, "topic", { s: 3, t: 2 }))).toEqual({ base: base.id, renders: [["s", "changed", hashOf(3), hashOf(1), feature.id]] }); + }); + + it("stops showing it once a batch of the default branch has it", async () => { + const { merge } = await merged(); + expect(await listed(await on(LATER, "main", { s: 3, t: 2 }))).toEqual({ base: merge.id, renders: [] }); + }); + + it("shows a batch rendered again at a commit what was approved on the first, since that's not its base", async () => { + const first = await on(BASE, "main", { s: 1 }); + await approve(first, "s", hashOf(1)); + expect(await listed(await on(BASE, "main", { s: 1 }))).toEqual({ base: null, renders: [["s", "added", hashOf(1), null, first.id]] }); + }); + + it("never takes a base from another branch, or a batch the commit doesn't reach", async () => { + const { base, feature } = await merged(); + await on("e".repeat(40), "main", { s: 9 }); + expect((await listed(feature)).base).toBe(base.id); + expect((await batches.approvedSinceReached(feature.id, "topic")).base).toBeNull(); + }); + + it("finds the same base from a list of commits", async () => { + const { base, merge } = await merged(); + expect((await batches.baseAmong("main", merge, [MERGE, FEATURE, BASE]))?.id).toBe(base.id); + expect(await batches.baseAmong("main", merge, [MERGE, FEATURE])).toBeNull(); + const decisions = await batches.decisionsAt(ENV, [MERGE, FEATURE, BASE]); + expect((await batches.approvedSinceFromHistory(merge.id, decisions, await batches.decisionsAt(ENV, [BASE]))).map(({ story, state }) => [story, state])).toEqual([["s", "changed"]]); + }); + }); + it("finds a branch's commits since a time, for a rebase's sake", async () => { const { batch } = await finished(SHAS[0], { a: 1 }); expect(await batches.commitsOfBranch("topic", ENV, 0, Number.MAX_SAFE_INTEGER)).toEqual([SHAS[0]]); diff --git a/ui/src/client/App.tsx b/ui/src/client/App.tsx index 4948065..2e4c5ea 100644 --- a/ui/src/client/App.tsx +++ b/ui/src/client/App.tsx @@ -19,7 +19,7 @@ function BatchRoutes({ batchId, route, search }: { batchId: string; route: Route const state = useBatch(batchId); const feedback = useFeedback(); const { data: context } = useFetched(batchId, (signal) => fetchBatchContext(batchId, signal)); - const tree = useBatchTree(state.view, state.unchanged, search, route.kind === "story" ? route.story : null); + const tree = useBatchTree(state.view, state, search, route.kind === "story" ? route.story : null); const deltas = useDeltas(batchId, state.view?.rows, route.kind === "batch" && state.view !== null && canDecide(state.view)); // Where to put focus on coming back to the list. const lastStory = useRef(null); diff --git a/ui/src/client/BatchLayout.tsx b/ui/src/client/BatchLayout.tsx index 914e7bf..f58989b 100644 --- a/ui/src/client/BatchLayout.tsx +++ b/ui/src/client/BatchLayout.tsx @@ -2,19 +2,22 @@ import { useMemo, type ReactNode } from "react"; import type { BatchContext, BatchView, RenderRow } from "../shared/api.ts"; import { findRenames } from "../shared/renames.ts"; -import { applyFilter, navigationThrough, NO_FILTER, parseFilter } from "./filter.ts"; +import { applyFilter, navigationThrough, NO_FILTER, parseFilter, rowsFor } from "./filter.ts"; +import type { BatchState } from "./hooks.ts"; import { usePageFacts } from "./PageFacts.tsx"; import { batchPath, storyPath } from "./router.tsx"; import { matchingStories, StoryNavLayout } from "./StoryNav.tsx"; import { differingStories, inTreeOrder } from "./storyTree.ts"; import { describePixels } from "./words.tsx"; +const NO_ROWS: readonly RenderRow[] = []; + /** The tree shows what the page's Show says, and its own search does the text. Next and previous go through just that. */ -export function useBatchTree(view: BatchView | null, unchanged: readonly RenderRow[] | null, search: string, story: string | null) { +export function useBatchTree(view: BatchView | null, read: Pick, search: string, story: string | null) { const rows = view?.rows; const renames = useMemo(() => findRenames(rows ?? []), [rows]); const { show, text } = parseFilter(search); - const source = show === "unchanged" ? (unchanged ?? []) : (rows ?? []); + const source = rowsFor(show, rows ?? [], read) ?? NO_ROWS; const shownRows = useMemo(() => applyFilter(source, { ...NO_FILTER, show }), [source, show]); const stories = useMemo(() => (view === null ? [] : differingStories({ ...view, rows: shownRows }, renames)), [view, shownRows, renames]); const rowOf = useMemo(() => new Map(source.map((row) => [row.story, row])), [source]); diff --git a/ui/src/client/BatchScreen.stories.tsx b/ui/src/client/BatchScreen.stories.tsx index 22e5670..0118a6d 100644 --- a/ui/src/client/BatchScreen.stories.tsx +++ b/ui/src/client/BatchScreen.stories.tsx @@ -121,6 +121,9 @@ export const ConfirmApproveAll: Story = { const AFFECTED = ["components-button--primary", "pages-settings--account", "components-list--with-new-row"]; const OFFERS = [deltaFrom("taller-golden", [...AFFECTED, "pages-home--rearranged"]), deltaFrom("changed-golden", AFFECTED), deltaFrom("spaced-golden", AFFECTED)]; +const APPROVED_EARLIER = { base: batchOf({ id: OTHER_BATCH_ID, branch: "main" }), rows: [{ ...rows.changed("components-button--primary", approved()), approvedIn: OTHER_BATCH_ID }, { ...rows.fresh("pages-home--new", approved()), approvedIn: OTHER_BATCH_ID }] }; +export const ChangesApprovedEarlier: Story = { args: { view: viewOf([]), state: { approvedSince: APPROVED_EARLIER } } }; +export const ChangesApprovedEarlierListed: Story = { args: { view: viewOf([]), state: { approvedSince: APPROVED_EARLIER }, search: "?show=approved-earlier" } }; export const LookingForRepeatedChanges: Story = { args: { view: viewOf(SOME_CHANGES), deltas: deltasOf({ progress: 40 }) } }; export const RepeatedChanges: Story = { args: { view: viewOf(SOME_CHANGES), deltas: deltasOf({ offers: OFFERS }) } }; export const LookingThroughADelta: Story = { diff --git a/ui/src/client/BatchScreen.tsx b/ui/src/client/BatchScreen.tsx index f8b022b..d023b36 100644 --- a/ui/src/client/BatchScreen.tsx +++ b/ui/src/client/BatchScreen.tsx @@ -16,11 +16,11 @@ import { } from "@primer/react"; import { Blankslate } from "@primer/react/experimental"; import { useEffect, useMemo, useState, type ComponentProps, type RefObject } from "react"; -import type { BlockerReason, DecisionKind, BatchView, RenderRow } from "../shared/api.ts"; +import type { ApprovedSinceView, BlockerReason, DecisionKind, BatchView, RenderRow } from "../shared/api.ts"; import { BulkDialog } from "./BulkDialog.tsx"; import { DeltaOffers } from "./DeltaOffers.tsx"; import type { DeltaState } from "./deltas.ts"; -import { applyFilter, decidableIn, filterQuery, NO_FILTER, parseFilter, showQuery, SHOW_OPTIONS, targetsOf, type RowFilter, type Show } from "./filter.ts"; +import { applyFilter, decidableIn, filterQuery, NO_FILTER, parseFilter, rowsFor, showQuery, SHOW_OPTIONS, targetsOf, type RowFilter, type Show } from "./filter.ts"; import { useStoryFocusKeys, useTitle, type BatchState } from "./hooks.ts"; import { Keys } from "./Keys.tsx"; import { AppLink, AppLinkButton, navigate, batchPath, storiesPath, storyPath } from "./router.tsx"; @@ -40,6 +40,11 @@ function Fact({ term, children }: { term: string; children: React.ReactNode }) { ); } +/** What the rows of what's approved since a base are compared with. */ +function approvedBaseWords({ base }: ApprovedSinceView) { + return base === null ? "Compared with nothing, since this commit doesn't reach a batch of the default branch." : `Compared with what was approved at ${shortSha(base.gitSha)} on ${base.branch}.`; +} + function Facts({ view }: { view: BatchView }) { const { batch, actor, siblings } = view; return ( @@ -228,13 +233,12 @@ function Differences({ if (wantsUnchanged && unchanged === null) loadUnchanged(); }, [wantsUnchanged, unchanged, loadUnchanged]); - const source = filter.show === "unchanged" ? (unchanged ?? []) : view.rows; - const shown = useMemo(() => applyFilter(source, filter), [source, filter]); + const source = rowsFor(filter.show, view.rows, state); + const shown = useMemo(() => applyFilter(source ?? [], filter), [source, filter]); const allDecidable = useMemo(() => decidableIn(view.rows), [view.rows]); const shownDecidable = useMemo(() => decidableIn(shown), [shown]); const undecided = useMemo(() => applyFilter(view.rows, { ...NO_FILTER, show: "undecided" }), [view.rows]); const isFiltered = filter.text !== "" || filter.show !== "all"; - const loadingUnchanged = wantsUnchanged && unchanged === null; const confirm = async (confirmed: boolean) => { const decided = pending; @@ -267,7 +271,7 @@ function Differences({ {canDecide(view) && ( - {isFiltered && ( + {isFiltered && source === view.rows && ( <> @@ -279,10 +283,11 @@ function Differences({ )} - {loadingUnchanged ? "Reading the unchanged stories…" : `Showing ${shown.length.toLocaleString()} of ${source.length.toLocaleString()}.`}{" "} + {source === null ? "Reading these stories…" : `Showing ${shown.length.toLocaleString()} of ${source.length.toLocaleString()}.`}{" "} and move through the list, searches. - {shown.length > 0 || loadingUnchanged ? null : ( + {filter.show === "approved-earlier" && state.approvedSince !== null && {approvedBaseWords(state.approvedSince)} Each was approved on another batch, so there's nothing to decide on here.} + {shown.length > 0 || source === null ? null : ( {isFiltered ? "Nothing matches this filter" : "Nothing differs from what's approved"} @@ -341,6 +346,8 @@ export function BatchScreen({ useTitle(`Batch at ${shortSha(batch.gitSha)}`); const newest = view.siblings.find(({ status }) => status !== "recording"); + const { approvedSince } = state; + const approvedEarlier = approvedSince?.rows.length ?? 0; return ( @@ -359,6 +366,14 @@ export function BatchScreen({ description={Open the newest batch} /> )} + {approvedSince !== null && approvedEarlier > 0 && filter.show !== "approved-earlier" && ( + showInList("approved-earlier")}>See what changed} + /> + )} {batch.dirty && ( + {outsideFilter > 0 && ( { expect(canvas.queryByText("Stacked")).toBeNull(); }; +const APPROVED_EARLIER = { ...rows.changed("components-button--primary", approved()), approvedIn: OTHER_BATCH_ID }; +export const ApprovedEarlier: Story = { + args: { row: APPROVED_EARLIER, approvedEarlier: [APPROVED_EARLIER] }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect(canvas.getByRole("link", { name: "See it on the batch it was approved on" })).toBeVisible(); + expect(canvas.queryByRole("button", { name: /Approve/ })).toBeNull(); + expect(canvas.queryByRole("button", { name: /Reject/ })).toBeNull(); + }, +}; + export const AddedAtFit: Story = { args: { row: ADDED }, play: async ({ canvasElement }) => { diff --git a/ui/src/client/DetailScreen.tsx b/ui/src/client/DetailScreen.tsx index 4d7fd1a..5c7f678 100644 --- a/ui/src/client/DetailScreen.tsx +++ b/ui/src/client/DetailScreen.tsx @@ -10,11 +10,11 @@ import { diffImageUrl, fetchDiff, imageUrl } from "./api.ts"; import { prepareNext, usePatchedGolden, type PatchedGolden } from "./deltas.ts"; import { canDecide, isHosted, signInHref } from "./base.ts"; import { ChatPanel } from "./ChatPanel.tsx"; -import { parseFilter, targetsOf, type StoryNavigation } from "./filter.ts"; +import { parseFilter, rowsFor, targetsOf, type StoryNavigation } from "./filter.ts"; import { useFetched, useHotkeys, useHunkSelection, useTitle, type FeedbackState, type BatchState } from "./hooks.ts"; import { imageCount, ImageCompare } from "./ImageCompare.tsx"; import { Keys } from "./Keys.tsx"; -import { AppLink, navigate } from "./router.tsx"; +import { AppLink, navigate, storyPath } from "./router.tsx"; import { ActionFailure } from "./ActionFailure.tsx"; import { LiveStories, StorybookLinkTo } from "./LiveStories.tsx"; import { LoadFailure, Loading } from "./Status.tsx"; @@ -78,7 +78,7 @@ function PixelStats({ stats }: { stats: PixelDiff }) { ); } -function Notes({ row, diff, renamedFrom, renamedTo }: { row: RenderRow; diff: PixelDiff | null; renamedFrom?: string | undefined; renamedTo?: string | undefined }) { +function Notes({ row, diff, renamedFrom, renamedTo, approvedIn }: { row: RenderRow; diff: PixelDiff | null; renamedFrom?: string | undefined; renamedTo?: string | undefined; approvedIn?: string | undefined }) { return ( <> {row.state === "failed" && ( @@ -99,6 +99,21 @@ function Notes({ row, diff, renamedFrom, renamedTo }: { row: RenderRow; diff: Pi The added story {renamedTo} has this very image, so it looks to have been renamed.} /> )} {row.state === "unchanged" && } + {approvedIn !== undefined && ( + + This is what this commit's history changed, and it reads as unchanged in this batch, since it's what's approved now.{" "} + + See it on the batch it was approved on + + . + + } + /> + )} {diff !== null && isLayoutOnly(diff) && ( )} @@ -169,12 +184,15 @@ export function DetailScreen({ const filter = useMemo(() => parseFilter(search), [search]); const renames = useMemo(() => findRenames(view.rows), [view.rows]); const inRows = view.rows.find((candidate) => candidate.story === story); - const needsUnchanged = (filter.show === "unchanged" || inRows === undefined) && unchanged === null; + const source = rowsFor(filter.show, view.rows, state); + const found = source?.find((candidate) => candidate.story === story) ?? inRows; + const needsUnchanged = (filter.show === "unchanged" || (source !== null && found === undefined)) && unchanged === null; useEffect(() => { if (needsUnchanged) loadUnchanged(); }, [needsUnchanged, loadUnchanged]); - const row = inRows ?? unchanged?.find((candidate) => candidate.story === story) ?? null; + const row = source === null ? null : (found ?? unchanged?.find((candidate) => candidate.story === story) ?? null); + const approvedIn = state.approvedSince?.rows.find((candidate) => candidate === row)?.approvedIn; const { previous, next, nextStory, list: listHref, position } = navigation; const [helpOpen, setHelpOpen] = useState(false); @@ -196,7 +214,9 @@ export function DetailScreen({ fetchDiff(row!.imageHash!, compared!.hash, signal), ); const recordedDiff = row?.pixelDiff ?? fetched; - const patched = usePatchedGolden(row, accepted); + // Only what this batch lists takes a decision, and a delta is only ever accepted on that. + const decidable = row !== null && row === inRows && isDecidable(row.state); + const patched = usePatchedGolden(row, decidable ? accepted : NOTHING_ACCEPTED); // Here, so that it stays as it is from story to story, like how someone's looking at the images. const [deltasShown, setDeltasShown] = useState(true); const applied = deltasShown ? patched : null; @@ -228,8 +248,8 @@ export function DetailScreen({ setMessage("Sign in to approve or reject."); return; } - if (!isDecidable(row.state)) { - setMessage(row.state === "unchanged" ? "An unchanged image needs no decision." : "This story failed, so it takes no decision. Fix it and render again."); + if (!decidable) { + setMessage(approvedIn !== undefined ? "This was approved on another batch, so it takes no decision here." : row.state === "unchanged" ? "An unchanged image needs no decision." : "This story failed, so it takes no decision. Fix it and render again."); return; } const recorded = await state.decide(decision, targetsOf([row])); @@ -251,11 +271,10 @@ export function DetailScreen({ }); if (row === null) { - if (unchanged === null) return state.actionError !== null ? : ; + if (source === null || unchanged === null) return state.actionError !== null ? : ; return Back to the batch} />; } - const decidable = isDecidable(row.state); const finished = view.counts.undecided === 0 && next === null && filter.show === "undecided"; const renamedFrom = renames.get(row.story); @@ -296,13 +315,13 @@ export function DetailScreen({ - {decidable && row.decision !== null && ( + {isDecidable(row.state) && row.decision !== null && ( by {row.decision.actor}, . )} - {row.state === "added" && !comparedWithNote && renamedFrom === undefined && Nothing has been approved for this story yet. Approving makes this image what later batches compare with.} - {row.state === "removed" && This story was approved, and this batch doesn't have it. Approving says it's meant to be gone, so later batches stop asking.} + {decidable && row.state === "added" && !comparedWithNote && renamedFrom === undefined && Nothing has been approved for this story yet. Approving makes this image what later batches compare with.} + {decidable && row.state === "removed" && This story was approved, and this batch doesn't have it. Approving says it's meant to be gone, so later batches stop asking.} {diff !== null && } {changedPair && diff === null && diffError === null && Comparing pixels…} {diffError !== null && The pixels couldn't be compared: {diffError.message}} @@ -326,11 +345,11 @@ export function DetailScreen({ {comparedWithNote ? ( ) : ( - from === row.story)?.[0]} /> + from === row.story)?.[0]} /> )} {patched !== null && } setLiveOpen(!liveOpen)} /> - {isHosted && row.state !== "unchanged" && } + {isHosted && row.state !== "unchanged" && approvedIn === undefined && } {helpOpen && setHelpOpen(false)} />} diff --git a/ui/src/client/api.ts b/ui/src/client/api.ts index f6bdd45..a91003c 100644 --- a/ui/src/client/api.ts +++ b/ui/src/client/api.ts @@ -3,6 +3,7 @@ import { REQUEST_HEADER, REQUEST_HEADER_VALUE, type ApiError, + type ApprovedSinceView, type BatchContext, type DecisionKind, type DecisionTarget, @@ -60,6 +61,8 @@ export const fetchBatchContext = (batchId: string, signal: AbortSignal) => reque export const fetchUnchanged = async (batchId: string) => (await request<{ rows: readonly RenderRow[] }>(`${batchRoute(batchId)}/unchanged`)).rows; export const fetchDiff = (current: string, golden: string, signal: AbortSignal | null = null) => request(`/diffs/${current}/${golden}`, { signal }); +export const fetchApprovedSince = (batchId: string, signal: AbortSignal) => request(`${batchRoute(batchId)}/approved-since`, { signal }); + export const postDecision = (batchId: string, decision: DecisionKind, renders: readonly DecisionTarget[]) => post(`${batchRoute(batchId)}/decisions`, { decision, renders }); diff --git a/ui/src/client/filter.ts b/ui/src/client/filter.ts index adc68f9..2d4d385 100644 --- a/ui/src/client/filter.ts +++ b/ui/src/client/filter.ts @@ -1,5 +1,6 @@ // Which rows a filter shows, and where next and previous go. Pure, so a test needs no browser. import { isDecidable } from "selfie-store"; +import type { BatchState } from "./hooks.ts"; import type { DecisionTarget, RenderRow } from "../shared/api.ts"; import { isLayoutOnly } from "../shared/hunks.ts"; import { plural } from "../shared/plural.ts"; @@ -15,6 +16,7 @@ export const SHOW_OPTIONS = [ ["approved", "Approved"], ["rejected", "Rejected"], ["unchanged", "Unchanged"], + ["approved-earlier", "Approved earlier"], ] as const; export type Show = (typeof SHOW_OPTIONS)[number][0]; @@ -35,6 +37,8 @@ function inShow(row: RenderRow, show: Show): boolean { case "approved": case "rejected": return isDecidable(row.state) && row.decision?.decision === show; + case "approved-earlier": + return true; case "layout": return row.state === "changed" && row.pixelDiff !== null && isLayoutOnly(row.pixelDiff); default: @@ -58,6 +62,10 @@ export function applyFilter(rows: readonly RenderRow[], { text, show }: RowFilte /** What these rows have to decide on. */ export const decidableIn = (rows: readonly RenderRow[]) => rows.filter(({ state }) => isDecidable(state)); +/** The rows a Show picks from: what differs, unless it asks for what's read separately, and then null until that's read. */ +export const rowsFor = (show: Show, rows: readonly RenderRow[], { unchanged, approvedSince }: Pick): readonly RenderRow[] | null => + show === "unchanged" ? unchanged : show === "approved-earlier" ? (approvedSince?.rows ?? null) : rows; + export const targetsOf = (rows: readonly RenderRow[]): DecisionTarget[] => rows.map(({ story, imageHash }) => ({ story, imageHash })); diff --git a/ui/src/client/hooks.ts b/ui/src/client/hooks.ts index cd9b020..fa3ac97 100644 --- a/ui/src/client/hooks.ts +++ b/ui/src/client/hooks.ts @@ -1,7 +1,7 @@ import { useCallback, useEffect, useRef, useState, useLayoutEffect } from "react"; import { isHosted } from "./base.ts"; -import type { DecisionKind, DecisionTarget, FeedbackView, StoriesView, NewNote, BatchView, SendVia, RenderProgress, RenderRow } from "../shared/api.ts"; -import { deleteNote, fetchFeedback, fetchRenderProgress, fetchStories, fetchBatch, fetchUnchanged, postAnswer, postRender, postDecision, postNote, postResolve, postSendNotes, RequestError } from "./api.ts"; +import type { ApprovedSinceView, DecisionKind, DecisionTarget, FeedbackView, StoriesView, NewNote, BatchView, SendVia, RenderProgress, RenderRow } from "../shared/api.ts"; +import { deleteNote, fetchApprovedSince, fetchFeedback, fetchRenderProgress, fetchStories, fetchBatch, fetchUnchanged, postAnswer, postRender, postDecision, postNote, postResolve, postSendNotes, RequestError } from "./api.ts"; import { reuseRows } from "./filter.ts"; import { toggled } from "./selection.ts"; @@ -13,6 +13,8 @@ export interface BatchState { readonly actionError: RequestError | null; readonly busy: boolean; readonly unchanged: readonly RenderRow[] | null; + /** What's been approved since this batch's base, once it's read. */ + readonly approvedSince: ApprovedSinceView | null; readonly loadUnchanged: () => void; readonly clearActionError: () => void; /** True if the decision was recorded. */ @@ -32,6 +34,7 @@ export function useBatch(batchId: string): BatchState { const [actionError, setActionError] = useState(null); const [busy, setBusy] = useState(false); const [unchanged, setUnchanged] = useState(null); + const [approvedSince, setApprovedSince] = useState(null); // At most one read is on its way, and a newer read or an action's answer aborts it, so a slow read // can't undo either. const reading = useRef(null); @@ -59,6 +62,15 @@ export function useBatch(batchId: string): BatchState { }; }, [read]); + // Once, since what it says of history seldom changes while someone's here, and nothing's decided by it. + useEffect(() => { + const controller = new AbortController(); + fetchApprovedSince(batchId, controller.signal).then(setApprovedSince, (error: unknown) => { + if (!controller.signal.aborted) setActionError(asRequestError(error)); + }); + return () => controller.abort(); + }, [batchId]); + const loadUnchanged = useCallback(() => { fetchUnchanged(batchId).then(setUnchanged, (error: unknown) => setActionError(asRequestError(error))); }, [batchId]); @@ -86,6 +98,7 @@ export function useBatch(batchId: string): BatchState { actionError, busy, unchanged, + approvedSince, loadUnchanged, clearActionError: useCallback(() => setActionError(null), []), decide: async (decision, targets) => { diff --git a/ui/src/client/stories/BatchFrame.tsx b/ui/src/client/stories/BatchFrame.tsx index 38a24ca..ed49c03 100644 --- a/ui/src/client/stories/BatchFrame.tsx +++ b/ui/src/client/stories/BatchFrame.tsx @@ -9,7 +9,7 @@ import { SHA } from "./fixtures.ts"; const CONTEXT: BatchContext = { head: SHA, branch: "topic" }; export function BatchFrame({ view, state, story = null, search = "", children }: { view: BatchView; state: BatchState; story?: string | null; search?: string; children: (navigation: StoryNavigation | null) => ReactNode }) { - const tree = useBatchTree(view, state.unchanged, search, story); + const tree = useBatchTree(view, state, search, story); return ( {children(tree.navigation)} diff --git a/ui/src/client/stories/fixtures.ts b/ui/src/client/stories/fixtures.ts index 756a3e9..82fcfc8 100644 --- a/ui/src/client/stories/fixtures.ts +++ b/ui/src/client/stories/fixtures.ts @@ -176,6 +176,7 @@ export function stateOf(view: BatchView, overrides: Partial = {}): B actionError: null, busy: false, unchanged: null, + approvedSince: null, loadUnchanged: fn(), clearActionError: fn(), decide: fn(async () => true), diff --git a/ui/src/client/stories/storyPage.tsx b/ui/src/client/stories/storyPage.tsx index a1036cc..40def8b 100644 --- a/ui/src/client/stories/storyPage.tsx +++ b/ui/src/client/stories/storyPage.tsx @@ -1,6 +1,6 @@ // What the stories of a story's page share. import { expect, userEvent, waitFor } from "storybook/test"; -import type { Note, BatchView, RenderRow } from "../../shared/api.ts"; +import type { ApprovedRow, Note, BatchView, RenderRow } from "../../shared/api.ts"; import { DetailScreen } from "../DetailScreen.tsx"; import { feedbackOf, rows, stateOf, viewOf } from "./fixtures.ts"; import { BatchFrame } from "./BatchFrame.tsx"; @@ -11,14 +11,17 @@ export interface StoryPageArgs { readonly view?: BatchView; readonly notes?: readonly Note[]; readonly accepted?: ReadonlySet; + /** What's approved since this batch's base, and that it's being looked at. */ + readonly approvedEarlier?: readonly ApprovedRow[]; } -export function StoryPage({ row, others = [], view, notes = [], accepted }: StoryPageArgs) { - const shown = view ?? viewOf([row, ...others]); - const state = stateOf(shown); +export function StoryPage({ row, others = [], view, notes = [], accepted, approvedEarlier }: StoryPageArgs) { + const shown = view ?? viewOf(approvedEarlier === undefined ? [row, ...others] : others); + const state = stateOf(shown, approvedEarlier === undefined ? {} : { approvedSince: { base: null, rows: approvedEarlier } }); + const search = approvedEarlier === undefined ? "" : "?show=approved-earlier"; return ( - - {(navigation) => navigation !== null && } + + {(navigation) => navigation !== null && } ); } diff --git a/ui/src/server/app.ts b/ui/src/server/app.ts index c10bec4..8101444 100644 --- a/ui/src/server/app.ts +++ b/ui/src/server/app.ts @@ -180,6 +180,8 @@ export function createReviewApi(given: ReviewSource | ((context: Context) => Pro context.json({ rows: await (await sourceOf(context)).unchanged(context.req.param("batchId")) }), ); + api.get("/batches/:batchId/approved-since", async (context) => context.json(await (await sourceOf(context)).approvedSince(context.req.param("batchId")))); + api.post("/batches/:batchId/decisions", async (context) => { const { decision, renders } = await jsonBody(context); if (decision !== "approved" && decision !== "rejected") { diff --git a/ui/src/server/batch-view.ts b/ui/src/server/batch-view.ts index fdd7ed7..a8b1791 100644 --- a/ui/src/server/batch-view.ts +++ b/ui/src/server/batch-view.ts @@ -1,7 +1,7 @@ // Everything a screen shows about a batch, from a comparison and what a platform can say about it. It // knows `selfie-store`'s core and nothing else, so a local server and a Worker show the same thing. -import { storyEntriesIn, summarizeBatch, type BuiltStorybooks, type Comparison, type RenderBatch, type RenderBatches, type StoryEntry } from "selfie-store"; -import type { BatchView, PixelDiff, RenderRow, StorybookLink } from "../shared/api.ts"; +import { storyEntriesIn, summarizeBatch, type ApprovedSince, type BuiltStorybooks, type ComparedRender, type Comparison, type RenderBatch, type RenderBatches, type StoryEntry } from "selfie-store"; +import type { ApprovedSinceView, BatchView, PixelDiff, RenderRow, StorybookLink } from "../shared/api.ts"; /** A person looks at a few batches at a time. */ const KEPT_INDEXES_HELD = 8; @@ -21,6 +21,7 @@ export interface BatchViewPlatform { export interface BatchViews { toRows(batchId: string, compared: Comparison["renders"]): Promise; viewOf(comparison: Comparison, actor: string | null): Promise; + approvedSinceView(approved: ApprovedSince): Promise; /** A kept built Storybook's stories, which never change, or nothing if it can't say. */ storiesOfKept(hash: string): Promise; } @@ -53,8 +54,7 @@ export function batchViews(platform: BatchViewPlatform): BatchViews { return [...named.values()].map(({ id: story, title, name }) => ({ story, title, name })); }; - const toRows = async (batchId: string, compared: Comparison["renders"]): Promise => { - const pixelDiffs = await platform.pixelDiffs(batchId); + const rowsWith = async (compared: readonly Compared[], pixelDiffs: ReadonlyMap) => { const references = new Map( await Promise.all( [...new Set(compared.flatMap(({ goldenBatchId }) => goldenBatchId ?? []))].map( @@ -73,6 +73,8 @@ export function batchViews(platform: BatchViewPlatform): BatchViews { }); }; + const toRows = async (batchId: string, compared: Comparison["renders"]): Promise => rowsWith(compared, await platform.pixelDiffs(batchId)); + /** Everything recomputed from what deciding compares with, which the caller already has. */ const viewOf = async (comparison: Comparison, actor: string | null): Promise => { const { batch } = comparison; @@ -100,5 +102,8 @@ export function batchViews(platform: BatchViewPlatform): BatchViews { }; }; - return { toRows, viewOf, storiesOfKept }; + /** Never a batch's cached stats, which are by story, of what it was compared with when it was reviewed. */ + const approvedSinceView = async ({ base, renders }: ApprovedSince): Promise => ({ base, rows: await rowsWith(renders, new Map()) }); + + return { toRows, viewOf, approvedSinceView, storiesOfKept }; } diff --git a/ui/src/server/hosted.ts b/ui/src/server/hosted.ts index eee1d20..8ffc0ac 100644 --- a/ui/src/server/hosted.ts +++ b/ui/src/server/hosted.ts @@ -65,6 +65,7 @@ export function hostedReviewSource( const { renders } = await batches.compareReached(batchId, { includeUnchanged: true }); return views.toRows(batchId, renders.filter(({ state }) => state === "unchanged")); }, + approvedSince: async (batchId) => views.approvedSinceView(await batches.approvedSinceReached(batchId, project.defaultBranch)), async decide(batchId: string, decision: DecisionKind, targets: readonly DecisionTarget[]) { if (actor === null) throw refusal("not-signed-in", "Sign in to approve or reject."); const before = await batches.compareReached(batchId); diff --git a/ui/src/server/sync.ts b/ui/src/server/sync.ts index 0720d8c..2b63c10 100644 --- a/ui/src/server/sync.ts +++ b/ui/src/server/sync.ts @@ -18,6 +18,8 @@ export interface SyncAccess { /** One project, as this token sees it. */ export interface SyncProject { readonly access: SyncAccess; + /** What a batch's base is on (`docs/specs/renders.md` -> Approved since a base). */ + readonly defaultBranch: string; readonly batches: RenderBatches; readonly store: ReceivingStore; readonly storybooks: BuiltStorybooks; @@ -324,9 +326,9 @@ export function createSyncApi(host: SyncHost): Hono { }); api.get("/projects/:project/batches/:id/diffs/needed", async (context) => { - const { batches, diffs } = await uploadable(context); - const { renders } = await batches.compareReached(readBatchId(context.req.param("id"))); - const pairs = renders.flatMap(({ state, imageHash, goldenHash }) => (state === "changed" && imageHash !== null && goldenHash !== null ? [{ current: imageHash, golden: goldenHash }] : [])); + const { batches, diffs, defaultBranch } = await uploadable(context); + const { current, renders } = await batches.approvedSinceReached(readBatchId(context.req.param("id")), defaultBranch); + const pairs = [...current.renders, ...renders].flatMap(({ state, imageHash, goldenHash }) => (state === "changed" && imageHash !== null && goldenHash !== null ? [{ current: imageHash, golden: goldenHash }] : [])); const held = await Promise.all(pairs.map(({ current, golden }) => diffs.has(current, golden))); return context.json({ pairs: pairs.filter((_, i) => !held[i]) }); }); diff --git a/ui/src/shared/api.ts b/ui/src/shared/api.ts index 542630a..42a5648 100644 --- a/ui/src/shared/api.ts +++ b/ui/src/shared/api.ts @@ -62,6 +62,15 @@ export interface RenderRow extends ComparedRender { readonly goldenViewport: Viewport | null; } +/** Approved on another batch, which is where its decision was made. */ +export type ApprovedRow = RenderRow & { readonly approvedIn: string }; + +/** What a batch's history has approved since its base, each row compared as that base would, so its change can still be seen. */ +export interface ApprovedSinceView { + readonly base: RenderBatch | null; + readonly rows: readonly ApprovedRow[]; +} + export type BatchListing = Pick; /** Where a kept built Storybook is, on a hosted Storybook's origin, ending in a slash. Crosses into `selfie-cli` from here. */ @@ -247,6 +256,7 @@ export interface ReviewSource { batchView(batchId: string): Promise; batchContext(batchId: string): Promise; unchanged(batchId: string): Promise; + approvedSince(batchId: string): Promise; /** Decides on exactly these images, or refuses all of them if one isn't what the batch holds. */ decide(batchId: string, decision: DecisionKind, targets: readonly DecisionTarget[]): Promise; feedback(): Promise; diff --git a/ui/src/worker/app.ts b/ui/src/worker/app.ts index abaa82d..a7d2bd5 100644 --- a/ui/src/worker/app.ts +++ b/ui/src/worker/app.ts @@ -161,6 +161,7 @@ export function createCloudApp(config: CloudConfig): Hono { const { batches, store, storybooks: kept, diffs } = found.hosted; return { access: await accessOf(token), + defaultBranch: found.project.defaultBranch, batches, store, storybooks: kept, diff --git a/ui/test/filter.test.ts b/ui/test/filter.test.ts index 7c157ff..175bedf 100644 --- a/ui/test/filter.test.ts +++ b/ui/test/filter.test.ts @@ -1,6 +1,6 @@ import { expect, test } from "vitest"; -import { applyFilter, decidableIn, filterQuery, neighbors, NO_FILTER, parseFilter, reuseRows, showQuery } from "../src/client/filter.ts"; -import type { RenderRow } from "../src/shared/api.ts"; +import { applyFilter, decidableIn, filterQuery, neighbors, NO_FILTER, parseFilter, reuseRows, rowsFor, showQuery } from "../src/client/filter.ts"; +import type { ApprovedRow, RenderRow } from "../src/shared/api.ts"; const row = (story: string, overrides: Partial = {}): RenderRow => ({ story, @@ -83,3 +83,14 @@ test("moved and spacing only is a changed row with nothing changed in it", () => expect(stories(applyFilter(candidates, { text: "", show: "layout" }))).toEqual(["only-moved"]); expect(parseFilter("?show=layout").show).toBe("layout"); }); + +test("what's read separately is null until it's been read, and otherwise a Show picks from what differs", () => { + const differs = { story: "differs", state: "changed", decision: null } as RenderRow; + const earlier = { story: "earlier", state: "changed", decision: { decision: "approved", actor: "a", at: 1 }, approvedIn: "elsewhere" } as ApprovedRow; + const read = { unchanged: null, approvedSince: { base: null, rows: [earlier] } }; + expect(rowsFor("all", [differs], read)).toEqual([differs]); + expect(rowsFor("unchanged", [differs], read)).toBeNull(); + expect(rowsFor("approved-earlier", [differs], read)).toEqual([earlier]); + expect(rowsFor("approved-earlier", [differs], { unchanged: null, approvedSince: null })).toBeNull(); + expect(applyFilter([earlier], { text: "", show: "approved-earlier" })).toEqual([earlier]); +}); diff --git a/ui/test/workerd/sync.workerd.test.ts b/ui/test/workerd/sync.workerd.test.ts index 583900e..8b1a10b 100644 --- a/ui/test/workerd/sync.workerd.test.ts +++ b/ui/test/workerd/sync.workerd.test.ts @@ -52,7 +52,7 @@ beforeAll(async () => { app = createSyncApi({ open: async (_project, token) => { const access = token === null ? undefined : ACCESS[token]; - return access === undefined ? null : { access, batches, store, storybooks, diffs, hasCommit: async () => true }; + return access === undefined ? null : { access, defaultBranch: "main", batches, store, storybooks, diffs, hasCommit: async () => true }; }, findProject: async () => "42", });