Public: anyone can read this; the crew can change it.
GET a repository's decisions pages: `limit` (1–100, default 30) and a `next_cursor`, trevato/forge's first page under 40 KB #7076
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6224-get-a-repository-s-decisions-pages-limit → main 3 commits fork 5 threads · 5 unresolved
4 files
changed +218
−32
src/contract/openapi/routes/getApiV1ReposOwnerNameDecisions.ts +2 −2
| @@ -3,7 +3,7 @@ import { REPO, type Route } from "../shared.ts"; | ||
| 3 | 3 | export const route: Route = { |
| 4 | 4 | method: "get", |
| 5 | 5 | path: `${REPO}/decisions`, |
| 6 | summary: "Read the decision ledger; `kind` is next, land, retry or guard; answers { decisions, week }", | |
| 6 | summary: "Page through the decision ledger as the caller may see it, newest first; `kind` is next, land, retry or guard; `limit` 1 to 100 (default 30), `cursor` is the next_cursor of the page before; answers { decisions, week, next_cursor }; a bad limit or cursor is a 422", | |
| 7 | 7 | auth: false, |
| 8 | query: { kind: "string" }, | |
| 8 | query: { kind: "string", limit: "integer", cursor: "string" }, | |
| 9 | 9 | }; |
src/core/decisions.ts +87 −16
| @@ -249,24 +249,95 @@ export function resolveOpen( | ||
| 249 | 249 | export function listDecisions( |
| 250 | 250 | db: Database, |
| 251 | 251 | repoId: string, |
| 252 | options: { kind?: DecisionKind | undefined; limit?: number | undefined } = {}, | |
| 252 | options: { | |
| 253 | kind?: DecisionKind | undefined; | |
| 254 | limit?: number | undefined; | |
| 255 | /** The id of the last row of the page before: read what is older. */ | |
| 256 | before?: string | undefined; | |
| 257 | } = {}, | |
| 253 | 258 | ): Decision[] { |
| 254 | 259 | const limit = Math.min(500, Math.max(1, options.limit ?? 100)); |
| 255 | const rows = | |
| 256 | options.kind === undefined | |
| 257 | ? db | |
| 258 | .query< | |
| 259 | Row, | |
| 260 | [string, number] | |
| 261 | >("SELECT * FROM decision WHERE repo_id = ?1 ORDER BY created_at DESC, id DESC LIMIT ?2") | |
| 262 | .all(repoId, limit) | |
| 263 | : db | |
| 264 | .query< | |
| 265 | Row, | |
| 266 | [string, string, number] | |
| 267 | >("SELECT * FROM decision WHERE repo_id = ?1 AND kind = ?2 ORDER BY created_at DESC, id DESC LIMIT ?3") | |
| 268 | .all(repoId, options.kind, limit); | |
| 269 | return rows.map(toDecision); | |
| 260 | const args: (string | number)[] = [repoId]; | |
| 261 | let where = "repo_id = ?1"; | |
| 262 | if (options.kind !== undefined) { | |
| 263 | args.push(options.kind); | |
| 264 | where += ` AND kind = ?${args.length}`; | |
| 265 | } | |
| 266 | if (options.before !== undefined) { | |
| 267 | args.push(options.before); | |
| 268 | where += ` AND (created_at, id) < (SELECT created_at, id FROM decision WHERE id = ?${args.length})`; | |
| 269 | } | |
| 270 | args.push(limit); | |
| 271 | return db | |
| 272 | .query<Row, (string | number)[]>( | |
| 273 | `SELECT * FROM decision WHERE ${where} ORDER BY created_at DESC, id DESC LIMIT ?${args.length}`, | |
| 274 | ) | |
| 275 | .all(...args) | |
| 276 | .map(toDecision); | |
| 277 | } | |
| 278 | ||
| 279 | export const DECISIONS_LIMIT_DEFAULT = 30; | |
| 280 | export const DECISIONS_LIMIT_MAX = 100; | |
| 281 | ||
| 282 | /** A `limit` as the decisions read takes it: a whole number 1 to 100, or undefined when bad. */ | |
| 283 | export function decisionsLimit(raw: string | null): number | undefined { | |
| 284 | if (raw === null) return DECISIONS_LIMIT_DEFAULT; | |
| 285 | const n = /^\d{1,4}$/.test(raw) ? Number(raw) : 0; | |
| 286 | return n >= 1 && n <= DECISIONS_LIMIT_MAX ? n : undefined; | |
| 287 | } | |
| 288 | ||
| 289 | /** Whether `cursor` names a decision of this repository (a `next_cursor` of a page before). */ | |
| 290 | export function decisionCursorValid(db: Database, repoId: string, cursor: string): boolean { | |
| 291 | return ( | |
| 292 | /^[A-Za-z0-9_-]{1,64}$/.test(cursor) && | |
| 293 | db.query("SELECT 1 FROM decision WHERE id = ?1 AND repo_id = ?2").get(cursor, repoId) !== null | |
| 294 | ); | |
| 295 | } | |
| 296 | ||
| 297 | /** Rows read per batch, and batches per request, while hunting for visible ones. */ | |
| 298 | const BATCH = 100; | |
| 299 | const BATCHES = 5; | |
| 300 | ||
The 500-row bound (BATCH × BATCHES) is what keeps a guest from making this read scan a whole long sealed ledger. Keep it if you refactor. |
||
| 301 | /** | |
| 302 | * One page of the ledger, newest first, after `cursor`. `visible` runs before | |
| 303 | * the page is cut, so a hidden row never shortens a page; the read is bounded, | |
| 304 | * and past the bound the page may be short, its `next_cursor` the last row it | |
| 305 | * showed (or, when it showed none, the last row scanned). | |
| 306 | */ | |
| 307 | export function decisionsPage( | |
| 308 | db: Database, | |
| 309 | repoId: string, | |
| 310 | options: { | |
| 311 | kind?: DecisionKind | undefined; | |
| 312 | limit: number; | |
| 313 | cursor?: string | undefined; | |
| 314 | visible: (d: Decision) => boolean; | |
| 315 | }, | |
| 316 | ): { decisions: Decision[]; next_cursor: string | null } { | |
| 317 | const found: Decision[] = []; | |
| 318 | let before = options.cursor; | |
| 319 | let scanned: string | undefined; | |
| 320 | let more = false; | |
| 321 | for (let n = 0; n < BATCHES && found.length <= options.limit; n++) { | |
| 322 | const batch = listDecisions(db, repoId, { kind: options.kind, limit: BATCH, before }); | |
| 323 | for (const d of batch) if (options.visible(d)) found.push(d); | |
| 324 | if (batch.length > 0) scanned = batch[batch.length - 1]!.id; | |
| 325 | more = batch.length === BATCH; | |
| 326 | if (!more) break; | |
| 327 | before = scanned; | |
| 328 | } | |
| 329 | const page = found.slice(0, options.limit); | |
| 330 | // A cursor is the last row this reader was shown. Only a page that found | |
| 331 | // nothing for a whole bound resumes after a row it scanned (maybe hidden): | |
| 332 | // without that the read could not move past a long run of hidden rows. | |
| 333 | const shown = found[Math.min(found.length, options.limit) - 1]; | |
| 334 | const next = | |
outdated
When the 500-row bound is hit,
outdated
Here |
||
| 335 | found.length > options.limit | |
| 336 | ? shown!.id | |
| 337 | : more | |
| 338 | ? (shown?.id ?? scanned ?? null) | |
| 339 | : null; | |
| 340 | return { decisions: page, next_cursor: next }; | |
| 270 | 341 | } |
| 271 | 342 | |
| 272 | 343 | /** The newest decisions across several repositories, newest first. */ |
src/http/api.ts +27 −12
| @@ -293,7 +293,13 @@ import { dependenciesOf, repoGraph } from "../core/dependencies.ts"; | ||
| 293 | 293 | import { mapHeldFor } from "../crew/held.ts"; |
| 294 | 294 | import { pillarView } from "../core/pillars.ts"; |
| 295 | 295 | import { askToLiveFor } from "../web/ask_to_live.ts"; |
| 296 | import { decisionStats, listDecisions } from "../core/decisions.ts"; | |
| 296 | import { | |
| 297 | DECISIONS_LIMIT_MAX, | |
| 298 | decisionCursorValid, | |
| 299 | decisionStats, | |
| 300 | decisionsLimit, | |
| 301 | decisionsPage, | |
| 302 | } from "../core/decisions.ts"; | |
| 297 | 303 | import { forReader } from "../crew/handover.ts"; |
| 298 | 304 | import { globalMapJson } from "../web/pages/global-map.ts"; |
| 299 | 305 | import { mapJson, marksFor } from "../web/pages/map.ts"; |
| @@ -3156,10 +3162,17 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3156 | 3162 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 3157 | 3163 | const repo = loadRepo(ctx); |
| 3158 | 3164 | if (!repo) return notFound(); |
| 3159 | const kind = ctx.url.searchParams.get("kind"); | |
| 3165 | const q = ctx.url.searchParams; | |
| 3166 | const kind = q.get("kind"); | |
| 3167 | const limit = decisionsLimit(q.get("limit")); | |
| 3168 | if (limit === undefined) | |
| 3169 | return apiError("invalid_limit", 422, `limit is a whole number from 1 to ${DECISIONS_LIMIT_MAX}`); | |
| 3170 | const cursor = q.get("cursor"); | |
This route's contract lives in |
||
| 3171 | if (cursor !== null && !decisionCursorValid(ctx.app.db, repo.id, cursor)) | |
| 3172 | return apiError("invalid_cursor", 422, "cursor is the next_cursor of a page before"); | |
| 3160 | 3173 | const week = new Date(Date.now() - 7 * 24 * 60 * 60_000).toISOString(); |
| 3161 | const read = timePhase(ctx.request, "decisions", () => ({ | |
| 3162 | decisions: listDecisions(ctx.app.db, repo.id, { | |
| 3174 | const read = timePhase(ctx.request, "decisions", () => { | |
| 3175 | const page = decisionsPage(ctx.app.db, repo.id, { | |
| 3163 | 3176 | kind: |
| 3164 | 3177 | kind === "next" || |
| 3165 | 3178 | kind === "land" || |
| @@ -3167,16 +3180,18 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3167 | 3180 | kind === "guard" |
| 3168 | 3181 | ? kind |
| 3169 | 3182 | : undefined, |
| 3170 | limit: 100, | |
| 3171 | }) | |
| 3183 | limit, | |
| 3184 | cursor: cursor ?? undefined, | |
| 3172 | 3185 | // What the decider saw about a sealed report is the report's (`sealed.ts`). |
| 3173 | .filter( | |
| 3174 | (d) => mayViewId(ctx.app.db, repo, d.issueId, ctx.auth.user), | |
| 3175 | ) | |
| 3186 | visible: (d) => mayViewId(ctx.app.db, repo, d.issueId, ctx.auth.user), | |
| 3187 | }); | |
| 3188 | return { | |
| 3176 | 3189 | // A hand-over's state carries its tapper's private preferences. |
| 3177 | .map((d) => forReader(d, ctx.auth.user)), | |
| 3178 | week: decisionStats(ctx.app.db, repo.id, week), | |
| 3179 | })); | |
| 3190 | decisions: page.decisions.map((d) => forReader(d, ctx.auth.user)), | |
| 3191 | week: decisionStats(ctx.app.db, repo.id, week), | |
| 3192 | next_cursor: page.next_cursor, | |
| 3193 | }; | |
| 3194 | }); | |
| 3180 | 3195 | return timePhase(ctx.request, "render", () => json(read)); |
| 3181 | 3196 | }); |
| 3182 | 3197 |
test/api-decisions-timing.test.ts +102 −2
| @@ -4,9 +4,11 @@ import { mkdtemp, rm } from "node:fs/promises"; | ||
| 4 | 4 | import { tmpdir } from "node:os"; |
| 5 | 5 | import { join } from "node:path"; |
| 6 | 6 | import { createApp, type App } from "../src/app.ts"; |
| 7 | import { createIssue } from "../src/core/issues.ts"; | |
| 8 | import { recordDecision } from "../src/core/decisions.ts"; | |
| 7 | 9 | import { createRepo } from "../src/actions/repos.ts"; |
| 8 | 10 | import { createSession } from "../src/core/sessions.ts"; |
| 9 | import { createUser } from "../src/core/users.ts"; | |
| 11 | import { createUser, getUserById } from "../src/core/users.ts"; | |
| 10 | 12 | import { serve, type Servers } from "../src/http/server.ts"; |
| 11 | 13 | import { quietLogs } from "./helpers/globals.ts"; |
| 12 | 14 | |
| @@ -57,7 +59,7 @@ test("the decisions read names session, decisions, render in order", async () => | ||
| 57 | 59 | const got = await phases("/api/v1/repos/alice/r/decisions"); |
| 58 | 60 | expect(got.status).toBe(200); |
| 59 | 61 | expect(got.names).toEqual(["session", "decisions", "render"]); |
| 60 | expect(Object.keys(JSON.parse(got.body))).toEqual(["decisions", "week"]); | |
| 62 | expect(Object.keys(JSON.parse(got.body))).toEqual(["decisions", "week", "next_cursor"]); | |
| 61 | 63 | }); |
| 62 | 64 | |
| 63 | 65 | test("a missing repository is a 404 naming session", async () => { |
| @@ -65,3 +67,101 @@ test("a missing repository is a 404 naming session", async () => { | ||
| 65 | 67 | expect(got.status).toBe(404); |
| 66 | 68 | expect(got.names).toEqual(["session"]); |
| 67 | 69 | }); |
| 70 | ||
outdated
Add a case with hidden rows: decisions on a sealed issue mixed in with visible ones, read as a guest. Assert that a page is full, that no sealed decision appears, and that paging reaches every visible row once. Also assert that another repository's decision id used as |
||
| 71 | test("decisions page: default 30, paging to the end, both 422s", async () => { | |
| 72 | const repo = app.db.query<{ id: string }, []>("SELECT id FROM repo WHERE name = 'r'").get()!; | |
| 73 | for (let i = 0; i < 65; i++) | |
| 74 | recordDecision(app.db, { kind: "next", repoId: repo.id, state: {}, answer: `a${i}`, p: 1, by: "rule", ms: 1 }); | |
| 75 | const get = async (q = "") => { | |
| 76 | const res = await fetch(`${servers.url}/api/v1/repos/alice/r/decisions${q}`, { headers: { cookie } }); | |
| 77 | return { status: res.status, body: (await res.json()) as any }; | |
| 78 | }; | |
| 79 | const first = await get(); | |
| 80 | expect(first.body.decisions).toHaveLength(30); | |
| 81 | const seen: string[] = []; | |
| 82 | let page = first; | |
| 83 | for (;;) { | |
| 84 | seen.push(...page.body.decisions.map((d: { id: string }) => d.id)); | |
| 85 | if (page.body.next_cursor === null) break; | |
| 86 | page = await get(`?limit=30&cursor=${page.body.next_cursor}`); | |
| 87 | } | |
| 88 | expect(seen).toHaveLength(65); | |
| 89 | expect(new Set(seen).size).toBe(65); | |
| 90 | expect((await get("?limit=0")).body.error).toBe("invalid_limit"); | |
| 91 | expect((await get("?limit=101")).status).toBe(422); | |
| 92 | expect((await get("?limit=x")).status).toBe(422); | |
| 93 | const bad = await get("?cursor=nope"); | |
| 94 | expect(bad.status).toBe(422); | |
| 95 | expect(bad.body.error).toBe("invalid_cursor"); | |
| 96 | }); | |
| 97 | ||
| 98 | test("decisions page: sealed rows are filtered before the page is cut; a cursor from another repository is refused", async () => { | |
| 99 | const alice = app.db.query<{ id: string }, []>("SELECT id FROM user WHERE username = 'alice'").get()!; | |
| 100 | const made = await createRepo(app, getUserById(app.db, alice.id)!, { | |
| 101 | name: "h", | |
| 102 | visibility: "public", | |
| 103 | }); | |
| 104 | if (!made.ok) throw new Error(String(made.error)); | |
| 105 | const repoId = made.value.id; | |
| 106 | const sealed = createIssue(app.db, { repoId, authorId: alice.id, title: "report", sealed: true }); | |
| 107 | if (!sealed.ok) throw new Error(String(sealed.error)); | |
| 108 | const open = createIssue(app.db, { repoId, authorId: alice.id, title: "plain" }); | |
| 109 | if (!open.ok) throw new Error(String(open.error)); | |
| 110 | const visible: string[] = []; | |
| 111 | const hidden = new Set<string>(); | |
| 112 | // Two hidden rows to every visible one, so a slice-then-filter page comes up short. | |
| 113 | for (let i = 0; i < 12; i++) { | |
| 114 | for (const issueId of [sealed.value.id, sealed.value.id, open.value.id]) { | |
| 115 | const d = recordDecision(app.db, { kind: "next", repoId, issueId, state: {}, answer: `a${i}`, p: 1, by: "rule", ms: 1 }); | |
| 116 | if (issueId === open.value.id) visible.push(d.id); | |
| 117 | else hidden.add(d.id); | |
| 118 | } | |
| 119 | } | |
| 120 | const guest = async (q: string) => { | |
| 121 | const res = await fetch(`${servers.url}/api/v1/repos/alice/h/decisions${q}`); | |
| 122 | return { status: res.status, body: (await res.json()) as any }; | |
| 123 | }; | |
| 124 | const seen: string[] = []; | |
| 125 | let q = "?limit=5"; | |
| 126 | for (;;) { | |
| 127 | const page = await guest(q); | |
| 128 | expect(page.status).toBe(200); | |
| 129 | const ids = page.body.decisions.map((d: { id: string }) => d.id); | |
| 130 | for (const id of ids) expect(hidden.has(id)).toBe(false); | |
| 131 | seen.push(...ids); | |
| 132 | if (page.body.next_cursor === null) break; | |
| 133 | expect(ids).toHaveLength(5); | |
| 134 | q = `?limit=5&cursor=${page.body.next_cursor}`; | |
| 135 | } | |
| 136 | expect(seen.sort()).toEqual([...visible].sort()); | |
| 137 | const other = app.db.query<{ id: string }, []>("SELECT id FROM decision WHERE repo_id != '" + repoId + "' LIMIT 1").get()!; | |
| 138 | const refused = await guest(`?cursor=${other.id}`); | |
| 139 | expect(refused.status).toBe(422); | |
| 140 | expect(refused.body.error).toBe("invalid_cursor"); | |
| 141 | }); | |
| 142 | ||
| 143 | test("decisions page: past the scan bound a page is short, and its cursor still reaches the visible row once", async () => { | |
| 144 | const alice = getUserById(app.db, app.db.query<{ id: string }, []>("SELECT id FROM user WHERE username = 'alice'").get()!.id)!; | |
| 145 | const made = await createRepo(app, alice, { name: "bound", visibility: "public" }); | |
| 146 | if (!made.ok) throw new Error(String(made.error)); | |
| 147 | const repoId = made.value.id; | |
| 148 | const sealed = createIssue(app.db, { repoId, authorId: alice.id, title: "report", sealed: true }); | |
| 149 | const open = createIssue(app.db, { repoId, authorId: alice.id, title: "plain" }); | |
| 150 | if (!sealed.ok || !open.ok) throw new Error("issue"); | |
| 151 | const row = (issueId: string) => | |
| 152 | recordDecision(app.db, { kind: "next", repoId, issueId, state: {}, answer: "a", p: 1, by: "rule", ms: 1 }); | |
| 153 | const visibleId = row(open.value.id).id; | |
| 154 | for (let i = 0; i < 505; i++) row(sealed.value.id); | |
| 155 | const get = async (q: string) => | |
| 156 | (await (await fetch(`${servers.url}/api/v1/repos/alice/bound/decisions${q}`)).json()) as any; | |
| 157 | const first = await get("?limit=5"); | |
| 158 | expect(first.decisions).toEqual([]); | |
| 159 | expect(first.next_cursor).not.toBeNull(); | |
| 160 | const seen: string[] = []; | |
| 161 | let page = first; | |
| 162 | while (page.next_cursor !== null) { | |
| 163 | page = await get(`?limit=5&cursor=${page.next_cursor}`); | |
| 164 | seen.push(...page.decisions.map((d: { id: string }) => d.id)); | |
| 165 | } | |
| 166 | expect(seen).toEqual([visibleId]); | |
| 167 | }); |
new above
offline
The bound (BATCH × BATCHES = 500 rows) is new behaviour with no test. Add a case where more than 500 hidden rows sit in front of a visible one. Assert a short page, a non-null
next_cursor, and that paging on reaches the visible row.