Public: anyone can read this; the crew can change it.
A repository's activity feed names its Server-Timing phases and answers a feed reader 304 when nothing changed #3757
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3753-a-repository-s-activity-feed-names-its-s → main 3 commits fork 3 threads · 3 unresolved
2 files
changed +141
−19
src/web/feeds.ts +56 −18
| @@ -20,6 +20,7 @@ import { listEvents, type ForgeEvent } from "../core/events.ts"; | ||
| 20 | 20 | import { getIssuesByRef } from "../core/issues.ts"; |
| 21 | 21 | import { canRead, forkParentFor, getRepo } from "../core/repos.ts"; |
| 22 | 22 | import { getUsersByIds } from "../core/users.ts"; |
| 23 | import { notePhase, takeSessionTime, timePhase } from "../core/route_timing.ts"; | |
| 23 | 24 | import type { Ctx } from "../http/context.ts"; |
| 24 | 25 | import { notFound } from "../http/respond.ts"; |
| 25 | 26 | |
| @@ -63,26 +64,63 @@ function pushedBranch(event: ForgeEvent): string { | ||
| 63 | 64 | return typeof ref === "string" ? ref.replace(/^refs\/heads\//, "") : ""; |
| 64 | 65 | } |
| 65 | 66 | |
| 67 | /** Does an `If-None-Match` header name this tag (weakly, or as `*`)? */ | |
| 68 | function holds(header: string | null, etag: string): boolean { | |
| 69 | if (header === null) return false; | |
| 70 | return header | |
| 71 | .split(",") | |
| 72 | .map((t) => t.trim().replace(/^W\//, "")) | |
| 73 | .some((t) => t === "*" || t === etag.replace(/^W\//, "")); | |
| 74 | } | |
| 75 | ||
| 66 | 76 | export function activityAtom(ctx: Ctx): Response { |
| 77 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); | |
| 67 | 78 | const repo = getRepo(ctx.app.db, ctx.params["owner"]!, ctx.params["name"]!); |
| 68 | 79 | if (!repo || !canRead(ctx.app.db, repo, ctx.auth.user)) return notFound(); |
| 69 | 80 | const origin = ctx.url.origin; |
| 70 | 81 | const base = `${origin}/${repo.ownerName}/${repo.name}`; |
| 71 | const events = listEvents(ctx.app.db, { repoId: repo.id, limit: 200 }) | |
| 72 | .filter((e) => FEED_KINDS.has(e.kind)) | |
| 73 | .filter(audienceFilter(ctx.app.db, ctx.auth.user)) | |
| 74 | .slice(0, FEED_ENTRIES); | |
| 75 | const actors = getUsersByIds( | |
| 76 | ctx.app.db, | |
| 77 | events.map((e) => e.actorId ?? "").filter(Boolean), | |
| 78 | ); | |
| 79 | const issues = getIssuesByRef( | |
| 80 | ctx.app.db, | |
| 81 | events | |
| 82 | .map((e) => e.payload["number"]) | |
| 83 | .filter((n): n is number => typeof n === "number") | |
| 84 | .map((number) => ({ repoId: repo.id, number })), | |
| 82 | const events = timePhase(ctx.request, "events", () => | |
| 83 | listEvents(ctx.app.db, { repoId: repo.id, limit: 200 }) | |
| 84 | .filter((e) => FEED_KINDS.has(e.kind)) | |
| 85 | .filter(audienceFilter(ctx.app.db, ctx.auth.user)) | |
| 86 | .slice(0, FEED_ENTRIES), | |
| 85 | 87 | ); |
| 88 | const { actors, issues } = timePhase(ctx.request, "people", () => ({ | |
| 89 | actors: getUsersByIds( | |
| 90 | ctx.app.db, | |
| 91 | events.map((e) => e.actorId ?? "").filter(Boolean), | |
| 92 | ), | |
| 93 | issues: getIssuesByRef( | |
outdated
The cache-control is shared-cacheable for public repos while the body differs by audience. Add |
||
| 94 | ctx.app.db, | |
| 95 | events | |
| 96 | .map((e) => e.payload["number"]) | |
| 97 | .filter((n): n is number => typeof n === "number") | |
| 98 | .map((number) => ({ repoId: repo.id, number })), | |
| 99 | ), | |
| 100 | })); | |
| 101 | // The newest event the reader may see, who the reader is (a writer and a | |
| 102 | // stranger see different lists), and the names and titles the body will | |
| 103 | // print, which can change with no event. | |
| 104 | const shown = Bun.hash( | |
| 105 | JSON.stringify([ | |
| 106 | repo.ownerName, | |
| 107 | repo.name, | |
| 108 | repo.description, | |
| 109 | [...actors.values()].map((u) => [u.id, u.username]), | |
outdated
Add repo.description and the repo name to this hashed list. <subtitle> and <title> print them, so a description edit currently yields a stale 304. Add a test for it. The tag omits whether forkParentFor resolves (body prints the parent name only when readable). Put that boolean into the hashed array so a permission change invalidates the tag. |
||
| 110 | [...issues.values()].map((i) => [i.number, i.title]), | |
| 111 | ]), | |
| 112 | ).toString(36); | |
| 113 | const etag = `W/"${events[0]?.id ?? "none"}.${ctx.auth.user?.id ?? "anon"}.${shown}"`; | |
| 114 | const cacheControl = | |
| 115 | repo.visibility === "public" ? "public, max-age=300" : "private, no-store"; | |
| 116 | // The body differs by who asks, so a shared cache must key on them. | |
| 117 | const vary = "authorization, cookie"; | |
| 118 | if (holds(ctx.request.headers.get("if-none-match"), etag)) | |
| 119 | return new Response(null, { | |
| 120 | status: 304, | |
| 121 | headers: { etag, "cache-control": cacheControl, vary }, | |
| 122 | }); | |
| 123 | const renderStart = performance.now(); | |
| 86 | 124 | |
| 87 | 125 | const entries = events.map((event) => { |
| 88 | 126 | const who = eventActor( |
| @@ -135,15 +173,15 @@ export function activityAtom(ctx: Ctx): Response { | ||
| 135 | 173 | ${entries.join("\n")} |
| 136 | 174 | </feed> |
| 137 | 175 | `; |
| 176 | notePhase(ctx.request, "render", performance.now() - renderStart); | |
| 138 | 177 | return new Response(body, { |
| 139 | 178 | headers: { |
| 140 | 179 | "content-type": "application/atom+xml; charset=utf-8", |
| 141 | 180 | // A public feed may be cached briefly by anyone; a private one by |
| 142 | 181 | // nobody but the reader who asked. |
| 143 | "cache-control": | |
| 144 | repo.visibility === "public" | |
| 145 | ? "public, max-age=300" | |
| 146 | : "private, no-store", | |
| 182 | "cache-control": cacheControl, | |
| 183 | etag, | |
| 184 | vary, | |
| 147 | 185 | }, |
| 148 | 186 | }); |
| 149 | 187 | } |
test/feeds.test.ts +85 −1
| @@ -8,13 +8,16 @@ import { mkdtemp, rm } from "node:fs/promises"; | ||
| 8 | 8 | import { tmpdir } from "node:os"; |
| 9 | 9 | import { join } from "node:path"; |
| 10 | 10 | import { createApp, type App } from "../src/app.ts"; |
| 11 | import { openIssue } from "../src/actions/issues.ts"; | |
| 11 | import { editIssue, openIssue } from "../src/actions/issues.ts"; | |
| 12 | 12 | import { |
| 13 | 13 | commitToBranch, |
| 14 | 14 | createRepo, |
| 15 | 15 | createWebhook, |
| 16 | 16 | forkRepo, |
| 17 | updateRepoSettings, | |
| 17 | 18 | } from "../src/actions/repos.ts"; |
| 19 | import { getIssue } from "../src/core/issues.ts"; | |
| 20 | import { getRepo } from "../src/core/repos.ts"; | |
| 18 | 21 | import { createToken } from "../src/core/tokens.ts"; |
| 19 | 22 | import { createUser, type User } from "../src/core/users.ts"; |
| 20 | 23 | import { serve, type Servers } from "../src/http/server.ts"; |
| @@ -169,6 +172,87 @@ describe("a repository's feed", () => { | ||
| 169 | 172 | ); |
| 170 | 173 | }); |
| 171 | 174 | |
| 175 | test("it names its Server-Timing phases", async () => { | |
| 176 | const res = await fetch(`${base}/alice/game/activity.atom`); | |
| 177 | const timing = res.headers.get("server-timing") ?? ""; | |
| 178 | for (const name of ["session", "events", "people", "render"]) | |
| 179 | expect(timing).toContain(`${name};dur=`); | |
| 180 | }); | |
| 181 | ||
| 182 | test("a repeat request with the ETag answers 304, and a new event a 200", async () => { | |
| 183 | const url = `${base}/alice/game/activity.atom`; | |
| 184 | const first = await fetch(url); | |
| 185 | const etag = first.headers.get("etag")!; | |
| 186 | expect(etag).toMatch(/^W\//); | |
| 187 | expect(first.headers.get("vary")).toBe("authorization, cookie"); | |
| 188 | await first.text(); | |
| 189 | const again = await fetch(url, { headers: { "if-none-match": etag } }); | |
| 190 | expect(again.status).toBe(304); | |
| 191 | expect(await again.text()).toBe(""); | |
| 192 | expect(again.headers.get("vary")).toBe("authorization, cookie"); | |
| 193 | // A different audience never shares the tag. | |
| 194 | const token = createToken(app.db, alice.id, "t", "ro").token; | |
| 195 | const mine = await fetch(url, { | |
| 196 | headers: { authorization: `Bearer ${token}` }, | |
| 197 | }); | |
| 198 | expect(mine.headers.get("etag")).not.toBe(etag); | |
| 199 | await mine.text(); | |
| 200 | const repo = getRepo(app.db, "alice", "game")!; | |
| 201 | const made = openIssue(app, bob, repo, { title: "a fresh one" }); | |
| 202 | if (!made.ok) throw new Error(String(made.error)); | |
| 203 | const after = await fetch(url, { headers: { "if-none-match": etag } }); | |
| 204 | expect(after.status).toBe(200); | |
| 205 | expect(after.headers.get("etag")).not.toBe(etag); | |
| 206 | expect(await after.text()).toContain("a fresh one"); | |
| 207 | }); | |
| 208 | ||
| 209 | test("a retitled issue changes the tag though no event arrived", async () => { | |
| 210 | const url = `${base}/alice/game/activity.atom`; | |
| 211 | const first = await fetch(url); | |
| 212 | const etag = first.headers.get("etag")!; | |
| 213 | await first.text(); | |
| 214 | const repo = getRepo(app.db, "alice", "game")!; | |
| 215 | const issue = getIssue(app.db, repo.id, 1)!; | |
| 216 | const edited = editIssue(app, bob, repo, issue, { title: "renamed quietly" }); | |
| 217 | if (!edited.ok) throw new Error(String(edited.error)); | |
| 218 | const after = await fetch(url, { headers: { "if-none-match": etag } }); | |
| 219 | expect(after.status).toBe(200); | |
| 220 | expect(await after.text()).toContain("renamed quietly"); | |
| 221 | }); | |
| 222 | ||
| 223 | test("an edited description changes the tag though no event arrived", async () => { | |
| 224 | const url = `${base}/alice/game/activity.atom`; | |
| 225 | const first = await fetch(url); | |
| 226 | const etag = first.headers.get("etag")!; | |
| 227 | await first.text(); | |
| 228 | const repo = getRepo(app.db, "alice", "game")!; | |
| 229 | const done = await updateRepoSettings(app, alice, repo, { | |
| 230 | description: "a quieter subtitle", | |
| 231 | }); | |
| 232 | if (!done.ok) throw new Error(String(done.error)); | |
| 233 | const after = await fetch(url, { headers: { "if-none-match": etag } }); | |
| 234 | expect(after.status).toBe(200); | |
| 235 | expect(await after.text()).toContain("<subtitle>a quieter subtitle</subtitle>"); | |
| 236 | }); | |
| 237 | ||
| 238 | test("a matching If-None-Match never gets a 304 before the read check", async () => { | |
| 239 | const alices = createToken(app.db, alice.id, "t", "ro").token; | |
| 240 | const mine = await fetch(`${base}/alice/secret/activity.atom`, { | |
| 241 | headers: { authorization: `Bearer ${alices}` }, | |
| 242 | }); | |
| 243 | const etag = mine.headers.get("etag")!; | |
| 244 | expect(etag).toBeTruthy(); | |
| 245 | await mine.text(); | |
| 246 | for (const headers of [ | |
| 247 | { "if-none-match": etag }, | |
| 248 | { "if-none-match": "*" }, | |
| 249 | ]) | |
| 250 | expect( | |
| 251 | (await fetch(`${base}/alice/secret/activity.atom`, { headers })) | |
| 252 | .status, | |
| 253 | ).toBe(404); | |
| 254 | }); | |
| 255 | ||
| 172 | 256 | test("the repository's page advertises its feed", async () => { |
| 173 | 257 | const html = await fetch(`${base}/alice/game`).then((r) => r.text()); |
| 174 | 258 | expect(html).toMatch( |
new above
offline
Tag = newest event id + user id. Issue titles and actor names rendered into the body can change with no new event, so a reader keeps getting 304 on stale content. Include the max updated-at of the issues/actors you render (compute them before the 304 check).