From 2b8d2c0825020b2b03706c4f9174072849464556 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 16:42:56 -0400 Subject: [PATCH 01/14] feat(comment-render): add shared live-link scope helpers --- apps/api/src/feed-service.ts | 9 +-- apps/api/test/comment-render-scope.test.ts | 63 +++++++++++++++++ packages/comment-render/package.json | 3 +- packages/comment-render/src/scope.ts | 67 +++++++++++++++++++ packages/uploads/scripts/inline-shared.mjs | 4 ++ .../src/comment-render-scope.generated.ts | 67 +++++++++++++++++++ 6 files changed, 208 insertions(+), 5 deletions(-) create mode 100644 apps/api/test/comment-render-scope.test.ts create mode 100644 packages/comment-render/src/scope.ts create mode 100644 packages/uploads/src/comment-render-scope.generated.ts diff --git a/apps/api/src/feed-service.ts b/apps/api/src/feed-service.ts index e5e5b207..37782194 100644 --- a/apps/api/src/feed-service.ts +++ b/apps/api/src/feed-service.ts @@ -24,7 +24,8 @@ import { createLaneResolver, objectPublicUrls, type LaneResolver } from "./stora import type { StorageConfig } from "@uploads/storage"; import { objectVisibility } from "./visibility"; import { webOrigin } from "./web-url"; -import { sha256Hex, type WorkspaceRecord } from "./workspace"; +import { feedItemIdFor } from "@uploads/comment-render/scope"; +import { type WorkspaceRecord } from "./workspace"; import { dbFor, type D1Queryable } from "./db-session"; type FeedObjectHead = { @@ -112,9 +113,9 @@ export function feedUrl(env: Env, id: string): string { return webOrigin(env) + "/c/" + encodeURIComponent(id); } -/** Stable public-item id: first 32 hex chars of SHA-256(object key). */ -export async function feedItemId(objectKey: string): Promise { - return (await sha256Hex(objectKey)).slice(0, 32); +/** Stable public-item id: first 32 hex chars of SHA-256(object key). One implementation, shared with the web and CLI. */ +export function feedItemId(objectKey: string): Promise { + return feedItemIdFor(objectKey); } export function feedItemUrl(env: Env, feedId: string, itemId: string): string { diff --git a/apps/api/test/comment-render-scope.test.ts b/apps/api/test/comment-render-scope.test.ts new file mode 100644 index 00000000..1a7c997d --- /dev/null +++ b/apps/api/test/comment-render-scope.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from "vitest"; +import { + FILE_TYPE_CLASSES, + IMAGE_EXTENSIONS, + VIDEO_EXTENSIONS, + feedItemIdFor, + fileTypeClassFromKey, + isInFeedScope, +} from "@uploads/comment-render/scope"; +import { feedItemId } from "../src/feed-service"; +import { sha256Hex } from "../src/workspace"; + +describe("fileTypeClassFromKey", () => { + it("uses the same extension sets as the web shotKindFromKey", () => { + expect([...IMAGE_EXTENSIONS]).toEqual(["png", "jpg", "jpeg", "webp", "gif", "avif"]); + expect([...VIDEO_EXTENSIONS]).toEqual(["mp4", "webm", "mov"]); + expect([...FILE_TYPE_CLASSES]).toEqual(["screenshot", "video", "other"]); + }); + + it("classifies by the last extension, case-insensitively", () => { + expect(fileTypeClassFromKey("gh/acme/app/pull/1/shot.png")).toBe("screenshot"); + expect(fileTypeClassFromKey("SHOT.JPEG")).toBe("screenshot"); + expect(fileTypeClassFromKey("x.avif")).toBe("screenshot"); + expect(fileTypeClassFromKey("clip.MOV")).toBe("video"); + expect(fileTypeClassFromKey("clip.webm")).toBe("video"); + expect(fileTypeClassFromKey("report.pdf")).toBe("other"); + expect(fileTypeClassFromKey("notes")).toBe("other"); + expect(fileTypeClassFromKey("archive.png.zip")).toBe("other"); + expect(fileTypeClassFromKey("dir.png/readme")).toBe("other"); + }); +}); + +describe("isInFeedScope", () => { + const scope = { repo: "acme/app", number: 7 }; + + it("matches repo and number, and treats a missing number as repo-wide", () => { + const meta = { "gh.repo": "acme/app", "gh.number": "7" }; + expect(isInFeedScope(meta, scope)).toBe(true); + expect(isInFeedScope({ ...meta, "gh.number": "8" }, scope)).toBe(false); + expect(isInFeedScope({ "gh.repo": "acme/app" }, { repo: "acme/app" })).toBe(true); + expect(isInFeedScope({ "gh.repo": "acme/app" }, { repo: "acme/app", number: 0 })).toBe(true); + }); + + it("excludes promoted shadows, other repos, and a mixed-case gh.repo (writes store it lowercased, Task 2)", () => { + expect( + isInFeedScope({ "gh.repo": "acme/app", "gh.number": "7", "gh.status": "promoted" }, scope), + ).toBe(false); + expect(isInFeedScope({ "gh.repo": "acme/web", "gh.number": "7" }, scope)).toBe(false); + expect(isInFeedScope({ "gh.number": "7" }, scope)).toBe(false); + expect(isInFeedScope({ "gh.repo": "Acme/App", "gh.number": "7" }, scope)).toBe(false); + }); +}); + +describe("feedItemIdFor", () => { + it("is the first 32 hex chars of sha256(key) and equals the API feedItemId", async () => { + for (const key of ["gh/acme/app/pull/7/shot.png", "shots/café-née.png"]) { + const id = await feedItemIdFor(key); + expect(id).toMatch(/^[0-9a-f]{32}$/); + expect(id).toBe((await sha256Hex(key)).slice(0, 32)); + expect(await feedItemId(key)).toBe(id); + } + }); +}); diff --git a/packages/comment-render/package.json b/packages/comment-render/package.json index e5aa8450..9ee866c3 100644 --- a/packages/comment-render/package.json +++ b/packages/comment-render/package.json @@ -4,7 +4,8 @@ "private": true, "type": "module", "exports": { - ".": "./src/index.ts" + ".": "./src/index.ts", + "./scope": "./src/scope.ts" }, "scripts": { "typecheck": "tsc --noEmit" diff --git a/packages/comment-render/src/scope.ts b/packages/comment-render/src/scope.ts new file mode 100644 index 00000000..4c5c1276 --- /dev/null +++ b/packages/comment-render/src/scope.ts @@ -0,0 +1,67 @@ +/** + * Env-free scope helpers shared by the API (`apps/api/src/pr-scope.ts`), the + * web app, and the CLI's generated copy. One type classifier, one scope + * predicate, and one item-id hash, so every surface agrees on which objects + * a live link (change feed) holds and how its items are addressed. + */ + +export type FileTypeClass = "screenshot" | "video" | "other"; + +export const FILE_TYPE_CLASSES: readonly FileTypeClass[] = ["screenshot", "video", "other"]; + +/** Same sets as `apps/web/src/lib/workspace-screenshots.ts` `shotKindFromKey`. */ +export const IMAGE_EXTENSIONS: readonly string[] = ["png", "jpg", "jpeg", "webp", "gif", "avif"]; +export const VIDEO_EXTENSIONS: readonly string[] = ["mp4", "webm", "mov"]; + +/** + * Media class from the key's last extension. `file_metadata` has no content + * type, so this is inferred, and keys without a known extension are "other". + * The API's SQL type filter is built from the same two lists. + */ +export function fileTypeClassFromKey(key: string): FileTypeClass { + const match = /\.([a-z0-9]{1,8})$/i.exec(key); + const ext = match?.[1]?.toLowerCase() ?? ""; + if (IMAGE_EXTENSIONS.includes(ext)) return "screenshot"; + if (VIDEO_EXTENSIONS.includes(ext)) return "video"; + return "other"; +} + +/** `repo` is lowercased `owner/repo`; `number` > 0 or absent (repo-wide). */ +export interface FeedScope { + repo: string; + number?: number; +} + +/** + * Whether an object's own metadata puts it in a live link's scope. Mirrors the + * API's scope SQL exactly: `gh.repo` equals the lowercased scope repo (no + * lowercasing of the stored value, because the SQL compares it verbatim and + * every write stores it lowercased: apps/api file-metadata.ts + * `canonicalMetaValue`), + * `gh.number` equals the number as a decimal string when the scope has one, + * and promoted branch shadows are excluded. + */ +export function isInFeedScope(metadata: Record, scope: FeedScope): boolean { + if (metadata["gh.repo"] !== scope.repo) return false; + if (scope.number !== undefined && scope.number > 0) { + if (metadata["gh.number"] !== String(scope.number)) return false; + } + return metadata["gh.status"] !== "promoted"; +} + +// Typed locally: this package compiles with `lib: ["ES2022"]` and no DOM or +// Workers types, while the API (Workers), web, and CLI (Node) all provide +// Web Crypto and TextEncoder at runtime. +interface WebCryptoGlobals { + crypto: { subtle: { digest(algorithm: "SHA-256", data: Uint8Array): Promise } }; + TextEncoder: new () => { encode(input: string): Uint8Array }; +} + +/** Stable live-link item id: first 32 hex chars of SHA-256(object key). */ +export async function feedItemIdFor(objectKey: string): Promise { + const g = globalThis as unknown as WebCryptoGlobals; + const digest = await g.crypto.subtle.digest("SHA-256", new g.TextEncoder().encode(objectKey)); + let hex = ""; + for (const byte of new Uint8Array(digest)) hex += byte.toString(16).padStart(2, "0"); + return hex.slice(0, 32); +} diff --git a/packages/uploads/scripts/inline-shared.mjs b/packages/uploads/scripts/inline-shared.mjs index 6607d0f2..a215865e 100644 --- a/packages/uploads/scripts/inline-shared.mjs +++ b/packages/uploads/scripts/inline-shared.mjs @@ -29,6 +29,10 @@ const copies = [ source: "packages/comment-render/src/index.ts", dest: "packages/uploads/src/comment-render.generated.ts", }, + { + source: "packages/comment-render/src/scope.ts", + dest: "packages/uploads/src/comment-render-scope.generated.ts", + }, { source: "packages/comment-config/src/index.ts", dest: "packages/uploads/src/comment-config.generated.ts", diff --git a/packages/uploads/src/comment-render-scope.generated.ts b/packages/uploads/src/comment-render-scope.generated.ts new file mode 100644 index 00000000..0cdc0aca --- /dev/null +++ b/packages/uploads/src/comment-render-scope.generated.ts @@ -0,0 +1,67 @@ +/** + * GENERATED by packages/uploads/scripts/inline-shared.mjs — do not edit. + * Canonical source: packages/comment-render/src/scope.ts + * The published CLI cannot import private @uploads/* packages, so this file + * is inlined into the tarball. Re-run the script after changing the source. + */ + +export type FileTypeClass = "screenshot" | "video" | "other"; + +export const FILE_TYPE_CLASSES: readonly FileTypeClass[] = ["screenshot", "video", "other"]; + +/** Same sets as `apps/web/src/lib/workspace-screenshots.ts` `shotKindFromKey`. */ +export const IMAGE_EXTENSIONS: readonly string[] = ["png", "jpg", "jpeg", "webp", "gif", "avif"]; +export const VIDEO_EXTENSIONS: readonly string[] = ["mp4", "webm", "mov"]; + +/** + * Media class from the key's last extension. `file_metadata` has no content + * type, so this is inferred, and keys without a known extension are "other". + * The API's SQL type filter is built from the same two lists. + */ +export function fileTypeClassFromKey(key: string): FileTypeClass { + const match = /\.([a-z0-9]{1,8})$/i.exec(key); + const ext = match?.[1]?.toLowerCase() ?? ""; + if (IMAGE_EXTENSIONS.includes(ext)) return "screenshot"; + if (VIDEO_EXTENSIONS.includes(ext)) return "video"; + return "other"; +} + +/** `repo` is lowercased `owner/repo`; `number` > 0 or absent (repo-wide). */ +export interface FeedScope { + repo: string; + number?: number; +} + +/** + * Whether an object's own metadata puts it in a live link's scope. Mirrors the + * API's scope SQL exactly: `gh.repo` equals the lowercased scope repo (no + * lowercasing of the stored value, because the SQL compares it verbatim and + * every write stores it lowercased: apps/api file-metadata.ts + * `canonicalMetaValue`), + * `gh.number` equals the number as a decimal string when the scope has one, + * and promoted branch shadows are excluded. + */ +export function isInFeedScope(metadata: Record, scope: FeedScope): boolean { + if (metadata["gh.repo"] !== scope.repo) return false; + if (scope.number !== undefined && scope.number > 0) { + if (metadata["gh.number"] !== String(scope.number)) return false; + } + return metadata["gh.status"] !== "promoted"; +} + +// Typed locally: this package compiles with `lib: ["ES2022"]` and no DOM or +// Workers types, while the API (Workers), web, and CLI (Node) all provide +// Web Crypto and TextEncoder at runtime. +interface WebCryptoGlobals { + crypto: { subtle: { digest(algorithm: "SHA-256", data: Uint8Array): Promise } }; + TextEncoder: new () => { encode(input: string): Uint8Array }; +} + +/** Stable live-link item id: first 32 hex chars of SHA-256(object key). */ +export async function feedItemIdFor(objectKey: string): Promise { + const g = globalThis as unknown as WebCryptoGlobals; + const digest = await g.crypto.subtle.digest("SHA-256", new g.TextEncoder().encode(objectKey)); + let hex = ""; + for (const byte of new Uint8Array(digest)) hex += byte.toString(16).padStart(2, "0"); + return hex.slice(0, 32); +} From 32149c5a85323be26f4d20d90746f0d816aeea9c Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 16:45:40 -0400 Subject: [PATCH 02/14] feat(api): add the shared PR scope query with type filter and keyset cursor --- ...261004120200_file_metadata_gh_repo_idx.sql | 12 + apps/api/src/feed-service.ts | 58 +-- apps/api/src/file-metadata.ts | 27 +- apps/api/src/file-type-sql.ts | 52 +++ apps/api/src/pr-scope.ts | 233 ++++++++++++ apps/api/test/file-type-sql.test.ts | 62 ++++ apps/api/test/pr-scope-sqlite.test.ts | 350 ++++++++++++++++++ 7 files changed, 745 insertions(+), 49 deletions(-) create mode 100644 apps/api/migrations/20261004120200_file_metadata_gh_repo_idx.sql create mode 100644 apps/api/src/file-type-sql.ts create mode 100644 apps/api/src/pr-scope.ts create mode 100644 apps/api/test/file-type-sql.test.ts create mode 100644 apps/api/test/pr-scope-sqlite.test.ts diff --git a/apps/api/migrations/20261004120200_file_metadata_gh_repo_idx.sql b/apps/api/migrations/20261004120200_file_metadata_gh_repo_idx.sql new file mode 100644 index 00000000..c63b4194 --- /dev/null +++ b/apps/api/migrations/20261004120200_file_metadata_gh_repo_idx.sql @@ -0,0 +1,12 @@ +-- Newest-first scope reads (apps/api/src/pr-scope.ts: live links, Files +-- views, the pager) and the By repo list. Partial on purpose: only `gh.repo` +-- rows (one per GitHub-tagged object) pay the extra write, unlike the +-- all-rows value index dropped in 20260722190000. Serves +-- WHERE workspace = ? AND meta_key = 'gh.repo' AND meta_value = ? +-- ORDER BY updated_at DESC, object_key ASC +-- as a covering, in-order scan (no temp sort), so LIMIT stops early, and +-- the distinct-repo GROUP BY reads only this index. Queries must use the +-- literal 'gh.repo' (not a bound parameter) for SQLite to pick it. +CREATE INDEX IF NOT EXISTS file_metadata_gh_repo_recent_idx + ON file_metadata (workspace, meta_value, updated_at DESC, object_key) + WHERE meta_key = 'gh.repo'; diff --git a/apps/api/src/feed-service.ts b/apps/api/src/feed-service.ts index 37782194..ef95f3cc 100644 --- a/apps/api/src/feed-service.ts +++ b/apps/api/src/feed-service.ts @@ -11,6 +11,7 @@ import { } from "@uploads/errors"; import { publicObjectDateFields } from "./files-core"; import { getMetadataForKeys } from "./file-metadata"; +import { prScopeQuery, type ScopeItem } from "./pr-scope"; import { FEED_ID_RE, FEED_ITEM_LIMIT, @@ -196,57 +197,24 @@ export function feedItemFilename(objectKey: string): string { } /** - * Newest-first objects tagged `gh.repo=`, optionally also `gh.number` - * and `path`. Drops promoted branch shadows so a promoted shot is not listed - * twice. Does not require `gh.merged` — merge signal is out of scope for v1. - * Kind is display-only; GitHub numbers are unique per repo. + * The newest (at most `FEED_ITEM_LIMIT`) items in a feed's scope. Thin + * wrapper over the shared scope query (`pr-scope.ts`), kept for the comment + * sync and owner-feed call sites. */ export async function findLatestRepoScreenshots( db: D1Queryable, workspace: string, opts: { repo: string; path?: string; number?: number; limit?: number }, -): Promise }>> { +): Promise { const limit = Math.max(1, Math.min(opts.limit ?? FEED_ITEM_LIMIT, FEED_ITEM_LIMIT)); - const params: unknown[] = [workspace, opts.repo]; - let sql = `SELECT r.object_key AS object_key, r.updated_at AS updated_at - FROM file_metadata r - WHERE r.workspace = ? AND r.meta_key = 'gh.repo' AND r.meta_value = ? - AND NOT EXISTS ( - SELECT 1 FROM file_metadata s - WHERE s.workspace = r.workspace AND s.object_key = r.object_key - AND s.meta_key = 'gh.status' AND s.meta_value = 'promoted' - )`; - if (opts.number) { - sql += ` AND EXISTS ( - SELECT 1 FROM file_metadata n - WHERE n.workspace = r.workspace AND n.object_key = r.object_key - AND n.meta_key = 'gh.number' AND n.meta_value = ? - )`; - params.push(String(opts.number)); - } - if (opts.path) { - sql += ` AND EXISTS ( - SELECT 1 FROM file_metadata p - WHERE p.workspace = r.workspace AND p.object_key = r.object_key - AND p.meta_key = 'path' AND p.meta_value = ? - )`; - params.push(opts.path); - } - sql += ` ORDER BY r.updated_at DESC, r.object_key ASC LIMIT ?`; - params.push(limit); - - const matched = await db - .prepare(sql) - .bind(...params) - .all<{ object_key: string; updated_at: string }>(); - const keys = matched.results.map((row) => row.object_key); - if (keys.length === 0) return []; - const byKey = await getMetadataForKeys(db, workspace, keys); - return matched.results.map((row) => ({ - key: row.object_key, - updatedAt: row.updated_at, - metadata: byKey.get(row.object_key) ?? {}, - })); + const page = await prScopeQuery(db, { + workspace, + repo: opts.repo, + ...(opts.number ? { number: opts.number } : {}), + ...(opts.path ? { path: opts.path } : {}), + limit, + }); + return page.items; } async function mapBounded( diff --git a/apps/api/src/file-metadata.ts b/apps/api/src/file-metadata.ts index 3141c999..189d1829 100644 --- a/apps/api/src/file-metadata.ts +++ b/apps/api/src/file-metadata.ts @@ -15,6 +15,19 @@ import { type D1Queryable } from "./db-session"; /** Lowercase key, optionally namespaced with dots (e.g. `gh.repo`). */ export const META_KEY_RE = /^[a-z][a-z0-9._-]{0,63}$/; +/** + * Metadata keys whose value is stored lowercased (every write and every search + * filter). `gh.repo`: the live-link scope (pr-scope.ts) and `isInFeedScope` + * match it exactly, and the PR rollup already lowercases its repo. The one-time + * backfill (Task 8, gh-repo-case-backfill.ts) reads this same list. + */ +export const LOWERCASED_META_KEYS: readonly string[] = ["gh.repo"]; + +/** Stored spelling of a metadata value; keys outside the list keep their value as written. */ +export function canonicalMetaValue(key: string, value: string): string { + return LOWERCASED_META_KEYS.includes(key) ? value.toLowerCase() : value; +} + /** * Server-set provenance keys (e.g. `content-sha256`) are reserved: a custom * metadata row with the same name would be a spoofable shadow of a value the @@ -296,7 +309,7 @@ function upsertStatements( ON CONFLICT(workspace, object_key, meta_key) DO UPDATE SET meta_value = excluded.meta_value, updated_at = excluded.updated_at`, ) - .bind(workspace, objectKey, key, value, now), + .bind(workspace, objectKey, key, canonicalMetaValue(key, value), now), ); } @@ -343,7 +356,7 @@ export async function setFileMetadata( const current = await getFileMetadata(db, workspace, objectKey); const next: Record = { ...current }; for (const key of remove) delete next[key]; - Object.assign(next, set); + for (const [key, value] of Object.entries(set)) next[key] = canonicalMetaValue(key, value); // `current` may already carry server-owned video.* rows (e.g. a poster), // so this post-merge pass enforces the count/byte caps on the merged @@ -424,7 +437,13 @@ export async function updateFileMetadataValue( .prepare( "UPDATE file_metadata SET meta_value = ?, updated_at = ? WHERE workspace = ? AND object_key = ? AND meta_key = ?", ) - .bind(value, new Date().toISOString(), workspace, objectKey, metaKey) + .bind( + canonicalMetaValue(metaKey, value), + new Date().toISOString(), + workspace, + objectKey, + metaKey, + ) .run(); } @@ -579,7 +598,7 @@ export async function findObjectsByMetadata( const limit = Math.max(1, Math.min(opts.limit ?? FIND_DEFAULT_LIMIT, FIND_PROBE_MAX_LIMIT)); const params: unknown[] = []; const legs = entries.map(([key, value]) => { - params.push(workspace, key, value); + params.push(workspace, key, canonicalMetaValue(key, value)); return `SELECT object_key FROM file_metadata WHERE workspace = ? AND meta_key = ? AND meta_value = ?`; }); diff --git a/apps/api/src/file-type-sql.ts b/apps/api/src/file-type-sql.ts new file mode 100644 index 00000000..ac480804 --- /dev/null +++ b/apps/api/src/file-type-sql.ts @@ -0,0 +1,52 @@ +/** + * The Files `type` filter as SQL (spec "Files views"): a key-extension + * class, matched with `lower(substr(col, -N))` because D1 caps LIKE/GLOB + * patterns at 50 bytes. Suffix sets come from `@uploads/comment-render/scope`, + * so the SQL and `fileTypeClassFromKey` agree (test/file-type-sql.test.ts + * pins the parity). One implementation for the scope query (pr-scope.ts), + * the Files endpoints (routes/workspace-scope.ts), and `GET /files/by-path`. + */ +import { ValidationError } from "@uploads/errors"; +import { + FILE_TYPE_CLASSES, + IMAGE_EXTENSIONS, + VIDEO_EXTENSIONS, + type FileTypeClass, +} from "@uploads/comment-render/scope"; + +const COLUMN_RE = /^[a-z_][a-z0-9_]*(\.[a-z_][a-z0-9_]*)?$/i; + +/** `(lower(substr(col, -4)) IN ('.png', …) OR lower(substr(col, -5)) IN (…))`. */ +function suffixMatchSql(column: string, extensions: readonly string[]): string { + const byLength = new Map(); + for (const ext of extensions) { + const suffix = `.${ext}`; + byLength.set(suffix.length, [...(byLength.get(suffix.length) ?? []), `'${suffix}'`]); + } + const terms = [...byLength.entries()] + .sort(([a], [b]) => a - b) + .map( + ([length, suffixes]) => `lower(substr(${column}, -${length})) IN (${suffixes.join(", ")})`, + ); + return `(${terms.join(" OR ")})`; +} + +/** SQL predicate on `column` (a trusted identifier such as `r.object_key`, never user input). */ +export function fileTypeSql(column: string, type: FileTypeClass): string { + if (!COLUMN_RE.test(column)) throw new Error(`fileTypeSql: bad column ${column}`); + const image = suffixMatchSql(column, IMAGE_EXTENSIONS); + const video = suffixMatchSql(column, VIDEO_EXTENSIONS); + if (type === "screenshot") return image; + if (type === "video") return video; + return `(NOT ${image} AND NOT ${video})`; +} + +/** `?type=`: undefined when absent or empty; 400 `invalid_type` for anything else. */ +export function parseFileTypeQuery(raw: string | undefined): FileTypeClass | undefined { + if (raw === undefined || raw === "") return undefined; + const match = FILE_TYPE_CLASSES.find((type) => type === raw); + if (match) return match; + throw new ValidationError("type must be screenshot, video, or other.", { + code: "invalid_type", + }); +} diff --git a/apps/api/src/pr-scope.ts b/apps/api/src/pr-scope.ts new file mode 100644 index 00000000..b1b74a1f --- /dev/null +++ b/apps/api/src/pr-scope.ts @@ -0,0 +1,233 @@ +/** + * The one scope query behind live links (change feeds), the signed-in Files + * views, and the live-link pager (spec + * .context/2026-10-04-pr-first-workspace-and-live-links.md, "Shared scope + * function"). Matches `gh.repo` exactly (every writer lowercases it), + * optionally `gh.number` and `path`, drops `gh.status=promoted` shadows, and + * never requires `path`. Newest first; keyset cursor on (updated_at, key). + * + * The type filter matches the key suffix with `substr`, never `LIKE`: D1 + * caps LIKE/GLOB patterns at 50 bytes (`fileTypeSql`, file-type-sql.ts, built + * from the `@uploads/comment-render/scope` lists so SQL and + * `fileTypeClassFromKey` agree). + * `file_metadata_gh_repo_recent_idx` (partial, `meta_key = 'gh.repo'`) + * serves the ORDER BY without a temp sort; keep the literal `'gh.repo'` in + * the SQL or SQLite cannot use that partial index. + */ +import { ValidationError } from "@uploads/errors"; +import type { FileTypeClass } from "@uploads/comment-render/scope"; +import { type D1Queryable } from "./db-session"; +import { getMetadataForKeys } from "./file-metadata"; +import { fileTypeSql } from "./file-type-sql"; + +export const SCOPE_DEFAULT_LIMIT = 50; +export const SCOPE_MAX_LIMIT = 100; +/** Hard cap on a whole-scope scan (pager, private count). */ +export const SCOPE_SCAN_CAP = 2000; + +export interface ScopeCursor { + updatedAt: string; + key: string; +} + +export interface ScopeQuery { + workspace: string; + /** Lowercased `owner/repo`. */ + repo: string; + number?: number; + path?: string; + /** Signed-in views only; live links never pass it. */ + type?: FileTypeClass; + cursor?: ScopeCursor | null; + /** Default 50, max 100. */ + limit?: number; +} + +export interface ScopeItem { + key: string; + updatedAt: string; + metadata: Record; +} + +interface ScopeRow { + object_key: string; + updated_at: string; +} + +function scopeFrom(q: Omit): { sql: string; params: unknown[] } { + const params: unknown[] = [q.workspace, q.repo]; + let sql = `FROM file_metadata r + WHERE r.workspace = ? AND r.meta_key = 'gh.repo' AND r.meta_value = ? + AND NOT EXISTS ( + SELECT 1 FROM file_metadata s + WHERE s.workspace = r.workspace AND s.object_key = r.object_key + AND s.meta_key = 'gh.status' AND s.meta_value = 'promoted' + )`; + if (q.number !== undefined && q.number > 0) { + sql += ` AND EXISTS ( + SELECT 1 FROM file_metadata n + WHERE n.workspace = r.workspace AND n.object_key = r.object_key + AND n.meta_key = 'gh.number' AND n.meta_value = ? + )`; + params.push(String(q.number)); + } + if (q.path) { + sql += ` AND EXISTS ( + SELECT 1 FROM file_metadata p + WHERE p.workspace = r.workspace AND p.object_key = r.object_key + AND p.meta_key = 'path' AND p.meta_value = ? + )`; + params.push(q.path); + } + if (q.type) sql += ` AND ${fileTypeSql("r.object_key", q.type)}`; + return { sql, params }; +} + +export function clampScopeLimit(limit: number | undefined): number { + if (limit === undefined || !Number.isFinite(limit)) return SCOPE_DEFAULT_LIMIT; + return Math.max(1, Math.min(SCOPE_MAX_LIMIT, Math.floor(limit))); +} + +/** One newest-first page of the scope, with metadata for each item. */ +export async function prScopeQuery( + db: D1Queryable, + q: ScopeQuery, +): Promise<{ items: ScopeItem[]; nextCursor: ScopeCursor | null }> { + const limit = clampScopeLimit(q.limit); + const { sql, params } = scopeFrom(q); + let select = `SELECT r.object_key AS object_key, r.updated_at AS updated_at ${sql}`; + if (q.cursor) { + select += ` AND (r.updated_at < ? OR (r.updated_at = ? AND r.object_key > ?))`; + params.push(q.cursor.updatedAt, q.cursor.updatedAt, q.cursor.key); + } + select += ` ORDER BY r.updated_at DESC, r.object_key ASC LIMIT ?`; + params.push(limit + 1); + + const { results } = await db + .prepare(select) + .bind(...params) + .all(); + const rows = results ?? []; + const hasMore = rows.length > limit; + const page = hasMore ? rows.slice(0, limit) : rows; + const byKey = + page.length > 0 + ? await getMetadataForKeys( + db, + q.workspace, + page.map((row) => row.object_key), + ) + : new Map>(); + const last = page.at(-1); + return { + items: page.map((row) => ({ + key: row.object_key, + updatedAt: row.updated_at, + metadata: byKey.get(row.object_key) ?? {}, + })), + nextCursor: hasMore && last ? { updatedAt: last.updated_at, key: last.object_key } : null, + }; +} + +/** + * The whole scope, newest first, up to `cap` keys. No metadata (`{}`): the + * pager and the private count need only keys. + */ +export async function scanScopeKeys( + db: D1Queryable, + q: Omit, + cap: number = SCOPE_SCAN_CAP, +): Promise { + const bounded = Math.max(1, Math.min(SCOPE_SCAN_CAP, Math.floor(cap))); + const { sql, params } = scopeFrom(q); + const { results } = await db + .prepare( + `SELECT r.object_key AS object_key, r.updated_at AS updated_at ${sql} + ORDER BY r.updated_at DESC, r.object_key ASC LIMIT ?`, + ) + .bind(...params, bounded) + .all(); + return (results ?? []).map((row) => ({ + key: row.object_key, + updatedAt: row.updated_at, + metadata: {}, + })); +} + +/** Distinct `gh.repo` values with their newest upload time, newest first. */ +export async function listWorkspaceRepos( + db: D1Queryable, + workspace: string, + opts: { cursor?: ScopeCursor | null; limit: number }, +): Promise<{ + repos: Array<{ repo: string; lastUpdatedAt: string }>; + nextCursor: ScopeCursor | null; +}> { + const params: unknown[] = [workspace]; + let sql = `SELECT meta_value AS repo, MAX(updated_at) AS last_updated_at + FROM file_metadata + WHERE workspace = ? AND meta_key = 'gh.repo' + GROUP BY meta_value`; + if (opts.cursor) { + sql += ` HAVING MAX(updated_at) < ? OR (MAX(updated_at) = ? AND meta_value > ?)`; + params.push(opts.cursor.updatedAt, opts.cursor.updatedAt, opts.cursor.key); + } + sql += ` ORDER BY last_updated_at DESC, repo ASC LIMIT ?`; + params.push(opts.limit + 1); + + const { results } = await db + .prepare(sql) + .bind(...params) + .all<{ repo: string; last_updated_at: string }>(); + const rows = results ?? []; + const hasMore = rows.length > opts.limit; + const page = hasMore ? rows.slice(0, opts.limit) : rows; + const last = page.at(-1); + return { + repos: page.map((row) => ({ repo: row.repo, lastUpdatedAt: row.last_updated_at })), + nextCursor: hasMore && last ? { updatedAt: last.last_updated_at, key: last.repo } : null, + }; +} + +function base64UrlEncode(text: string): string { + const bytes = new TextEncoder().encode(text); + let binary = ""; + for (const byte of bytes) binary += String.fromCharCode(byte); + return btoa(binary).replace(/\+/g, "-").replace(/\//g, "_").replace(/=+$/, ""); +} + +function base64UrlDecode(text: string): string { + const binary = atob(text.replace(/-/g, "+").replace(/_/g, "/")); + const bytes = new Uint8Array(binary.length); + for (let i = 0; i < binary.length; i += 1) bytes[i] = binary.charCodeAt(i); + return new TextDecoder("utf-8", { fatal: true, ignoreBOM: false }).decode(bytes); +} + +/** Opaque keyset cursor. Also carries the `/pulls` (ref) and `/repos` (repo) cursors. */ +export function encodeScopeCursor(cursor: ScopeCursor): string { + return base64UrlEncode(JSON.stringify({ v: 1, u: cursor.updatedAt, k: cursor.key })); +} + +export function decodeScopeCursor(raw: string | undefined): ScopeCursor | null { + if (raw === undefined || raw === "") return null; + const invalid = () => + new ValidationError("cursor is not valid for this query", { code: "invalid_cursor" }); + let parsed: unknown; + try { + parsed = JSON.parse(base64UrlDecode(raw)); + } catch { + throw invalid(); + } + if (typeof parsed !== "object" || parsed === null) throw invalid(); + const record = parsed as Record; + if ( + record.v !== 1 || + typeof record.u !== "string" || + !Number.isFinite(Date.parse(record.u)) || + typeof record.k !== "string" || + record.k.length === 0 + ) { + throw invalid(); + } + return { updatedAt: record.u, key: record.k }; +} diff --git a/apps/api/test/file-type-sql.test.ts b/apps/api/test/file-type-sql.test.ts new file mode 100644 index 00000000..8b10fe88 --- /dev/null +++ b/apps/api/test/file-type-sql.test.ts @@ -0,0 +1,62 @@ +/// +import { DatabaseSync } from "node:sqlite"; +import { fileTypeClassFromKey } from "@uploads/comment-render/scope"; +import { describe, expect, it } from "vitest"; +import { fileTypeSql, parseFileTypeQuery } from "../src/file-type-sql"; + +const KEYS = [ + "a/shot.png", + "a/SHOT.JPG", + "b.jpeg", + "c.webp", + "d.gif", + "e.avif", + "f.mp4", + "g.WEBM", + "h.mov", + "i.pdf", + "j.txt", + "no-extension", + "k.png.zip", + "l.mov.png", + "x.jpg/", + "mp4", +]; + +describe("fileTypeSql", () => { + for (const type of ["screenshot", "video", "other"] as const) { + it(`selects exactly the keys fileTypeClassFromKey calls ${type}`, () => { + const db = new DatabaseSync(":memory:"); + try { + const values = KEYS.map(() => "(?)").join(", "); + const rows = db + .prepare( + `WITH t(k) AS (VALUES ${values}) SELECT k FROM t WHERE ${fileTypeSql("k", type)} ORDER BY k`, + ) + .all(...KEYS) as Array<{ k: string }>; + expect(rows.map((row) => row.k)).toEqual( + KEYS.filter((key) => fileTypeClassFromKey(key) === type).sort(), + ); + } finally { + db.close(); + } + }); + } + + it("never uses LIKE (D1's 50-byte pattern cap)", () => { + expect(fileTypeSql("r.object_key", "screenshot")).not.toMatch(/LIKE|GLOB/i); + }); + + it("refuses a column that is not a plain identifier", () => { + expect(() => fileTypeSql("k); DROP TABLE x; --", "video")).toThrow(); + }); +}); + +describe("parseFileTypeQuery", () => { + it("passes valid values, ignores empty, rejects the rest with invalid_type", () => { + expect(parseFileTypeQuery(undefined)).toBeUndefined(); + expect(parseFileTypeQuery("")).toBeUndefined(); + expect(parseFileTypeQuery("video")).toBe("video"); + expect(() => parseFileTypeQuery("gif")).toThrow(/type must be/); + }); +}); diff --git a/apps/api/test/pr-scope-sqlite.test.ts b/apps/api/test/pr-scope-sqlite.test.ts new file mode 100644 index 00000000..7706da4c --- /dev/null +++ b/apps/api/test/pr-scope-sqlite.test.ts @@ -0,0 +1,350 @@ +/// + +import type { SQLInputValue } from "node:sqlite"; +import { AppError } from "@uploads/errors"; +import { describe, expect, it } from "vitest"; +import { FILE_TYPE_CLASSES, fileTypeClassFromKey } from "@uploads/comment-render/scope"; +import { findObjectsByMetadata, replaceFileMetadata, setFileMetadata } from "../src/file-metadata"; +import { + decodeScopeCursor, + encodeScopeCursor, + listWorkspaceRepos, + prScopeQuery, + scanScopeKeys, + type ScopeItem, +} from "../src/pr-scope"; +import { SqliteD1, database } from "./helpers/sqlite-d1"; + +const MIGRATIONS = [ + "migrations/20260713210559_file_metadata.sql", + "migrations/20261004120200_file_metadata_gh_repo_idx.sql", +]; + +async function seed( + sqlite: SqliteD1, + key: string, + meta: Record, + updatedAt: string, + workspace = "alpha", +) { + await replaceFileMetadata(database(sqlite), workspace, key, meta); + sqlite.db + .prepare(`UPDATE file_metadata SET updated_at = ? WHERE workspace = ? AND object_key = ?`) + .run(updatedAt, workspace, key); +} + +/** Wraps the fake so a test can read back the SQL and binds a call issued. */ +function recordingDb(sqlite: SqliteD1) { + const seen: Array<{ sql: string; values: unknown[] }> = []; + const db = { + prepare(sql: string) { + const statement = sqlite.prepare(sql); + const bind = statement.bind.bind(statement); + statement.bind = (...values: unknown[]) => { + seen.push({ sql, values }); + return bind(...values); + }; + return statement; + }, + batch: sqlite.batch.bind(sqlite), + }; + return { db: db as unknown as D1Database, seen }; +} + +function queryPlan(sqlite: SqliteD1, entry: { sql: string; values: unknown[] }): string { + const rows = sqlite.db + .prepare(`EXPLAIN QUERY PLAN ${entry.sql}`) + .all(...(entry.values as SQLInputValue[])) as Array<{ detail: string }>; + return rows.map((row) => row.detail).join("\n"); +} + +const keysOf = (items: ScopeItem[]) => items.map((item) => item.key); + +describe("prScopeQuery", () => { + it("matches gh.repo and gh.number, drops promoted shadows, and does not require path", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + await seed( + sqlite, + "gh/acme/app/pull/1/a.png", + { "gh.repo": "acme/app", "gh.number": "1", path: "/x" }, + "2026-10-01T01:00:00.000Z", + ); + await seed( + sqlite, + "gh/acme/app/pull/1/b.mp4", + { "gh.repo": "acme/app", "gh.number": "1" }, + "2026-10-01T02:00:00.000Z", + ); + await seed( + sqlite, + "gh/acme/app/pull/2/c.png", + { "gh.repo": "acme/app", "gh.number": "2" }, + "2026-10-01T03:00:00.000Z", + ); + await seed( + sqlite, + "gh/acme/app/branch/f/d.png", + { "gh.repo": "acme/app", "gh.number": "1", "gh.status": "promoted" }, + "2026-10-01T04:00:00.000Z", + ); + await seed( + sqlite, + "gh/acme/web/pull/1/e.png", + { "gh.repo": "acme/web", "gh.number": "1" }, + "2026-10-01T05:00:00.000Z", + ); + await seed( + sqlite, + "gh/acme/app/pull/1/f.png", + { "gh.repo": "acme/app", "gh.number": "1" }, + "2026-10-01T06:00:00.000Z", + "beta", + ); + const db = database(sqlite); + + const repo = await prScopeQuery(db, { workspace: "alpha", repo: "acme/app" }); + expect(keysOf(repo.items)).toEqual([ + "gh/acme/app/pull/2/c.png", + "gh/acme/app/pull/1/b.mp4", + "gh/acme/app/pull/1/a.png", + ]); + expect(repo.nextCursor).toBeNull(); + expect(repo.items[2]?.metadata).toMatchObject({ "gh.number": "1", path: "/x" }); + + const pr = await prScopeQuery(db, { workspace: "alpha", repo: "acme/app", number: 1 }); + expect(keysOf(pr.items)).toEqual(["gh/acme/app/pull/1/b.mp4", "gh/acme/app/pull/1/a.png"]); + + const byPath = await prScopeQuery(db, { workspace: "alpha", repo: "acme/app", path: "/x" }); + expect(keysOf(byPath.items)).toEqual(["gh/acme/app/pull/1/a.png"]); + } finally { + sqlite.close(); + } + }); + + it("pages newest first and breaks updated_at ties by key without repeats or gaps", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + const tie = "2026-10-01T00:00:00.000Z"; + for (const name of ["k1", "k2", "k3", "k4", "k5"]) { + await seed(sqlite, `s/${name}.png`, { "gh.repo": "acme/app" }, tie); + } + await seed(sqlite, "s/newest.png", { "gh.repo": "acme/app" }, "2026-10-02T00:00:00.000Z"); + const db = database(sqlite); + + const seen: string[] = []; + let cursor = null as Awaited>["nextCursor"]; + const pages: string[][] = []; + do { + const page = await prScopeQuery(db, { + workspace: "alpha", + repo: "acme/app", + cursor, + limit: 2, + }); + pages.push(keysOf(page.items)); + seen.push(...keysOf(page.items)); + cursor = page.nextCursor; + } while (cursor); + + expect(pages).toEqual([ + ["s/newest.png", "s/k1.png"], + ["s/k2.png", "s/k3.png"], + ["s/k4.png", "s/k5.png"], + ]); + expect(new Set(seen).size).toBe(6); + } finally { + sqlite.close(); + } + }); + + it("filters by key suffix exactly as fileTypeClassFromKey classifies", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + const keys = [ + "s/one.png", + "s/TWO.JPEG", + "s/three.webp", + "s/four.gif", + "s/five.avif", + "s/six.jpg", + "v/a.mp4", + "v/b.WEBM", + "v/c.mov", + "o/d.pdf", + "o/notes", + "o/e.png.zip", + "o/f.txt", + "gh/private/0123456789abcdef0123456789abcdef/pull/12/a-long-file-name.png", + ]; + for (const [i, key] of keys.entries()) { + await seed( + sqlite, + key, + { "gh.repo": "acme/app" }, + `2026-10-01T00:00:${String(i).padStart(2, "0")}.000Z`, + ); + } + const db = database(sqlite); + for (const type of FILE_TYPE_CLASSES) { + const page = await prScopeQuery(db, { + workspace: "alpha", + repo: "acme/app", + type, + limit: 100, + }); + expect(keysOf(page.items).sort()).toEqual( + keys.filter((key) => fileTypeClassFromKey(key) === type).sort(), + ); + } + } finally { + sqlite.close(); + } + }); + + it("reads the partial gh.repo index in order, with no temp sort", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + const { db, seen } = recordingDb(sqlite); + await prScopeQuery(db, { + workspace: "alpha", + repo: "acme/app", + number: 3, + type: "screenshot", + cursor: { updatedAt: "2026-10-01T00:00:00.000Z", key: "a" }, + }); + const scope = seen.find((entry) => entry.sql.includes("r.meta_key = 'gh.repo'")); + expect(scope).toBeDefined(); + const plan = queryPlan(sqlite, scope!); + expect(plan).toContain("COVERING INDEX file_metadata_gh_repo_recent_idx"); + expect(plan).not.toContain("TEMP B-TREE FOR ORDER BY"); + + await listWorkspaceRepos(db, "alpha", { limit: 20 }); + const repos = seen.find((entry) => entry.sql.includes("GROUP BY meta_value")); + expect(queryPlan(sqlite, repos!)).toContain( + "COVERING INDEX file_metadata_gh_repo_recent_idx", + ); + } finally { + sqlite.close(); + } + }); +}); + +describe("scanScopeKeys", () => { + it("returns scope keys newest first up to the cap, without metadata", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + for (let i = 0; i < 7; i++) { + await seed( + sqlite, + `s/${i}.png`, + { "gh.repo": "acme/app", "gh.number": "4" }, + `2026-10-01T00:00:0${i}.000Z`, + ); + } + const db = database(sqlite); + const all = await scanScopeKeys(db, { workspace: "alpha", repo: "acme/app", number: 4 }); + expect(keysOf(all)).toEqual([ + "s/6.png", + "s/5.png", + "s/4.png", + "s/3.png", + "s/2.png", + "s/1.png", + "s/0.png", + ]); + expect(all[0]?.metadata).toEqual({}); + expect(all[0]?.updatedAt).toBe("2026-10-01T00:00:06.000Z"); + const capped = await scanScopeKeys(db, { workspace: "alpha", repo: "acme/app" }, 5); + expect(capped).toHaveLength(5); + } finally { + sqlite.close(); + } + }); +}); + +describe("listWorkspaceRepos", () => { + it("lists distinct repos newest first with a keyset cursor, per workspace", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + await seed(sqlite, "a/1.png", { "gh.repo": "acme/app" }, "2026-10-01T01:00:00.000Z"); + await seed(sqlite, "a/2.png", { "gh.repo": "acme/app" }, "2026-10-01T05:00:00.000Z"); + await seed(sqlite, "s/1.png", { "gh.repo": "acme/site" }, "2026-10-01T03:00:00.000Z"); + await seed(sqlite, "d/1.png", { "gh.repo": "acme/docs" }, "2026-10-01T03:00:00.000Z"); + await seed(sqlite, "x/1.png", { "gh.repo": "beta/only" }, "2026-10-01T09:00:00.000Z", "beta"); + const db = database(sqlite); + + const first = await listWorkspaceRepos(db, "alpha", { limit: 2 }); + expect(first.repos).toEqual([ + { repo: "acme/app", lastUpdatedAt: "2026-10-01T05:00:00.000Z" }, + { repo: "acme/docs", lastUpdatedAt: "2026-10-01T03:00:00.000Z" }, + ]); + expect(first.nextCursor).toEqual({ updatedAt: "2026-10-01T03:00:00.000Z", key: "acme/docs" }); + const second = await listWorkspaceRepos(db, "alpha", { limit: 2, cursor: first.nextCursor }); + expect(second.repos).toEqual([ + { repo: "acme/site", lastUpdatedAt: "2026-10-01T03:00:00.000Z" }, + ]); + expect(second.nextCursor).toBeNull(); + } finally { + sqlite.close(); + } + }); +}); + +describe("gh.repo canonical spelling", () => { + it("stores gh.repo lowercased, so a hand-set mixed-case repo stays in scope and in search", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + const db = database(sqlite); + await replaceFileMetadata(db, "alpha", "s/hand.png", { + "gh.repo": "Acme/App", + path: "/Mixed", + }); + await setFileMetadata(db, "alpha", "s/set.png", { "gh.repo": "ACME/app" }); + + const page = await prScopeQuery(db, { workspace: "alpha", repo: "acme/app" }); + expect(keysOf(page.items).sort()).toEqual(["s/hand.png", "s/set.png"]); + // Only gh.repo is canonicalized; other values keep their case. + expect(page.items.find((item) => item.key === "s/hand.png")?.metadata).toMatchObject({ + "gh.repo": "acme/app", + path: "/Mixed", + }); + + const found = await findObjectsByMetadata(db, "alpha", { "gh.repo": "Acme/App" }); + expect(found.map((row) => row.key).sort()).toEqual(["s/hand.png", "s/set.png"]); + } finally { + sqlite.close(); + } + }); +}); + +describe("scope cursor codec", () => { + function expectRejected(raw: string): void { + try { + decodeScopeCursor(raw); + } catch (err) { + expect((err as AppError).code).toBe("invalid_cursor"); + expect((err as AppError).status).toBe(400); + return; + } + throw new Error(`expected ${raw} to be rejected`); + } + + it("round-trips a non-ASCII key, is URL-safe, and treats empty as no cursor", () => { + const cursor = { updatedAt: "2026-10-01T00:00:00.000Z", key: "shots/café-née.png" }; + const raw = encodeScopeCursor(cursor); + expect(encodeURIComponent(raw)).toBe(raw); + expect(raw).not.toContain("shots/"); + expect(decodeScopeCursor(raw)).toEqual(cursor); + expect(decodeScopeCursor(undefined)).toBeNull(); + expect(decodeScopeCursor("")).toBeNull(); + }); + + it("rejects garbage, other versions, bad dates, and empty keys with one code", () => { + expectRejected("not-a-cursor"); + expectRejected(btoa(JSON.stringify({ hello: "world" }))); + expectRejected(btoa(JSON.stringify({ v: 2, u: "2026-10-01T00:00:00.000Z", k: "a" }))); + expectRejected(btoa(JSON.stringify({ v: 1, u: "yesterday", k: "a" }))); + expectRejected(btoa(JSON.stringify({ v: 1, u: "2026-10-01T00:00:00.000Z", k: "" }))); + }); +}); From 1e6716d6c461de452b140e4ee5db657bd8f6463f Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 16:50:03 -0400 Subject: [PATCH 03/14] feat(api): record who created each live feed and cap only user repo feeds --- .../20261004120000_feeds_source.sql | 7 + apps/api/src/feed-service.ts | 5 + apps/api/src/feeds.ts | 44 +++++- apps/api/src/github-comment.test.ts | 2 + apps/api/src/github-comment.ts | 5 + apps/api/test/feeds-sqlite.test.ts | 134 ++++++++++++++++++ apps/api/test/routes-feeds.test.ts | 52 +++++++ apps/web/public/.well-known/openapi.json | 5 + 8 files changed, 248 insertions(+), 6 deletions(-) create mode 100644 apps/api/migrations/20261004120000_feeds_source.sql diff --git a/apps/api/migrations/20261004120000_feeds_source.sql b/apps/api/migrations/20261004120000_feeds_source.sql new file mode 100644 index 00000000..5a2fa1b1 --- /dev/null +++ b/apps/api/migrations/20261004120000_feeds_source.sql @@ -0,0 +1,7 @@ +-- Who created a live feed row: 'comment' (GitHub comment sync) or 'user' +-- (web, CLI, MCP, plugin). NULL on rows created before this column. Set on +-- insert only: a reused scope keeps its first source. The per-workspace cap +-- (apps/api/src/feeds.ts) counts only 'user' repo-scoped rows (50); PR and +-- issue feeds are uncapped, so comment sync never falls back to /f/ links. +ALTER TABLE feeds ADD COLUMN source TEXT + CHECK (source IS NULL OR source IN ('comment', 'user')); diff --git a/apps/api/src/feed-service.ts b/apps/api/src/feed-service.ts index ef95f3cc..669e562b 100644 --- a/apps/api/src/feed-service.ts +++ b/apps/api/src/feed-service.ts @@ -19,6 +19,7 @@ import { type FeedCursor, type FeedMutationResult, type FeedRecord, + type FeedSource, } from "./feeds"; import { isDerivedPosterContentType, videoPresentation, type VideoDimensions } from "./poster"; import { createLaneResolver, objectPublicUrls, type LaneResolver } from "./storage"; @@ -80,6 +81,7 @@ export interface FeedDto { number: number | null; kind: "pull" | "issue" | null; title: string; + source: FeedSource | null; createdAt: string; updatedAt: string; items: FeedItemDto[]; @@ -94,6 +96,8 @@ export interface FeedSummaryDto { number: number | null; kind: "pull" | "issue" | null; title: string; + /** "comment" (PR comment sync), "user", or null for feeds created before this field. */ + source: FeedSource | null; createdAt: string; updatedAt: string; } @@ -133,6 +137,7 @@ export function feedSummary(env: Env, record: FeedRecord): FeedSummaryDto { number: record.number > 0 ? record.number : null, kind: record.kind === "pull" || record.kind === "issue" ? record.kind : null, title: feedTitle(record.repo, record.path, record.number), + source: record.source, createdAt: record.created_at, updatedAt: record.updated_at, }; diff --git a/apps/api/src/feeds.ts b/apps/api/src/feeds.ts index 2201b55e..0342e0e7 100644 --- a/apps/api/src/feeds.ts +++ b/apps/api/src/feeds.ts @@ -6,6 +6,12 @@ */ import { type D1Queryable } from "./db-session"; +/** + * Live repo-scoped feeds (`number = 0`) with `source = 'user'` per workspace. + * PR- and issue-scoped feeds (`number > 0`) are uncapped from every source + * (API, CLI `gh` fallback, GitHub App): they are one row per real PR or issue. + * Repo-scoped feeds created by comment sync are uncapped too. + */ export const MAX_FEEDS_PER_WORKSPACE = 50; export const MAX_FEED_PAGE_SIZE = 100; export const FEED_ITEM_LIMIT = 50; @@ -14,7 +20,11 @@ export const FEED_REPO_RE = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; export const FEED_KIND_VALUES = ["", "pull", "issue"] as const; export type FeedKind = (typeof FEED_KIND_VALUES)[number]; -const FEED_SELECT = "id, workspace, repo, path, number, kind, created_at, updated_at, deleted_at"; +const FEED_SELECT = + "id, workspace, repo, path, number, kind, source, created_at, updated_at, deleted_at"; + +/** Who created a feed row. Comment-sync rows are uncapped; see `createFeed`. */ +export type FeedSource = "comment" | "user"; export interface FeedRecord { id: string; @@ -23,6 +33,8 @@ export interface FeedRecord { path: string; number: number; kind: FeedKind; + /** `null` on rows created before the column existed. */ + source: FeedSource | null; created_at: string; updated_at: string; deleted_at: string | null; @@ -151,6 +163,7 @@ function row(record: FeedRecord): FeedRecord { path: record.path, number: Number(record.number) || 0, kind: record.kind === "pull" || record.kind === "issue" ? record.kind : "", + source: record.source === "comment" || record.source === "user" ? record.source : null, created_at: record.created_at, updated_at: record.updated_at, deleted_at: record.deleted_at, @@ -212,9 +225,16 @@ export async function findFeedByRepoPath( return findFeedByScope(db, workspace, repo, path, 0); } -async function countLiveFeeds(db: D1Queryable, workspace: string): Promise { +/** + * Live user-created repo-scoped feeds (`number = 0`). Comment-sync rows, + * legacy (NULL) rows, and PR/issue-scoped rows do not count. + */ +async function countUserRepoFeeds(db: D1Queryable, workspace: string): Promise { const found = await db - .prepare(`SELECT COUNT(*) AS count FROM feeds WHERE workspace = ? AND deleted_at IS NULL`) + .prepare( + `SELECT COUNT(*) AS count FROM feeds + WHERE workspace = ? AND deleted_at IS NULL AND source = 'user' AND number = 0`, + ) .bind(workspace) .first<{ count: number }>(); return found?.count ?? 0; @@ -230,6 +250,7 @@ export async function createFeed( kind?: unknown; pr?: unknown; issue?: unknown; + source?: FeedSource; now?: Date; }, ): Promise> { @@ -249,7 +270,16 @@ export async function createFeed( ); if (existing) return { status: "ok", value: existing, created: false }; - if ((await countLiveFeeds(db, input.workspace)) >= MAX_FEEDS_PER_WORKSPACE) { + // One live row per scope whatever the source: the lookup above already + // returned an existing comment-sync row to a user caller without touching + // the cap. Only user-created repo-scoped feeds are capped; PR/issue-scoped + // feeds (number > 0) are uncapped whatever the source. + const source: FeedSource = input.source ?? "user"; + if ( + source === "user" && + scopeResult.value.number === 0 && + (await countUserRepoFeeds(db, input.workspace)) >= MAX_FEEDS_PER_WORKSPACE + ) { return { status: "limit", limit: MAX_FEEDS_PER_WORKSPACE }; } @@ -261,14 +291,15 @@ export async function createFeed( path: pathResult.value, number: scopeResult.value.number, kind: scopeResult.value.kind, + source, created_at: now, updated_at: now, deleted_at: null, }; await db .prepare( - `INSERT INTO feeds (id, workspace, repo, path, number, kind, created_at, updated_at, deleted_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, NULL)`, + `INSERT INTO feeds (id, workspace, repo, path, number, kind, source, created_at, updated_at, deleted_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, NULL)`, ) .bind( record.id, @@ -277,6 +308,7 @@ export async function createFeed( record.path, record.number, record.kind, + record.source, record.created_at, record.updated_at, ) diff --git a/apps/api/src/github-comment.test.ts b/apps/api/src/github-comment.test.ts index ab4c25d3..c7aa0341 100644 --- a/apps/api/src/github-comment.test.ts +++ b/apps/api/src/github-comment.test.ts @@ -25,6 +25,7 @@ const MIGRATION = [ "migrations/20260903120000_github_attachments.sql", "migrations/20260915120000_feeds.sql", "migrations/20260915153000_feeds_number.sql", + "migrations/20261004120000_feeds_source.sql", ]; const PRAGMAS = ["PRAGMA foreign_keys = ON"]; const PNG = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); @@ -268,6 +269,7 @@ describe("gatherCommentBody", () => { ); const feed = await findFeedByScope(env.DB, workspaceName, "acme/web", "", 12); expect(feed).toBeTruthy(); + expect(feed!.source).toBe("comment"); const idA = await feedItemId(keyA); const idB = await feedItemId(keyB); expect(first.body).toContain(`/c/${feed!.id}/${idA}`); diff --git a/apps/api/src/github-comment.ts b/apps/api/src/github-comment.ts index 9f90a44f..c4e020f9 100644 --- a/apps/api/src/github-comment.ts +++ b/apps/api/src/github-comment.ts @@ -316,9 +316,14 @@ async function applyPrFeedPageUrls( linkToFilePage: boolean, ): Promise { try { + // Comment-sync rows are uncapped and keep `comment` for life. GhTarget + // spells issues `issues`; createFeed takes `pr`/`issue`, and the slot + // lookup ignores kind, so a later web/CLI create for the same number + // reuses this row. const created = await createFeed(dbFor(env), { workspace: workspaceName, repo: target.repo, + source: "comment", ...(target.kind === "pull" ? { pr: target.num } : { issue: target.num }), }); if (created.status !== "ok") return; diff --git a/apps/api/test/feeds-sqlite.test.ts b/apps/api/test/feeds-sqlite.test.ts index 5a4d8525..396ca5a9 100644 --- a/apps/api/test/feeds-sqlite.test.ts +++ b/apps/api/test/feeds-sqlite.test.ts @@ -19,6 +19,7 @@ const MIGRATIONS = [ "migrations/20260713210559_file_metadata.sql", "migrations/20260915120000_feeds.sql", "migrations/20260915153000_feeds_number.sql", + "migrations/20261004120000_feeds_source.sql", ]; function newSqlite(): SqliteD1 { @@ -263,4 +264,137 @@ describe("feed persistence against SQLite", () => { sqlite.close(); } }); + + it("leaves PR/issue-scoped feeds uncapped and caps repo-scoped user feeds at 50", async () => { + const sqlite = newSqlite(); + try { + const db = database(sqlite); + for (let i = 0; i < MAX_FEEDS_PER_WORKSPACE; i++) { + const created = await createFeed(db, { workspace: "alpha", repo: `acme/app${i}` }); + expect(created).toMatchObject({ status: "ok", value: { source: "user" } }); + } + expect(await createFeed(db, { workspace: "alpha", repo: "acme/overflow" })).toEqual({ + status: "limit", + limit: MAX_FEEDS_PER_WORKSPACE, + }); + // The full repo-scoped quota does not block PR or issue feeds, and + // they have no quota of their own: 60 user-created PR feeds all succeed. + for (let n = 1; n <= 60; n++) { + expect(await createFeed(db, { workspace: "alpha", repo: "acme/web", pr: n })).toMatchObject( + { + status: "ok", + created: true, + value: { source: "user" }, + }, + ); + } + expect( + await createFeed(db, { workspace: "alpha", repo: "acme/web", issue: 61 }), + ).toMatchObject({ + status: "ok", + created: true, + }); + // Comment sync stays uncapped, repo-scoped included. + expect( + await createFeed(db, { workspace: "alpha", repo: "acme/overflow", source: "comment" }), + ).toMatchObject({ status: "ok", created: true, value: { source: "comment" } }); + } finally { + sqlite.close(); + } + }); + + it("reuses a comment-sync row for a user create without counting it (index review focus 4)", async () => { + const sqlite = newSqlite(); + try { + const db = database(sqlite); + const comment = await createFeed(db, { + workspace: "alpha", + repo: "acme/web", + issue: 7, + source: "comment", + }); + if (comment.status !== "ok") throw new Error("comment create failed"); + for (let i = 0; i < MAX_FEEDS_PER_WORKSPACE; i++) { + await createFeed(db, { workspace: "alpha", repo: `acme/app${i}` }); + } + + const user = await createFeed(db, { workspace: "alpha", repo: "Acme/Web", pr: 7 }); + expect(user).toMatchObject({ + status: "ok", + created: false, + value: { id: comment.value.id, source: "comment", kind: "issue" }, + }); + const byNumber = await createFeed(db, { workspace: "alpha", repo: "acme/web", number: 7 }); + expect(byNumber).toMatchObject({ status: "ok", value: { id: comment.value.id } }); + + const counted = sqlite.db + .prepare( + `SELECT COUNT(*) AS n FROM feeds + WHERE workspace = 'alpha' AND deleted_at IS NULL AND source = 'user' AND number = 0`, + ) + .get() as { n: number }; + expect(counted.n).toBe(MAX_FEEDS_PER_WORKSPACE); + } finally { + sqlite.close(); + } + }); + + it("recreates a revoked link with the source of whoever creates the new row", async () => { + const sqlite = newSqlite(); + try { + const db = database(sqlite); + const comment = await createFeed(db, { + workspace: "alpha", + repo: "acme/web", + pr: 1, + source: "comment", + }); + if (comment.status !== "ok") throw new Error("create failed"); + await softDeleteFeed(db, "alpha", comment.value.id); + const resynced = await createFeed(db, { + workspace: "alpha", + repo: "acme/web", + pr: 1, + source: "comment", + }); + expect(resynced).toMatchObject({ status: "ok", created: true, value: { source: "comment" } }); + if (resynced.status !== "ok") throw new Error("recreate failed"); + expect(resynced.value.id).not.toBe(comment.value.id); + + const user = await createFeed(db, { workspace: "alpha", repo: "acme/web", pr: 2 }); + if (user.status !== "ok") throw new Error("create failed"); + await softDeleteFeed(db, "alpha", user.value.id); + const synced = await createFeed(db, { + workspace: "alpha", + repo: "acme/web", + pr: 2, + source: "comment", + }); + expect(synced).toMatchObject({ status: "ok", created: true, value: { source: "comment" } }); + } finally { + sqlite.close(); + } + }); + + it("maps legacy NULL-source rows to null and leaves them out of the cap", async () => { + const sqlite = newSqlite(); + try { + const db = database(sqlite); + sqlite.db + .prepare( + `INSERT INTO feeds (id, workspace, repo, path, number, kind, created_at, updated_at, deleted_at) + VALUES ('feed_legacylegacylegacy0000', 'alpha', 'acme/old', '', 0, '', '2026-09-20T00:00:00Z', '2026-09-20T00:00:00Z', NULL)`, + ) + .run(); + expect(await getFeed(db, "alpha", "feed_legacylegacylegacy0000")).toMatchObject({ + source: null, + }); + for (let i = 0; i < MAX_FEEDS_PER_WORKSPACE; i++) { + const created = await createFeed(db, { workspace: "alpha", repo: `acme/app${i}` }); + expect(created.status).toBe("ok"); + } + } finally { + sqlite.close(); + } + }); }); diff --git a/apps/api/test/routes-feeds.test.ts b/apps/api/test/routes-feeds.test.ts index da6c4422..63184635 100644 --- a/apps/api/test/routes-feeds.test.ts +++ b/apps/api/test/routes-feeds.test.ts @@ -19,6 +19,7 @@ const MIGRATIONS = [ "migrations/20260713210559_file_metadata.sql", "migrations/20260915120000_feeds.sql", "migrations/20260915153000_feeds_number.sql", + "migrations/20261004120000_feeds_source.sql", ]; beforeAll(() => { @@ -327,4 +328,55 @@ describe("feed routes", () => { expect(again.status).toBe(200); expect(((await again.json()) as { id: string; url: string }).url).toBe(uploads.url); }); + + it("reports who created each feed on the owner DTOs and ignores a client-sent source", async () => { + const created = await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: "acme/app", source: "comment" }), + }); + expect(created.status).toBe(201); + const feed = (await created.json()) as { id: string; source: string | null }; + expect(feed.source).toBe("user"); + + const listed = await request("/v1/workspaces/alpha/feeds"); + const page = (await listed.json()) as { feeds: Array<{ id: string; source: string | null }> }; + expect(page.feeds).toEqual([expect.objectContaining({ id: feed.id, source: "user" })]); + + const single = await request(`/v1/workspaces/alpha/feeds/${feed.id}`); + expect(((await single.json()) as { source: string | null }).source).toBe("user"); + + const publicFeed = await app.request(`/public/feeds/${feed.id}`, {}, env); + expect(await publicFeed.json()).not.toHaveProperty("source"); + }); + + it("creates 60 PR-scoped feeds over the API and caps only repo-scoped feeds at 50", async () => { + for (let n = 1; n <= 60; n++) { + const created = await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: "acme/web", pr: n }), + }); + expect(created.status).toBe(201); + } + for (let i = 0; i < 50; i++) { + const created = await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: `acme/app${i}` }), + }); + expect(created.status).toBe(201); + } + const overflow = await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: "acme/overflow" }), + }); + expect(overflow.status).toBe(409); + expect(await overflow.json()).toMatchObject({ + error: { code: "feed_limit_reached", details: { limit: 50 } }, + }); + // PR-scoped creates are still accepted after the repo quota is full. + const next = await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: "acme/web", pr: 61 }), + }); + expect(next.status).toBe(201); + }); }); diff --git a/apps/web/public/.well-known/openapi.json b/apps/web/public/.well-known/openapi.json index ae8b8b74..4badf7b1 100644 --- a/apps/web/public/.well-known/openapi.json +++ b/apps/web/public/.well-known/openapi.json @@ -1299,6 +1299,11 @@ "number": { "type": ["integer", "null"], "minimum": 1 }, "kind": { "type": ["string", "null"], "enum": ["pull", "issue"] }, "title": { "type": "string" }, + "source": { + "type": ["string", "null"], + "enum": ["comment", "user", null], + "description": "Who created the feed: PR comment sync or a user. Null for feeds created before this field." + }, "createdAt": { "type": "string", "format": "date-time" }, "updatedAt": { "type": "string", "format": "date-time" } } From e957c3d5cc5a3e3721e0e4a06fca83984c4db710 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 16:52:49 -0400 Subject: [PATCH 04/14] feat(api): keep PR title and state on the activity rollup --- ...20261004120100_pr_activity_title_state.sql | 9 + apps/api/src/github-pr-activity.ts | 169 +++++++++++++ apps/api/src/github-webhook-queue.test.ts | 75 ++++++ apps/api/src/github-webhook.ts | 44 +++- .../test/github-pr-activity-sqlite.test.ts | 237 ++++++++++++++++++ 5 files changed, 533 insertions(+), 1 deletion(-) create mode 100644 apps/api/migrations/20261004120100_pr_activity_title_state.sql diff --git a/apps/api/migrations/20261004120100_pr_activity_title_state.sql b/apps/api/migrations/20261004120100_pr_activity_title_state.sql new file mode 100644 index 00000000..0d37deee --- /dev/null +++ b/apps/api/migrations/20261004120100_pr_activity_title_state.sql @@ -0,0 +1,9 @@ +-- PR title and state on the rollup row, for the Files "By pull request" +-- list and its state filter. Written by the pull_request webhook (rows whose +-- repo is linked to the row's workspace only) and backfilled by +-- GET /v1/workspaces/:ws/pulls from resolveTitles. NULL until either runs. +-- The cursor index serves ORDER BY last_media_at DESC, ref ASC keyset pages. +ALTER TABLE github_pr_activity ADD COLUMN title TEXT; +ALTER TABLE github_pr_activity ADD COLUMN state TEXT; +CREATE INDEX github_pr_activity_workspace_cursor_idx + ON github_pr_activity (workspace_name, last_media_at DESC, ref); diff --git a/apps/api/src/github-pr-activity.ts b/apps/api/src/github-pr-activity.ts index 73af1f4b..82ddf247 100644 --- a/apps/api/src/github-pr-activity.ts +++ b/apps/api/src/github-pr-activity.ts @@ -12,6 +12,7 @@ * per repo, per the `github_repo_links` binding model). */ import { type D1Queryable } from "./db-session"; +import type { ScopeCursor } from "./pr-scope"; export interface PrActivity { ref: string; @@ -154,3 +155,171 @@ export async function listPrActivityForWorkspace( .all(); return (results ?? []).map(rowToActivity); } + +export type PrState = "open" | "closed" | "merged"; + +export function isPrState(value: unknown): value is PrState { + return value === "open" || value === "closed" || value === "merged"; +} + +export interface PrActivityPageRow extends PrActivity { + title: string | null; + state: PrState | null; +} + +/** Rollup columns plus title/state; shared by `listPrActivityPage` and `getPrActivityRow` (Task 6). */ +const PAGE_ROW_COLUMNS = `ref, repo_full_name, pr_number, branch, workspace_name, + media_count, first_media_at, last_media_at, title, state`; + +type PageRowRecord = PrActivityRow & { title: string | null; state: string | null }; + +function toPageRow(row: PageRowRecord): PrActivityPageRow { + return { + ...rowToActivity(row), + title: row.title ?? null, + state: isPrState(row.state) ? row.state : null, + }; +} + +/** + * One keyset page of the workspace's PRs with media, newest activity first. + * Rows never resolved (null state) drop out when a `state` filter is set. + * `since` bounds the window by `last_media_at` (the `/pulls` default is 90 days). + * Strict: a D1 failure surfaces as a 5xx. + */ +export async function listPrActivityPage( + db: D1Queryable, + workspaceName: string, + opts: { + repo?: string; + state?: PrState; + /** ISO timestamp: keep rows with `last_media_at >= since` (the `/pulls` recency window). */ + since?: string; + cursor?: ScopeCursor | null; + limit: number; + }, +): Promise<{ rows: PrActivityPageRow[]; nextCursor: ScopeCursor | null }> { + const params: unknown[] = [workspaceName]; + let sql = `SELECT ${PAGE_ROW_COLUMNS} + FROM github_pr_activity + WHERE workspace_name = ?`; + if (opts.repo) { + sql += ` AND repo_full_name = ?`; + params.push(opts.repo); + } + if (opts.state) { + sql += ` AND state = ?`; + params.push(opts.state); + } + if (opts.since) { + sql += ` AND last_media_at >= ?`; + params.push(opts.since); + } + if (opts.cursor) { + sql += ` AND (last_media_at < ? OR (last_media_at = ? AND ref > ?))`; + params.push(opts.cursor.updatedAt, opts.cursor.updatedAt, opts.cursor.key); + } + sql += ` ORDER BY last_media_at DESC, ref ASC LIMIT ?`; + params.push(opts.limit + 1); + + const { results } = await db + .prepare(sql) + .bind(...params) + .all(); + const rows = (results ?? []).map(toPageRow); + const hasMore = rows.length > opts.limit; + const page = hasMore ? rows.slice(0, opts.limit) : rows; + const last = page.at(-1); + return { + rows: page, + nextCursor: hasMore && last ? { updatedAt: last.lastMediaAt, key: last.ref } : null, + }; +} + +/** Open-PR count per repo from the rollup. One bound parameter per repo: pass at most 99. */ +export async function countOpenPullsByRepo( + db: D1Queryable, + workspaceName: string, + repos: string[], +): Promise> { + const out = new Map(); + if (repos.length === 0) return out; + const { results } = await db + .prepare( + `SELECT repo_full_name AS repo, COUNT(*) AS n + FROM github_pr_activity + WHERE workspace_name = ? AND state = 'open' + AND repo_full_name IN (${repos.map(() => "?").join(", ")}) + GROUP BY repo_full_name`, + ) + .bind(workspaceName, ...repos) + .all<{ repo: string; n: number }>(); + for (const row of results ?? []) out.set(row.repo, Number(row.n)); + return out; +} + +/** + * `pull_request` webhook write-through: title + state from the payload onto + * the PR's existing rollup row. UPDATE only (rows come from media writes). + * Gated to rows whose repo is linked to the row's own workspace: rows are + * created from client-writable `gh.*` metadata, so without the gate another + * workspace could fabricate a row and receive a private PR title. Unlinked + * rows heal through the `/pulls` backfill instead. Never throws. + */ +export async function applyPrActivityWebhook( + db: D1Queryable, + input: { repo: string; number: number; title: string; state: PrState }, +): Promise { + const ref = `${input.repo.toLowerCase()}#${input.number}`; + try { + await db + .prepare( + `UPDATE github_pr_activity SET title = ?, state = ? + WHERE ref = ? + AND EXISTS ( + SELECT 1 FROM github_repo_links l + WHERE l.repo_full_name = github_pr_activity.repo_full_name + AND l.workspace_name = github_pr_activity.workspace_name + )`, + ) + .bind(input.title, input.state, ref) + .run(); + } catch (err) { + console.error( + JSON.stringify({ + message: "pr activity webhook update failed", + ref, + error: err instanceof Error ? err.message : String(err), + }), + ); + } +} + +/** + * Lazy fill from the `/pulls` handler: rows whose resolved title/state + * differs from the stored one. Invalid states are skipped. Never throws. + */ +export async function backfillPrActivityState( + db: D1Queryable, + rows: Array<{ ref: string; title: string; state: string }>, +): Promise { + const valid = rows.filter((row) => isPrState(row.state)); + if (valid.length === 0) return; + try { + await db.batch( + valid.map((row) => + db + .prepare(`UPDATE github_pr_activity SET title = ?, state = ? WHERE ref = ?`) + .bind(row.title, row.state, row.ref), + ), + ); + } catch (err) { + console.error( + JSON.stringify({ + message: "pr activity backfill failed", + count: valid.length, + error: err instanceof Error ? err.message : String(err), + }), + ); + } +} diff --git a/apps/api/src/github-webhook-queue.test.ts b/apps/api/src/github-webhook-queue.test.ts index a7388c31..b3655059 100644 --- a/apps/api/src/github-webhook-queue.test.ts +++ b/apps/api/src/github-webhook-queue.test.ts @@ -471,3 +471,78 @@ describe("handleGithubWebhookBatch", () => { expect(m.acked).toBe(false); }); }); + +describe("extractWebhookEvent — PR rollup title/state", () => { + const repository = { full_name: "Acme/Web" }; + const pull = (extra: Record = {}) => ({ + number: 7, + title: "Add dark mode", + head: { ref: "feat", repo: { full_name: "acme/web" } }, + ...extra, + }); + + it("opened and reopened write state open", () => { + for (const action of ["opened", "reopened"]) { + const ev = extractWebhookEvent("pull_request", { action, repository, pull_request: pull() }); + expect(ev?.prActivity).toEqual({ + repo: "Acme/Web", + number: 7, + title: "Add dark mode", + state: "open", + }); + } + }); + + it("closed writes merged or closed from the payload", () => { + const merged = extractWebhookEvent("pull_request", { + action: "closed", + repository, + pull_request: pull({ merged: true, state: "closed" }), + }); + expect(merged?.prActivity?.state).toBe("merged"); + const closed = extractWebhookEvent("pull_request", { + action: "closed", + repository, + pull_request: pull({ merged: false, state: "closed" }), + }); + expect(closed?.prActivity?.state).toBe("closed"); + }); + + it("edited trusts the payload's own state and title", () => { + const ev = extractWebhookEvent("pull_request", { + action: "edited", + repository, + pull_request: pull({ title: "Renamed", state: "closed", merged_at: "2026-10-01T00:00:00Z" }), + }); + expect(ev?.prActivity).toEqual({ + repo: "Acme/Web", + number: 7, + title: "Renamed", + state: "merged", + }); + }); + + it("ignores synchronize, a missing title, and issues events", () => { + expect( + extractWebhookEvent("pull_request", { + action: "synchronize", + repository, + pull_request: pull(), + })?.prActivity, + ).toBeUndefined(); + expect( + extractWebhookEvent("pull_request", { + action: "opened", + repository, + pull_request: pull({ title: undefined }), + })?.prActivity, + ).toBeUndefined(); + expect( + extractWebhookEvent("issues", { + action: "opened", + repository, + issue: { number: 7, title: "Issue" }, + })?.prActivity, + ).toBeUndefined(); + }); +}); diff --git a/apps/api/src/github-webhook.ts b/apps/api/src/github-webhook.ts index 3c5b79bc..9d8ec365 100644 --- a/apps/api/src/github-webhook.ts +++ b/apps/api/src/github-webhook.ts @@ -65,6 +65,7 @@ import { hasIngestableAttachmentUrl } from "./github-attachment-extract"; import { cacheRepoPrivacy, githubAppConfig, installationForRepo } from "./github-app"; import { commentCacheKey, gatherCommentBody, upsertBotComment } from "./github-comment"; import { bumpPublicTitleEpoch, titleCacheKeys } from "./github-titles"; +import { applyPrActivityWebhook } from "./github-pr-activity"; import { ATTACHMENTS_MARKER } from "./github-comment-render"; import { findObjectsByMetadata, setFileMetadata } from "./file-metadata"; import type { GhTarget } from "./github-comment-render"; @@ -140,6 +141,24 @@ function repoFullNames(value: unknown): string[] { /** `pull_request` actions that trigger auto-promotion (a fresh/updated head to promote from). */ const PROMOTE_ACTIONS = new Set(["opened", "reopened", "synchronize"]); +/** `pull_request` actions whose payload title/state goes onto the PR rollup row. */ +const PR_ACTIVITY_ACTIONS = new Set(["opened", "edited", "closed", "reopened"]); + +/** Rollup state from a `pull_request` delivery, or null when the action is not one we record. */ +function pullRequestState( + action: unknown, + pr: PullRequestPayload["pull_request"], +): "open" | "closed" | "merged" | null { + if (typeof action !== "string" || !PR_ACTIVITY_ACTIONS.has(action) || !pr) return null; + const merged = + pr.merged === true || (typeof pr.merged_at === "string" && pr.merged_at.length > 0); + if (action === "closed") return merged ? "merged" : "closed"; + if (action === "opened" || action === "reopened") return "open"; + if (pr.state === "closed") return merged ? "merged" : "closed"; + if (pr.state === "open") return "open"; + return null; +} + /** * Privacy write-through (issue #631): `{ repo, isPrivate }` when `repo` * carries a boolean `private` field, else `undefined`. Takes the typed @@ -161,6 +180,9 @@ interface PullRequestPayload { repository?: { full_name?: unknown; private?: unknown }; pull_request?: { number?: unknown; + title?: unknown; + state?: unknown; + merged_at?: unknown; /** Only meaningful on `action: "closed"` — true iff the PR was merged * (vs. closed without merging). */ merged?: unknown; @@ -424,6 +446,10 @@ export interface WebhookEvent { * public-title epoch to bump, so its cached `ghref:pub:` titles stop * being served. */ privatized?: string; + /** `pull_request` opened/edited/closed/reopened → title + state onto the + * PR's rollup row (`applyPrActivityWebhook`: UPDATE only, linked repos + * only). GitHub caps titles at 256 chars, so this stays queue-compact. */ + prActivity?: { repo: string; number: number; title: string; state: "open" | "closed" | "merged" }; } /** @@ -495,6 +521,15 @@ export function extractWebhookEvent(eventType: string, payload: unknown): Webhoo const action = pp.action; const repo = pp.repository?.full_name; const pr = pp.pull_request; + const prState = pullRequestState(action, pr); + if ( + prState && + typeof repo === "string" && + typeof pr?.number === "number" && + typeof pr.title === "string" + ) { + ev.prActivity = { repo, number: pr.number, title: pr.title, state: prState }; + } if ( typeof action === "string" && PROMOTE_ACTIONS.has(action) && @@ -590,7 +625,8 @@ export function extractWebhookEvent(eventType: string, payload: unknown): Webhoo ev.adopt || ev.merge || ev.privacy || - ev.privatized + ev.privatized || + ev.prActivity ? ev : null; } @@ -629,6 +665,12 @@ export async function processWebhookEvent(env: Env, ev: WebhookEvent): Promise env.GITHUB_CACHE.delete(key))); + // Before promote/reconcile: those throw for queue retry, and a retry must + // not be the only way the rollup row learns its title/state. Never throws. + if (ev.prActivity) { + await applyPrActivityWebhook(dbFor(env), ev.prActivity); + } + if (ev.promote) { await autoPromoteAndComment(env, ev.promote.repo, ev.promote.num, ev.promote.branch); } diff --git a/apps/api/test/github-pr-activity-sqlite.test.ts b/apps/api/test/github-pr-activity-sqlite.test.ts index 8662c874..a3616587 100644 --- a/apps/api/test/github-pr-activity-sqlite.test.ts +++ b/apps/api/test/github-pr-activity-sqlite.test.ts @@ -2,10 +2,16 @@ import { describe, expect, it } from "vitest"; import { + applyPrActivityWebhook, + backfillPrActivityState, + countOpenPullsByRepo, listPrActivityForWorkspace, + listPrActivityPage, recordPrActivityFromMetadata, recordPrMediaActivity, } from "../src/github-pr-activity"; +import { processWebhookEvent } from "../src/github-webhook"; +import { FakeKv } from "./fake-kv"; import { SqliteD1, database } from "./helpers/sqlite-d1"; const MIGRATION = "migrations/20260721150000_github_pr_activity.sql"; @@ -148,3 +154,234 @@ describe("recordPrActivityFromMetadata", () => { } }); }); + +const ROLLUP_MIGRATIONS = [ + "migrations/20260720120000_github_repo_links.sql", + "migrations/20260721150000_github_pr_activity.sql", + "migrations/20261004120100_pr_activity_title_state.sql", +]; + +function linkRepo(sqlite: SqliteD1, repo: string, workspace: string) { + sqlite.db + .prepare( + `INSERT INTO github_repo_links (repo_full_name, workspace_name, installation_id, source, created_at) + VALUES (?, ?, NULL, 'test', '2026-10-01T00:00:00.000Z')`, + ) + .run(repo, workspace); +} + +function rollup(sqlite: SqliteD1, ref: string) { + return sqlite.db + .prepare(`SELECT title, state, workspace_name FROM github_pr_activity WHERE ref = ?`) + .get(ref) as { title: string | null; state: string | null; workspace_name: string } | undefined; +} + +describe("PR rollup title/state", () => { + it("pages newest first, breaks last_media_at ties by ref, and filters by repo and state", async () => { + const sqlite = new SqliteD1(ROLLUP_MIGRATIONS); + try { + const db = database(sqlite); + const t = (h: number) => new Date(Date.UTC(2026, 9, 1, h)); + await recordPrMediaActivity( + db, + { repo: "acme/web", prNumber: 1, workspaceName: "acme", count: 1 }, + t(1), + ); + await recordPrMediaActivity( + db, + { repo: "acme/web", prNumber: 2, workspaceName: "acme", count: 1 }, + t(2), + ); + await recordPrMediaActivity( + db, + { repo: "acme/web", prNumber: 3, workspaceName: "acme", count: 1 }, + t(2), + ); + await recordPrMediaActivity( + db, + { repo: "acme/api", prNumber: 4, workspaceName: "acme", count: 1 }, + t(0), + ); + await recordPrMediaActivity( + db, + { repo: "acme/zzz", prNumber: 9, workspaceName: "beta", count: 1 }, + t(5), + ); + sqlite.db + .prepare(`UPDATE github_pr_activity SET state = 'merged' WHERE ref = 'acme/web#1'`) + .run(); + sqlite.db + .prepare(`UPDATE github_pr_activity SET state = 'open' WHERE ref = 'acme/web#2'`) + .run(); + + const first = await listPrActivityPage(db, "acme", { limit: 2 }); + expect(first.rows.map((r) => r.ref)).toEqual(["acme/web#2", "acme/web#3"]); + expect(first.rows[0]).toMatchObject({ state: "open", title: null, prNumber: 2 }); + expect(first.nextCursor).toEqual({ updatedAt: t(2).toISOString(), key: "acme/web#3" }); + const second = await listPrActivityPage(db, "acme", { limit: 2, cursor: first.nextCursor }); + expect(second.rows.map((r) => r.ref)).toEqual(["acme/web#1", "acme/api#4"]); + expect(second.nextCursor).toBeNull(); + + const open = await listPrActivityPage(db, "acme", { limit: 20, state: "open" }); + expect(open.rows.map((r) => r.ref)).toEqual(["acme/web#2"]); + const api = await listPrActivityPage(db, "acme", { limit: 20, repo: "acme/api" }); + expect(api.rows.map((r) => r.ref)).toEqual(["acme/api#4"]); + } finally { + sqlite.close(); + } + }); + + it("keeps only rows at or after `since` and pages inside that window", async () => { + const sqlite = new SqliteD1(ROLLUP_MIGRATIONS); + try { + const db = database(sqlite); + const t = (h: number) => new Date(Date.UTC(2026, 9, 1, h)); + await recordPrMediaActivity( + db, + { repo: "acme/web", prNumber: 1, workspaceName: "acme", count: 1 }, + t(1), + ); + await recordPrMediaActivity( + db, + { repo: "acme/web", prNumber: 2, workspaceName: "acme", count: 1 }, + t(2), + ); + await recordPrMediaActivity( + db, + { repo: "acme/web", prNumber: 3, workspaceName: "acme", count: 1 }, + t(3), + ); + + const since = t(2).toISOString(); + const first = await listPrActivityPage(db, "acme", { limit: 1, since }); + expect(first.rows.map((r) => r.ref)).toEqual(["acme/web#3"]); + const second = await listPrActivityPage(db, "acme", { + limit: 1, + since, + cursor: first.nextCursor, + }); + expect(second.rows.map((r) => r.ref)).toEqual(["acme/web#2"]); // boundary row is kept + expect(second.nextCursor).toBeNull(); // #1 is before the window + } finally { + sqlite.close(); + } + }); + + it("applies webhook title/state to a linked row, lowercases the ref, and never inserts", async () => { + const sqlite = new SqliteD1(ROLLUP_MIGRATIONS); + try { + const db = database(sqlite); + await recordPrMediaActivity(db, { + repo: "acme/web", + prNumber: 7, + workspaceName: "acme", + count: 1, + }); + linkRepo(sqlite, "acme/web", "acme"); + + await applyPrActivityWebhook(db, { + repo: "Acme/Web", + number: 7, + title: "Fix login", + state: "merged", + }); + expect(rollup(sqlite, "acme/web#7")).toMatchObject({ title: "Fix login", state: "merged" }); + + await applyPrActivityWebhook(db, { + repo: "acme/web", + number: 8, + title: "No media", + state: "open", + }); + expect(rollup(sqlite, "acme/web#8")).toBeUndefined(); + } finally { + sqlite.close(); + } + }); + + it("does not write a title into a row another workspace fabricated (review focus 3)", async () => { + const sqlite = new SqliteD1(ROLLUP_MIGRATIONS); + try { + const db = database(sqlite); + // mallory tagged an upload gh.repo=acme/secret, gh.number=1; acme owns the repo link. + await recordPrMediaActivity(db, { + repo: "acme/secret", + prNumber: 1, + workspaceName: "mallory", + count: 1, + }); + linkRepo(sqlite, "acme/secret", "acme"); + + await applyPrActivityWebhook(db, { + repo: "acme/secret", + number: 1, + title: "Secret plan", + state: "open", + }); + expect(rollup(sqlite, "acme/secret#1")).toMatchObject({ + workspace_name: "mallory", + title: null, + state: null, + }); + } finally { + sqlite.close(); + } + }); + + it("backfills title/state, skipping invalid states, and counts open PRs per repo", async () => { + const sqlite = new SqliteD1(ROLLUP_MIGRATIONS); + try { + const db = database(sqlite); + for (const n of [1, 2, 3]) { + await recordPrMediaActivity(db, { + repo: "acme/web", + prNumber: n, + workspaceName: "acme", + count: 1, + }); + } + await recordPrMediaActivity(db, { + repo: "acme/api", + prNumber: 4, + workspaceName: "acme", + count: 1, + }); + await backfillPrActivityState(db, [ + { ref: "acme/web#1", title: "One", state: "open" }, + { ref: "acme/web#2", title: "Two", state: "open" }, + { ref: "acme/web#3", title: "Three", state: "draft" }, + { ref: "acme/api#4", title: "Four", state: "closed" }, + ]); + expect(rollup(sqlite, "acme/web#1")).toMatchObject({ title: "One", state: "open" }); + expect(rollup(sqlite, "acme/web#3")).toMatchObject({ title: null, state: null }); + + const counts = await countOpenPullsByRepo(db, "acme", ["acme/web", "acme/api", "acme/none"]); + expect(Object.fromEntries(counts)).toEqual({ "acme/web": 2 }); + expect((await countOpenPullsByRepo(db, "acme", [])).size).toBe(0); + } finally { + sqlite.close(); + } + }); + + it("processWebhookEvent writes a prActivity event through to D1", async () => { + const sqlite = new SqliteD1(ROLLUP_MIGRATIONS); + try { + const db = database(sqlite); + await recordPrMediaActivity(db, { + repo: "acme/web", + prNumber: 5, + workspaceName: "acme", + count: 1, + }); + linkRepo(sqlite, "acme/web", "acme"); + const env = { DB: db, GITHUB_CACHE: new FakeKv() } as unknown as Env; + await processWebhookEvent(env, { + keys: [], + prActivity: { repo: "acme/web", number: 5, title: "Ship it", state: "closed" }, + }); + expect(rollup(sqlite, "acme/web#5")).toMatchObject({ title: "Ship it", state: "closed" }); + } finally { + sqlite.close(); + } + }); +}); From 2a82a8ec0a88bd9b40d2d163eaa660bcf09512f3 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 16:55:55 -0400 Subject: [PATCH 05/14] fix(api): drop PR title and state when another workspace takes over the rollup row --- apps/api/src/github-pr-activity.ts | 21 ++++- apps/api/src/github-webhook.ts | 9 +-- .../test/github-pr-activity-sqlite.test.ts | 81 ++++++++++++++++++- 3 files changed, 99 insertions(+), 12 deletions(-) diff --git a/apps/api/src/github-pr-activity.ts b/apps/api/src/github-pr-activity.ts index 82ddf247..601fcf6e 100644 --- a/apps/api/src/github-pr-activity.ts +++ b/apps/api/src/github-pr-activity.ts @@ -63,7 +63,12 @@ export interface PrMediaEvent { * Best-effort upsert: never throws — activity tracking rides along with an * upload/promote response and a D1 blip here must not fail that request. * `first_media_at` is set once; `last_media_at` always advances; a null - * branch never clobbers a previously recorded one. + * branch never clobbers a previously recorded one. Invariant: `title`/`state` + * belong to the workspace that owns the row, so when the writer's workspace + * differs from the stored one the row changes hands and title/state reset to + * NULL (they refill via the linked-repo webhook or the `/pulls` backfill). + * Without this, a workspace that tags another's `gh.repo`/`gh.number` would + * inherit that workspace's private title and state. */ export async function recordPrMediaActivity( db: D1Queryable, @@ -82,6 +87,8 @@ export async function recordPrMediaActivity( ON CONFLICT(ref) DO UPDATE SET media_count = media_count + excluded.media_count, branch = COALESCE(excluded.branch, branch), + title = CASE WHEN workspace_name = excluded.workspace_name THEN title ELSE NULL END, + state = CASE WHEN workspace_name = excluded.workspace_name THEN state ELSE NULL END, workspace_name = excluded.workspace_name, last_media_at = excluded.last_media_at`, ) @@ -297,10 +304,13 @@ export async function applyPrActivityWebhook( /** * Lazy fill from the `/pulls` handler: rows whose resolved title/state - * differs from the stored one. Invalid states are skipped. Never throws. + * differs from the stored one. Scoped to `workspaceName`: a row is only + * written while it still belongs to the caller's workspace. Invalid states + * are skipped. Never throws. */ export async function backfillPrActivityState( db: D1Queryable, + workspaceName: string, rows: Array<{ ref: string; title: string; state: string }>, ): Promise { const valid = rows.filter((row) => isPrState(row.state)); @@ -309,8 +319,11 @@ export async function backfillPrActivityState( await db.batch( valid.map((row) => db - .prepare(`UPDATE github_pr_activity SET title = ?, state = ? WHERE ref = ?`) - .bind(row.title, row.state, row.ref), + .prepare( + `UPDATE github_pr_activity SET title = ?, state = ? + WHERE ref = ? AND workspace_name = ?`, + ) + .bind(row.title, row.state, row.ref, workspaceName), ), ); } catch (err) { diff --git a/apps/api/src/github-webhook.ts b/apps/api/src/github-webhook.ts index 9d8ec365..14526955 100644 --- a/apps/api/src/github-webhook.ts +++ b/apps/api/src/github-webhook.ts @@ -65,7 +65,7 @@ import { hasIngestableAttachmentUrl } from "./github-attachment-extract"; import { cacheRepoPrivacy, githubAppConfig, installationForRepo } from "./github-app"; import { commentCacheKey, gatherCommentBody, upsertBotComment } from "./github-comment"; import { bumpPublicTitleEpoch, titleCacheKeys } from "./github-titles"; -import { applyPrActivityWebhook } from "./github-pr-activity"; +import { applyPrActivityWebhook, type PrState } from "./github-pr-activity"; import { ATTACHMENTS_MARKER } from "./github-comment-render"; import { findObjectsByMetadata, setFileMetadata } from "./file-metadata"; import type { GhTarget } from "./github-comment-render"; @@ -145,10 +145,7 @@ const PROMOTE_ACTIONS = new Set(["opened", "reopened", "synchronize"]); const PR_ACTIVITY_ACTIONS = new Set(["opened", "edited", "closed", "reopened"]); /** Rollup state from a `pull_request` delivery, or null when the action is not one we record. */ -function pullRequestState( - action: unknown, - pr: PullRequestPayload["pull_request"], -): "open" | "closed" | "merged" | null { +function pullRequestState(action: unknown, pr: PullRequestPayload["pull_request"]): PrState | null { if (typeof action !== "string" || !PR_ACTIVITY_ACTIONS.has(action) || !pr) return null; const merged = pr.merged === true || (typeof pr.merged_at === "string" && pr.merged_at.length > 0); @@ -449,7 +446,7 @@ export interface WebhookEvent { /** `pull_request` opened/edited/closed/reopened → title + state onto the * PR's rollup row (`applyPrActivityWebhook`: UPDATE only, linked repos * only). GitHub caps titles at 256 chars, so this stays queue-compact. */ - prActivity?: { repo: string; number: number; title: string; state: "open" | "closed" | "merged" }; + prActivity?: { repo: string; number: number; title: string; state: PrState }; } /** diff --git a/apps/api/test/github-pr-activity-sqlite.test.ts b/apps/api/test/github-pr-activity-sqlite.test.ts index a3616587..cd98ca02 100644 --- a/apps/api/test/github-pr-activity-sqlite.test.ts +++ b/apps/api/test/github-pr-activity-sqlite.test.ts @@ -14,7 +14,10 @@ import { processWebhookEvent } from "../src/github-webhook"; import { FakeKv } from "./fake-kv"; import { SqliteD1, database } from "./helpers/sqlite-d1"; -const MIGRATION = "migrations/20260721150000_github_pr_activity.sql"; +const MIGRATION = [ + "migrations/20260721150000_github_pr_activity.sql", + "migrations/20261004120100_pr_activity_title_state.sql", +]; describe("github pr activity persistence against SQLite", () => { it("returns an empty feed for a workspace with no activity", async () => { @@ -328,6 +331,80 @@ describe("PR rollup title/state", () => { } }); + it("resets title/state when another workspace's media upsert takes the row over", async () => { + const sqlite = new SqliteD1(ROLLUP_MIGRATIONS); + try { + const db = database(sqlite); + await recordPrMediaActivity(db, { + repo: "acme/secret", + prNumber: 1, + workspaceName: "acme", + count: 1, + }); + linkRepo(sqlite, "acme/secret", "acme"); + await applyPrActivityWebhook(db, { + repo: "acme/secret", + number: 1, + title: "Secret plan", + state: "open", + }); + expect(rollup(sqlite, "acme/secret#1")).toMatchObject({ + title: "Secret plan", + state: "open", + }); + + // Same workspace writing again keeps title/state. + await recordPrMediaActivity(db, { + repo: "acme/secret", + prNumber: 1, + workspaceName: "acme", + count: 1, + }); + expect(rollup(sqlite, "acme/secret#1")).toMatchObject({ + title: "Secret plan", + state: "open", + }); + + // mallory tags the same gh.repo/gh.number afterwards: the row changes hands without the title. + await recordPrMediaActivity(db, { + repo: "acme/secret", + prNumber: 1, + workspaceName: "mallory", + count: 1, + }); + expect(rollup(sqlite, "acme/secret#1")).toMatchObject({ + workspace_name: "mallory", + title: null, + state: null, + }); + const page = await listPrActivityPage(db, "mallory", { limit: 10 }); + expect(page.rows).toHaveLength(1); + expect(page.rows[0]).toMatchObject({ title: null, state: null }); + expect((await countOpenPullsByRepo(db, "mallory", ["acme/secret"])).size).toBe(0); + } finally { + sqlite.close(); + } + }); + + it("backfill never writes a row owned by another workspace", async () => { + const sqlite = new SqliteD1(ROLLUP_MIGRATIONS); + try { + const db = database(sqlite); + await recordPrMediaActivity(db, { + repo: "acme/web", + prNumber: 1, + workspaceName: "acme", + count: 1, + }); + await backfillPrActivityState(db, "mallory", [ + { ref: "acme/web#1", title: "Nope", state: "open" }, + ]); + expect(rollup(sqlite, "acme/web#1")).toMatchObject({ title: null, state: null }); + } finally { + sqlite.close(); + } + }); + it("backfills title/state, skipping invalid states, and counts open PRs per repo", async () => { const sqlite = new SqliteD1(ROLLUP_MIGRATIONS); try { @@ -346,7 +423,7 @@ describe("PR rollup title/state", () => { workspaceName: "acme", count: 1, }); - await backfillPrActivityState(db, [ + await backfillPrActivityState(db, "acme", [ { ref: "acme/web#1", title: "One", state: "open" }, { ref: "acme/web#2", title: "Two", state: "open" }, { ref: "acme/web#3", title: "Three", state: "draft" }, From 7b718a9a17d92202b9d5f1b5e78751f093a45982 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 16:59:42 -0400 Subject: [PATCH 06/14] feat(api): add workspace pulls and repos endpoints --- apps/api/package.json | 1 + apps/api/src/feed-service.ts | 53 +-- apps/api/src/feeds.ts | 3 +- apps/api/src/github-repo-links.ts | 15 + apps/api/src/index.ts | 4 + apps/api/src/poster.ts | 7 +- apps/api/src/routes/feeds.ts | 6 +- apps/api/src/routes/workspace-github.ts | 14 +- apps/api/src/routes/workspace-scope.ts | 219 ++++++++++ apps/api/src/scope-service.ts | 32 ++ apps/api/src/scope-wire.ts | 163 ++++++++ apps/api/test/routes-workspace-scope.test.ts | 402 +++++++++++++++++++ 12 files changed, 854 insertions(+), 65 deletions(-) create mode 100644 apps/api/src/routes/workspace-scope.ts create mode 100644 apps/api/src/scope-service.ts create mode 100644 apps/api/src/scope-wire.ts create mode 100644 apps/api/test/routes-workspace-scope.test.ts diff --git a/apps/api/package.json b/apps/api/package.json index 2905dbe7..4d3de367 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -21,6 +21,7 @@ "./gallery-service": "./src/gallery-service.ts", "./feeds": "./src/feeds.ts", "./feed-service": "./src/feed-service.ts", + "./scope-wire": "./src/scope-wire.ts", "./external-references": "./src/external-references.ts", "./uploader-identity": "./src/uploader-identity.ts", "./github-comment-service": "./src/github-comment-service.ts", diff --git a/apps/api/src/feed-service.ts b/apps/api/src/feed-service.ts index 669e562b..22d57e25 100644 --- a/apps/api/src/feed-service.ts +++ b/apps/api/src/feed-service.ts @@ -21,7 +21,8 @@ import { type FeedRecord, type FeedSource, } from "./feeds"; -import { isDerivedPosterContentType, videoPresentation, type VideoDimensions } from "./poster"; +import { isDerivedPosterContentType, videoPresentation } from "./poster"; +import type { FeedItemDto, FeedSummaryDto, PublicFeedItemDto } from "./scope-wire"; import { createLaneResolver, objectPublicUrls, type LaneResolver } from "./storage"; import type { StorageConfig } from "@uploads/storage"; import { objectVisibility } from "./visibility"; @@ -37,40 +38,7 @@ type FeedObjectHead = { metadata?: Record; }; -export interface FeedItemDto { - id: string; - objectKey: string; - filename: string; - status: "available" | "missing" | "withheld"; - url: string | null; - embedUrl: string | null; - /** Owner-only item page (`/c//`). Absent on the public DTO. */ - pageUrl?: string; - contentType: string | null; - size: number | null; - uploaded: string | null; - modified: string | null; - path: string | null; - state: string | null; - posterUrl?: string; - videoDimensions?: VideoDimensions; -} - -export interface PublicFeedItemDto { - id: string; - filename: string; - status: "available" | "missing" | "withheld"; - url: string | null; - embedUrl: string | null; - contentType: string | null; - size: number | null; - uploaded?: string; - modified?: string; - path: string | null; - state: string | null; - posterUrl?: string; - videoDimensions?: VideoDimensions; -} +export type { FeedItemDto, FeedSummaryDto, PublicFeedItemDto }; export interface FeedDto { id: string; @@ -87,21 +55,6 @@ export interface FeedDto { items: FeedItemDto[]; } -export interface FeedSummaryDto { - id: string; - url: string; - workspace: string; - repo: string; - path: string | null; - number: number | null; - kind: "pull" | "issue" | null; - title: string; - /** "comment" (PR comment sync), "user", or null for feeds created before this field. */ - source: FeedSource | null; - createdAt: string; - updatedAt: string; -} - export type PublicFeedDto = { id: string; title: string; diff --git a/apps/api/src/feeds.ts b/apps/api/src/feeds.ts index 0342e0e7..dd0e9017 100644 --- a/apps/api/src/feeds.ts +++ b/apps/api/src/feeds.ts @@ -5,6 +5,7 @@ * matches galleries: anyone who knows the URL can view the feed. */ import { type D1Queryable } from "./db-session"; +import type { FeedSource } from "./scope-wire"; /** * Live repo-scoped feeds (`number = 0`) with `source = 'user'` per workspace. @@ -24,7 +25,7 @@ const FEED_SELECT = "id, workspace, repo, path, number, kind, source, created_at, updated_at, deleted_at"; /** Who created a feed row. Comment-sync rows are uncapped; see `createFeed`. */ -export type FeedSource = "comment" | "user"; +export type { FeedSource }; export interface FeedRecord { id: string; diff --git a/apps/api/src/github-repo-links.ts b/apps/api/src/github-repo-links.ts index e7b103da..661891cb 100644 --- a/apps/api/src/github-repo-links.ts +++ b/apps/api/src/github-repo-links.ts @@ -260,3 +260,18 @@ export async function setRepoLink( .bind(normalizeRepo(repo), workspaceName, installationId, source, now.toISOString()) .run(); } + +/** + * Lowercased repos bound to `workspaceName`: the `linkedRepos` of the member + * title audience (`resolveTitles(env, refs, { audience: "member", linkedRepos })`, + * github-titles.ts). Fails closed: a D1 error yields an empty set, so every + * ref falls back to the public ladder. + */ +export async function linkedRepoSet(db: D1Queryable, workspaceName: string): Promise> { + return new Set( + await listRepoLinksForWorkspace(db, workspaceName).then( + (links) => links.map((link) => link.repo.toLowerCase()), + () => [], + ), + ); +} diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index 8944a86c..fb67b79c 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -28,6 +28,7 @@ import { galleries } from "./routes/galleries"; import { publicGalleries } from "./routes/public-galleries"; import { feeds } from "./routes/feeds"; import { workspaceFeeds } from "./routes/workspace-feeds"; +import { workspaceScope } from "./routes/workspace-scope"; import { publicFeeds } from "./routes/public-feeds"; import { publicFiles } from "./routes/public-files"; import { publicGithubAvatars } from "./routes/public-github-avatars"; @@ -163,6 +164,9 @@ export const app = new Hono() // `workspaceFiles` above. .route("/v1/workspaces", workspaceGalleries) .route("/v1/workspaces", workspaceFeeds) + // Files views (spec 2026-10-04): `/pulls`, `/repos`, `/scope/:owner/:repo/files`. + // Dual-auth, files:read; own auth + error boundary like the verticals above. + .route("/v1/workspaces", workspaceScope) .route("/v1/workspaces", workspaceUsage) // Canonical dual-auth github vertical (issue #613 phase 3): // `/v1/workspaces/:workspace/github/*`, collapsing the five sub-routers diff --git a/apps/api/src/poster.ts b/apps/api/src/poster.ts index fbd166bd..7ce268b5 100644 --- a/apps/api/src/poster.ts +++ b/apps/api/src/poster.ts @@ -24,6 +24,7 @@ import type { StorageConfig } from "@uploads/storage"; import { allowPoster, VIDEO_TYPES } from "./guards"; import { overlayPlayButton } from "./poster-overlay"; import { objectPublicUrls } from "./storage"; +import type { VideoDimensions } from "./scope-wire"; /** Server-owned namespace for derived artifacts — never listed to users. */ export const POSTER_KEY_PREFIX = "_internal/posters/"; @@ -37,11 +38,7 @@ export function posterKeyFor(key: string): string { return `${POSTER_KEY_PREFIX}${key}.jpg`; } -/** Real display dimensions of a video, as stamped in `video.width`/`video.height`. */ -export interface VideoDimensions { - width: number; - height: number; -} +export type { VideoDimensions }; const POSITIVE_INT_RE_STRICT = /^[1-9][0-9]*$/; diff --git a/apps/api/src/routes/feeds.ts b/apps/api/src/routes/feeds.ts index 652e6a5f..0318694e 100644 --- a/apps/api/src/routes/feeds.ts +++ b/apps/api/src/routes/feeds.ts @@ -13,6 +13,7 @@ import { requireScope, type WorkspaceVars } from "../workspace"; import { jsonBody } from "./json-body"; import { dbFor } from "../db-session"; import { boundedDataRead } from "../data-read-bounds"; +import type { FeedListResponse } from "../scope-wire"; async function ownerFeed(c: Context, id: string) { const record = await getFeed(dbFor(c.env), c.get("workspaceName"), id); @@ -51,10 +52,11 @@ export async function listFeedsHandler(c: Context) { }), { name: "d1_feeds_list" }, ); - return c.json({ + const body: FeedListResponse = { feeds: page.feeds.map((feed) => feedSummary(c.env, feed)), nextCursor: page.nextCursor ? encodeFeedCursor(page.nextCursor) : null, - }); + }; + return c.json(body); } export async function getFeedHandler(c: Context) { diff --git a/apps/api/src/routes/workspace-github.ts b/apps/api/src/routes/workspace-github.ts index afeb5e99..c0173edd 100644 --- a/apps/api/src/routes/workspace-github.ts +++ b/apps/api/src/routes/workspace-github.ts @@ -71,7 +71,12 @@ import { parseExternalReference } from "../external-references"; import { respondError } from "../error-response"; import { githubInstallStatus, type GithubInstallStatus } from "../github-install-status"; import { reconcileIngestTarget } from "../github-ingest"; -import { deriveRepoBinding, findRepoLink, listRepoLinksForWorkspace } from "../github-repo-links"; +import { + deriveRepoBinding, + findRepoLink, + linkedRepoSet, + listRepoLinksForWorkspace, +} from "../github-repo-links"; import { resolveTitles } from "../github-titles"; import { writeRateLimit } from "../guards"; import { adminWorkspaceOr403, memberWorkspaceOr404 } from "../org-workspaces"; @@ -247,12 +252,7 @@ const githubTitlesHandler: Handler = async (c) => { const name = c.req.param("workspace") ?? ""; // Fail closed: a D1 blip degrades every ref to the public audience. - const linkedRepos = new Set( - await listRepoLinksForWorkspace(dbFor(c.env), name).then( - (links) => links.map((link) => link.repo.toLowerCase()), - () => [], - ), - ); + const linkedRepos = await linkedRepoSet(dbFor(c.env), name); const titles = await resolveTitles(c.env, [...new Set(normalized)], { audience: "member", linkedRepos, diff --git a/apps/api/src/routes/workspace-scope.ts b/apps/api/src/routes/workspace-scope.ts new file mode 100644 index 00000000..5f860b33 --- /dev/null +++ b/apps/api/src/routes/workspace-scope.ts @@ -0,0 +1,219 @@ +/** + * Signed-in Files views (spec .context/2026-10-04-pr-first-workspace-and-live-links.md): + * `GET /:workspace/pulls`, `GET /:workspace/repos`, and (Task 6) + * `GET /:workspace/scope/:owner/:repo/files`, mounted at `/v1/workspaces`. + * Dual-auth (session or bearer), `files:read`, tight read limiter: each + * request fans out into one D1 scope query per row for thumbnails. + */ +import { ValidationError } from "@uploads/errors"; +import { Hono, type Context, type MiddlewareHandler } from "hono"; +import { boundedDataRead } from "../data-read-bounds"; +import { dbFor } from "../db-session"; +import { dualWorkspaceAuth, type DualAuthVars } from "../dual-workspace-auth"; +import { respondError } from "../error-response"; +import { unwrapFeedMutation } from "../feed-service"; +import { normalizeFeedRepo } from "../feeds"; +import { + backfillPrActivityState, + countOpenPullsByRepo, + isPrState, + listPrActivityPage, + type PrState, +} from "../github-pr-activity"; +import { parseFileTypeQuery } from "../file-type-sql"; +import { linkedRepoSet } from "../github-repo-links"; +import { resolveTitles, withPublicTitleBudget, type TitleInfo } from "../github-titles"; +import { + decodeScopeCursor, + encodeScopeCursor, + listWorkspaceRepos, + prScopeQuery, +} from "../pr-scope"; +import { heavyReadRateLimit } from "../read-limits"; +import { toThumbItem } from "../scope-service"; +import type { PullsResponse, ReposResponse } from "../scope-wire"; +import { storageConfig } from "../storage"; +import { requireScope } from "../workspace"; + +const PULLS_DEFAULT_LIMIT = 20; +const PULLS_MAX_LIMIT = 100; +/** `/pulls` shows PRs with media in this many days unless `all=1`. */ +export const PULLS_DEFAULT_WINDOW_DAYS = 90; +const REPOS_DEFAULT_LIMIT = 20; +/** `countOpenPullsByRepo` binds one parameter per repo; D1 allows 100 per query. */ +const REPOS_MAX_LIMIT = 50; +const THUMBNAIL_LIMIT = 4; + +function scoped(scope: Parameters[0]): MiddlewareHandler { + return requireScope(scope) as unknown as MiddlewareHandler; +} +const heavyRead = heavyReadRateLimit as unknown as MiddlewareHandler; + +function parseLimit(raw: string | undefined, fallback: number, max: number): number { + if (raw === undefined) return fallback; + const limit = Number(raw); + if (!Number.isInteger(limit) || limit < 1 || limit > max) { + throw new ValidationError(`limit must be an integer between 1 and ${max}.`, { + code: "invalid_limit", + }); + } + return limit; +} + +function parseState(raw: string | undefined): PrState | undefined { + if (raw === undefined || raw === "") return undefined; + if (isPrState(raw)) return raw; + throw new ValidationError("state must be open, closed, or merged.", { code: "invalid_state" }); +} + +/** `?all=1` opts out of the recency window; any other value keeps it. */ +function pullsSince(all: string | undefined, now: number = Date.now()): string | undefined { + if (all === "1") return undefined; + return new Date(now - PULLS_DEFAULT_WINDOW_DAYS * 86_400_000).toISOString(); +} + +/** Lowercased `owner/repo`; 400 `feed_invalid_field` otherwise. */ +function parseRepo(raw: string): string { + return unwrapFeedMutation(normalizeFeedRepo(raw)).value; +} + +/** + * Signed-in title lookup for PR rows (member audience, #1065): private titles + * resolve only for repos linked to this workspace; every other ref uses the + * public ladder. Under the public title budget so a slow GitHub never stalls + * the list (stored titles of linked repos fill in on timeout). + */ +async function resolvePullTitles( + env: Env, + refs: string[], + linkedRepos: ReadonlySet, +): Promise> { + if (refs.length === 0) return {}; + return ( + (await withPublicTitleBudget(resolveTitles(env, refs, { audience: "member", linkedRepos }))) ?? + {} + ); +} + +export async function pullsHandler(c: Context) { + const workspace = c.get("workspaceName"); + const db = dbFor(c.env); + const limit = parseLimit(c.req.query("limit"), PULLS_DEFAULT_LIMIT, PULLS_MAX_LIMIT); + const state = parseState(c.req.query("state")); + const repoParam = c.req.query("repo"); + const repo = repoParam ? parseRepo(repoParam) : undefined; + // Narrows thumbnails only: a PR with no media of this type stays listed. + const type = parseFileTypeQuery(c.req.query("type")); + const cursor = decodeScopeCursor(c.req.query("cursor")); + // Recency window instead of a row cap: only PRs with media in the last 90 + // days unless `all=1` (the Files list's "Show older pull requests"). + const since = pullsSince(c.req.query("all")); + + const page = await boundedDataRead( + c, + () => listPrActivityPage(db, workspace, { repo, state, since, cursor, limit }), + { name: "d1_pulls_list" }, + ); + const linkedRepos = await linkedRepoSet(db, workspace); + const titles = await resolvePullTitles( + c.env, + page.rows.map((row) => row.ref), + linkedRepos, + ); + // Every resolved title here came from the member audience built from this + // workspace's own links, and the UPDATE is scoped to this workspace's rows. + await backfillPrActivityState( + db, + workspace, + page.rows.flatMap((row) => { + const info = titles[row.ref]; + return info && (info.title !== row.title || info.state !== row.state) + ? [{ ref: row.ref, title: info.title, state: info.state }] + : []; + }), + ); + const thumbs = await boundedDataRead( + c, + () => + Promise.all( + page.rows.map((row) => + prScopeQuery(db, { + workspace, + repo: row.repo, + number: row.prNumber, + type, + limit: THUMBNAIL_LIMIT, + }), + ), + ), + { name: "d1_pulls_thumbs" }, + ); + const cfg = await storageConfig(c.env, c.get("workspace")); + + const response: PullsResponse = { + workspace, + pulls: page.rows.map((row, index) => { + const info = titles[row.ref]; + return { + ref: row.ref, + repo: row.repo, + number: row.prNumber, + branch: row.branch, + // A stored title may predate a link change or a repo going private, + // or come from another workspace that owned the row: serve it only + // when this workspace may see the repo's private titles. + title: info?.title ?? (linkedRepos.has(row.repo) ? row.title : null), + state: info?.state ?? row.state, + lastMediaAt: row.lastMediaAt, + thumbnails: (thumbs[index]?.items ?? []).map((item) => toThumbItem(c.env, cfg, item)), + }; + }), + nextCursor: page.nextCursor ? encodeScopeCursor(page.nextCursor) : null, + }; + return c.json(response); +} + +export async function reposHandler(c: Context) { + const workspace = c.get("workspaceName"); + const db = dbFor(c.env); + const limit = parseLimit(c.req.query("limit"), REPOS_DEFAULT_LIMIT, REPOS_MAX_LIMIT); + // Narrows thumbnails only: every repo with GitHub-tagged media stays listed. + const type = parseFileTypeQuery(c.req.query("type")); + const cursor = decodeScopeCursor(c.req.query("cursor")); + + const page = await boundedDataRead( + c, + () => listWorkspaceRepos(db, workspace, { cursor, limit }), + { name: "d1_repos_list" }, + ); + const repos = page.repos.map((row) => row.repo); + const [openCounts, thumbs] = await boundedDataRead( + c, + () => + Promise.all([ + countOpenPullsByRepo(db, workspace, repos), + Promise.all( + repos.map((repo) => prScopeQuery(db, { workspace, repo, type, limit: THUMBNAIL_LIMIT })), + ), + ]), + { name: "d1_repos_detail" }, + ); + const cfg = await storageConfig(c.env, c.get("workspace")); + + const response: ReposResponse = { + workspace, + repos: page.repos.map((row, index) => ({ + repo: row.repo, + lastUpdatedAt: row.lastUpdatedAt, + openPullCount: openCounts.get(row.repo) ?? 0, + thumbnails: (thumbs[index]?.items ?? []).map((item) => toThumbItem(c.env, cfg, item)), + })), + nextCursor: page.nextCursor ? encodeScopeCursor(page.nextCursor) : null, + }; + return c.json(response); +} + +export const workspaceScope = new Hono() + .get("/:workspace/pulls", dualWorkspaceAuth(), heavyRead, scoped("files:read"), pullsHandler) + .get("/:workspace/repos", dualWorkspaceAuth(), heavyRead, scoped("files:read"), reposHandler) + .onError((err, c) => respondError(c, err)); diff --git a/apps/api/src/scope-service.ts b/apps/api/src/scope-service.ts new file mode 100644 index 00000000..70ccfac2 --- /dev/null +++ b/apps/api/src/scope-service.ts @@ -0,0 +1,32 @@ +/** + * Thumbnails for the Files views. D1-only: URLs and poster URLs come from the + * active storage config and the `video.poster` / `pdf.poster` metadata flags, + * with no R2 HEAD per tile. The signed-in audience never withholds, and a + * missing object is not detected here, so `status` is always "available". + */ +import { fileTypeClassFromKey } from "@uploads/comment-render/scope"; +import type { StorageConfig } from "@uploads/storage"; +import { videoPresentation } from "./poster"; +import type { ScopeItem } from "./pr-scope"; +import type { ThumbItem } from "./scope-wire"; +import { objectPublicUrls } from "./storage"; + +export function toThumbItem(env: Env, cfg: StorageConfig, item: ScopeItem): ThumbItem { + const urls = objectPublicUrls(env, cfg, item.key); + const isPdf = item.key.toLowerCase().endsWith(".pdf"); + const { posterUrl } = videoPresentation( + env, + cfg, + item.key, + item.metadata, + isPdf ? "application/pdf" : undefined, + ); + return { + key: item.key, + kind: fileTypeClassFromKey(item.key), + url: urls.url, + embedUrl: urls.embedUrl, + posterUrl: posterUrl ?? null, + status: "available", + }; +} diff --git a/apps/api/src/scope-wire.ts b/apps/api/src/scope-wire.ts new file mode 100644 index 00000000..3b4f2129 --- /dev/null +++ b/apps/api/src/scope-wire.ts @@ -0,0 +1,163 @@ +/** + * Env-free wire types for the signed-in Files views (`/pulls`, `/repos`, + * `/scope/:owner/:repo/files`), the owner feed DTOs (Links tab), the feed + * item DTOs, and the public live-link pager. Exported as + * `@uploads/api/scope-wire` so apps/web can `import type` them without + * pulling a module that references the API worker's `Env` (PR #896 rule in + * AGENTS.md). Producers: `routes/workspace-scope.ts`, `routes/feeds.ts`, + * `feed-service.ts`. + */ +import type { FileTypeClass } from "@uploads/comment-render/scope"; + +/** Who created a feed row. `null` on rows created before `feeds.source`. */ +export type FeedSource = "comment" | "user"; + +/** Real display dimensions of a video, as stamped in `video.width`/`video.height`. */ +export interface VideoDimensions { + width: number; + height: number; +} + +export interface FeedItemDto { + id: string; + objectKey: string; + filename: string; + status: "available" | "missing" | "withheld"; + url: string | null; + embedUrl: string | null; + /** Owner-only item page (`/c//`). Absent on the public DTO. */ + pageUrl?: string; + contentType: string | null; + size: number | null; + uploaded: string | null; + modified: string | null; + path: string | null; + state: string | null; + posterUrl?: string; + videoDimensions?: VideoDimensions; +} + +export interface PublicFeedItemDto { + id: string; + filename: string; + status: "available" | "missing" | "withheld"; + url: string | null; + embedUrl: string | null; + contentType: string | null; + size: number | null; + uploaded?: string; + modified?: string; + path: string | null; + state: string | null; + posterUrl?: string; + videoDimensions?: VideoDimensions; +} + +/** Owner feed summary: `GET /v1/workspaces/:ws/feeds` rows and the create/get bodies minus `items`. */ +export interface FeedSummaryDto { + id: string; + url: string; + workspace: string; + repo: string; + path: string | null; + number: number | null; + kind: "pull" | "issue" | null; + title: string; + createdAt: string; + updatedAt: string; + /** "comment" (PR comment sync), "user", or null for feeds created before this field. */ + source: FeedSource | null; +} + +/** `GET /v1/workspaces/:ws/feeds`. */ +export interface FeedListResponse { + feeds: FeedSummaryDto[]; + nextCursor: string | null; +} + +/** D1-only tile: no R2 HEAD, so `status` is always "available" today. */ +export interface ThumbItem { + key: string; + kind: FileTypeClass; + url: string | null; + embedUrl: string | null; + posterUrl: string | null; + status: "available" | "missing" | "withheld"; +} + +export interface PullRow { + /** "owner/repo#123", lowercased. */ + ref: string; + repo: string; + number: number; + branch: string | null; + title: string | null; + state: "open" | "closed" | "merged" | null; + lastMediaAt: string; + /** Up to 4, newest first. */ + thumbnails: ThumbItem[]; +} + +export interface PullsResponse { + workspace: string; + pulls: PullRow[]; + nextCursor: string | null; +} + +export interface RepoRow { + repo: string; + lastUpdatedAt: string; + openPullCount: number; + thumbnails: ThumbItem[]; +} + +export interface ReposResponse { + workspace: string; + repos: RepoRow[]; + nextCursor: string | null; +} + +export interface LiveLinkRef { + id: string; + url: string; + source: FeedSource | null; +} + +/** PR page header data from the workspace's rollup row (Task 6). */ +export interface ScopePull { + branch: string | null; + title: string | null; + state: "open" | "closed" | "merged" | null; +} + +export interface ScopeFilesResponse { + repo: string; + number: number | null; + items: FeedItemDto[]; + nextCursor: string | null; + /** Withheld-on-public items across the whole scope (cap 2,000), ignoring `type`. First page only; `null` on cursor pages. */ + privateCount: number | null; + /** The existing live feed for exactly this scope, if any. */ + liveLink: LiveLinkRef | null; + /** Set when `number` is set and this workspace has a rollup row for that PR; null otherwise. */ + pull: ScopePull | null; +} + +export interface PublicFeedItemPage { + /** `kind` is the feed summary's (null for a repo-scope live link), the same value `GET /public/feeds/:id` returns. */ + feed: { + id: string; + title: string; + repo: string; + number: number | null; + kind: "pull" | "issue" | null; + }; + item: PublicFeedItemDto; + /** Neighbour item ids in newest-first order. */ + prev: string | null; + next: string | null; + /** 0-based position in the scope. */ + index: number; + /** Scope size, capped at 2,000. */ + total: number; +} diff --git a/apps/api/test/routes-workspace-scope.test.ts b/apps/api/test/routes-workspace-scope.test.ts new file mode 100644 index 00000000..f35043c6 --- /dev/null +++ b/apps/api/test/routes-workspace-scope.test.ts @@ -0,0 +1,402 @@ +/// + +import { afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; +import { app } from "../src/index"; +import { deleteFileMetadata, replaceFileMetadata } from "../src/file-metadata"; +import type { PullsResponse, ReposResponse } from "../src/scope-wire"; +import { sha256Hex, type WorkspaceRecord } from "../src/workspace"; +import { FakeKv } from "./fake-kv"; +import { FakeR2Bucket } from "./fake-r2"; +import { SqliteD1, database } from "./helpers/sqlite-d1"; + +const TOKEN = "scope-token"; +const PNG = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); + +const MIGRATIONS = [ + "migrations/20260710120000_auth.sql", + "migrations/20260710140000_workspace_usage.sql", + "migrations/20260822120100_workspace_usage_shared_subset.sql", + "migrations/20260712230000_token_minting_user.sql", + "migrations/20260817180000_token_last_used.sql", + "migrations/20260925120000_workspace_service_tokens.sql", + "migrations/20260713210559_file_metadata.sql", + "migrations/20260720120000_github_repo_links.sql", + "migrations/20260721150000_github_pr_activity.sql", + "migrations/20260915120000_feeds.sql", + "migrations/20260915153000_feeds_number.sql", + "migrations/20261004120000_feeds_source.sql", + "migrations/20261004120100_pr_activity_title_state.sql", + "migrations/20261004120200_file_metadata_gh_repo_idx.sql", +]; + +beforeAll(() => { + if (!(crypto.subtle as SubtleCrypto & { timingSafeEqual?: unknown }).timingSafeEqual) { + Object.defineProperty(crypto.subtle, "timingSafeEqual", { + value: (left: ArrayBufferView, right: ArrayBufferView) => { + const a = new Uint8Array(left.buffer, left.byteOffset, left.byteLength); + const b = new Uint8Array(right.buffer, right.byteOffset, right.byteLength); + if (a.length !== b.length) return false; + let difference = 0; + for (let index = 0; index < a.length; index++) difference |= a[index] ^ b[index]; + return difference === 0; + }, + }); + } +}); + +let sqlite: SqliteD1; +let bucket: FakeR2Bucket; +let kv: FakeKv; +let env: Parameters[2]; + +afterEach(() => { + sqlite?.close(); +}); + +beforeEach(async () => { + sqlite = new SqliteD1(MIGRATIONS); + bucket = new FakeR2Bucket(); + kv = new FakeKv(); + const record = (prefix: string): WorkspaceRecord => ({ + provider: "r2", + bucket: "shared", + binding: "UPLOADS_DEFAULT", + prefix, + publicBaseUrl: "https://storage.uploads.sh", + }); + const records: Record = { + alpha: { ...record("alpha/"), tokenHash: await sha256Hex(TOKEN) }, + beta: { ...record("beta/"), tokenHash: await sha256Hex(TOKEN) }, + }; + env = { + DB: sqlite as unknown as D1Database, + WEB_ORIGIN: "https://uploads.test", + REGISTRY: { get: async (key: string) => records[key.slice(3)] ?? null }, + UPLOADS_DEFAULT: bucket, + WRITE_LIMITER: { limit: async () => ({ success: true }) }, + GITHUB_CACHE: kv, + } as Parameters[2]; +}); + +function request(path: string, init: RequestInit = {}) { + return app.request( + path, + { + ...init, + headers: { + Authorization: `Bearer ${TOKEN}`, + "Content-Type": "application/json", + ...init.headers, + }, + }, + env, + ); +} + +async function getJson(path: string): Promise { + const res = await request(path); + expect(res.status).toBe(200); + return (await res.json()) as T; +} + +async function errorOf(path: string): Promise<{ status: number; code: string | undefined }> { + const res = await request(path); + const body = (await res.json()) as { error?: { code?: string } }; + return { status: res.status, code: body.error?.code }; +} + +/** Upload through the real PUT route, so a `gh.kind=pull` tag creates the rollup row. */ +async function putShot(workspace: string, key: string, meta: Record) { + const headers: Record = { + Authorization: `Bearer ${TOKEN}`, + "Content-Type": "image/png", + }; + for (const [name, value] of Object.entries(meta)) headers[`X-Uploads-Meta-${name}`] = value; + const response = await app.request( + `/v1/${workspace}/files/${key}`, + { method: "PUT", headers, body: PNG }, + env, + ); + expect(response.status).toBe(201); +} + +/** Seed R2 + D1 directly (any key, any content type, optional private). No rollup row. */ +async function seedObject( + key: string, + meta: Record, + opts: { private?: boolean; contentType?: string; workspace?: string } = {}, +) { + const workspace = opts.workspace ?? "alpha"; + await bucket.put(`${workspace}/${key}`, PNG, { + httpMetadata: { contentType: opts.contentType ?? "image/png" }, + ...(opts.private ? { customMetadata: { visibility: "private" } } : {}), + }); + await replaceFileMetadata(database(sqlite), workspace, key, meta); +} + +/** Pin every metadata row of `key` to one `updated_at`, for deterministic order. */ +function stamp(key: string, iso: string, workspace = "alpha") { + sqlite.db + .prepare(`UPDATE file_metadata SET updated_at = ? WHERE workspace = ? AND object_key = ?`) + .run(iso, workspace, key); +} + +function setRollup(ref: string, fields: { lastMediaAt?: string; state?: string | null }) { + if (fields.lastMediaAt) { + sqlite.db + .prepare(`UPDATE github_pr_activity SET last_media_at = ? WHERE ref = ?`) + .run(fields.lastMediaAt, ref); + } + if (fields.state !== undefined) { + sqlite.db + .prepare(`UPDATE github_pr_activity SET state = ? WHERE ref = ?`) + .run(fields.state, ref); + } +} + +const prMeta = (repo: string, number: number): Record => ({ + "gh.repo": repo, + "gh.number": String(number), + "gh.kind": "pull", +}); + +/** Bind `repo` to `workspace` in github_repo_links (the member title audience reads this). */ +function linkRepo(repo: string, workspace = "alpha") { + sqlite.db + .prepare( + `INSERT INTO github_repo_links (repo_full_name, workspace_name, installation_id, source, created_at) + VALUES (?, ?, NULL, 'test', '2026-10-01T00:00:00.000Z')`, + ) + .run(repo, workspace); +} + +describe("GET /v1/workspaces/:ws/pulls", () => { + it("lists PRs newest first with titles, thumbnails, a keyset cursor, and backfills the row", async () => { + // Linked, so the member ladder reads the `ghref:` cache entry seeded below. + linkRepo("acme/app"); + await putShot("alpha", "gh/acme/app/pull/1/one.png", prMeta("acme/app", 1)); + await putShot("alpha", "gh/acme/app/pull/2/two.png", prMeta("acme/app", 2)); + await putShot("alpha", "gh/acme/app/pull/3/three.png", prMeta("acme/app", 3)); + // Relative to now: the default window is 90 days, so fixed dates would age out. + // One base instant, so the seeded and the asserted timestamps are equal. + const base = Date.now(); + const at = (hours: number) => new Date(base - (10 - hours) * 3_600_000).toISOString(); + setRollup("acme/app#1", { lastMediaAt: at(1) }); + setRollup("acme/app#2", { lastMediaAt: at(2) }); + setRollup("acme/app#3", { lastMediaAt: at(3) }); + kv.store.set("ghref:acme/app#3", { + value: JSON.stringify({ v: { title: "Add dark mode", state: "open", kind: "pull" } }), + }); + + const first = await getJson("/v1/workspaces/alpha/pulls?limit=2"); + expect(first.workspace).toBe("alpha"); + expect(first.pulls.map((p) => p.ref)).toEqual(["acme/app#3", "acme/app#2"]); + expect(first.pulls[0]).toMatchObject({ + repo: "acme/app", + number: 3, + title: "Add dark mode", + state: "open", + lastMediaAt: at(3), + }); + expect(first.pulls[1]).toMatchObject({ number: 2, title: null, state: null }); + expect(first.pulls[0]?.thumbnails).toEqual([ + expect.objectContaining({ + key: "gh/acme/app/pull/3/three.png", + kind: "screenshot", + status: "available", + posterUrl: null, + }), + ]); + expect(first.pulls[0]?.thumbnails[0]?.url).toContain("gh/acme/app/pull/3/three.png"); + expect(first.nextCursor).toEqual(expect.any(String)); + + const second = await getJson( + `/v1/workspaces/alpha/pulls?limit=2&cursor=${first.nextCursor}`, + ); + expect(second.pulls.map((p) => p.ref)).toEqual(["acme/app#1"]); + expect(second.nextCursor).toBeNull(); + + expect( + sqlite.db + .prepare(`SELECT title, state FROM github_pr_activity WHERE ref = ?`) + .get("acme/app#3"), + ).toMatchObject({ title: "Add dark mode", state: "open" }); + }); + + it("serves private titles only for repos linked to this workspace (#1065 member audience)", async () => { + // acme/secret is not linked to alpha: its member-cache entry and a title + // stored on its rollup row (written while another workspace owned the row) + // must not reach alpha. State is still served. + await putShot("alpha", "gh/acme/secret/pull/1/a.png", prMeta("acme/secret", 1)); + kv.store.set("ghref:acme/secret#1", { + value: JSON.stringify({ v: { title: "Secret plan", state: "open", kind: "pull" } }), + }); + sqlite.db + .prepare(`UPDATE github_pr_activity SET title = ?, state = ? WHERE ref = ?`) + .run("Secret plan", "open", "acme/secret#1"); + + const unlinked = await getJson("/v1/workspaces/alpha/pulls"); + expect(unlinked.pulls[0]).toMatchObject({ ref: "acme/secret#1", title: null, state: "open" }); + + linkRepo("acme/secret"); + const linked = await getJson("/v1/workspaces/alpha/pulls"); + expect(linked.pulls[0]).toMatchObject({ ref: "acme/secret#1", title: "Secret plan" }); + }); + + it("filters by repo and state; rows with unknown state appear only unfiltered", async () => { + await putShot("alpha", "gh/acme/app/pull/1/one.png", prMeta("acme/app", 1)); + await putShot("alpha", "gh/acme/app/pull/2/two.png", prMeta("acme/app", 2)); + await putShot("alpha", "gh/acme/app/pull/3/three.png", prMeta("acme/app", 3)); + await putShot("alpha", "gh/acme/site/pull/5/five.png", prMeta("acme/site", 5)); + setRollup("acme/app#1", { state: "merged" }); + setRollup("acme/app#3", { state: "open" }); + + const refs = async (query: string) => + (await getJson(`/v1/workspaces/alpha/pulls${query}`)).pulls + .map((p) => p.ref) + .sort(); + expect(await refs("?state=merged")).toEqual(["acme/app#1"]); + expect(await refs("?state=open")).toEqual(["acme/app#3"]); + expect(await refs("")).toEqual(["acme/app#1", "acme/app#2", "acme/app#3", "acme/site#5"]); + expect(await refs("?repo=Acme/Site")).toEqual(["acme/site#5"]); + expect(await refs("?repo=acme/none")).toEqual([]); + }); + + it("applies type to thumbnails only, on /pulls and /repos; rows are never filtered", async () => { + await putShot("alpha", "gh/acme/app/pull/1/one.png", prMeta("acme/app", 1)); + await seedObject("gh/acme/app/pull/1/clip.mp4", prMeta("acme/app", 1), { + contentType: "video/mp4", + }); + await putShot("alpha", "gh/acme/app/pull/2/two.png", prMeta("acme/app", 2)); + + const pulls = await getJson("/v1/workspaces/alpha/pulls?type=video"); + const thumbsByRef = new Map(pulls.pulls.map((p) => [p.ref, p.thumbnails.map((t) => t.key)])); + expect([...thumbsByRef.keys()].sort()).toEqual(["acme/app#1", "acme/app#2"]); + expect(thumbsByRef.get("acme/app#1")).toEqual(["gh/acme/app/pull/1/clip.mp4"]); + expect(thumbsByRef.get("acme/app#2")).toEqual([]); + + const repos = await getJson("/v1/workspaces/alpha/repos?type=video"); + expect(repos.repos.map((r) => [r.repo, r.thumbnails.map((t) => t.key)])).toEqual([ + ["acme/app", ["gh/acme/app/pull/1/clip.mp4"]], + ]); + + expect(await errorOf("/v1/workspaces/alpha/pulls?type=gif")).toEqual({ + status: 400, + code: "invalid_type", + }); + expect(await errorOf("/v1/workspaces/alpha/repos?type=gif")).toEqual({ + status: 400, + code: "invalid_type", + }); + }); + + it("excludes PRs idle for over 90 days by default and includes them with all=1", async () => { + await putShot("alpha", "gh/acme/app/pull/1/old.png", prMeta("acme/app", 1)); + await putShot("alpha", "gh/acme/app/pull/2/new.png", prMeta("acme/app", 2)); + const daysAgo = (days: number) => new Date(Date.now() - days * 86_400_000).toISOString(); + setRollup("acme/app#1", { lastMediaAt: daysAgo(120) }); + setRollup("acme/app#2", { lastMediaAt: daysAgo(10) }); + + const refs = async (query: string) => + (await getJson(`/v1/workspaces/alpha/pulls${query}`)).pulls.map((p) => p.ref); + expect(await refs("")).toEqual(["acme/app#2"]); + expect(await refs("?all=1")).toEqual(["acme/app#2", "acme/app#1"]); // newest first + // Anything but "1" keeps the window. + expect(await refs("?all=0")).toEqual(["acme/app#2"]); + // A PR just inside the window stays listed. + setRollup("acme/app#1", { lastMediaAt: daysAgo(89) }); + expect(await refs("")).toEqual(["acme/app#2", "acme/app#1"]); + // The window composes with the repo filter and the cursor. + setRollup("acme/app#1", { lastMediaAt: daysAgo(120) }); + expect(await refs("?repo=acme/app")).toEqual(["acme/app#2"]); + const page = await getJson("/v1/workspaces/alpha/pulls?all=1&limit=1"); + expect(page.pulls.map((p) => p.ref)).toEqual(["acme/app#2"]); + const next = await getJson( + `/v1/workspaces/alpha/pulls?all=1&limit=1&cursor=${page.nextCursor}`, + ); + expect(next.pulls.map((p) => p.ref)).toEqual(["acme/app#1"]); + }); + + it("keeps a PR whose media were all deleted, with no thumbnails (index review focus 1)", async () => { + await putShot("alpha", "gh/acme/app/pull/9/gone.png", prMeta("acme/app", 9)); + await deleteFileMetadata(database(sqlite), "alpha", "gh/acme/app/pull/9/gone.png"); + const body = await getJson("/v1/workspaces/alpha/pulls"); + expect(body.pulls).toHaveLength(1); + expect(body.pulls[0]).toMatchObject({ ref: "acme/app#9", thumbnails: [] }); + }); + + it("rejects a bad state, limit, cursor, and repo, and isolates workspaces", async () => { + await putShot("alpha", "gh/acme/app/pull/1/one.png", prMeta("acme/app", 1)); + expect(await errorOf("/v1/workspaces/alpha/pulls?state=draft")).toEqual({ + status: 400, + code: "invalid_state", + }); + expect(await errorOf("/v1/workspaces/alpha/pulls?limit=0")).toEqual({ + status: 400, + code: "invalid_limit", + }); + expect(await errorOf("/v1/workspaces/alpha/pulls?cursor=not-a-cursor")).toEqual({ + status: 400, + code: "invalid_cursor", + }); + expect(await errorOf("/v1/workspaces/alpha/pulls?repo=nope")).toEqual({ + status: 400, + code: "feed_invalid_field", + }); + expect((await getJson("/v1/workspaces/beta/pulls")).pulls).toEqual([]); + }); +}); + +describe("GET /v1/workspaces/:ws/repos", () => { + it("lists distinct repos newest first with open PR counts, thumbnails, and a cursor", async () => { + await putShot("alpha", "gh/acme/app/pull/1/one.png", prMeta("acme/app", 1)); + await putShot("alpha", "gh/acme/app/pull/2/two.png", prMeta("acme/app", 2)); + await putShot("alpha", "gh/acme/site/pull/5/five.png", prMeta("acme/site", 5)); + await seedObject("gh/acme/docs/issues/3/three.png", { + "gh.repo": "acme/docs", + "gh.number": "3", + "gh.kind": "issue", + }); + stamp("gh/acme/app/pull/1/one.png", "2026-10-01T01:00:00.000Z"); + stamp("gh/acme/app/pull/2/two.png", "2026-10-01T02:00:00.000Z"); + stamp("gh/acme/site/pull/5/five.png", "2026-10-01T03:00:00.000Z"); + stamp("gh/acme/docs/issues/3/three.png", "2026-10-01T04:00:00.000Z"); + setRollup("acme/app#1", { state: "open" }); + setRollup("acme/app#2", { state: "open" }); + setRollup("acme/site#5", { state: "closed" }); + + const first = await getJson("/v1/workspaces/alpha/repos?limit=2"); + expect(first.workspace).toBe("alpha"); + expect(first.repos.map((r) => r.repo)).toEqual(["acme/docs", "acme/site"]); + expect(first.repos[0]).toMatchObject({ + lastUpdatedAt: "2026-10-01T04:00:00.000Z", + openPullCount: 0, + }); + expect(first.repos[1]).toMatchObject({ openPullCount: 0 }); + expect(first.nextCursor).toEqual(expect.any(String)); + + const second = await getJson( + `/v1/workspaces/alpha/repos?limit=2&cursor=${first.nextCursor}`, + ); + expect(second.repos).toHaveLength(1); + expect(second.repos[0]).toMatchObject({ + repo: "acme/app", + openPullCount: 2, + lastUpdatedAt: "2026-10-01T02:00:00.000Z", + }); + expect(second.repos[0]?.thumbnails.map((t) => t.key)).toEqual([ + "gh/acme/app/pull/2/two.png", + "gh/acme/app/pull/1/one.png", + ]); + expect(second.nextCursor).toBeNull(); + }); + + it("caps limit at 50 and isolates workspaces", async () => { + await seedObject("gh/acme/app/pull/1/one.png", prMeta("acme/app", 1)); + expect(await errorOf("/v1/workspaces/alpha/repos?limit=51")).toEqual({ + status: 400, + code: "invalid_limit", + }); + expect((await getJson("/v1/workspaces/beta/repos")).repos).toEqual([]); + }); +}); From ed70dd5892c4bd1aabf0067b1a1bda26a40641f8 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 17:05:38 -0400 Subject: [PATCH 07/14] feat(api): add the scope files endpoint with private count, live link, and PR header --- apps/api/src/feed-service.ts | 43 ++++- apps/api/src/github-pr-activity.ts | 23 +++ apps/api/src/routes/workspace-scope.ts | 100 +++++++++- apps/api/test/routes-workspace-scope.test.ts | 183 ++++++++++++++++++- 4 files changed, 343 insertions(+), 6 deletions(-) diff --git a/apps/api/src/feed-service.ts b/apps/api/src/feed-service.ts index 22d57e25..00e0dfc3 100644 --- a/apps/api/src/feed-service.ts +++ b/apps/api/src/feed-service.ts @@ -193,11 +193,16 @@ async function mapBounded( return result; } -async function hydrateFeedItems( +/** + * HEAD each match (lane-aware) and build its DTO. `privateKeys`, when given, + * collects every key whose object is private, so a caller can reuse these + * HEADs (see `countPrivateScopeItems`). + */ +export async function hydrateFeedItems( env: Env, workspace: WorkspaceRecord, matches: Array<{ key: string; metadata: Record }>, - opts: { audience: "owner" | "public" }, + opts: { audience: "owner" | "public"; privateKeys?: Set }, ): Promise { const resolver: LaneResolver = createLaneResolver(env, workspace); try { @@ -228,6 +233,7 @@ async function hydrateFeedItems( }); } const isPrivate = meta ? objectVisibility(meta.metadata) === "private" : false; + if (isPrivate) opts.privateKeys?.add(match.key); const withheld = opts.audience === "public" && isPrivate; const urls = meta && !withheld && itemConfig @@ -291,6 +297,39 @@ async function hydrateFeedItems( return hydrated; } +/** + * How many of `keys` are private (what a live link renders as `withheld`). + * Visibility lives only in R2 custom metadata (`visibility.ts`; D1 rejects + * the key), so each key the page did not already HEAD (`seen.checked`) costs + * one lane lookup + HEAD. Keys already HEADed count from `seen.privateKeys`. + */ +export async function countPrivateScopeItems( + env: Env, + workspace: WorkspaceRecord, + keys: string[], + seen: { checked: Set; privateKeys: Set }, +): Promise { + const known = keys.filter((key) => seen.privateKeys.has(key)).length; + const unchecked = keys.filter((key) => !seen.checked.has(key)); + if (unchecked.length === 0) return known; + const resolver = createLaneResolver(env, workspace); + let flags: boolean[]; + try { + flags = await mapBounded(unchecked, 8, async (key) => { + const lane = await resolver.resolve(key); + if (!lane) return false; + const head = (await lane.store.head(key)) as FeedObjectHead | null; + return objectVisibility(head?.metadata) === "private"; + }); + } catch (cause) { + throw new ServiceUnavailableError("Feed storage unavailable.", { + code: "feed_storage_unavailable", + cause, + }); + } + return known + flags.filter(Boolean).length; +} + function toPublicItem(item: FeedItemDto): PublicFeedItemDto { return { id: item.id, diff --git a/apps/api/src/github-pr-activity.ts b/apps/api/src/github-pr-activity.ts index 601fcf6e..ecfd95d0 100644 --- a/apps/api/src/github-pr-activity.ts +++ b/apps/api/src/github-pr-activity.ts @@ -336,3 +336,26 @@ export async function backfillPrActivityState( ); } } + +/** + * One PR's rollup row, only when it belongs to `workspaceName` (rows are + * keyed by ref, and another workspace may have written one for the same + * PR). Feeds the PR page header (`ScopeFilesResponse.pull`). Strict: a D1 + * failure surfaces as a 5xx. + */ +export async function getPrActivityRow( + db: D1Queryable, + workspaceName: string, + repo: string, + number: number, +): Promise { + const row = await db + .prepare( + `SELECT ${PAGE_ROW_COLUMNS} + FROM github_pr_activity + WHERE ref = ? AND workspace_name = ?`, + ) + .bind(`${repo.toLowerCase()}#${number}`, workspaceName) + .first(); + return row ? toPageRow(row) : null; +} diff --git a/apps/api/src/routes/workspace-scope.ts b/apps/api/src/routes/workspace-scope.ts index 5f860b33..8e493e95 100644 --- a/apps/api/src/routes/workspace-scope.ts +++ b/apps/api/src/routes/workspace-scope.ts @@ -11,11 +11,18 @@ import { boundedDataRead } from "../data-read-bounds"; import { dbFor } from "../db-session"; import { dualWorkspaceAuth, type DualAuthVars } from "../dual-workspace-auth"; import { respondError } from "../error-response"; -import { unwrapFeedMutation } from "../feed-service"; -import { normalizeFeedRepo } from "../feeds"; +import { + countPrivateScopeItems, + feedItemUrl, + feedUrl, + hydrateFeedItems, + unwrapFeedMutation, +} from "../feed-service"; +import { findFeedByScope, normalizeFeedNumber, normalizeFeedRepo } from "../feeds"; import { backfillPrActivityState, countOpenPullsByRepo, + getPrActivityRow, isPrState, listPrActivityPage, type PrState, @@ -28,10 +35,13 @@ import { encodeScopeCursor, listWorkspaceRepos, prScopeQuery, + scanScopeKeys, + SCOPE_DEFAULT_LIMIT, + SCOPE_MAX_LIMIT, } from "../pr-scope"; import { heavyReadRateLimit } from "../read-limits"; import { toThumbItem } from "../scope-service"; -import type { PullsResponse, ReposResponse } from "../scope-wire"; +import type { PullsResponse, ReposResponse, ScopeFilesResponse } from "../scope-wire"; import { storageConfig } from "../storage"; import { requireScope } from "../workspace"; @@ -213,7 +223,91 @@ export async function reposHandler(c: Context) { return c.json(response); } +/** + * One scope's files (a PR when `number` is set, else the whole repo), newest + * first, hydrated like a live link for the owner audience. Owner and repo + * are lowercased before matching. `privateCount` runs on the first page only + * and reuses the page's HEADs; `liveLink` is the existing feed for exactly + * this scope; `pull` is this workspace's rollup row for the PR. + */ +export async function scopeFilesHandler(c: Context) { + const workspace = c.get("workspaceName"); + const record = c.get("workspace"); + const db = dbFor(c.env); + const repo = parseRepo(`${c.req.param("owner") ?? ""}/${c.req.param("repo") ?? ""}`); + // Positive integer or absent; anything else is a 400 (`scopeFrom` would + // silently drop a bad value and widen the scope to the whole repo). + const number = unwrapFeedMutation(normalizeFeedNumber(c.req.query("number"))).value; + const type = parseFileTypeQuery(c.req.query("type")); + const cursor = decodeScopeCursor(c.req.query("cursor")); + const limit = parseLimit(c.req.query("limit"), SCOPE_DEFAULT_LIMIT, SCOPE_MAX_LIMIT); + const scope = { workspace, repo, ...(number > 0 ? { number } : {}) }; + + const [page, pullRow] = await boundedDataRead( + c, + () => + Promise.all([ + prScopeQuery(db, { ...scope, type, cursor, limit }), + number > 0 ? getPrActivityRow(db, workspace, repo, number) : Promise.resolve(null), + ]), + { name: "d1_scope_files" }, + ); + const privateKeys = new Set(); + const items = await hydrateFeedItems(c.env, record, page.items, { + audience: "owner", + privateKeys, + }); + const feed = await findFeedByScope(db, workspace, repo, "", number); + if (feed) { + for (const item of items) item.pageUrl = feedItemUrl(c.env, feed.id, item.id); + } + + // First page only: the share confirm reads it before the first copy. + let privateCount: number | null = null; + if (!cursor) { + const pageIsWholeScope = page.nextCursor === null && type === undefined; + const keys = pageIsWholeScope + ? page.items.map((item) => item.key) + : (await boundedDataRead(c, () => scanScopeKeys(db, scope), { name: "d1_scope_scan" })).map( + (item) => item.key, + ); + privateCount = await countPrivateScopeItems(c.env, record, keys, { + checked: new Set(page.items.map((item) => item.key)), + privateKeys, + }); + } + + // Same rule as /pulls: a stored title is served only for a repo linked to + // this workspace. Otherwise `pull.title` is null and the PR page resolves + // it through the member titles route (public ladder for unlinked repos). + const titleVisible = pullRow?.title != null && (await linkedRepoSet(db, workspace)).has(repo); + + const response: ScopeFilesResponse = { + repo, + number: number > 0 ? number : null, + items, + nextCursor: page.nextCursor ? encodeScopeCursor(page.nextCursor) : null, + privateCount, + liveLink: feed ? { id: feed.id, url: feedUrl(c.env, feed.id), source: feed.source } : null, + pull: pullRow + ? { + branch: pullRow.branch, + title: titleVisible ? pullRow.title : null, + state: pullRow.state, + } + : null, + }; + return c.json(response); +} + export const workspaceScope = new Hono() .get("/:workspace/pulls", dualWorkspaceAuth(), heavyRead, scoped("files:read"), pullsHandler) .get("/:workspace/repos", dualWorkspaceAuth(), heavyRead, scoped("files:read"), reposHandler) + .get( + "/:workspace/scope/:owner/:repo/files", + dualWorkspaceAuth(), + heavyRead, + scoped("files:read"), + scopeFilesHandler, + ) .onError((err, c) => respondError(c, err)); diff --git a/apps/api/test/routes-workspace-scope.test.ts b/apps/api/test/routes-workspace-scope.test.ts index f35043c6..003bd0a8 100644 --- a/apps/api/test/routes-workspace-scope.test.ts +++ b/apps/api/test/routes-workspace-scope.test.ts @@ -3,7 +3,7 @@ import { afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; import { app } from "../src/index"; import { deleteFileMetadata, replaceFileMetadata } from "../src/file-metadata"; -import type { PullsResponse, ReposResponse } from "../src/scope-wire"; +import type { PullsResponse, ReposResponse, ScopeFilesResponse } from "../src/scope-wire"; import { sha256Hex, type WorkspaceRecord } from "../src/workspace"; import { FakeKv } from "./fake-kv"; import { FakeR2Bucket } from "./fake-r2"; @@ -400,3 +400,184 @@ describe("GET /v1/workspaces/:ws/repos", () => { expect((await getJson("/v1/workspaces/beta/repos")).repos).toEqual([]); }); }); + +describe("GET /v1/workspaces/:ws/scope/:owner/:repo/files", () => { + it("lists a PR's files newest first, links items to the existing live link, and pages", async () => { + await seedObject("gh/acme/app/pull/4/a.png", prMeta("acme/app", 4)); + await seedObject("gh/acme/app/pull/4/b.png", prMeta("acme/app", 4)); + await seedObject("gh/acme/app/pull/4/c.mp4", prMeta("acme/app", 4), { + contentType: "video/mp4", + }); + await seedObject("gh/acme/app/pull/5/other.png", prMeta("acme/app", 5)); + stamp("gh/acme/app/pull/4/a.png", "2026-10-01T01:00:00.000Z"); + stamp("gh/acme/app/pull/4/b.png", "2026-10-01T02:00:00.000Z"); + stamp("gh/acme/app/pull/4/c.mp4", "2026-10-01T03:00:00.000Z"); + stamp("gh/acme/app/pull/5/other.png", "2026-10-01T04:00:00.000Z"); + const created = await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: "acme/app", pr: 4 }), + }); + const feed = (await created.json()) as { id: string; url: string }; + + const page = await getJson( + "/v1/workspaces/alpha/scope/acme/app/files?number=4&limit=2", + ); + expect(page).toMatchObject({ + repo: "acme/app", + number: 4, + privateCount: 0, + liveLink: { id: feed.id, url: feed.url, source: "user" }, + }); + expect(page.items.map((i) => i.objectKey)).toEqual([ + "gh/acme/app/pull/4/c.mp4", + "gh/acme/app/pull/4/b.png", + ]); + expect(page.items[0]?.pageUrl).toBe( + `${feed.url}/${(await sha256Hex("gh/acme/app/pull/4/c.mp4")).slice(0, 32)}`, + ); + + const next = await getJson( + `/v1/workspaces/alpha/scope/acme/app/files?number=4&limit=2&cursor=${page.nextCursor}`, + ); + expect(next.items.map((i) => i.objectKey)).toEqual(["gh/acme/app/pull/4/a.png"]); + expect(next.nextCursor).toBeNull(); + expect(next.privateCount).toBeNull(); + + const repoWide = await getJson("/v1/workspaces/alpha/scope/acme/app/files"); + expect(repoWide.number).toBeNull(); + expect(repoWide.items).toHaveLength(4); + expect(repoWide.liveLink).toBeNull(); + + // Scope parity: the live link and the Files view hold the same objects. + const repoFeed = (await ( + await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: "acme/app" }), + }) + ).json()) as { items: Array<{ objectKey: string }> }; + expect(repoFeed.items.map((i) => i.objectKey)).toEqual(repoWide.items.map((i) => i.objectKey)); + }); + + it("counts private items across the whole scope, ignoring type and page size (review focus 4)", async () => { + const meta = prMeta("acme/app", 6); + await seedObject("gh/acme/app/pull/6/p1.png", meta); + await seedObject("gh/acme/app/pull/6/p2.png", meta); + await seedObject("gh/acme/app/pull/6/s1.png", meta, { private: true }); + await seedObject("gh/acme/app/pull/6/s2.mp4", meta, { + private: true, + contentType: "video/mp4", + }); + await seedObject("gh/acme/app/pull/6/p3.png", meta); + stamp("gh/acme/app/pull/6/p1.png", "2026-10-01T01:00:00.000Z"); + stamp("gh/acme/app/pull/6/p2.png", "2026-10-01T02:00:00.000Z"); + stamp("gh/acme/app/pull/6/s1.png", "2026-10-01T03:00:00.000Z"); + stamp("gh/acme/app/pull/6/s2.mp4", "2026-10-01T04:00:00.000Z"); + stamp("gh/acme/app/pull/6/p3.png", "2026-10-01T05:00:00.000Z"); + + const videos = await getJson( + "/v1/workspaces/alpha/scope/acme/app/files?number=6&type=video&limit=1", + ); + expect(videos.items.map((i) => i.objectKey)).toEqual(["gh/acme/app/pull/6/s2.mp4"]); + expect(videos.items[0]?.status).toBe("available"); + expect(videos.privateCount).toBe(2); + + const firstPage = await getJson( + "/v1/workspaces/alpha/scope/acme/app/files?number=6&limit=2", + ); + expect(firstPage.items.map((i) => i.objectKey)).toEqual([ + "gh/acme/app/pull/6/p3.png", + "gh/acme/app/pull/6/s2.mp4", + ]); + expect(firstPage.privateCount).toBe(2); + + const all = await getJson( + "/v1/workspaces/alpha/scope/acme/app/files?number=6", + ); + expect(all.items).toHaveLength(5); + expect(all.privateCount).toBe(2); + }); + + it("lowercases an owner/repo with dots, dashes, underscores, and capitals (index review focus 2)", async () => { + await seedObject("gh/foo.bar/my-repo_x/pull/2/shot.png", prMeta("foo.bar/my-repo_x", 2)); + const created = await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: "Foo.Bar/My-Repo_x", pr: 2 }), + }); + expect(created.status).toBe(201); + const feedId = ((await created.json()) as { id: string }).id; + + const body = await getJson( + "/v1/workspaces/alpha/scope/Foo.Bar/My-Repo_x/files?number=2", + ); + expect(body.repo).toBe("foo.bar/my-repo_x"); + expect(body.items.map((i) => i.objectKey)).toEqual(["gh/foo.bar/my-repo_x/pull/2/shot.png"]); + expect(body.liveLink?.id).toBe(feedId); + }); + + it("carries the PR's branch, title, and state from this workspace's rollup row", async () => { + await putShot("alpha", "gh/acme/app/pull/3/a.png", { + ...prMeta("acme/app", 3), + "gh.branch": "feat/dark", + }); + sqlite.db + .prepare(`UPDATE github_pr_activity SET title = ?, state = ? WHERE ref = ?`) + .run("Add dark mode", "open", "acme/app#3"); + + // Unlinked repo: the stored title is withheld (member title rule), branch and state are not. + const unlinked = await getJson( + "/v1/workspaces/alpha/scope/acme/app/files?number=3", + ); + expect(unlinked.pull).toEqual({ branch: "feat/dark", title: null, state: "open" }); + + linkRepo("acme/app"); + const pr = await getJson( + "/v1/workspaces/alpha/scope/acme/app/files?number=3", + ); + expect(pr.pull).toEqual({ branch: "feat/dark", title: "Add dark mode", state: "open" }); + + // Repo scope, a PR without a rollup row, and another workspace all get null. + expect( + (await getJson("/v1/workspaces/alpha/scope/acme/app/files")).pull, + ).toBeNull(); + expect( + (await getJson("/v1/workspaces/alpha/scope/acme/app/files?number=99")) + .pull, + ).toBeNull(); + expect( + (await getJson("/v1/workspaces/beta/scope/acme/app/files?number=3")).pull, + ).toBeNull(); + }); + + it("returns an empty scope for a PR whose media were deleted (index review focus 1)", async () => { + await seedObject("gh/acme/app/pull/9/gone.png", prMeta("acme/app", 9)); + await deleteFileMetadata(database(sqlite), "alpha", "gh/acme/app/pull/9/gone.png"); + const body = await getJson( + "/v1/workspaces/alpha/scope/acme/app/files?number=9", + ); + expect(body).toMatchObject({ items: [], nextCursor: null, privateCount: 0, liveLink: null }); + }); + + it("rejects a bad type, number, owner/repo, and cursor, and isolates workspaces", async () => { + await seedObject("gh/acme/app/pull/4/a.png", prMeta("acme/app", 4)); + expect(await errorOf("/v1/workspaces/alpha/scope/acme/app/files?type=gif")).toEqual({ + status: 400, + code: "invalid_type", + }); + expect(await errorOf("/v1/workspaces/alpha/scope/acme/app/files?number=abc")).toEqual({ + status: 400, + code: "feed_invalid_field", + }); + expect(await errorOf("/v1/workspaces/alpha/scope/a%20b/app/files")).toEqual({ + status: 400, + code: "feed_invalid_field", + }); + expect(await errorOf("/v1/workspaces/alpha/scope/acme/app/files?cursor=not-a-cursor")).toEqual({ + status: 400, + code: "invalid_cursor", + }); + const beta = await getJson( + "/v1/workspaces/beta/scope/acme/app/files?number=4", + ); + expect(beta).toMatchObject({ items: [], liveLink: null, privateCount: 0 }); + }); +}); From 9850301ff197216925a32f8178927be55a045897 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 17:08:59 -0400 Subject: [PATCH 08/14] feat(api): paginate public feeds and add the pager item endpoint --- apps/api/src/feed-service.ts | 88 ++++++++++++- apps/api/src/routes/public-feeds.ts | 38 ++++-- apps/api/test/routes-feeds.test.ts | 149 ++++++++++++++++++++++- apps/web/public/.well-known/openapi.json | 3 +- 4 files changed, 260 insertions(+), 18 deletions(-) diff --git a/apps/api/src/feed-service.ts b/apps/api/src/feed-service.ts index 00e0dfc3..1fd6dddd 100644 --- a/apps/api/src/feed-service.ts +++ b/apps/api/src/feed-service.ts @@ -11,7 +11,13 @@ import { } from "@uploads/errors"; import { publicObjectDateFields } from "./files-core"; import { getMetadataForKeys } from "./file-metadata"; -import { prScopeQuery, type ScopeItem } from "./pr-scope"; +import { + encodeScopeCursor, + prScopeQuery, + scanScopeKeys, + type ScopeCursor, + type ScopeItem, +} from "./pr-scope"; import { FEED_ID_RE, FEED_ITEM_LIMIT, @@ -22,7 +28,12 @@ import { type FeedSource, } from "./feeds"; import { isDerivedPosterContentType, videoPresentation } from "./poster"; -import type { FeedItemDto, FeedSummaryDto, PublicFeedItemDto } from "./scope-wire"; +import type { + FeedItemDto, + FeedSummaryDto, + PublicFeedItemDto, + PublicFeedItemPage, +} from "./scope-wire"; import { createLaneResolver, objectPublicUrls, type LaneResolver } from "./storage"; import type { StorageConfig } from "@uploads/storage"; import { objectVisibility } from "./visibility"; @@ -65,6 +76,8 @@ export type PublicFeedDto = { createdAt: string; updatedAt: string; items: PublicFeedItemDto[]; + /** Opaque cursor for the next 50 items, or null on the last page. */ + nextCursor: string | null; }; export function feedUrl(env: Env, id: string): string { @@ -372,13 +385,17 @@ export async function hydratePublicFeed( env: Env, workspace: WorkspaceRecord, record: FeedRecord, + opts: { cursor?: ScopeCursor | null } = {}, ): Promise { - const matches = await findLatestRepoScreenshots(dbFor(env), record.workspace, { + const page = await prScopeQuery(dbFor(env), { + workspace: record.workspace, repo: record.repo, - path: record.path || undefined, - number: record.number > 0 ? record.number : undefined, + ...(record.path ? { path: record.path } : {}), + ...(record.number > 0 ? { number: record.number } : {}), + cursor: opts.cursor ?? null, + limit: FEED_ITEM_LIMIT, }); - const items = await hydrateFeedItems(env, workspace, matches, { audience: "public" }); + const items = await hydrateFeedItems(env, workspace, page.items, { audience: "public" }); const summary = feedSummary(env, record); return { id: record.id, @@ -390,5 +407,64 @@ export async function hydratePublicFeed( createdAt: record.created_at, updatedAt: record.updated_at, items: items.map(toPublicItem), + nextCursor: page.nextCursor ? encodeScopeCursor(page.nextCursor) : null, + }; +} + +const FEED_ITEM_ID_RE = /^[0-9a-f]{32}$/; + +/** + * One public item plus its neighbours, for the `/c//` pager. Scans + * the scope's keys (cap 2,000) and hashes them until one matches: item ids + * are `sha256(key)`, so there is no id-to-key table to keep in step with + * every upload. Null when the id is malformed or not in scope. + */ +export async function publicFeedItemPage( + env: Env, + workspace: WorkspaceRecord, + record: FeedRecord, + itemId: string, +): Promise { + if (!FEED_ITEM_ID_RE.test(itemId)) return null; + const db = dbFor(env); + const scope: ScopeItem[] = await scanScopeKeys(db, { + workspace: record.workspace, + repo: record.repo, + ...(record.path ? { path: record.path } : {}), + ...(record.number > 0 ? { number: record.number } : {}), + }); + const ids: string[] = []; + let index = -1; + for (const entry of scope) { + const id = await feedItemId(entry.key); + ids.push(id); + if (id === itemId) { + index = ids.length - 1; + break; + } + } + if (index < 0) return null; + + const match = scope[index]; + const metadata = + (await getMetadataForKeys(db, record.workspace, [match.key])).get(match.key) ?? {}; + const [item] = await hydrateFeedItems(env, workspace, [{ key: match.key, metadata }], { + audience: "public", + }); + const summary = feedSummary(env, record); + return { + feed: { + id: record.id, + title: summary.title, + repo: record.repo, + number: summary.number, + // The summary's kind (null for a repo scope), same as GET /public/feeds/:id. + kind: summary.kind, + }, + item: toPublicItem(item), + prev: index > 0 ? ids[index - 1] : null, + next: index + 1 < scope.length ? await feedItemId(scope[index + 1].key) : null, + index, + total: scope.length, }; } diff --git a/apps/api/src/routes/public-feeds.ts b/apps/api/src/routes/public-feeds.ts index 6e384aa9..8b4be881 100644 --- a/apps/api/src/routes/public-feeds.ts +++ b/apps/api/src/routes/public-feeds.ts @@ -1,16 +1,34 @@ import { NotFoundError } from "@uploads/errors"; import { Hono } from "hono"; import { resolvePublicFeed } from "../feeds"; -import { hydratePublicFeed } from "../feed-service"; +import { hydratePublicFeed, publicFeedItemPage } from "../feed-service"; +import { decodeScopeCursor } from "../pr-scope"; import { loadWorkspaceRecord, type WorkspaceVars } from "../workspace"; import { dbFor } from "../db-session"; -export const publicFeeds = new Hono().get("/:id", async (c) => { - const record = await resolvePublicFeed(dbFor(c.env), c.req.param("id")); - if (!record) throw new NotFoundError("Feed not found.", { code: "feed_not_found" }); - const workspace = await loadWorkspaceRecord(c.env, record.workspace); - if (!workspace) { - throw new NotFoundError("Feed not found.", { code: "feed_not_found" }); - } - return c.json(await hydratePublicFeed(c.env, workspace, record)); -}); +function feedNotFound(): NotFoundError { + return new NotFoundError("Feed not found.", { code: "feed_not_found" }); +} + +async function liveFeed(env: Env, id: string) { + const record = await resolvePublicFeed(dbFor(env), id); + if (!record) throw feedNotFound(); + const workspace = await loadWorkspaceRecord(env, record.workspace); + if (!workspace) throw feedNotFound(); + return { record, workspace }; +} + +export const publicFeeds = new Hono() + .get("/:id", async (c) => { + const { record, workspace } = await liveFeed(c.env, c.req.param("id")); + const cursor = decodeScopeCursor(c.req.query("cursor")); + return c.json(await hydratePublicFeed(c.env, workspace, record, { cursor })); + }) + // Pager item plus neighbours, found by a scope scan (cap 2,000) so items + // older than the first page still resolve. + .get("/:id/items/:item", async (c) => { + const { record, workspace } = await liveFeed(c.env, c.req.param("id")); + const page = await publicFeedItemPage(c.env, workspace, record, c.req.param("item")); + if (!page) throw new NotFoundError("Feed item not found.", { code: "feed_item_not_found" }); + return c.json(page); + }); diff --git a/apps/api/test/routes-feeds.test.ts b/apps/api/test/routes-feeds.test.ts index 63184635..f4f33ea7 100644 --- a/apps/api/test/routes-feeds.test.ts +++ b/apps/api/test/routes-feeds.test.ts @@ -4,7 +4,9 @@ import { afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; import { app } from "../src/index"; import { sha256Hex, type WorkspaceRecord } from "../src/workspace"; import { FakeR2Bucket } from "./fake-r2"; -import { SqliteD1 } from "./helpers/sqlite-d1"; +import { SqliteD1, database } from "./helpers/sqlite-d1"; +import { replaceFileMetadata } from "../src/file-metadata"; +import type { PublicFeedItemPage } from "../src/scope-wire"; const TOKEN = "feed-token"; const PNG = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); @@ -380,3 +382,148 @@ describe("feed routes", () => { expect(next.status).toBe(201); }); }); + +/** Seed one object straight into R2 + D1, pinned to `updatedAt`. */ +async function seedFeedObject( + key: string, + meta: Record, + updatedAt: string, + opts: { private?: boolean } = {}, +) { + await bucket.put(`alpha/${key}`, PNG, { + httpMetadata: { contentType: "image/png" }, + ...(opts.private ? { customMetadata: { visibility: "private" } } : {}), + }); + await replaceFileMetadata(database(sqlite), "alpha", key, meta); + sqlite.db + .prepare(`UPDATE file_metadata SET updated_at = ? WHERE workspace = ? AND object_key = ?`) + .run(updatedAt, "alpha", key); +} + +const shotKey = (i: number) => `gh/acme/app/pull/7/shot-${String(i).padStart(2, "0")}.png`; +const minutesBefore = (i: number) => new Date(Date.UTC(2026, 9, 1) - i * 60_000).toISOString(); +const itemIdFor = async (key: string) => (await sha256Hex(key)).slice(0, 32); + +/** `count` PR #7 shots; index 0 is newest. */ +async function seedPr7(count: number, privateIndex?: number) { + for (let i = 0; i < count; i++) { + await seedFeedObject( + shotKey(i), + { "gh.repo": "acme/app", "gh.number": "7", "gh.kind": "pull" }, + minutesBefore(i), + { private: i === privateIndex }, + ); + } +} + +async function createPr7Feed(): Promise { + const created = await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: "acme/app", pr: 7 }), + }); + expect(created.status).toBe(201); + return ((await created.json()) as { id: string }).id; +} + +describe("public feed pagination and pager", () => { + it("pages a public feed past 50 items with nextCursor", async () => { + await seedPr7(55); + const id = await createPr7Feed(); + + const first = await app.request(`/public/feeds/${id}`, {}, env); + expect(first.status).toBe(200); + const page = (await first.json()) as { + items: Array<{ filename: string }>; + nextCursor: string | null; + }; + expect(page.items).toHaveLength(50); + expect(page.items[0]?.filename).toBe("shot-00.png"); + expect(page.nextCursor).toEqual(expect.any(String)); + + const second = (await ( + await app.request(`/public/feeds/${id}?cursor=${page.nextCursor}`, {}, env) + ).json()) as typeof page; + expect(second.items.map((item) => item.filename)).toEqual([ + "shot-50.png", + "shot-51.png", + "shot-52.png", + "shot-53.png", + "shot-54.png", + ]); + expect(second.nextCursor).toBeNull(); + + const bad = await app.request(`/public/feeds/${id}?cursor=not-a-cursor`, {}, env); + expect(bad.status).toBe(400); + expect(await bad.json()).toMatchObject({ error: { code: "invalid_cursor" } }); + }); + + it("resolves a pager item older than the newest 50 with its neighbours", async () => { + await seedPr7(55); + const id = await createPr7Feed(); + + const res = await app.request( + `/public/feeds/${id}/items/${await itemIdFor(shotKey(52))}`, + {}, + env, + ); + expect(res.status).toBe(200); + const body = (await res.json()) as PublicFeedItemPage; + expect(body).toMatchObject({ + feed: { id, title: "acme/app#7", repo: "acme/app", number: 7, kind: "pull" }, + item: { id: await itemIdFor(shotKey(52)), filename: "shot-52.png", status: "available" }, + prev: await itemIdFor(shotKey(51)), + next: await itemIdFor(shotKey(53)), + index: 52, + total: 55, + }); + expect(body.item).not.toHaveProperty("objectKey"); + expect(body.feed).not.toHaveProperty("source"); + + const newest = (await ( + await app.request(`/public/feeds/${id}/items/${await itemIdFor(shotKey(0))}`, {}, env) + ).json()) as PublicFeedItemPage; + expect(newest).toMatchObject({ index: 0, prev: null, next: await itemIdFor(shotKey(1)) }); + const oldest = (await ( + await app.request(`/public/feeds/${id}/items/${await itemIdFor(shotKey(54))}`, {}, env) + ).json()) as PublicFeedItemPage; + expect(oldest).toMatchObject({ index: 54, next: null }); + }); + + it("reports a repo-scope pager item's feed kind as null, like the public feed", async () => { + await seedPr7(2); + const created = await request("/v1/workspaces/alpha/feeds", { + method: "POST", + body: JSON.stringify({ repo: "acme/app" }), + }); + const id = ((await created.json()) as { id: string }).id; + const body = (await ( + await app.request(`/public/feeds/${id}/items/${await itemIdFor(shotKey(0))}`, {}, env) + ).json()) as PublicFeedItemPage; + expect(body.feed).toMatchObject({ id, number: null, kind: null }); + }); + + it("withholds a private pager item and 404s unknown, malformed, and revoked ids", async () => { + await seedPr7(3, 1); + const id = await createPr7Feed(); + + const hidden = (await ( + await app.request(`/public/feeds/${id}/items/${await itemIdFor(shotKey(1))}`, {}, env) + ).json()) as PublicFeedItemPage; + expect(hidden.item).toMatchObject({ status: "withheld", url: null, embedUrl: null }); + + const unknown = await app.request(`/public/feeds/${id}/items/${"0".repeat(32)}`, {}, env); + expect(unknown.status).toBe(404); + expect(await unknown.json()).toMatchObject({ error: { code: "feed_item_not_found" } }); + expect((await app.request(`/public/feeds/${id}/items/not-hex`, {}, env)).status).toBe(404); + + expect((await request(`/v1/workspaces/alpha/feeds/${id}`, { method: "DELETE" })).status).toBe( + 200, + ); + const revoked = await app.request( + `/public/feeds/${id}/items/${await itemIdFor(shotKey(0))}`, + {}, + env, + ); + expect(revoked.status).toBe(404); + }); +}); diff --git a/apps/web/public/.well-known/openapi.json b/apps/web/public/.well-known/openapi.json index 4badf7b1..e81cfa40 100644 --- a/apps/web/public/.well-known/openapi.json +++ b/apps/web/public/.well-known/openapi.json @@ -24,8 +24,9 @@ "get": { "operationId": "getPublicFeed", "summary": "Read a public change feed", - "description": "Returns the public newest-first screenshot list for a feed. It omits workspace ownership and object keys. Items are resolved at view time from `gh.repo` and optional `gh.number` metadata.", + "description": "Returns one page (up to 50) of the public newest-first item list for a feed, plus `nextCursor` for the next page. It omits workspace ownership and object keys. Items are resolved at view time from `gh.repo` and optional `gh.number` metadata.", "security": [], + "parameters": [{ "$ref": "#/components/parameters/Cursor" }], "responses": { "200": { "description": "The public feed and its current items." }, "404": { "$ref": "#/components/responses/NotFound" }, From 253b6af51f8870f49f0bb4269306b94832233d71 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 17:12:09 -0400 Subject: [PATCH 09/14] fix(api): keep object keys out of the public feed cursor --- apps/api/src/feed-service.ts | 8 +-- apps/api/src/pr-scope.ts | 84 +++++++++++++++++++++++++++- apps/api/src/routes/public-feeds.ts | 13 ++++- apps/api/test/routes-feeds.test.ts | 86 +++++++++++++++++++++++++++++ 4 files changed, 182 insertions(+), 9 deletions(-) diff --git a/apps/api/src/feed-service.ts b/apps/api/src/feed-service.ts index 1fd6dddd..e251b1d1 100644 --- a/apps/api/src/feed-service.ts +++ b/apps/api/src/feed-service.ts @@ -12,10 +12,10 @@ import { import { publicObjectDateFields } from "./files-core"; import { getMetadataForKeys } from "./file-metadata"; import { - encodeScopeCursor, + encodePublicFeedCursor, prScopeQuery, scanScopeKeys, - type ScopeCursor, + type ScopeResume, type ScopeItem, } from "./pr-scope"; import { @@ -385,7 +385,7 @@ export async function hydratePublicFeed( env: Env, workspace: WorkspaceRecord, record: FeedRecord, - opts: { cursor?: ScopeCursor | null } = {}, + opts: { cursor?: ScopeResume | null } = {}, ): Promise { const page = await prScopeQuery(dbFor(env), { workspace: record.workspace, @@ -407,7 +407,7 @@ export async function hydratePublicFeed( createdAt: record.created_at, updatedAt: record.updated_at, items: items.map(toPublicItem), - nextCursor: page.nextCursor ? encodeScopeCursor(page.nextCursor) : null, + nextCursor: page.nextCursor ? await encodePublicFeedCursor(page.nextCursor) : null, }; } diff --git a/apps/api/src/pr-scope.ts b/apps/api/src/pr-scope.ts index b1b74a1f..7d1c3fa3 100644 --- a/apps/api/src/pr-scope.ts +++ b/apps/api/src/pr-scope.ts @@ -15,7 +15,7 @@ * the SQL or SQLite cannot use that partial index. */ import { ValidationError } from "@uploads/errors"; -import type { FileTypeClass } from "@uploads/comment-render/scope"; +import { feedItemIdFor, type FileTypeClass } from "@uploads/comment-render/scope"; import { type D1Queryable } from "./db-session"; import { getMetadataForKeys } from "./file-metadata"; import { fileTypeSql } from "./file-type-sql"; @@ -30,6 +30,15 @@ export interface ScopeCursor { key: string; } +/** + * Where a page resumes. A null `key` means "strictly older than `updatedAt`" + * (no tie-break among rows sharing that timestamp). + */ +export interface ScopeResume { + updatedAt: string; + key: string | null; +} + export interface ScopeQuery { workspace: string; /** Lowercased `owner/repo`. */ @@ -38,7 +47,7 @@ export interface ScopeQuery { path?: string; /** Signed-in views only; live links never pass it. */ type?: FileTypeClass; - cursor?: ScopeCursor | null; + cursor?: ScopeResume | null; /** Default 50, max 100. */ limit?: number; } @@ -96,7 +105,10 @@ export async function prScopeQuery( const limit = clampScopeLimit(q.limit); const { sql, params } = scopeFrom(q); let select = `SELECT r.object_key AS object_key, r.updated_at AS updated_at ${sql}`; - if (q.cursor) { + if (q.cursor && q.cursor.key === null) { + select += ` AND r.updated_at < ?`; + params.push(q.cursor.updatedAt); + } else if (q.cursor) { select += ` AND (r.updated_at < ? OR (r.updated_at = ? AND r.object_key > ?))`; params.push(q.cursor.updatedAt, q.cursor.updatedAt, q.cursor.key); } @@ -154,6 +166,22 @@ export async function scanScopeKeys( })); } +/** Keys in the scope stamped exactly `updatedAt`, in tie-break order. Small: rows sharing one timestamp. */ +export async function scopeKeysAt( + db: D1Queryable, + q: Omit, + updatedAt: string, +): Promise { + const { sql, params } = scopeFrom(q); + const { results } = await db + .prepare( + `SELECT r.object_key AS object_key ${sql} AND r.updated_at = ? ORDER BY r.object_key ASC LIMIT ?`, + ) + .bind(...params, updatedAt, SCOPE_SCAN_CAP) + .all<{ object_key: string }>(); + return (results ?? []).map((row) => row.object_key); +} + /** Distinct `gh.repo` values with their newest upload time, newest first. */ export async function listWorkspaceRepos( db: D1Queryable, @@ -231,3 +259,53 @@ export function decodeScopeCursor(raw: string | undefined): ScopeCursor | null { } return { updatedAt: record.u, key: record.k }; } + +/** + * Public live-link cursor. Public pages must not expose object keys (a + * withheld item's key can be a capability URL), so it carries the item id + * (`sha256(key)` prefix) instead of the key: `{ v: 1, u: updatedAt, h: itemId }`. + * Signed-in routes keep `encodeScopeCursor`. + */ +export async function encodePublicFeedCursor(cursor: ScopeCursor): Promise { + return base64UrlEncode( + JSON.stringify({ v: 1, u: cursor.updatedAt, h: await feedItemIdFor(cursor.key) }), + ); +} + +/** + * Decode a public cursor and resolve its tie-break key from the scope rows + * stamped `updatedAt`. If no row matches (the item was deleted or re-scoped + * since the cursor was issued), resume strictly older than `updatedAt` + * (`key: null`): same-timestamp rows past the vanished item are skipped + * rather than risking a duplicate. Malformed input is a 400 `invalid_cursor`. + */ +export async function decodePublicFeedCursor( + db: D1Queryable, + scope: Omit, + raw: string | undefined, +): Promise { + if (raw === undefined || raw === "") return null; + const invalid = () => + new ValidationError("cursor is not valid for this query", { code: "invalid_cursor" }); + let parsed: unknown; + try { + parsed = JSON.parse(base64UrlDecode(raw)); + } catch { + throw invalid(); + } + if (typeof parsed !== "object" || parsed === null) throw invalid(); + const record = parsed as Record; + if ( + record.v !== 1 || + typeof record.u !== "string" || + !Number.isFinite(Date.parse(record.u)) || + typeof record.h !== "string" || + !/^[0-9a-f]{32}$/.test(record.h) + ) { + throw invalid(); + } + for (const key of await scopeKeysAt(db, scope, record.u)) { + if ((await feedItemIdFor(key)) === record.h) return { updatedAt: record.u, key }; + } + return { updatedAt: record.u, key: null }; +} diff --git a/apps/api/src/routes/public-feeds.ts b/apps/api/src/routes/public-feeds.ts index 8b4be881..0e64a626 100644 --- a/apps/api/src/routes/public-feeds.ts +++ b/apps/api/src/routes/public-feeds.ts @@ -2,7 +2,7 @@ import { NotFoundError } from "@uploads/errors"; import { Hono } from "hono"; import { resolvePublicFeed } from "../feeds"; import { hydratePublicFeed, publicFeedItemPage } from "../feed-service"; -import { decodeScopeCursor } from "../pr-scope"; +import { decodePublicFeedCursor } from "../pr-scope"; import { loadWorkspaceRecord, type WorkspaceVars } from "../workspace"; import { dbFor } from "../db-session"; @@ -21,7 +21,16 @@ async function liveFeed(env: Env, id: string) { export const publicFeeds = new Hono() .get("/:id", async (c) => { const { record, workspace } = await liveFeed(c.env, c.req.param("id")); - const cursor = decodeScopeCursor(c.req.query("cursor")); + const cursor = await decodePublicFeedCursor( + dbFor(c.env), + { + workspace: record.workspace, + repo: record.repo, + ...(record.path ? { path: record.path } : {}), + ...(record.number > 0 ? { number: record.number } : {}), + }, + c.req.query("cursor"), + ); return c.json(await hydratePublicFeed(c.env, workspace, record, { cursor })); }) // Pager item plus neighbours, found by a scope scan (cap 2,000) so items diff --git a/apps/api/test/routes-feeds.test.ts b/apps/api/test/routes-feeds.test.ts index f4f33ea7..e96862bc 100644 --- a/apps/api/test/routes-feeds.test.ts +++ b/apps/api/test/routes-feeds.test.ts @@ -457,6 +457,92 @@ describe("public feed pagination and pager", () => { expect(await bad.json()).toMatchObject({ error: { code: "invalid_cursor" } }); }); + it("keeps object keys out of the public nextCursor when the last item is private", async () => { + const privateKey = `gh/private/${"a1".repeat(16)}/acme/app/pull/7/secret.png`; + for (let i = 0; i < 52; i++) { + const key = i === 49 ? privateKey : shotKey(i); + await seedFeedObject( + key, + { "gh.repo": "acme/app", "gh.number": "7", "gh.kind": "pull" }, + minutesBefore(i), + { private: i === 49 }, + ); + } + const id = await createPr7Feed(); + const page = (await (await app.request(`/public/feeds/${id}`, {}, env)).json()) as { + items: Array<{ filename: string; status: string }>; + nextCursor: string; + }; + expect(page.items.at(-1)).toMatchObject({ filename: "secret.png", status: "withheld" }); + const decoded = Buffer.from(page.nextCursor, "base64url").toString("utf8"); + expect(decoded).not.toContain("gh/private/"); + expect(decoded).not.toContain("secret.png"); + expect(decoded).not.toContain("acme/app"); + expect(decoded).toContain(await itemIdFor(privateKey)); + + const next = (await ( + await app.request(`/public/feeds/${id}?cursor=${page.nextCursor}`, {}, env) + ).json()) as { items: Array<{ filename: string }>; nextCursor: string | null }; + expect(next.items.map((item) => item.filename)).toEqual(["shot-50.png", "shot-51.png"]); + }); + + it("pages every item exactly once across a boundary with shared updated_at values", async () => { + // Items 49, 50, and 51 share one timestamp, so the 50-item boundary splits a tie. + for (let i = 0; i < 55; i++) { + await seedFeedObject( + shotKey(i), + { "gh.repo": "acme/app", "gh.number": "7", "gh.kind": "pull" }, + minutesBefore(i >= 49 && i <= 51 ? 49 : i), + ); + } + const id = await createPr7Feed(); + const seen: string[] = []; + let cursor: string | null = null; + let pages = 0; + do { + const res = await app.request( + `/public/feeds/${id}${cursor ? `?cursor=${cursor}` : ""}`, + {}, + env, + ); + expect(res.status).toBe(200); + const body = (await res.json()) as { + items: Array<{ filename: string }>; + nextCursor: string | null; + }; + seen.push(...body.items.map((item) => item.filename)); + cursor = body.nextCursor; + pages++; + } while (cursor && pages < 5); + expect(pages).toBe(2); + expect(seen).toHaveLength(55); + expect(new Set(seen).size).toBe(55); + expect(new Set(seen)).toEqual( + new Set(Array.from({ length: 55 }, (_, i) => `shot-${String(i).padStart(2, "0")}.png`)), + ); + }); + + it("resumes strictly older when the cursor's item vanished", async () => { + await seedPr7(55); + const id = await createPr7Feed(); + const page = (await (await app.request(`/public/feeds/${id}`, {}, env)).json()) as { + nextCursor: string; + }; + sqlite.db + .prepare(`DELETE FROM file_metadata WHERE workspace = ? AND object_key = ?`) + .run("alpha", shotKey(49)); + const next = (await ( + await app.request(`/public/feeds/${id}?cursor=${page.nextCursor}`, {}, env) + ).json()) as { items: Array<{ filename: string }> }; + expect(next.items.map((item) => item.filename)).toEqual([ + "shot-50.png", + "shot-51.png", + "shot-52.png", + "shot-53.png", + "shot-54.png", + ]); + }); + it("resolves a pager item older than the newest 50 with its neighbours", async () => { await seedPr7(55); const id = await createPr7Feed(); From 7bf5b06f4982c1316a3b54f3f53158a41bf0a33d Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 17:14:58 -0400 Subject: [PATCH 10/14] feat(api): add a one-time backfill that lowercases stored gh.repo values --- apps/api/src/gh-repo-case-backfill.ts | 88 ++++++++++ apps/api/src/routes/admin.ts | 19 ++- .../test/gh-repo-case-backfill-sqlite.test.ts | 160 ++++++++++++++++++ docs/ops.md | 22 +++ 4 files changed, 288 insertions(+), 1 deletion(-) create mode 100644 apps/api/src/gh-repo-case-backfill.ts create mode 100644 apps/api/test/gh-repo-case-backfill-sqlite.test.ts diff --git a/apps/api/src/gh-repo-case-backfill.ts b/apps/api/src/gh-repo-case-backfill.ts new file mode 100644 index 00000000..bec5c139 --- /dev/null +++ b/apps/api/src/gh-repo-case-backfill.ts @@ -0,0 +1,88 @@ +/** + * One-time, idempotent backfill: lowercases `file_metadata.meta_value` for the + * keys in `LOWERCASED_META_KEYS` (today `gh.repo`) on rows written before + * `canonicalMetaValue` existed. Mixed-case rows are invisible to the scope + * queries and live links, which match the lowercase spelling exactly. + * + * Safe against the primary key: `file_metadata` is keyed by + * `(workspace, object_key, meta_key)` and `meta_value` is in no unique index, + * so changing a value can never collide with another row. `updated_at` is left + * alone so listing order does not move. Mirrors self-serve-plan-backfill.ts: + * admin-gated route, `dryRun`, a result object, hand-run once. + * + * Batches by `rowid` so no single D1 statement is large. Each call does at most + * `maxBatches` batches; re-run until `remaining` is 0. + */ +import type { D1Queryable } from "./db-session"; +import { LOWERCASED_META_KEYS, META_KEY_RE } from "./file-metadata"; + +export interface GhRepoCaseBackfillResult { + dryRun: boolean; + /** Rows needing the fix before this run. */ + affected: number; + /** Rows rewritten by this run (0 in a dry run). */ + updated: number; + batches: number; + /** Rows still needing the fix after this run. */ + remaining: number; +} + +const DEFAULT_BATCH_SIZE = 500; +const DEFAULT_MAX_BATCHES = 40; + +/** + * `meta_key IN ('gh.repo')` with the keys inlined as literals so SQLite can use + * the partial `gh.repo` index. They are code constants; the regex guards a bad edit. + */ +function keyList(): string { + for (const key of LOWERCASED_META_KEYS) { + if (!META_KEY_RE.test(key)) throw new Error(`invalid lowercased meta key: ${key}`); + } + return LOWERCASED_META_KEYS.map((key) => `'${key}'`).join(", "); +} + +async function countNeedingFix(db: D1Queryable): Promise { + const row = await db + .prepare( + `SELECT COUNT(*) AS n FROM file_metadata + WHERE meta_key IN (${keyList()}) AND meta_value <> lower(meta_value)`, + ) + .first<{ n: number }>(); + return row?.n ?? 0; +} + +export async function backfillLowercasedMetaValues( + db: D1Queryable, + opts: { dryRun?: boolean; batchSize?: number; maxBatches?: number } = {}, +): Promise { + const dryRun = opts.dryRun === true; + const batchSize = Math.max(1, Math.floor(opts.batchSize ?? DEFAULT_BATCH_SIZE)); + const maxBatches = Math.max(1, Math.floor(opts.maxBatches ?? DEFAULT_MAX_BATCHES)); + + const affected = await countNeedingFix(db); + if (dryRun || affected === 0) { + return { dryRun, affected, updated: 0, batches: 0, remaining: affected }; + } + + let updated = 0; + let batches = 0; + while (batches < maxBatches) { + const keys = keyList(); + const result = await db + .prepare( + `UPDATE file_metadata SET meta_value = lower(meta_value) + WHERE rowid IN ( + SELECT rowid FROM file_metadata + WHERE meta_key IN (${keys}) AND meta_value <> lower(meta_value) + LIMIT ? + )`, + ) + .bind(batchSize) + .run(); + const changed = result.meta?.changes ?? 0; + if (changed === 0) break; + updated += changed; + batches += 1; + } + return { dryRun, affected, updated, batches, remaining: await countNeedingFix(db) }; +} diff --git a/apps/api/src/routes/admin.ts b/apps/api/src/routes/admin.ts index 13a575d3..aeb7c9b6 100644 --- a/apps/api/src/routes/admin.ts +++ b/apps/api/src/routes/admin.ts @@ -24,6 +24,7 @@ import { import { deriveWebOrigin, inviteLinkUrl as inviteMagicLink } from "../invite-links"; import { reencryptRegistryCredentials } from "../reencrypt-registry"; import { backfillSelfServePlans } from "../self-serve-plan-backfill"; +import { backfillLowercasedMetaValues } from "../gh-repo-case-backfill"; import { storage } from "../storage"; import { mutateWorkspaceRecord } from "../workspace-mutate"; import { teardownWorkspace } from "../workspace-teardown"; @@ -35,7 +36,7 @@ import { stampSoftDelete, type WorkspaceRecord, } from "../workspace"; -import { dbFor } from "../db-session"; +import { dbFor, primaryDbFor } from "../db-session"; import { createMemberlessOrg, membersForOrg } from "../org-workspaces"; const WS_NAME_RE = /^[a-z0-9][a-z0-9-]{1,62}$/; @@ -473,6 +474,22 @@ export const admin = new Hono<{ Bindings: Env }>() } }) + /** + * One-time backfill for slice 1 of the Files/live-link work: lowercases + * `gh.repo` (and any other key in LOWERCASED_META_KEYS) on file_metadata + * rows written before canonical spelling existed, so those objects appear in + * By pull request files, PR pages, and live links. Idempotent; run by hand + * once after the API deploy (docs/ops.md). Repeat while `remaining` > 0. + * Query: ?dryRun=1 + */ + .post("/file-metadata/backfill-gh-repo-case", async (c) => { + const dryRun = + c.req.query("dryRun") === "1" || + c.req.query("dryRun") === "true" || + c.req.query("dry_run") === "1"; + return c.json(await backfillLowercasedMetaValues(primaryDbFor(c.env), { dryRun })); + }) + /** * On-demand run of the hosted-host SVG/XML sandboxing-CSP probe (issue * #929 final-review) — the same sweep the daily cron runs (`index.ts`'s diff --git a/apps/api/test/gh-repo-case-backfill-sqlite.test.ts b/apps/api/test/gh-repo-case-backfill-sqlite.test.ts new file mode 100644 index 00000000..1c7401e1 --- /dev/null +++ b/apps/api/test/gh-repo-case-backfill-sqlite.test.ts @@ -0,0 +1,160 @@ +/// + +import { Hono } from "hono"; +import { describe, expect, it } from "vitest"; +import { backfillLowercasedMetaValues } from "../src/gh-repo-case-backfill"; +import { respondError } from "../src/error-response"; +import { admin } from "../src/routes/admin"; +import { SqliteD1, database } from "./helpers/sqlite-d1"; + +const MIGRATIONS = ["migrations/20260713210559_file_metadata.sql"]; +const ADMIN_TOKEN = "test-admin-token"; + +if (typeof crypto.subtle.timingSafeEqual !== "function") { + ( + crypto.subtle as unknown as { timingSafeEqual: (a: Uint8Array, b: Uint8Array) => boolean } + ).timingSafeEqual = (a: Uint8Array, b: Uint8Array) => + a.length === b.length && a.every((byte, i) => byte === b[i]); +} + +function put(sqlite: SqliteD1, workspace: string, key: string, metaKey: string, value: string) { + sqlite.db + .prepare( + `INSERT INTO file_metadata (workspace, object_key, meta_key, meta_value, updated_at) + VALUES (?, ?, ?, ?, '2026-09-01T00:00:00.000Z')`, + ) + .run(workspace, key, metaKey, value); +} + +function value(sqlite: SqliteD1, workspace: string, key: string, metaKey: string) { + return sqlite.db + .prepare( + `SELECT meta_value AS v, updated_at AS u FROM file_metadata + WHERE workspace = ? AND object_key = ? AND meta_key = ?`, + ) + .get(workspace, key, metaKey) as { v: string; u: string } | undefined; +} + +function seed(sqlite: SqliteD1) { + put(sqlite, "alpha", "a.png", "gh.repo", "Acme/Web"); + put(sqlite, "alpha", "b.png", "gh.repo", "acme/web"); // already canonical + put(sqlite, "beta", "a.png", "gh.repo", "ACME/API"); // another workspace, same key name + put(sqlite, "alpha", "a.png", "gh.number", "12"); + put(sqlite, "alpha", "a.png", "path", "/Settings/Billing"); // other keys keep their case + put(sqlite, "alpha", "a.png", "gh.ref", "Acme/Web#12"); // not canonicalized by Task 2 +} + +describe("backfillLowercasedMetaValues", () => { + it("dry-run reports the affected row count and writes nothing", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + seed(sqlite); + const result = await backfillLowercasedMetaValues(database(sqlite), { dryRun: true }); + expect(result).toEqual({ dryRun: true, affected: 2, updated: 0, batches: 0, remaining: 2 }); + expect(value(sqlite, "alpha", "a.png", "gh.repo")?.v).toBe("Acme/Web"); + } finally { + sqlite.close(); + } + }); + + it("lowercases only gh.repo, keeps updated_at, and is idempotent", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + seed(sqlite); + const result = await backfillLowercasedMetaValues(database(sqlite)); + expect(result).toMatchObject({ dryRun: false, affected: 2, updated: 2, remaining: 0 }); + expect(value(sqlite, "alpha", "a.png", "gh.repo")).toEqual({ + v: "acme/web", + u: "2026-09-01T00:00:00.000Z", + }); + expect(value(sqlite, "beta", "a.png", "gh.repo")?.v).toBe("acme/api"); + expect(value(sqlite, "alpha", "a.png", "path")?.v).toBe("/Settings/Billing"); + expect(value(sqlite, "alpha", "a.png", "gh.ref")?.v).toBe("Acme/Web#12"); + expect(value(sqlite, "alpha", "a.png", "gh.number")?.v).toBe("12"); + + expect(await backfillLowercasedMetaValues(database(sqlite))).toEqual({ + dryRun: false, + affected: 0, + updated: 0, + batches: 0, + remaining: 0, + }); + } finally { + sqlite.close(); + } + }); + + it("works in bounded batches and reports what remains", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + for (let i = 0; i < 7; i++) put(sqlite, "alpha", `k${i}.png`, "gh.repo", "Acme/Web"); + const first = await backfillLowercasedMetaValues(database(sqlite), { + batchSize: 3, + maxBatches: 2, + }); + expect(first).toMatchObject({ affected: 7, updated: 6, batches: 2, remaining: 1 }); + const second = await backfillLowercasedMetaValues(database(sqlite), { + batchSize: 3, + maxBatches: 2, + }); + expect(second).toMatchObject({ affected: 1, updated: 1, remaining: 0 }); + } finally { + sqlite.close(); + } + }); + + it("repairs the scope: a backfilled object now matches the lowercase scope query", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + put(sqlite, "alpha", "a.png", "gh.repo", "Acme/Web"); + const scoped = () => + sqlite.db + .prepare( + `SELECT object_key FROM file_metadata + WHERE workspace = 'alpha' AND meta_key = 'gh.repo' AND meta_value = 'acme/web'`, + ) + .all(); + expect(scoped()).toHaveLength(0); + await backfillLowercasedMetaValues(database(sqlite)); + expect(scoped()).toHaveLength(1); + } finally { + sqlite.close(); + } + }); +}); + +describe("POST /admin/file-metadata/backfill-gh-repo-case", () => { + function appFor(sqlite: SqliteD1) { + const app = new Hono<{ Bindings: Env }>() + .route("/admin", admin) + .onError((err, c) => respondError(c, err)); + const env = { ADMIN_TOKEN, DB: sqlite } as unknown as Env; + const call = (query = "", token = ADMIN_TOKEN) => + app.request( + `https://api.uploads.sh/admin/file-metadata/backfill-gh-repo-case${query}`, + { method: "POST", headers: { authorization: `Bearer ${token}` } }, + env, + ); + return call; + } + + it("requires the admin token, honors ?dryRun=1, and applies otherwise", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + seed(sqlite); + const call = appFor(sqlite); + expect((await call("", "wrong")).status).toBe(401); + + const dry = await call("?dryRun=1"); + expect(dry.status).toBe(200); + expect(await dry.json()).toMatchObject({ dryRun: true, affected: 2, updated: 0 }); + expect(value(sqlite, "alpha", "a.png", "gh.repo")?.v).toBe("Acme/Web"); + + const live = await call(); + expect(await live.json()).toMatchObject({ dryRun: false, updated: 2, remaining: 0 }); + expect(value(sqlite, "alpha", "a.png", "gh.repo")?.v).toBe("acme/web"); + } finally { + sqlite.close(); + } + }); +}); diff --git a/docs/ops.md b/docs/ops.md index c11c8e61..41665f5a 100644 --- a/docs/ops.md +++ b/docs/ops.md @@ -347,6 +347,28 @@ node --env-file=.env apps/api/scripts/backfill-gh-metadata.mjs for one run. Test against a local `wrangler dev` stack first — never point this at production while testing. +## Backfill mixed-case `gh.repo` + +Run **by hand, once, after the Files/live-link API slice deploys.** Before that +slice, a generic write (`uploads put --meta gh.repo=Acme/Web`, an +`X-Uploads-Meta-gh.repo` header, `set_metadata`) stored `gh.repo` as typed. The +API now stores it lowercased, and the scope views and live links match the +lowercase spelling exactly, so older mixed-case rows stay hidden until this runs. +It is not a migration (migrations auto-apply on merge and never rewrite data). + +```bash +# dry run: prints { affected, remaining } and writes nothing +curl -XPOST -H "Authorization: Bearer $ADMIN_TOKEN" \ + 'https://api.uploads.sh/admin/file-metadata/backfill-gh-repo-case?dryRun=1' +# live: repeat until "remaining" is 0 +curl -XPOST -H "Authorization: Bearer $ADMIN_TOKEN" \ + https://api.uploads.sh/admin/file-metadata/backfill-gh-repo-case +``` + +Idempotent. Each call rewrites at most 20,000 rows (`updated_at` is not +touched). `github_pr_activity` and `feeds` already store the repo lowercased and +need no backfill. + ## Account linking (issue #233) A person can end up with two Better Auth users for one identity: a From d1ed314710207e71601887463b0d730ed58b3012 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 17:24:31 -0400 Subject: [PATCH 11/14] fix(api): cap privateCount storage probes and drop promoted shadows from repos countPrivateScopeItems probed every key past the first page (up to 2,000), at two or more R2 operations each, which could pass the 1,000-subrequest ceiling and fail /scope with a 503 on busy repos. It now returns null when more than 300 keys need a probe, and probes none of them. listWorkspaceRepos now drops gh.status=promoted shadows like scopeFrom, so a repo holding only shadows does not list and a shadow never sets lastUpdatedAt. docs/ops.md: the backfill dry-run comment now matches the route's output. --- apps/api/src/feed-service.ts | 29 ++++++++++++++--- apps/api/src/pr-scope.ts | 21 ++++++++---- apps/api/src/routes/workspace-scope.ts | 7 ++-- apps/api/src/scope-wire.ts | 7 +++- apps/api/test/pr-scope-sqlite.test.ts | 26 ++++++++++++++- apps/api/test/routes-workspace-scope.test.ts | 34 ++++++++++++++++++++ docs/ops.md | 3 +- 7 files changed, 111 insertions(+), 16 deletions(-) diff --git a/apps/api/src/feed-service.ts b/apps/api/src/feed-service.ts index e251b1d1..6f3f22e5 100644 --- a/apps/api/src/feed-service.ts +++ b/apps/api/src/feed-service.ts @@ -311,20 +311,41 @@ export async function hydrateFeedItems( } /** - * How many of `keys` are private (what a live link renders as `withheld`). + * Most keys `countPrivateScopeItems` will probe beyond the ones the page + * already HEADed. Each probe costs at least 2 R2 operations, so 300 probes + * stay inside the Workers ceiling of 1,000 subrequests per request + * (github-promote.ts) next to the page's own hydration. + */ +export const PRIVATE_COUNT_PROBE_CAP = 300; + +/** + * How many of `keys` are private: objects whose R2 custom metadata says + * `visibility: private`. A live link renders the same objects as `withheld`. * Visibility lives only in R2 custom metadata (`visibility.ts`; D1 rejects - * the key), so each key the page did not already HEAD (`seen.checked`) costs - * one lane lookup + HEAD. Keys already HEADed count from `seen.privateKeys`. + * the key), so the count needs storage reads: + * + * - Keys the page already HEADed (`seen.checked`) are free. They count from + * `seen.privateKeys`. + * - Every other key costs one `exists` per lane tried (active lane first, + * then each fallback lane until a hit), then one HEAD on the lane that has + * it. That is 2 R2 operations for a key in the active lane, and more for a + * key in a fallback lane. + * + * When more than `PRIVATE_COUNT_PROBE_CAP` keys need a probe, this returns + * `null` without probing any of them: the count is unknown, and the share + * confirm is skipped. `keys` is itself capped at `SCOPE_SCAN_CAP` by the + * caller's scan. */ export async function countPrivateScopeItems( env: Env, workspace: WorkspaceRecord, keys: string[], seen: { checked: Set; privateKeys: Set }, -): Promise { +): Promise { const known = keys.filter((key) => seen.privateKeys.has(key)).length; const unchecked = keys.filter((key) => !seen.checked.has(key)); if (unchecked.length === 0) return known; + if (unchecked.length > PRIVATE_COUNT_PROBE_CAP) return null; const resolver = createLaneResolver(env, workspace); let flags: boolean[]; try { diff --git a/apps/api/src/pr-scope.ts b/apps/api/src/pr-scope.ts index 7d1c3fa3..4f54f592 100644 --- a/apps/api/src/pr-scope.ts +++ b/apps/api/src/pr-scope.ts @@ -182,7 +182,11 @@ export async function scopeKeysAt( return (results ?? []).map((row) => row.object_key); } -/** Distinct `gh.repo` values with their newest upload time, newest first. */ +/** + * Distinct `gh.repo` values with their newest upload time, newest first. + * Drops `gh.status=promoted` shadows the same way `scopeFrom` does, so a repo + * holding only shadows does not list and a shadow never sets `lastUpdatedAt`. + */ export async function listWorkspaceRepos( db: D1Queryable, workspace: string, @@ -192,12 +196,17 @@ export async function listWorkspaceRepos( nextCursor: ScopeCursor | null; }> { const params: unknown[] = [workspace]; - let sql = `SELECT meta_value AS repo, MAX(updated_at) AS last_updated_at - FROM file_metadata - WHERE workspace = ? AND meta_key = 'gh.repo' - GROUP BY meta_value`; + let sql = `SELECT r.meta_value AS repo, MAX(r.updated_at) AS last_updated_at + FROM file_metadata r + WHERE r.workspace = ? AND r.meta_key = 'gh.repo' + AND NOT EXISTS ( + SELECT 1 FROM file_metadata s + WHERE s.workspace = r.workspace AND s.object_key = r.object_key + AND s.meta_key = 'gh.status' AND s.meta_value = 'promoted' + ) + GROUP BY r.meta_value`; if (opts.cursor) { - sql += ` HAVING MAX(updated_at) < ? OR (MAX(updated_at) = ? AND meta_value > ?)`; + sql += ` HAVING MAX(r.updated_at) < ? OR (MAX(r.updated_at) = ? AND r.meta_value > ?)`; params.push(opts.cursor.updatedAt, opts.cursor.updatedAt, opts.cursor.key); } sql += ` ORDER BY last_updated_at DESC, repo ASC LIMIT ?`; diff --git a/apps/api/src/routes/workspace-scope.ts b/apps/api/src/routes/workspace-scope.ts index 8e493e95..85c0f58d 100644 --- a/apps/api/src/routes/workspace-scope.ts +++ b/apps/api/src/routes/workspace-scope.ts @@ -226,9 +226,10 @@ export async function reposHandler(c: Context) { /** * One scope's files (a PR when `number` is set, else the whole repo), newest * first, hydrated like a live link for the owner audience. Owner and repo - * are lowercased before matching. `privateCount` runs on the first page only - * and reuses the page's HEADs; `liveLink` is the existing feed for exactly - * this scope; `pull` is this workspace's rollup row for the PR. + * are lowercased before matching. `privateCount` runs on the first page only, + * reuses the page's HEADs, and is null when the rest of the scope needs more + * than `PRIVATE_COUNT_PROBE_CAP` storage probes; `liveLink` is the existing + * feed for exactly this scope; `pull` is this workspace's rollup row for the PR. */ export async function scopeFilesHandler(c: Context) { const workspace = c.get("workspaceName"); diff --git a/apps/api/src/scope-wire.ts b/apps/api/src/scope-wire.ts index 3b4f2129..0b4d2d55 100644 --- a/apps/api/src/scope-wire.ts +++ b/apps/api/src/scope-wire.ts @@ -135,7 +135,12 @@ export interface ScopeFilesResponse { number: number | null; items: FeedItemDto[]; nextCursor: string | null; - /** Withheld-on-public items across the whole scope (cap 2,000), ignoring `type`. First page only; `null` on cursor pages. */ + /** + * Private items across the whole scope, ignoring `type`: the items a live + * link for this scope renders as `withheld`. First page only. `null` on + * cursor pages, and `null` when counting would need more than 300 storage + * probes beyond the page (unknown: skip the share confirm). + */ privateCount: number | null; /** The existing live feed for exactly this scope, if any. */ liveLink: LiveLinkRef | null; diff --git a/apps/api/test/pr-scope-sqlite.test.ts b/apps/api/test/pr-scope-sqlite.test.ts index 7706da4c..aef8eefa 100644 --- a/apps/api/test/pr-scope-sqlite.test.ts +++ b/apps/api/test/pr-scope-sqlite.test.ts @@ -220,7 +220,7 @@ describe("prScopeQuery", () => { expect(plan).not.toContain("TEMP B-TREE FOR ORDER BY"); await listWorkspaceRepos(db, "alpha", { limit: 20 }); - const repos = seen.find((entry) => entry.sql.includes("GROUP BY meta_value")); + const repos = seen.find((entry) => entry.sql.includes("GROUP BY r.meta_value")); expect(queryPlan(sqlite, repos!)).toContain( "COVERING INDEX file_metadata_gh_repo_recent_idx", ); @@ -289,6 +289,30 @@ describe("listWorkspaceRepos", () => { sqlite.close(); } }); + + it("skips promoted shadows, so they neither list a repo nor set lastUpdatedAt", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + const shadow = { "gh.status": "promoted" }; + await seed(sqlite, "a/1.png", { "gh.repo": "acme/app" }, "2026-10-01T01:00:00.000Z"); + await seed( + sqlite, + "a/shadow.png", + { "gh.repo": "acme/app", ...shadow }, + "2026-10-01T08:00:00.000Z", + ); + await seed( + sqlite, + "o/shadow.png", + { "gh.repo": "acme/only-shadows", ...shadow }, + "2026-10-01T09:00:00.000Z", + ); + const page = await listWorkspaceRepos(database(sqlite), "alpha", { limit: 20 }); + expect(page.repos).toEqual([{ repo: "acme/app", lastUpdatedAt: "2026-10-01T01:00:00.000Z" }]); + } finally { + sqlite.close(); + } + }); }); describe("gh.repo canonical spelling", () => { diff --git a/apps/api/test/routes-workspace-scope.test.ts b/apps/api/test/routes-workspace-scope.test.ts index 003bd0a8..5236dce3 100644 --- a/apps/api/test/routes-workspace-scope.test.ts +++ b/apps/api/test/routes-workspace-scope.test.ts @@ -497,6 +497,40 @@ describe("GET /v1/workspaces/:ws/scope/:owner/:repo/files", () => { expect(all.privateCount).toBe(2); }); + it("counts exactly up to 300 extra probes, and returns null past the cap without probing", async () => { + const meta = prMeta("acme/app", 7); + // 301 objects: a 1-item page leaves exactly 300 keys to probe. + for (let index = 0; index < 301; index++) { + await seedObject(`gh/acme/app/pull/7/${String(index).padStart(3, "0")}.png`, meta, { + private: index === 10 || index === 200, + }); + } + const headed: string[] = []; + const head = bucket.head.bind(bucket); + bucket.head = async (key: string) => { + headed.push(key); + return head(key); + }; + + const atCap = await getJson( + "/v1/workspaces/alpha/scope/acme/app/files?number=7&limit=1", + ); + expect(atCap.items).toHaveLength(1); + expect(atCap.privateCount).toBe(2); + + // One more object: 301 keys past the page, so the count is unknown. + await seedObject("gh/acme/app/pull/7/301.png", meta); + headed.length = 0; + const pastCap = await getJson( + "/v1/workspaces/alpha/scope/acme/app/files?number=7&limit=1", + ); + expect(pastCap.items).toHaveLength(1); + expect(pastCap.privateCount).toBeNull(); + // Only the page's own item reached storage. + const pageKey = `alpha/${pastCap.items[0]?.objectKey}`; + expect(new Set(headed)).toEqual(new Set([pageKey])); + }); + it("lowercases an owner/repo with dots, dashes, underscores, and capitals (index review focus 2)", async () => { await seedObject("gh/foo.bar/my-repo_x/pull/2/shot.png", prMeta("foo.bar/my-repo_x", 2)); const created = await request("/v1/workspaces/alpha/feeds", { diff --git a/docs/ops.md b/docs/ops.md index 41665f5a..01a697b2 100644 --- a/docs/ops.md +++ b/docs/ops.md @@ -357,7 +357,8 @@ lowercase spelling exactly, so older mixed-case rows stay hidden until this runs It is not a migration (migrations auto-apply on merge and never rewrite data). ```bash -# dry run: prints { affected, remaining } and writes nothing +# dry run: prints { dryRun: true, affected, updated: 0, batches: 0, remaining } +# ("remaining" equals "affected") and writes nothing curl -XPOST -H "Authorization: Bearer $ADMIN_TOKEN" \ 'https://api.uploads.sh/admin/file-metadata/backfill-gh-repo-case?dryRun=1' # live: repeat until "remaining" is 0 From 66991cda851157a667a4dde07ea04cbebc055796 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 17:42:50 -0400 Subject: [PATCH 12/14] refactor(api): share live-link scope, cursor, and title helpers --- apps/api/src/feed-service.ts | 40 ++--- apps/api/src/file-metadata.ts | 2 +- apps/api/src/gh-repo-case-backfill.ts | 2 +- apps/api/src/github-comment.test.ts | 6 +- apps/api/src/github-comment.ts | 5 +- apps/api/src/pr-scope.ts | 145 ++++++++---------- apps/api/src/routes/admin.ts | 26 ++-- apps/api/src/routes/public-feeds.ts | 9 +- apps/api/src/routes/workspace-scope.ts | 28 ++-- apps/api/src/scope-service.ts | 4 +- apps/api/test/comment-render-scope.test.ts | 6 +- apps/api/test/pr-scope-sqlite.test.ts | 2 +- packages/comment-render/src/scope.ts | 3 + .../src/comment-render-scope.generated.ts | 3 + 14 files changed, 131 insertions(+), 150 deletions(-) diff --git a/apps/api/src/feed-service.ts b/apps/api/src/feed-service.ts index 6f3f22e5..476d5621 100644 --- a/apps/api/src/feed-service.ts +++ b/apps/api/src/feed-service.ts @@ -13,10 +13,11 @@ import { publicObjectDateFields } from "./files-core"; import { getMetadataForKeys } from "./file-metadata"; import { encodePublicFeedCursor, + feedRecordScope, prScopeQuery, scanScopeKeys, - type ScopeResume, type ScopeItem, + type ScopeResume, } from "./pr-scope"; import { FEED_ID_RE, @@ -38,7 +39,7 @@ import { createLaneResolver, objectPublicUrls, type LaneResolver } from "./stora import type { StorageConfig } from "@uploads/storage"; import { objectVisibility } from "./visibility"; import { webOrigin } from "./web-url"; -import { feedItemIdFor } from "@uploads/comment-render/scope"; +import { FEED_ITEM_ID_RE, feedItemIdFor } from "@uploads/comment-render/scope"; import { type WorkspaceRecord } from "./workspace"; import { dbFor, type D1Queryable } from "./db-session"; @@ -84,11 +85,6 @@ export function feedUrl(env: Env, id: string): string { return webOrigin(env) + "/c/" + encodeURIComponent(id); } -/** Stable public-item id: first 32 hex chars of SHA-256(object key). One implementation, shared with the web and CLI. */ -export function feedItemId(objectKey: string): Promise { - return feedItemIdFor(objectKey); -} - export function feedItemUrl(env: Env, feedId: string, itemId: string): string { return feedUrl(env, feedId) + "/" + encodeURIComponent(itemId); } @@ -257,7 +253,7 @@ export async function hydrateFeedItems( code: "feed_object_not_public", }); const dates = meta && !withheld ? publicObjectDateFields(meta) : {}; - const id = await feedItemId(match.key); + const id = await feedItemIdFor(match.key); return { id, objectKey: match.key, @@ -409,10 +405,7 @@ export async function hydratePublicFeed( opts: { cursor?: ScopeResume | null } = {}, ): Promise { const page = await prScopeQuery(dbFor(env), { - workspace: record.workspace, - repo: record.repo, - ...(record.path ? { path: record.path } : {}), - ...(record.number > 0 ? { number: record.number } : {}), + ...feedRecordScope(record), cursor: opts.cursor ?? null, limit: FEED_ITEM_LIMIT, }); @@ -432,8 +425,6 @@ export async function hydratePublicFeed( }; } -const FEED_ITEM_ID_RE = /^[0-9a-f]{32}$/; - /** * One public item plus its neighbours, for the `/c//` pager. Scans * the scope's keys (cap 2,000) and hashes them until one matches: item ids @@ -448,21 +439,16 @@ export async function publicFeedItemPage( ): Promise { if (!FEED_ITEM_ID_RE.test(itemId)) return null; const db = dbFor(env); - const scope: ScopeItem[] = await scanScopeKeys(db, { - workspace: record.workspace, - repo: record.repo, - ...(record.path ? { path: record.path } : {}), - ...(record.number > 0 ? { number: record.number } : {}), - }); - const ids: string[] = []; + const scope = await scanScopeKeys(db, feedRecordScope(record)); + let prevId: string | null = null; let index = -1; - for (const entry of scope) { - const id = await feedItemId(entry.key); - ids.push(id); + for (const [i, entry] of scope.entries()) { + const id = await feedItemIdFor(entry.key); if (id === itemId) { - index = ids.length - 1; + index = i; break; } + prevId = id; } if (index < 0) return null; @@ -483,8 +469,8 @@ export async function publicFeedItemPage( kind: summary.kind, }, item: toPublicItem(item), - prev: index > 0 ? ids[index - 1] : null, - next: index + 1 < scope.length ? await feedItemId(scope[index + 1].key) : null, + prev: prevId, + next: index + 1 < scope.length ? await feedItemIdFor(scope[index + 1].key) : null, index, total: scope.length, }; diff --git a/apps/api/src/file-metadata.ts b/apps/api/src/file-metadata.ts index 189d1829..fd278709 100644 --- a/apps/api/src/file-metadata.ts +++ b/apps/api/src/file-metadata.ts @@ -541,7 +541,7 @@ const PREFIX_FILTER_SQL = `substr(object_key, 1, length(?)) = ?`; * in — `find_files({ "gh.status": "promoted" })` must still return it. Written * against a subquery alias `s`; the caller supplies how to reach the outer row. */ -const PROMOTED_SHADOW_STATUS_SQL = `s.meta_key = 'gh.status' AND s.meta_value = 'promoted'`; +export const PROMOTED_SHADOW_STATUS_SQL = `s.meta_key = 'gh.status' AND s.meta_value = 'promoted'`; /** * Predicate matching an object stamped `gh.merged=true` — written by diff --git a/apps/api/src/gh-repo-case-backfill.ts b/apps/api/src/gh-repo-case-backfill.ts index bec5c139..1e138368 100644 --- a/apps/api/src/gh-repo-case-backfill.ts +++ b/apps/api/src/gh-repo-case-backfill.ts @@ -64,10 +64,10 @@ export async function backfillLowercasedMetaValues( return { dryRun, affected, updated: 0, batches: 0, remaining: affected }; } + const keys = keyList(); let updated = 0; let batches = 0; while (batches < maxBatches) { - const keys = keyList(); const result = await db .prepare( `UPDATE file_metadata SET meta_value = lower(meta_value) diff --git a/apps/api/src/github-comment.test.ts b/apps/api/src/github-comment.test.ts index c7aa0341..f66b1daf 100644 --- a/apps/api/src/github-comment.test.ts +++ b/apps/api/src/github-comment.test.ts @@ -4,7 +4,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { gatherCommentBody, upsertBotComment } from "./github-comment"; import { ATTACHMENTS_MARKER, attachmentsMarker, ghPrivateKeyPrefix } from "./github-comment-render"; import { findFeedByScope } from "./feeds"; -import { feedItemId } from "./feed-service"; +import { feedItemIdFor } from "@uploads/comment-render/scope"; import { addExternalReference, addGalleryItem, createGallery } from "./galleries"; import { replaceFileMetadata, setServerFileMetadata } from "./file-metadata"; import { objectPublicUrls, storageConfig } from "./storage"; @@ -270,8 +270,8 @@ describe("gatherCommentBody", () => { const feed = await findFeedByScope(env.DB, workspaceName, "acme/web", "", 12); expect(feed).toBeTruthy(); expect(feed!.source).toBe("comment"); - const idA = await feedItemId(keyA); - const idB = await feedItemId(keyB); + const idA = await feedItemIdFor(keyA); + const idB = await feedItemIdFor(keyB); expect(first.body).toContain(`/c/${feed!.id}/${idA}`); expect(first.body).toContain(`/c/${feed!.id}/${idB}`); expect(first.body).not.toContain(`/f/${workspaceName}/`); diff --git a/apps/api/src/github-comment.ts b/apps/api/src/github-comment.ts index c4e020f9..38aa711f 100644 --- a/apps/api/src/github-comment.ts +++ b/apps/api/src/github-comment.ts @@ -8,7 +8,8 @@ */ import { dbFor } from "./db-session"; -import { feedItemId, feedItemUrl, findLatestRepoScreenshots } from "./feed-service"; +import { feedItemIdFor } from "@uploads/comment-render/scope"; +import { feedItemUrl, findLatestRepoScreenshots } from "./feed-service"; import { createFeed } from "./feeds"; import { listObjects } from "./files-core"; import { getMetadataForKeys } from "./file-metadata"; @@ -336,7 +337,7 @@ async function applyPrFeedPageUrls( const idByKey = new Map(); await Promise.all( matches.map(async (match) => { - idByKey.set(match.key, await feedItemId(match.key)); + idByKey.set(match.key, await feedItemIdFor(match.key)); }), ); for (const item of items) { diff --git a/apps/api/src/pr-scope.ts b/apps/api/src/pr-scope.ts index 4f54f592..e40f2cd8 100644 --- a/apps/api/src/pr-scope.ts +++ b/apps/api/src/pr-scope.ts @@ -15,10 +15,12 @@ * the SQL or SQLite cannot use that partial index. */ import { ValidationError } from "@uploads/errors"; -import { feedItemIdFor, type FileTypeClass } from "@uploads/comment-render/scope"; +import { FEED_ITEM_ID_RE, feedItemIdFor, type FileTypeClass } from "@uploads/comment-render/scope"; import { type D1Queryable } from "./db-session"; -import { getMetadataForKeys } from "./file-metadata"; +import type { FeedRecord } from "./feeds"; +import { getMetadataForKeys, PROMOTED_SHADOW_STATUS_SQL } from "./file-metadata"; import { fileTypeSql } from "./file-type-sql"; +import { b64urlDecode, b64urlEncode } from "./secrets"; export const SCOPE_DEFAULT_LIMIT = 50; export const SCOPE_MAX_LIMIT = 100; @@ -63,6 +65,18 @@ interface ScopeRow { updated_at: string; } +/** A live link's scope: repo, plus `path` and `number` when the feed has them. */ +export function feedRecordScope( + record: Pick, +): Omit { + return { + workspace: record.workspace, + repo: record.repo, + ...(record.path ? { path: record.path } : {}), + ...(record.number > 0 ? { number: record.number } : {}), + }; +} + function scopeFrom(q: Omit): { sql: string; params: unknown[] } { const params: unknown[] = [q.workspace, q.repo]; let sql = `FROM file_metadata r @@ -70,7 +84,7 @@ function scopeFrom(q: Omit): { sql: string; para AND NOT EXISTS ( SELECT 1 FROM file_metadata s WHERE s.workspace = r.workspace AND s.object_key = r.object_key - AND s.meta_key = 'gh.status' AND s.meta_value = 'promoted' + AND ${PROMOTED_SHADOW_STATUS_SQL} )`; if (q.number !== undefined && q.number > 0) { sql += ` AND EXISTS ( @@ -143,21 +157,26 @@ export async function prScopeQuery( /** * The whole scope, newest first, up to `cap` keys. No metadata (`{}`): the - * pager and the private count need only keys. + * pager and the private count need only keys. `at` keeps only rows stamped + * exactly that `updated_at` (in tie-break order), for resolving a public cursor. */ export async function scanScopeKeys( db: D1Queryable, q: Omit, - cap: number = SCOPE_SCAN_CAP, + opts: { cap?: number; at?: string } = {}, ): Promise { - const bounded = Math.max(1, Math.min(SCOPE_SCAN_CAP, Math.floor(cap))); + const bounded = Math.max(1, Math.min(SCOPE_SCAN_CAP, Math.floor(opts.cap ?? SCOPE_SCAN_CAP))); const { sql, params } = scopeFrom(q); + let select = `SELECT r.object_key AS object_key, r.updated_at AS updated_at ${sql}`; + if (opts.at !== undefined) { + select += ` AND r.updated_at = ?`; + params.push(opts.at); + } + select += ` ORDER BY r.updated_at DESC, r.object_key ASC LIMIT ?`; + params.push(bounded); const { results } = await db - .prepare( - `SELECT r.object_key AS object_key, r.updated_at AS updated_at ${sql} - ORDER BY r.updated_at DESC, r.object_key ASC LIMIT ?`, - ) - .bind(...params, bounded) + .prepare(select) + .bind(...params) .all(); return (results ?? []).map((row) => ({ key: row.object_key, @@ -166,22 +185,6 @@ export async function scanScopeKeys( })); } -/** Keys in the scope stamped exactly `updatedAt`, in tie-break order. Small: rows sharing one timestamp. */ -export async function scopeKeysAt( - db: D1Queryable, - q: Omit, - updatedAt: string, -): Promise { - const { sql, params } = scopeFrom(q); - const { results } = await db - .prepare( - `SELECT r.object_key AS object_key ${sql} AND r.updated_at = ? ORDER BY r.object_key ASC LIMIT ?`, - ) - .bind(...params, updatedAt, SCOPE_SCAN_CAP) - .all<{ object_key: string }>(); - return (results ?? []).map((row) => row.object_key); -} - /** * Distinct `gh.repo` values with their newest upload time, newest first. * Drops `gh.status=promoted` shadows the same way `scopeFrom` does, so a repo @@ -202,7 +205,7 @@ export async function listWorkspaceRepos( AND NOT EXISTS ( SELECT 1 FROM file_metadata s WHERE s.workspace = r.workspace AND s.object_key = r.object_key - AND s.meta_key = 'gh.status' AND s.meta_value = 'promoted' + AND ${PROMOTED_SHADOW_STATUS_SQL} ) GROUP BY r.meta_value`; if (opts.cursor) { @@ -226,46 +229,45 @@ export async function listWorkspaceRepos( }; } -function base64UrlEncode(text: string): string { - const bytes = new TextEncoder().encode(text); - let binary = ""; - for (const byte of bytes) binary += String.fromCharCode(byte); - return btoa(binary).replace(/\+/g, "-").replace(/\//g, "_").replace(/=+$/, ""); +function invalidCursor(): ValidationError { + return new ValidationError("cursor is not valid for this query", { code: "invalid_cursor" }); } -function base64UrlDecode(text: string): string { - const binary = atob(text.replace(/-/g, "+").replace(/_/g, "/")); - const bytes = new Uint8Array(binary.length); - for (let i = 0; i < binary.length; i += 1) bytes[i] = binary.charCodeAt(i); - return new TextDecoder("utf-8", { fatal: true, ignoreBOM: false }).decode(bytes); -} - -/** Opaque keyset cursor. Also carries the `/pulls` (ref) and `/repos` (repo) cursors. */ -export function encodeScopeCursor(cursor: ScopeCursor): string { - return base64UrlEncode(JSON.stringify({ v: 1, u: cursor.updatedAt, k: cursor.key })); +function encodeCursorEnvelope(body: Record): string { + return b64urlEncode(new TextEncoder().encode(JSON.stringify(body))); } -export function decodeScopeCursor(raw: string | undefined): ScopeCursor | null { - if (raw === undefined || raw === "") return null; - const invalid = () => - new ValidationError("cursor is not valid for this query", { code: "invalid_cursor" }); +/** + * The `{ v: 1, u: updatedAt, ... }` envelope both cursors share. Malformed + * input (bad base64url, bad UTF-8, non-object JSON, wrong version, unparseable + * `u`) is a 400 `invalid_cursor`; each decoder checks its own fields. + */ +function parseCursorEnvelope(raw: string): Record & { u: string } { let parsed: unknown; try { - parsed = JSON.parse(base64UrlDecode(raw)); + parsed = JSON.parse( + new TextDecoder("utf-8", { fatal: true, ignoreBOM: false }).decode(b64urlDecode(raw)), + ); } catch { - throw invalid(); + throw invalidCursor(); } - if (typeof parsed !== "object" || parsed === null) throw invalid(); + if (typeof parsed !== "object" || parsed === null) throw invalidCursor(); const record = parsed as Record; - if ( - record.v !== 1 || - typeof record.u !== "string" || - !Number.isFinite(Date.parse(record.u)) || - typeof record.k !== "string" || - record.k.length === 0 - ) { - throw invalid(); + if (record.v !== 1 || typeof record.u !== "string" || !Number.isFinite(Date.parse(record.u))) { + throw invalidCursor(); } + return record as Record & { u: string }; +} + +/** Opaque keyset cursor. Also carries the `/pulls` (ref) and `/repos` (repo) cursors. */ +export function encodeScopeCursor(cursor: ScopeCursor): string { + return encodeCursorEnvelope({ v: 1, u: cursor.updatedAt, k: cursor.key }); +} + +export function decodeScopeCursor(raw: string | undefined): ScopeCursor | null { + if (raw === undefined || raw === "") return null; + const record = parseCursorEnvelope(raw); + if (typeof record.k !== "string" || record.k.length === 0) throw invalidCursor(); return { updatedAt: record.u, key: record.k }; } @@ -276,9 +278,7 @@ export function decodeScopeCursor(raw: string | undefined): ScopeCursor | null { * Signed-in routes keep `encodeScopeCursor`. */ export async function encodePublicFeedCursor(cursor: ScopeCursor): Promise { - return base64UrlEncode( - JSON.stringify({ v: 1, u: cursor.updatedAt, h: await feedItemIdFor(cursor.key) }), - ); + return encodeCursorEnvelope({ v: 1, u: cursor.updatedAt, h: await feedItemIdFor(cursor.key) }); } /** @@ -294,26 +294,9 @@ export async function decodePublicFeedCursor( raw: string | undefined, ): Promise { if (raw === undefined || raw === "") return null; - const invalid = () => - new ValidationError("cursor is not valid for this query", { code: "invalid_cursor" }); - let parsed: unknown; - try { - parsed = JSON.parse(base64UrlDecode(raw)); - } catch { - throw invalid(); - } - if (typeof parsed !== "object" || parsed === null) throw invalid(); - const record = parsed as Record; - if ( - record.v !== 1 || - typeof record.u !== "string" || - !Number.isFinite(Date.parse(record.u)) || - typeof record.h !== "string" || - !/^[0-9a-f]{32}$/.test(record.h) - ) { - throw invalid(); - } - for (const key of await scopeKeysAt(db, scope, record.u)) { + const record = parseCursorEnvelope(raw); + if (typeof record.h !== "string" || !FEED_ITEM_ID_RE.test(record.h)) throw invalidCursor(); + for (const { key } of await scanScopeKeys(db, scope, { at: record.u })) { if ((await feedItemIdFor(key)) === record.h) return { updatedAt: record.u, key }; } return { updatedAt: record.u, key: null }; diff --git a/apps/api/src/routes/admin.ts b/apps/api/src/routes/admin.ts index aeb7c9b6..779e0994 100644 --- a/apps/api/src/routes/admin.ts +++ b/apps/api/src/routes/admin.ts @@ -6,7 +6,7 @@ import { RateLimitedError, ValidationError, } from "@uploads/errors"; -import { Hono } from "hono"; +import { Hono, type Context } from "hono"; import { adminAuth } from "../admin"; import { adminTokenRows, HASH_PREFIX_LEN, legacyTokens } from "../admin-token-list"; import { runActiveContentHostSweep } from "../active-content-hosts"; @@ -40,6 +40,15 @@ import { dbFor, primaryDbFor } from "../db-session"; import { createMemberlessOrg, membersForOrg } from "../org-workspaces"; const WS_NAME_RE = /^[a-z0-9][a-z0-9-]{1,62}$/; + +/** `?dryRun=1`, `?dryRun=true`, or `?dry_run=1` on the one-off backfill routes. */ +function isDryRun(c: Context): boolean { + return ( + c.req.query("dryRun") === "1" || + c.req.query("dryRun") === "true" || + c.req.query("dry_run") === "1" + ); +} const SECONDS_PER_DAY = 24 * 60 * 60; // Ceiling for --expires-in. 24h caps how long a single-use invite secret can // live; the floor stays 60s at the validation site below. @@ -438,10 +447,7 @@ export const admin = new Hono<{ Bindings: Env }>() * Query: ?dryRun=1 */ .post("/credentials/reencrypt", async (c) => { - const dryRun = - c.req.query("dryRun") === "1" || - c.req.query("dryRun") === "true" || - c.req.query("dry_run") === "1"; + const dryRun = isDryRun(c); try { const result = await reencryptRegistryCredentials(c.env, { dryRun }); return c.json(result); @@ -461,10 +467,7 @@ export const admin = new Hono<{ Bindings: Env }>() * ?dryRun=1 */ .post("/self-serve/backfill-plan", async (c) => { - const dryRun = - c.req.query("dryRun") === "1" || - c.req.query("dryRun") === "true" || - c.req.query("dry_run") === "1"; + const dryRun = isDryRun(c); try { const result = await backfillSelfServePlans(c.env, { dryRun }); return c.json(result); @@ -483,10 +486,7 @@ export const admin = new Hono<{ Bindings: Env }>() * Query: ?dryRun=1 */ .post("/file-metadata/backfill-gh-repo-case", async (c) => { - const dryRun = - c.req.query("dryRun") === "1" || - c.req.query("dryRun") === "true" || - c.req.query("dry_run") === "1"; + const dryRun = isDryRun(c); return c.json(await backfillLowercasedMetaValues(primaryDbFor(c.env), { dryRun })); }) diff --git a/apps/api/src/routes/public-feeds.ts b/apps/api/src/routes/public-feeds.ts index 0e64a626..b45e8951 100644 --- a/apps/api/src/routes/public-feeds.ts +++ b/apps/api/src/routes/public-feeds.ts @@ -2,7 +2,7 @@ import { NotFoundError } from "@uploads/errors"; import { Hono } from "hono"; import { resolvePublicFeed } from "../feeds"; import { hydratePublicFeed, publicFeedItemPage } from "../feed-service"; -import { decodePublicFeedCursor } from "../pr-scope"; +import { decodePublicFeedCursor, feedRecordScope } from "../pr-scope"; import { loadWorkspaceRecord, type WorkspaceVars } from "../workspace"; import { dbFor } from "../db-session"; @@ -23,12 +23,7 @@ export const publicFeeds = new Hono() const { record, workspace } = await liveFeed(c.env, c.req.param("id")); const cursor = await decodePublicFeedCursor( dbFor(c.env), - { - workspace: record.workspace, - repo: record.repo, - ...(record.path ? { path: record.path } : {}), - ...(record.number > 0 ? { number: record.number } : {}), - }, + feedRecordScope(record), c.req.query("cursor"), ); return c.json(await hydratePublicFeed(c.env, workspace, record, { cursor })); diff --git a/apps/api/src/routes/workspace-scope.ts b/apps/api/src/routes/workspace-scope.ts index 85c0f58d..3c8403fd 100644 --- a/apps/api/src/routes/workspace-scope.ts +++ b/apps/api/src/routes/workspace-scope.ts @@ -105,6 +105,20 @@ async function resolvePullTitles( ); } +/** + * A PR title stored on the rollup row, served only when this workspace may see + * the repo's private titles (the repo is linked to it). A stored title may + * predate a link change or a repo going private, or come from another + * workspace that owned the row. + */ +function storedTitleFor( + linkedRepos: ReadonlySet, + repo: string, + title: string | null, +): string | null { + return linkedRepos.has(repo) ? title : null; +} + export async function pullsHandler(c: Context) { const workspace = c.get("workspaceName"); const db = dbFor(c.env); @@ -169,10 +183,7 @@ export async function pullsHandler(c: Context) { repo: row.repo, number: row.prNumber, branch: row.branch, - // A stored title may predate a link change or a repo going private, - // or come from another workspace that owned the row: serve it only - // when this workspace may see the repo's private titles. - title: info?.title ?? (linkedRepos.has(row.repo) ? row.title : null), + title: info?.title ?? storedTitleFor(linkedRepos, row.repo, row.title), state: info?.state ?? row.state, lastMediaAt: row.lastMediaAt, thumbnails: (thumbs[index]?.items ?? []).map((item) => toThumbItem(c.env, cfg, item)), @@ -278,10 +289,9 @@ export async function scopeFilesHandler(c: Context) { }); } - // Same rule as /pulls: a stored title is served only for a repo linked to - // this workspace. Otherwise `pull.title` is null and the PR page resolves - // it through the member titles route (public ladder for unlinked repos). - const titleVisible = pullRow?.title != null && (await linkedRepoSet(db, workspace)).has(repo); + // Same rule as /pulls. When `pull.title` is null the PR page resolves it + // through the member titles route (public ladder for unlinked repos). + const linkedRepos = await linkedRepoSet(db, workspace); const response: ScopeFilesResponse = { repo, @@ -293,7 +303,7 @@ export async function scopeFilesHandler(c: Context) { pull: pullRow ? { branch: pullRow.branch, - title: titleVisible ? pullRow.title : null, + title: storedTitleFor(linkedRepos, repo, pullRow.title), state: pullRow.state, } : null, diff --git a/apps/api/src/scope-service.ts b/apps/api/src/scope-service.ts index 70ccfac2..1f692b6d 100644 --- a/apps/api/src/scope-service.ts +++ b/apps/api/src/scope-service.ts @@ -6,6 +6,7 @@ */ import { fileTypeClassFromKey } from "@uploads/comment-render/scope"; import type { StorageConfig } from "@uploads/storage"; +import { contentTypeFromKey } from "./guards"; import { videoPresentation } from "./poster"; import type { ScopeItem } from "./pr-scope"; import type { ThumbItem } from "./scope-wire"; @@ -13,13 +14,12 @@ import { objectPublicUrls } from "./storage"; export function toThumbItem(env: Env, cfg: StorageConfig, item: ScopeItem): ThumbItem { const urls = objectPublicUrls(env, cfg, item.key); - const isPdf = item.key.toLowerCase().endsWith(".pdf"); const { posterUrl } = videoPresentation( env, cfg, item.key, item.metadata, - isPdf ? "application/pdf" : undefined, + contentTypeFromKey(item.key), ); return { key: item.key, diff --git a/apps/api/test/comment-render-scope.test.ts b/apps/api/test/comment-render-scope.test.ts index 1a7c997d..586a3df3 100644 --- a/apps/api/test/comment-render-scope.test.ts +++ b/apps/api/test/comment-render-scope.test.ts @@ -3,11 +3,11 @@ import { FILE_TYPE_CLASSES, IMAGE_EXTENSIONS, VIDEO_EXTENSIONS, + FEED_ITEM_ID_RE, feedItemIdFor, fileTypeClassFromKey, isInFeedScope, } from "@uploads/comment-render/scope"; -import { feedItemId } from "../src/feed-service"; import { sha256Hex } from "../src/workspace"; describe("fileTypeClassFromKey", () => { @@ -52,12 +52,12 @@ describe("isInFeedScope", () => { }); describe("feedItemIdFor", () => { - it("is the first 32 hex chars of sha256(key) and equals the API feedItemId", async () => { + it("is the first 32 hex chars of sha256(key) and matches FEED_ITEM_ID_RE", async () => { for (const key of ["gh/acme/app/pull/7/shot.png", "shots/café-née.png"]) { const id = await feedItemIdFor(key); expect(id).toMatch(/^[0-9a-f]{32}$/); expect(id).toBe((await sha256Hex(key)).slice(0, 32)); - expect(await feedItemId(key)).toBe(id); + expect(FEED_ITEM_ID_RE.test(id)).toBe(true); } }); }); diff --git a/apps/api/test/pr-scope-sqlite.test.ts b/apps/api/test/pr-scope-sqlite.test.ts index aef8eefa..59cd714d 100644 --- a/apps/api/test/pr-scope-sqlite.test.ts +++ b/apps/api/test/pr-scope-sqlite.test.ts @@ -255,7 +255,7 @@ describe("scanScopeKeys", () => { ]); expect(all[0]?.metadata).toEqual({}); expect(all[0]?.updatedAt).toBe("2026-10-01T00:00:06.000Z"); - const capped = await scanScopeKeys(db, { workspace: "alpha", repo: "acme/app" }, 5); + const capped = await scanScopeKeys(db, { workspace: "alpha", repo: "acme/app" }, { cap: 5 }); expect(capped).toHaveLength(5); } finally { sqlite.close(); diff --git a/packages/comment-render/src/scope.ts b/packages/comment-render/src/scope.ts index 4c5c1276..611556e7 100644 --- a/packages/comment-render/src/scope.ts +++ b/packages/comment-render/src/scope.ts @@ -57,6 +57,9 @@ interface WebCryptoGlobals { TextEncoder: new () => { encode(input: string): Uint8Array }; } +/** Shape of a live-link item id (`feedItemIdFor`): 32 lowercase hex chars. */ +export const FEED_ITEM_ID_RE = /^[0-9a-f]{32}$/; + /** Stable live-link item id: first 32 hex chars of SHA-256(object key). */ export async function feedItemIdFor(objectKey: string): Promise { const g = globalThis as unknown as WebCryptoGlobals; diff --git a/packages/uploads/src/comment-render-scope.generated.ts b/packages/uploads/src/comment-render-scope.generated.ts index 0cdc0aca..9618c900 100644 --- a/packages/uploads/src/comment-render-scope.generated.ts +++ b/packages/uploads/src/comment-render-scope.generated.ts @@ -57,6 +57,9 @@ interface WebCryptoGlobals { TextEncoder: new () => { encode(input: string): Uint8Array }; } +/** Shape of a live-link item id (`feedItemIdFor`): 32 lowercase hex chars. */ +export const FEED_ITEM_ID_RE = /^[0-9a-f]{32}$/; + /** Stable live-link item id: first 32 hex chars of SHA-256(object key). */ export async function feedItemIdFor(objectKey: string): Promise { const g = globalThis as unknown as WebCryptoGlobals; From eb46c7c19c06f1c6dbd4600ddfe14d7d815da112 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 17:44:53 -0400 Subject: [PATCH 13/14] perf(api): overlap scope route reads and batch list thumbnails --- apps/api/src/pr-scope.ts | 75 +++++++++--- apps/api/src/routes/workspace-scope.ts | 151 +++++++++++++++---------- apps/api/src/scope-service.ts | 3 + 3 files changed, 153 insertions(+), 76 deletions(-) diff --git a/apps/api/src/pr-scope.ts b/apps/api/src/pr-scope.ts index e40f2cd8..3cb6796d 100644 --- a/apps/api/src/pr-scope.ts +++ b/apps/api/src/pr-scope.ts @@ -111,11 +111,11 @@ export function clampScopeLimit(limit: number | undefined): number { return Math.max(1, Math.min(SCOPE_MAX_LIMIT, Math.floor(limit))); } -/** One newest-first page of the scope, with metadata for each item. */ -export async function prScopeQuery( +/** The SELECT for one newest-first page (`limit + 1` rows, to detect a next page). */ +function scopePageStatement( db: D1Queryable, q: ScopeQuery, -): Promise<{ items: ScopeItem[]; nextCursor: ScopeCursor | null }> { +): { statement: D1PreparedStatement; limit: number } { const limit = clampScopeLimit(q.limit); const { sql, params } = scopeFrom(q); let select = `SELECT r.object_key AS object_key, r.updated_at AS updated_at ${sql}`; @@ -128,14 +128,38 @@ export async function prScopeQuery( } select += ` ORDER BY r.updated_at DESC, r.object_key ASC LIMIT ?`; params.push(limit + 1); + return { statement: db.prepare(select).bind(...params), limit }; +} - const { results } = await db - .prepare(select) - .bind(...params) - .all(); - const rows = results ?? []; +function splitScopePage( + rows: ScopeRow[], + limit: number, +): { page: ScopeRow[]; nextCursor: ScopeCursor | null } { const hasMore = rows.length > limit; const page = hasMore ? rows.slice(0, limit) : rows; + const last = page.at(-1); + return { + page, + nextCursor: hasMore && last ? { updatedAt: last.updated_at, key: last.object_key } : null, + }; +} + +function toScopeItems(page: ScopeRow[], byKey: Map>): ScopeItem[] { + return page.map((row) => ({ + key: row.object_key, + updatedAt: row.updated_at, + metadata: byKey.get(row.object_key) ?? {}, + })); +} + +/** One newest-first page of the scope, with metadata for each item. */ +export async function prScopeQuery( + db: D1Queryable, + q: ScopeQuery, +): Promise<{ items: ScopeItem[]; nextCursor: ScopeCursor | null }> { + const { statement, limit } = scopePageStatement(db, q); + const { results } = await statement.all(); + const { page, nextCursor } = splitScopePage(results ?? [], limit); const byKey = page.length > 0 ? await getMetadataForKeys( @@ -144,15 +168,32 @@ export async function prScopeQuery( page.map((row) => row.object_key), ) : new Map>(); - const last = page.at(-1); - return { - items: page.map((row) => ({ - key: row.object_key, - updatedAt: row.updated_at, - metadata: byKey.get(row.object_key) ?? {}, - })), - nextCursor: hasMore && last ? { updatedAt: last.updated_at, key: last.object_key } : null, - }; + return { items: toScopeItems(page, byKey), nextCursor }; +} + +/** + * The first page of several scopes in one workspace: every page SELECT in one + * D1 batch, then one metadata lookup over the union of their keys limited to + * `metaKeys`. For list thumbnails (`/pulls`, `/repos`), one entry per scope. + */ +export async function prScopeFirstPages( + db: D1Queryable, + workspace: string, + scopes: Array>, + opts: { metaKeys: string[] }, +): Promise { + if (scopes.length === 0) return []; + const built = scopes.map((q) => scopePageStatement(db, { ...q, workspace })); + const results = await db.batch(built.map((entry) => entry.statement)); + const pages = results.map( + (result, index) => splitScopePage(result.results ?? [], built[index].limit).page, + ); + const keys = [...new Set(pages.flat().map((row) => row.object_key))]; + const byKey = + keys.length > 0 + ? await getMetadataForKeys(db, workspace, keys, { metaKeys: opts.metaKeys }) + : new Map>(); + return pages.map((page) => toScopeItems(page, byKey)); } /** diff --git a/apps/api/src/routes/workspace-scope.ts b/apps/api/src/routes/workspace-scope.ts index 3c8403fd..cf8b87d2 100644 --- a/apps/api/src/routes/workspace-scope.ts +++ b/apps/api/src/routes/workspace-scope.ts @@ -13,6 +13,7 @@ import { dualWorkspaceAuth, type DualAuthVars } from "../dual-workspace-auth"; import { respondError } from "../error-response"; import { countPrivateScopeItems, + PRIVATE_COUNT_PROBE_CAP, feedItemUrl, feedUrl, hydrateFeedItems, @@ -34,13 +35,14 @@ import { decodeScopeCursor, encodeScopeCursor, listWorkspaceRepos, + prScopeFirstPages, prScopeQuery, scanScopeKeys, SCOPE_DEFAULT_LIMIT, SCOPE_MAX_LIMIT, } from "../pr-scope"; import { heavyReadRateLimit } from "../read-limits"; -import { toThumbItem } from "../scope-service"; +import { THUMB_META_KEYS, toThumbItem } from "../scope-service"; import type { PullsResponse, ReposResponse, ScopeFilesResponse } from "../scope-wire"; import { storageConfig } from "../storage"; import { requireScope } from "../workspace"; @@ -119,6 +121,20 @@ function storedTitleFor( return linkedRepos.has(repo) ? title : null; } +/** + * Run best-effort `task` after the response via `waitUntil`. Awaits it when + * there is no ExecutionContext (vitest `app.request` supplies none). + */ +async function afterResponse(c: Context, task: Promise): Promise { + try { + c.executionCtx.waitUntil(task); + return; + } catch { + // No ExecutionContext: fall through and await. + } + await task; +} + export async function pullsHandler(c: Context) { const workspace = c.get("workspaceName"); const db = dbFor(c.env); @@ -133,46 +149,54 @@ export async function pullsHandler(c: Context) { // days unless `all=1` (the Files list's "Show older pull requests"). const since = pullsSince(c.req.query("all")); - const page = await boundedDataRead( - c, - () => listPrActivityPage(db, workspace, { repo, state, since, cursor, limit }), - { name: "d1_pulls_list" }, - ); - const linkedRepos = await linkedRepoSet(db, workspace); - const titles = await resolvePullTitles( - c.env, - page.rows.map((row) => row.ref), - linkedRepos, - ); - // Every resolved title here came from the member audience built from this - // workspace's own links, and the UPDATE is scoped to this workspace's rows. - await backfillPrActivityState( - db, - workspace, - page.rows.flatMap((row) => { - const info = titles[row.ref]; - return info && (info.title !== row.title || info.state !== row.state) - ? [{ ref: row.ref, title: info.title, state: info.state }] - : []; - }), - ); - const thumbs = await boundedDataRead( - c, - () => - Promise.all( - page.rows.map((row) => - prScopeQuery(db, { - workspace, + const [page, linkedRepos, cfg] = await Promise.all([ + boundedDataRead( + c, + () => listPrActivityPage(db, workspace, { repo, state, since, cursor, limit }), + { name: "d1_pulls_list" }, + ), + linkedRepoSet(db, workspace), + storageConfig(c.env, c.get("workspace")), + ]); + const [titles, thumbs] = await Promise.all([ + resolvePullTitles( + c.env, + page.rows.map((row) => row.ref), + linkedRepos, + ), + boundedDataRead( + c, + () => + prScopeFirstPages( + db, + workspace, + page.rows.map((row) => ({ repo: row.repo, number: row.prNumber, type, limit: THUMBNAIL_LIMIT, - }), + })), + { metaKeys: THUMB_META_KEYS }, ), - ), - { name: "d1_pulls_thumbs" }, + { name: "d1_pulls_thumbs" }, + ), + ]); + // Best effort, off the response path. Every resolved title here came from + // the member audience built from this workspace's own links, and the UPDATE + // is scoped to this workspace's rows. + await afterResponse( + c, + backfillPrActivityState( + db, + workspace, + page.rows.flatMap((row) => { + const info = titles[row.ref]; + return info && (info.title !== row.title || info.state !== row.state) + ? [{ ref: row.ref, title: info.title, state: info.state }] + : []; + }), + ), ); - const cfg = await storageConfig(c.env, c.get("workspace")); const response: PullsResponse = { workspace, @@ -186,7 +210,7 @@ export async function pullsHandler(c: Context) { title: info?.title ?? storedTitleFor(linkedRepos, row.repo, row.title), state: info?.state ?? row.state, lastMediaAt: row.lastMediaAt, - thumbnails: (thumbs[index]?.items ?? []).map((item) => toThumbItem(c.env, cfg, item)), + thumbnails: (thumbs[index] ?? []).map((item) => toThumbItem(c.env, cfg, item)), }; }), nextCursor: page.nextCursor ? encodeScopeCursor(page.nextCursor) : null, @@ -202,24 +226,27 @@ export async function reposHandler(c: Context) { const type = parseFileTypeQuery(c.req.query("type")); const cursor = decodeScopeCursor(c.req.query("cursor")); - const page = await boundedDataRead( - c, - () => listWorkspaceRepos(db, workspace, { cursor, limit }), - { name: "d1_repos_list" }, - ); + const [page, cfg] = await Promise.all([ + boundedDataRead(c, () => listWorkspaceRepos(db, workspace, { cursor, limit }), { + name: "d1_repos_list", + }), + storageConfig(c.env, c.get("workspace")), + ]); const repos = page.repos.map((row) => row.repo); const [openCounts, thumbs] = await boundedDataRead( c, () => Promise.all([ countOpenPullsByRepo(db, workspace, repos), - Promise.all( - repos.map((repo) => prScopeQuery(db, { workspace, repo, type, limit: THUMBNAIL_LIMIT })), + prScopeFirstPages( + db, + workspace, + repos.map((repo) => ({ repo, type, limit: THUMBNAIL_LIMIT })), + { metaKeys: THUMB_META_KEYS }, ), ]), { name: "d1_repos_detail" }, ); - const cfg = await storageConfig(c.env, c.get("workspace")); const response: ReposResponse = { workspace, @@ -227,7 +254,7 @@ export async function reposHandler(c: Context) { repo: row.repo, lastUpdatedAt: row.lastUpdatedAt, openPullCount: openCounts.get(row.repo) ?? 0, - thumbnails: (thumbs[index]?.items ?? []).map((item) => toThumbItem(c.env, cfg, item)), + thumbnails: (thumbs[index] ?? []).map((item) => toThumbItem(c.env, cfg, item)), })), nextCursor: page.nextCursor ? encodeScopeCursor(page.nextCursor) : null, }; @@ -255,34 +282,41 @@ export async function scopeFilesHandler(c: Context) { const limit = parseLimit(c.req.query("limit"), SCOPE_DEFAULT_LIMIT, SCOPE_MAX_LIMIT); const scope = { workspace, repo, ...(number > 0 ? { number } : {}) }; - const [page, pullRow] = await boundedDataRead( + const [page, pullRow, feed, linkedRepos] = await boundedDataRead( c, () => Promise.all([ prScopeQuery(db, { ...scope, type, cursor, limit }), number > 0 ? getPrActivityRow(db, workspace, repo, number) : Promise.resolve(null), + findFeedByScope(db, workspace, repo, "", number), + linkedRepoSet(db, workspace), ]), { name: "d1_scope_files" }, ); + + // privateCount is first page only: the share confirm reads it before the + // first copy. The scan runs alongside hydration. Its cap is the probe cap + // plus the page plus one: past that, more than PRIVATE_COUNT_PROBE_CAP keys + // would need a probe, so the count is null whether or not the scan stops. + const needsScan = !cursor && !(page.nextCursor === null && type === undefined); const privateKeys = new Set(); - const items = await hydrateFeedItems(c.env, record, page.items, { - audience: "owner", - privateKeys, - }); - const feed = await findFeedByScope(db, workspace, repo, "", number); + const [items, scanned] = await Promise.all([ + hydrateFeedItems(c.env, record, page.items, { audience: "owner", privateKeys }), + needsScan + ? boundedDataRead( + c, + () => scanScopeKeys(db, scope, { cap: PRIVATE_COUNT_PROBE_CAP + page.items.length + 1 }), + { name: "d1_scope_scan" }, + ) + : null, + ]); if (feed) { for (const item of items) item.pageUrl = feedItemUrl(c.env, feed.id, item.id); } - // First page only: the share confirm reads it before the first copy. let privateCount: number | null = null; if (!cursor) { - const pageIsWholeScope = page.nextCursor === null && type === undefined; - const keys = pageIsWholeScope - ? page.items.map((item) => item.key) - : (await boundedDataRead(c, () => scanScopeKeys(db, scope), { name: "d1_scope_scan" })).map( - (item) => item.key, - ); + const keys = (scanned ?? page.items).map((item) => item.key); privateCount = await countPrivateScopeItems(c.env, record, keys, { checked: new Set(page.items.map((item) => item.key)), privateKeys, @@ -291,7 +325,6 @@ export async function scopeFilesHandler(c: Context) { // Same rule as /pulls. When `pull.title` is null the PR page resolves it // through the member titles route (public ladder for unlinked repos). - const linkedRepos = await linkedRepoSet(db, workspace); const response: ScopeFilesResponse = { repo, diff --git a/apps/api/src/scope-service.ts b/apps/api/src/scope-service.ts index 1f692b6d..89f62f8b 100644 --- a/apps/api/src/scope-service.ts +++ b/apps/api/src/scope-service.ts @@ -12,6 +12,9 @@ import type { ScopeItem } from "./pr-scope"; import type { ThumbItem } from "./scope-wire"; import { objectPublicUrls } from "./storage"; +/** The only metadata `toThumbItem` reads: the poster flags. */ +export const THUMB_META_KEYS = ["video.poster", "pdf.poster"]; + export function toThumbItem(env: Env, cfg: StorageConfig, item: ScopeItem): ThumbItem { const urls = objectPublicUrls(env, cfg, item.key); const { posterUrl } = videoPresentation( From 7df2056ea145843dad7f34ad19df173a03ac7d97 Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Sun, 4 Oct 2026 17:46:28 -0400 Subject: [PATCH 14/14] fix(api): count every live repo-scoped feed toward the 50-feed cap --- .../20261004120000_feeds_source.sql | 5 +++-- apps/api/src/feeds.ts | 21 ++++++++++--------- apps/api/test/feeds-sqlite.test.ts | 14 +++++++++---- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/apps/api/migrations/20261004120000_feeds_source.sql b/apps/api/migrations/20261004120000_feeds_source.sql index 5a2fa1b1..af00c366 100644 --- a/apps/api/migrations/20261004120000_feeds_source.sql +++ b/apps/api/migrations/20261004120000_feeds_source.sql @@ -1,7 +1,8 @@ -- Who created a live feed row: 'comment' (GitHub comment sync) or 'user' -- (web, CLI, MCP, plugin). NULL on rows created before this column. Set on -- insert only: a reused scope keeps its first source. The per-workspace cap --- (apps/api/src/feeds.ts) counts only 'user' repo-scoped rows (50); PR and --- issue feeds are uncapped, so comment sync never falls back to /f/ links. +-- (apps/api/src/feeds.ts) counts live repo-scoped rows of any source (50) and +-- applies only to user creates; PR and issue feeds are uncapped, so comment +-- sync never falls back to /f/ links. ALTER TABLE feeds ADD COLUMN source TEXT CHECK (source IS NULL OR source IN ('comment', 'user')); diff --git a/apps/api/src/feeds.ts b/apps/api/src/feeds.ts index dd0e9017..51ec3fd5 100644 --- a/apps/api/src/feeds.ts +++ b/apps/api/src/feeds.ts @@ -8,10 +8,10 @@ import { type D1Queryable } from "./db-session"; import type { FeedSource } from "./scope-wire"; /** - * Live repo-scoped feeds (`number = 0`) with `source = 'user'` per workspace. - * PR- and issue-scoped feeds (`number > 0`) are uncapped from every source + * Live repo-scoped feeds (`number = 0`) per workspace, whatever their source + * (legacy NULL-source rows included). Only user creates are checked against + * it. PR- and issue-scoped feeds (`number > 0`) are uncapped from every source * (API, CLI `gh` fallback, GitHub App): they are one row per real PR or issue. - * Repo-scoped feeds created by comment sync are uncapped too. */ export const MAX_FEEDS_PER_WORKSPACE = 50; export const MAX_FEED_PAGE_SIZE = 100; @@ -227,14 +227,15 @@ export async function findFeedByRepoPath( } /** - * Live user-created repo-scoped feeds (`number = 0`). Comment-sync rows, - * legacy (NULL) rows, and PR/issue-scoped rows do not count. + * Live repo-scoped feeds (`number = 0`), whatever their source. Comment sync + * only creates `number > 0` rows, so in practice these are user-created and + * legacy (NULL-source) repo feeds. PR/issue-scoped rows do not count. */ -async function countUserRepoFeeds(db: D1Queryable, workspace: string): Promise { +async function countRepoFeeds(db: D1Queryable, workspace: string): Promise { const found = await db .prepare( `SELECT COUNT(*) AS count FROM feeds - WHERE workspace = ? AND deleted_at IS NULL AND source = 'user' AND number = 0`, + WHERE workspace = ? AND deleted_at IS NULL AND number = 0`, ) .bind(workspace) .first<{ count: number }>(); @@ -273,13 +274,13 @@ export async function createFeed( // One live row per scope whatever the source: the lookup above already // returned an existing comment-sync row to a user caller without touching - // the cap. Only user-created repo-scoped feeds are capped; PR/issue-scoped - // feeds (number > 0) are uncapped whatever the source. + // the cap. Only user creates of repo-scoped feeds are checked against it; + // PR/issue-scoped feeds (number > 0) are uncapped whatever the source. const source: FeedSource = input.source ?? "user"; if ( source === "user" && scopeResult.value.number === 0 && - (await countUserRepoFeeds(db, input.workspace)) >= MAX_FEEDS_PER_WORKSPACE + (await countRepoFeeds(db, input.workspace)) >= MAX_FEEDS_PER_WORKSPACE ) { return { status: "limit", limit: MAX_FEEDS_PER_WORKSPACE }; } diff --git a/apps/api/test/feeds-sqlite.test.ts b/apps/api/test/feeds-sqlite.test.ts index 396ca5a9..87f76391 100644 --- a/apps/api/test/feeds-sqlite.test.ts +++ b/apps/api/test/feeds-sqlite.test.ts @@ -330,7 +330,7 @@ describe("feed persistence against SQLite", () => { const counted = sqlite.db .prepare( `SELECT COUNT(*) AS n FROM feeds - WHERE workspace = 'alpha' AND deleted_at IS NULL AND source = 'user' AND number = 0`, + WHERE workspace = 'alpha' AND deleted_at IS NULL AND number = 0`, ) .get() as { n: number }; expect(counted.n).toBe(MAX_FEEDS_PER_WORKSPACE); @@ -376,23 +376,29 @@ describe("feed persistence against SQLite", () => { } }); - it("maps legacy NULL-source rows to null and leaves them out of the cap", async () => { + it("maps legacy NULL-source rows to null and counts repo-scoped ones toward the cap", async () => { const sqlite = newSqlite(); try { const db = database(sqlite); sqlite.db .prepare( `INSERT INTO feeds (id, workspace, repo, path, number, kind, created_at, updated_at, deleted_at) - VALUES ('feed_legacylegacylegacy0000', 'alpha', 'acme/old', '', 0, '', '2026-09-20T00:00:00Z', '2026-09-20T00:00:00Z', NULL)`, + VALUES ('feed_legacylegacylegacy0000', 'alpha', 'acme/old', '', 0, '', '2026-09-20T00:00:00Z', '2026-09-20T00:00:00Z', NULL), + ('feed_legacylegacylegacy0001', 'alpha', 'acme/old', '', 5, 'pull', '2026-09-20T00:00:00Z', '2026-09-20T00:00:00Z', NULL)`, ) .run(); expect(await getFeed(db, "alpha", "feed_legacylegacylegacy0000")).toMatchObject({ source: null, }); - for (let i = 0; i < MAX_FEEDS_PER_WORKSPACE; i++) { + // The legacy repo-scoped row takes one of the 50 slots; the legacy PR row does not. + for (let i = 0; i < MAX_FEEDS_PER_WORKSPACE - 1; i++) { const created = await createFeed(db, { workspace: "alpha", repo: `acme/app${i}` }); expect(created.status).toBe("ok"); } + expect(await createFeed(db, { workspace: "alpha", repo: "acme/overflow" })).toEqual({ + status: "limit", + limit: MAX_FEEDS_PER_WORKSPACE, + }); } finally { sqlite.close(); }