From 2d6efb50019446b7366fce4c9dfe32fecd2a614d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 02:42:17 -0400 Subject: [PATCH 1/5] The megabyte nobody asked for, and the click that now answers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five frontend-performance findings, each opened at its cited line and confirmed before it was changed. A sixth was written, measured, and taken back out. WHAT A PILOT USER GETS · Nobody downloads a spreadsheet engine to read a filename. SheetJS is 896 KB raw / 225 KB gzipped (measured on node_modules/xlsx/xlsx.mjs) and was a top-level import in two client components, so it shipped with the documents LIST — via DocumentRow -> DocumentViewerOverlay — and with every club's finance page. Both now `await import("xlsx")` at the moment somebody picks a file or saves a workbook. The client module graph now reaches no document parser at all. · Twenty avatars cost twenty round trips instead of twenty every time. Both image proxies answered a 307 with no freshness directive, which is not storable, so /messages re-ran auth + sharesAnInstitution + findUnique + an S3 presign per face on every load. They now carry `private, max-age=300` — half the presign's own 600s life, so no window widens — and the refusals deliberately do not. · The nav entry you clicked says so. `active` comes from usePathname, which does not move until the navigation COMMITS, so between the click and the answer the shell marked the page being left and the entry just clicked showed nothing. It now carries the useLinkStatus mark RangeFilter already uses. · Three force-dynamic pages stopped waiting on themselves. /feed, /messages and /settings awaited independent reads in series. /messages also read all fourteen scalar columns of every Organization at the institution to print a name; /settings read the same Institution row twice for a delegating OSE Director. · A tenant's manifest is read once per render, not twice, on seven gated surfaces. · A tab nobody is looking at asks for nothing. The notification bell polled every 30s regardless of document.visibilityState, on every page, for the life of a session. WHAT WAS WITHDRAWN, AND WHY The obvious fix for the dead-click — one app/(app)/loading.tsx covering forty routes — was written and then measured on a standalone Next 15.5.20 app built for the question. Two pages with byte-identical bodies, differing only in whether a sibling loading.tsx existed: notFound() without a boundary -> 404 WITH a boundary -> 200 redirect() without a boundary -> 307 WITH a boundary -> 200, no Location Forty-one pages in this group refuse with notFound(). A group-level loading.tsx turns every one of those refusals into a 200 — the product answering "you may not see this club" with "success". It is deterministic, not a race: 200 whether the page refuses before or after the layout's own awaits resolve. So the file is not here. A test is, carrying the measurement and forbidding the next one. CONTROLS Four suites, 29 tests, every one mutation-proved. Two mutants survived a first draft and the guards were widened until they did not. --- ...ding-boundary-swallows-the-refusal.test.ts | 131 ++++++++++ apps/web/src/app/(app)/feed/page.tsx | 87 ++++--- apps/web/src/app/(app)/messages/page.tsx | 82 +++--- apps/web/src/app/(app)/settings/page.tsx | 59 +++-- .../an-image-proxy-answer-is-reusable.test.ts | 204 +++++++++++++++ .../src/app/api/org-image/[orgId]/route.ts | 6 +- .../app/api/profile-image/[userId]/route.ts | 9 +- ...parser-does-not-ship-with-the-page.test.ts | 245 ++++++++++++++++++ .../documents/DocumentViewerOverlay.tsx | 48 +++- .../src/components/finance/BudgetUpload.tsx | 33 ++- .../src/components/shell/NotificationBell.tsx | 39 ++- apps/web/src/components/shell/SideNav.tsx | 41 ++- .../shell/a-hidden-tab-stops-asking.test.tsx | 189 ++++++++++++++ .../src/lib/capability-registry/manifest.ts | 31 ++- apps/web/src/lib/storage/image-proxy-cache.ts | 39 +++ 15 files changed, 1145 insertions(+), 98 deletions(-) create mode 100644 apps/web/src/app/(app)/a-loading-boundary-swallows-the-refusal.test.ts create mode 100644 apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts create mode 100644 apps/web/src/components/a-heavy-parser-does-not-ship-with-the-page.test.ts create mode 100644 apps/web/src/components/shell/a-hidden-tab-stops-asking.test.tsx create mode 100644 apps/web/src/lib/storage/image-proxy-cache.ts diff --git a/apps/web/src/app/(app)/a-loading-boundary-swallows-the-refusal.test.ts b/apps/web/src/app/(app)/a-loading-boundary-swallows-the-refusal.test.ts new file mode 100644 index 00000000..a719116e --- /dev/null +++ b/apps/web/src/app/(app)/a-loading-boundary-swallows-the-refusal.test.ts @@ -0,0 +1,131 @@ +import { readFileSync, readdirSync, statSync } from "node:fs" +import path from "node:path" + +/** + * Why the app shell has no `loading.tsx`, written down so nobody adds one. + * + * ── The problem that wants one ────────────────────────────────────────────── + * + * It is real and it is not fixed. Every route in this group is `force-dynamic` + * and several resolve through many sequential awaits before emitting a byte. + * Next keeps the PREVIOUS page fully on screen for that entire time, and the + * side nav cannot help either — `usePathname` does not change until the + * navigation commits — so on campus wifi the officer who clicked Approvals sees + * an unchanged Dashboard and clicks again. `SideNav`'s `useLinkStatus` mark now + * answers the click; the content region still does not. + * + * ── Why the obvious fix cannot ship as-is ─────────────────────────────────── + * + * MEASURED on a standalone Next 15.5.20 app built for this question — two + * pages, byte-identical bodies, differing only in whether a sibling + * `loading.tsx` existed: + * + * notFound() without a boundary → HTTP 404 + * notFound() WITH a boundary → HTTP 200 (not-found UI in the body) + * redirect(…) without a boundary → HTTP 307 + Location + * redirect(…) WITH a boundary → HTTP 200, no Location header + * + * The mechanism is visible in Next's own source. `renderToInitialFizzStream` + * awaits `ReactDOMServer.renderToReadableStream`, which resolves at SHELL + * ready, and `continueFizzStream` only awaits `allReady` when + * `isStaticGeneration` — which a `force-dynamic` route never is. With a + * `loading.tsx` the shell is layout + fallback, so it is ready and the 200 is + * committed before the page has run. The refusal then arrives inside a boundary + * that has already flushed, and app-render's handler never sees it — its own + * comment says so: "If a bailout made it to this point, it means it wasn't + * wrapped inside a suspense boundary." + * + * Forty-one pages in this group refuse with `notFound()`, and all but one of + * the forty-two use `notFound()` or `redirect()`. A group-level `loading.tsx` + * would turn every one of those refusals into a 200. That is not a test + * artifact; it is the product answering "you may not see this club" with + * "success". + * + * ── What the real fix needs ───────────────────────────────────────────────── + * + * The refusal has to be decided ABOVE the boundary — in `(app)/layout.tsx`, in + * middleware, or by a route the boundary does not cover — or the wait has to be + * expressed some other way (a Suspense boundary INSIDE the page, below its + * authorization checks, which is per-page work rather than one file). Either is + * a design change, not the one-line file this test forbids. + */ + +const APP_DIR = path.resolve(__dirname, "../../app") + +/** + * `reports/loading.tsx` predates this and is WRONG in exactly the way described + * above — `reports/page.tsx:46` and `reports/finance/page.tsx:34` both call + * `notFound()` for a non-OSE viewer, and both therefore answer 200 today. It is + * listed rather than deleted because removing it is its own decision with its + * own visual consequence, and nothing here measured what /reports looks like + * without it. It is the exception, not the precedent. + */ +const PRE_EXISTING = ["(app)/reports"] + +/** Directories holding a `loading.tsx`, relative to `app/`. */ +function loadingBoundaries(): string[] { + const out: string[] = [] + const walk = (dir: string) => { + const entries = readdirSync(dir) + if (entries.includes("loading.tsx")) out.push(path.relative(APP_DIR, dir) || "/") + for (const entry of entries) { + const child = path.join(dir, entry) + if (statSync(child).isDirectory()) walk(child) + } + } + walk(APP_DIR) + return out.sort() +} + +/** Pages at or below `dir` that refuse by throwing, and would lose their status. */ +function refusalsUnder(dir: string): string[] { + const out: string[] = [] + const walk = (d: string) => { + for (const entry of readdirSync(d)) { + const child = path.join(d, entry) + if (statSync(child).isDirectory()) walk(child) + else if (entry === "page.tsx") { + const src = readFileSync(child, "utf8") + if (/\bnotFound\(\)|\bredirect\(/.test(src)) out.push(path.relative(APP_DIR, child)) + } + } + } + walk(path.join(APP_DIR, dir === "/" ? "" : dir)) + return out.sort() +} + +describe("a loading boundary must not sit above a refusal", () => { + it("keeps the app shell free of a group-level loading.tsx", () => { + // The specific file this whole note is about. It was written, measured, + // and taken back out. + expect(loadingBoundaries()).not.toContain("(app)") + }) + + it("adds no boundary above a page that refuses by throwing", () => { + const offending = loadingBoundaries() + .filter((dir) => !PRE_EXISTING.includes(dir)) + .flatMap((dir) => refusalsUnder(dir).map((page) => `${dir}/loading.tsx swallows ${page}`)) + + expect(offending).toEqual([]) + }) + + it("is looking at pages that really do refuse", () => { + // The negative control, and it is not decoration: every assertion above is + // satisfied by a scanner that found no refusals at all, which is what a + // wrong APP_DIR or a broken pattern produces — and it would report the rule + // held while measuring nothing. + // + // `grep -c` prints 0 and exits 1; this counts in-process for the same + // reason that idiom keeps costing this repo time. + const refusing = refusalsUnder("(app)") + + expect(refusing.length).toBeGreaterThan(35) + expect(refusing).toContain("(app)/admin/metering/page.tsx") + }) + + it("names the one boundary that is grandfathered, and finds it still there", () => { + // If somebody removes `reports/loading.tsx`, this list must shrink with it + // rather than quietly permitting a future one under the same name. + expect(loadingBoundaries()).toContain("(app)/reports") + }) +}) diff --git a/apps/web/src/app/(app)/feed/page.tsx b/apps/web/src/app/(app)/feed/page.tsx index 00916931..89959700 100644 --- a/apps/web/src/app/(app)/feed/page.tsx +++ b/apps/web/src/app/(app)/feed/page.tsx @@ -63,20 +63,59 @@ export default async function FeedPage() { ), ] - const posts = await db.feedPost.findMany({ - where: { institutionId: { in: institutionIds }, isArchived: false }, - orderBy: { createdAt: "desc" }, - take: 30, - include: { - organization: { select: { id: true, name: true, slug: true } }, - event: { select: { id: true, title: true, startAt: true } }, - comments: { orderBy: { createdAt: "asc" }, take: 30 }, - interests: { - include: { organization: { select: { name: true } } }, - orderBy: { createdAt: "asc" }, + /* + * The three reads this page can start at once. + * + * The feed itself, the clubs this person may post on behalf of, and those + * clubs' upcoming events are independent: each takes only ids already + * derived from `ctx`. They were three sequential `await`s on a + * `force-dynamic` route, so the reader waited for the sum. + * + * `authors` is NOT in here and cannot be: its `where` is built from the ids + * inside `posts`, so it is genuinely a second round trip and stays one. + * + * `viewerTimeZone` above is likewise left alone. Hoisting it alongside + * `getUserContext` looks like a fourth saving and is not one — its first + * statement is `await getUserContext(userId)`, and that call is `cache()`d, + * so it already awaits the very promise it would be racing. + */ + const [posts, myClubs, myEvents] = await Promise.all([ + db.feedPost.findMany({ + where: { institutionId: { in: institutionIds }, isArchived: false }, + orderBy: { createdAt: "desc" }, + take: 30, + include: { + organization: { select: { id: true, name: true, slug: true } }, + event: { select: { id: true, title: true, startAt: true } }, + comments: { orderBy: { createdAt: "asc" }, take: 30 }, + interests: { + include: { organization: { select: { name: true } } }, + orderBy: { createdAt: "asc" }, + }, }, - }, - }) + }), + // Clubs this user can post/collaborate on behalf of + activeOrgIds.length + ? db.organization.findMany({ + where: { id: { in: activeOrgIds } }, + select: { id: true, name: true }, + orderBy: { name: "asc" }, + }) + : [], + // Upcoming events of those clubs (composer's optional link) + activeOrgIds.length + ? db.event.findMany({ + where: { + organizationId: { in: activeOrgIds }, + status: { not: "CANCELLED" }, + startAt: { gte: new Date() }, + }, + select: { id: true, title: true, organizationId: true }, + orderBy: { startAt: "asc" }, + take: 20, + }) + : [], + ]) // Resolve author names in one pass const authorIds = [ @@ -94,28 +133,6 @@ export default async function FeedPage() { ).map((u) => [u.id, u.name ?? "Unknown"]) ) - // Clubs this user can post/collaborate on behalf of - const myClubs = activeOrgIds.length - ? await db.organization.findMany({ - where: { id: { in: activeOrgIds } }, - select: { id: true, name: true }, - orderBy: { name: "asc" }, - }) - : [] - - // Upcoming events of those clubs (composer's optional link) - const myEvents = activeOrgIds.length - ? await db.event.findMany({ - where: { - organizationId: { in: activeOrgIds }, - status: { not: "CANCELLED" }, - startAt: { gte: new Date() }, - }, - select: { id: true, title: true, organizationId: true }, - orderBy: { startAt: "asc" }, - take: 20, - }) - : [] const director = institutionIds.some((i) => isOseDirector(ctx, i)) const pendingForDirector = director diff --git a/apps/web/src/app/(app)/messages/page.tsx b/apps/web/src/app/(app)/messages/page.tsx index b6615025..d48df54a 100644 --- a/apps/web/src/app/(app)/messages/page.tsx +++ b/apps/web/src/app/(app)/messages/page.tsx @@ -44,39 +44,61 @@ export default async function MessagesPage() { .filter((r) => r.status === "ACTIVE" || r.status === "SHADOW") .map((r) => r.organizationId) - const conversations = await db.conversation.findMany({ - where: { - OR: [ - { participants: { some: { userId } } }, - { type: "BOARD_CHANNEL", organizationId: { in: currentOrgIds } }, - { type: "BOARD_CHANNEL", institutionId: { in: oseInstitutionIds } }, - ], - }, - orderBy: { updatedAt: "desc" }, - take: 50, - include: { - organization: { select: { name: true } }, - participants: { include: { user: { select: { id: true, name: true, image: true } } } }, - messages: { - orderBy: { createdAt: "desc" }, - take: 1, - include: { sender: { select: { name: true } } }, + /* + * Three reads, none of which is an input to another. + * + * They were three sequential `await`s, so the page paid the sum of three + * round trips before it emitted a byte — on a `force-dynamic` route with no + * loading boundary, which is the whole of the delay a person feels after + * clicking Messages. Every argument below is derived from `ctx`, which is + * already in hand, so there is nothing to sequence them for. + * + * The board-channel list needs a club's `id` and its `name` and reads + * nothing else — `boardChannelList` below uses exactly `o.id` and `o.name`. + * Unselected, this pulled all fourteen scalar columns of every Organization + * at the institution (description, rosterNote, logoUrl, imageKey, …) to + * print a name in a button. `select` here is not a micro-optimisation of + * bytes so much as a statement of what this page is entitled to see. + */ + const [conversations, unread, myOrgs] = await Promise.all([ + db.conversation.findMany({ + where: { + OR: [ + { participants: { some: { userId } } }, + { type: "BOARD_CHANNEL", organizationId: { in: currentOrgIds } }, + { type: "BOARD_CHANNEL", institutionId: { in: oseInstitutionIds } }, + ], }, - }, - }) - - // Unread counts per conversation - const unread = await db.delivery.groupBy({ - by: ["participantId"], - where: { readAt: null, participant: { userId } }, - _count: true, - }) + orderBy: { updatedAt: "desc" }, + take: 50, + include: { + organization: { select: { name: true } }, + participants: { include: { user: { select: { id: true, name: true, image: true } } } }, + messages: { + orderBy: { createdAt: "desc" }, + take: 1, + include: { sender: { select: { name: true } } }, + }, + }, + }), + // Unread counts per conversation + db.delivery.groupBy({ + by: ["participantId"], + where: { readAt: null, participant: { userId } }, + _count: true, + }), + currentOrgIds.length + ? db.organization.findMany({ + where: { id: { in: currentOrgIds } }, + select: { id: true, name: true }, + }) + : db.organization.findMany({ + where: { institutionId: { in: oseInstitutionIds } }, + select: { id: true, name: true }, + }), + ]) const unreadByParticipant = new Map(unread.map((u) => [u.participantId, u._count])) - const myOrgs = currentOrgIds.length - ? await db.organization.findMany({ where: { id: { in: currentOrgIds } } }) - : await db.organization.findMany({ where: { institutionId: { in: oseInstitutionIds } } }) - const canBroadcast = oseInstitutionIds.length > 0 const boardChannelList = (orgs: typeof myOrgs) => ( diff --git a/apps/web/src/app/(app)/settings/page.tsx b/apps/web/src/app/(app)/settings/page.tsx index 21439e35..d29bd76b 100644 --- a/apps/web/src/app/(app)/settings/page.tsx +++ b/apps/web/src/app/(app)/settings/page.tsx @@ -118,13 +118,17 @@ export default async function SettingsPage() { // The tenant, resolved from this person's OWN membership. Nothing on this // page accepts an institution id, and this is why it never needs to. const institutionId = ctx.institutionRoles[0]?.institutionId - const modules = institutionId ? await declaredModules(institutionId) : null - // WHICH workspace this request is in. Read through the same helper the app - // layout uses, so the panels a person is offered and the nav they are - // offered cannot disagree. The cookie behind it is a PREFERENCE and never an - // entitlement — `resolveActiveWorkspace` re-checks it against current rows. - const workspace = await activeWorkspace(ctx, institutionId) + // Two reads, both keyed on facts already in hand and neither an input to + // the other, so they go out together. WHICH workspace this request is in is + // read through the same helper the app layout uses, so the panels a person + // is offered and the nav they are offered cannot disagree. The cookie + // behind it is a PREFERENCE and never an entitlement — + // `resolveActiveWorkspace` re-checks it against current rows. + const [modules, workspace] = await Promise.all([ + institutionId ? declaredModules(institutionId) : null, + activeWorkspace(ctx, institutionId), + ]) if (workspace === null) redirect("/access-pending") const offered = buildSettings({ @@ -185,6 +189,36 @@ export default async function SettingsPage() { gate pay nothing for it. */ let institutionName: string | null = null + /* + * This page's Institution row, read at most once. + * + * Two panels want one field each off the SAME row: the delegation dialog + * needs `name` so it can say WHERE the authority lands, and the Tenure AI + * panel needs `aiModelKey` so it can say which model this tenant chose. + * They sit in different branches, so each took its own `findUnique` and an + * OSE Director who can delegate read the row twice per page load. + * + * Keyed on the id rather than assuming one. In the pilot the two ids are + * necessarily equal — `delegationScopeFrom(ctx)` is called with no + * `atInstitutionId`, so its `institutionId` is `ctx.institutionRoles[0]`, + * which is exactly what `institutionId` above is, and the branch that could + * make them differ (falling back to a president's club) only fires when + * `institutionId` is undefined, which is precisely when the AI panel is not + * rendered at all. Storing the key anyway means a future caller passing a + * different institution gets a correct second read instead of the first + * one's answer. + */ + let institutionRead: { id: string; row: { name: string; aiModelKey: string | null } | null } | null = null + const institutionOnce = async (id: string) => { + const cached = institutionRead + if (cached && cached.id === id) return cached.row + const row = await db.institution.findUnique({ + where: { id }, + select: { name: true, aiModelKey: true }, + }) + institutionRead = { id, row } + return row + } let currentDelegation: | { id: string @@ -215,9 +249,7 @@ export default async function SettingsPage() { }) } if (instId) { - institutionName = - (await db.institution.findUnique({ where: { id: instId }, select: { name: true } })) - ?.name ?? null + institutionName = (await institutionOnce(instId))?.name ?? null currentDelegation = await db.approvalDelegation.findFirst({ where: { fromUserId: user.id, revokedAt: null, institutionId: instId }, select: { @@ -266,14 +298,7 @@ export default async function SettingsPage() { const aiPanel: { chosen: AiModelKey | null; deploymentDefault: AiModelKey } | null = shown("tenure-ai") && institutionId && cellProvider.kind === "bedrock" ? { - chosen: registeredModelKey( - ( - await db.institution.findUnique({ - where: { id: institutionId }, - select: { aiModelKey: true }, - }) - )?.aiModelKey, - ), + chosen: registeredModelKey((await institutionOnce(institutionId))?.aiModelKey), deploymentDefault: cellProvider.modelKey, } : null diff --git a/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts b/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts new file mode 100644 index 00000000..c234dd84 --- /dev/null +++ b/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts @@ -0,0 +1,204 @@ +/** + * Twenty faces on /messages cost twenty round trips, and then twenty more. + * + * `/api/profile-image/[userId]` and `/api/org-image/[orgId]` answer a 307 to a + * signed S3 URL. A 307 carrying no freshness directive is not storable, so a + * DM list with twenty threads ran the whole route twenty times — `auth`, the + * visibility check, a `findUnique` and a presign each — and ran the identical + * twenty again on the next visit. Both writers have always stamped + * `?v=${Date.now()}` on the stored URL, which is only worth doing if the answer + * was meant to be reusable. + * + * ── What these tests are actually protecting ──────────────────────────────── + * + * Not "a header is present". The header is one line and nobody deletes it by + * accident. What is easy to get wrong, and what every case below is about, is + * WHICH answers carry it: + * + * · the redirect, yes; + * · the 404 that means "no image", no; + * · the 404 that means "not yours to see", no — the two 404s are deliberately + * indistinguishable, so caching either caches both; + * · the org route's 403, no; + * · the 401, no. + * + * A refusal cached for five minutes keeps refusing for five minutes after a + * seat is granted, which is a support ticket that reproduces for nobody. + * + * And it must stay `private` and stay inside the presign's own life. `private` + * is what stops a CDN holding one viewer's authorization decision and serving + * it to another; the 300s ceiling is what keeps this from being the thing that + * decides how long a departed member can still fetch the bytes — the presign + * already answers that, and it says 600. + */ +import { IMAGE_PROXY_CACHE_CONTROL } from "@/lib/storage/image-proxy-cache" + +const state = { + session: null as { user: { id: string } } | null, + shares: true, + userImageKey: null as string | null, + org: null as { id: string; institutionId: string; imageKey: string | null } | null, + canViewOrg: true, +} + +jest.mock("@/lib/auth", () => ({ auth: async () => state.session })) + +jest.mock("@/lib/db", () => ({ + db: { + user: { findUnique: async () => ({ imageKey: state.userImageKey }) }, + organization: { findUnique: async () => state.org }, + }, +})) + +jest.mock("@/lib/people/shared-institution", () => ({ + sharesAnInstitution: async () => state.shares, +})) + +jest.mock("@/lib/rbac", () => ({ + getUserContext: async () => ({ institutionRoles: [], orgRoles: [] }), + canViewOrg: () => state.canViewOrg, +})) + +jest.mock("@/lib/tenant-scope", () => ({ + withTenantScope: (_userId: string, fn: () => T) => fn(), +})) + +jest.mock("@/lib/s3", () => ({ + documentViewUrl: async (key: string) => `https://storage.example/${key}?signature=abc`, +})) + +import { GET as profileImage } from "./profile-image/[userId]/route" +import { GET as orgImage } from "./org-image/[orgId]/route" + +const request = new Request("https://tenure.example/api/profile-image/u2") +const asUser = (id: string) => ({ user: { id } }) + +beforeEach(() => { + state.session = asUser("viewer") + state.shares = true + state.userImageKey = "profile-images/abc.png" + state.org = { id: "org1", institutionId: "inst1", imageKey: "org-images/def.png" } + state.canViewOrg = true +}) + +const profile = (userId: string) => + profileImage(request, { params: Promise.resolve({ userId }) }) +const org = (orgId: string) => orgImage(request, { params: Promise.resolve({ orgId }) }) + +describe("the redirect is reusable", () => { + it("stamps the avatar redirect so a second look at the same page is free", async () => { + const res = await profile("u2") + + expect(res.status).toBe(307) + expect(res.headers.get("Cache-Control")).toBe(IMAGE_PROXY_CACHE_CONTROL) + }) + + it("stamps the club-image redirect the same way, from the same constant", async () => { + const res = await org("org1") + + expect(res.status).toBe(307) + expect(res.headers.get("Cache-Control")).toBe(IMAGE_PROXY_CACHE_CONTROL) + }) + + it("still sends the Location it was sending before", async () => { + // The header is an addition, not a replacement: `NextResponse.redirect` + // takes the init object as a whole, and passing one that omitted the status + // or dropped Location would be a silent way to break every avatar. + const res = await profile("u2") + + expect(res.headers.get("Location")).toBe( + "https://storage.example/profile-images/abc.png?signature=abc", + ) + }) +}) + +describe("a refusal is not reusable", () => { + // Each of these is a DIFFERENT branch of the two routes. Caching any one of + // them would keep answering "no" after the answer became "yes". + it("does not stamp the 404 for a person who has no photograph", async () => { + state.userImageKey = null + + const res = await profile("u2") + + expect(res.status).toBe(404) + expect(res.headers.get("Cache-Control")).toBeNull() + }) + + it("does not stamp the 404 that actually means 'not yours to see'", async () => { + // The interesting one. This 404 and the one above are identical ON PURPOSE + // — a distinguishable pair is an existence oracle. Because they are + // identical, a cached one is indistinguishable too, and a viewer admitted + // to the institution a moment later would keep being told the person does + // not exist. + state.shares = false + + const res = await profile("stranger-at-another-university") + + expect(res.status).toBe(404) + expect(res.headers.get("Cache-Control")).toBeNull() + }) + + it("does not stamp the club route's 404 either", async () => { + // Added after a mutant survived. The first draft covered the AVATAR route's + // two 404s and the club route's 403, and quietly assumed the club route's + // own 404 was the same code path. It is not — it is a separate `return` in + // a separate file, and stamping it shipped green. A club that uploads its + // first logo would have gone on 404ing for five minutes for everyone who + // had already looked. + state.org = { id: "org1", institutionId: "inst1", imageKey: null } + + const res = await org("org1") + + expect(res.status).toBe(404) + expect(res.headers.get("Cache-Control")).toBeNull() + }) + + it("does not stamp the club route's 404 for a club that is not there at all", async () => { + // The other way into the same `return`: `findUnique` answered null rather + // than answering a row with no image. + state.org = null + + const res = await org("no-such-club") + + expect(res.status).toBe(404) + expect(res.headers.get("Cache-Control")).toBeNull() + }) + + it("does not stamp the club route's 403", async () => { + state.canViewOrg = false + + const res = await org("org1") + + expect(res.status).toBe(403) + expect(res.headers.get("Cache-Control")).toBeNull() + }) + + it("does not stamp either route's 401", async () => { + state.session = null + + expect((await profile("u2")).headers.get("Cache-Control")).toBeNull() + expect((await org("org1")).headers.get("Cache-Control")).toBeNull() + }) +}) + +describe("the directive itself", () => { + it("is private, so no CDN holds one viewer's authorization decision", () => { + // Both routes decide per viewer — `sharesAnInstitution` on one, `canViewOrg` + // on the other. A shared cache that stored the answer would hand it to + // somebody who would have been refused. + expect(IMAGE_PROXY_CACHE_CONTROL).toMatch(/\bprivate\b/) + expect(IMAGE_PROXY_CACHE_CONTROL).not.toMatch(/\bpublic\b|\bs-maxage\b|\bimmutable\b/) + }) + + it("cannot outlive the presigned URL it points at", () => { + // `documentViewUrl` presigns for 600s. The exposure window is set by that + // number whatever this file says; what must never happen is this number + // GROWING past it, which would let a browser keep re-following a redirect + // to a URL that has already expired — an image that silently stops loading. + const maxAge = Number(/max-age=(\d+)/.exec(IMAGE_PROXY_CACHE_CONTROL)?.[1]) + + expect(Number.isFinite(maxAge)).toBe(true) + expect(maxAge).toBeGreaterThan(0) + expect(maxAge).toBeLessThanOrEqual(600 / 2) + }) +}) diff --git a/apps/web/src/app/api/org-image/[orgId]/route.ts b/apps/web/src/app/api/org-image/[orgId]/route.ts index ab065f66..db5e6b3e 100644 --- a/apps/web/src/app/api/org-image/[orgId]/route.ts +++ b/apps/web/src/app/api/org-image/[orgId]/route.ts @@ -4,6 +4,7 @@ import { db } from "@/lib/db" import { canViewOrg, getUserContext } from "@/lib/rbac" import { withTenantScope } from "@/lib/tenant-scope" import { documentViewUrl } from "@/lib/s3" +import { IMAGE_PROXY_CACHE_CONTROL } from "@/lib/storage/image-proxy-cache" /** * Serves an uploaded club image by redirecting to a short-lived signed URL. @@ -34,6 +35,9 @@ export async function GET( if (!canViewOrg(ctx, org)) return new NextResponse("Forbidden", { status: 403 }) const url = await documentViewUrl(org.imageKey) - return NextResponse.redirect(url) + // Same window, same reasoning, one constant. /orgs draws one of these per + // club spine and re-ran this whole route for each of them on every load. + // The 404 and the 403 above are left uncacheable deliberately. + return NextResponse.redirect(url, { headers: { "Cache-Control": IMAGE_PROXY_CACHE_CONTROL } }) }) } diff --git a/apps/web/src/app/api/profile-image/[userId]/route.ts b/apps/web/src/app/api/profile-image/[userId]/route.ts index e4b7215d..9cf69363 100644 --- a/apps/web/src/app/api/profile-image/[userId]/route.ts +++ b/apps/web/src/app/api/profile-image/[userId]/route.ts @@ -3,6 +3,7 @@ import { auth } from "@/lib/auth" import { db } from "@/lib/db" import { sharesAnInstitution } from "@/lib/people/shared-institution" import { documentViewUrl } from "@/lib/s3" +import { IMAGE_PROXY_CACHE_CONTROL } from "@/lib/storage/image-proxy-cache" /** * Serves an uploaded profile picture by redirecting to a short-lived signed URL. @@ -59,5 +60,11 @@ export async function GET( const user = await db.user.findUnique({ where: { id: userId }, select: { imageKey: true } }) if (!user?.imageKey) return new NextResponse("Not found", { status: 404 }) - return NextResponse.redirect(await documentViewUrl(user.imageKey)) + // Reusable for five minutes by THIS browser and nothing else — see + // IMAGE_PROXY_CACHE_CONTROL for why that widens nothing. Only the redirect + // carries it; the two 404s above stay uncacheable on purpose, because one of + // them means "not yours to see" and access can be granted a second later. + return NextResponse.redirect(await documentViewUrl(user.imageKey), { + headers: { "Cache-Control": IMAGE_PROXY_CACHE_CONTROL }, + }) } diff --git a/apps/web/src/components/a-heavy-parser-does-not-ship-with-the-page.test.ts b/apps/web/src/components/a-heavy-parser-does-not-ship-with-the-page.test.ts new file mode 100644 index 00000000..defc210e --- /dev/null +++ b/apps/web/src/components/a-heavy-parser-does-not-ship-with-the-page.test.ts @@ -0,0 +1,245 @@ +import { existsSync, readFileSync, readdirSync, statSync } from "node:fs" +import path from "node:path" + +/** + * A spreadsheet engine was downloaded by people who came to read a total. + * + * SheetJS is 896 KB raw and 225 KB gzipped — measured on `node_modules/xlsx/ + * xlsx.mjs`, the `module` entry a bundler takes. `BudgetUpload` imported it at + * the top of the file, so every visitor to /orgs//finance fetched, parsed + * and evaluated the whole of it before the page became interactive: the club + * members who can only READ finance and are never shown the upload control, and + * the treasurers who came to look at a number. Nothing in that file touches + * XLSX at module scope; every use is inside a click handler. + * + * ── Why this walks the graph instead of grepping the component ────────────── + * + * A bundle is transitive. `DocumentRow` imports no parser and ships one anyway, + * because it imports `DocumentViewerOverlay`, which imports `xlsx` — and a test + * that looked only at files with `"use client"` at the top of them would have + * called `DocumentRow` clean. So this resolves `@/…` and relative imports + * through the app's own source and reports the CHAIN, which is the thing a + * person needs in order to fix it. + * + * The walk stops at any `"use server"` module, and that is load-bearing rather + * than an optimisation. `BudgetUpload` imports `finance/actions.ts` for its + * server action; Next replaces that import with a network reference and ships + * none of the module, so following it would report every heavy dependency the + * server has as if it were in the browser — a scanner that cries wolf gets + * deleted. + */ + +const SRC = path.resolve(__dirname, "..") + +/** + * Packages that must never be evaluated to render a page. + * + * Each is a document parser measured in hundreds of kilobytes, and each is only + * ever wanted after somebody picks a file or opens a preview — which is to say, + * always inside a handler, which is always somewhere `await import()` works. + */ +const HEAVY_PARSERS = ["xlsx", "mammoth", "jszip", "pdfjs-dist"] + +/** The parser a bare import specifier names, if any. */ +const heavyParserIn = (spec: string) => + HEAVY_PARSERS.find((p) => spec === p || spec.startsWith(`${p}/`)) ?? null + +function sourceFiles(): string[] { + const out: string[] = [] + const walk = (dir: string) => { + for (const entry of readdirSync(dir)) { + const p = path.join(dir, entry) + if (statSync(p).isDirectory()) walk(p) + else if (/\.tsx?$/.test(p) && !/\.(test|spec|itest)\.tsx?$/.test(p)) out.push(p) + } + } + walk(SRC) + return out +} + +const read = (file: string) => readFileSync(file, "utf8") + +/** `"use client"` as the first statement, past any leading comment block. */ +const isClientEntry = (file: string) => + /^\s*(\/\/.*\n|\/\*[\s\S]*?\*\/\s*\n)*\s*["']use client["']/.test(read(file)) + +/** `"use server"` at the top — a module boundary the client never crosses. */ +const isServerModule = (file: string) => + /^\s*["']use server["']/m.test(read(file).split("\n").slice(0, 3).join("\n")) + +/** STATIC imports only. `await import(…)` is the fix, so it must not be a hit. */ +const staticImportsOf = (file: string) => + [...read(file).matchAll(/(?:^|\n)\s*import\s[^;]*?from\s*["']([^"']+)["']/g)].map((m) => m[1]) + +function resolveWithinApp(spec: string, importer: string): string | null { + let base: string + if (spec.startsWith("@/")) base = path.join(SRC, spec.slice(2)) + else if (spec.startsWith(".")) base = path.resolve(path.dirname(importer), spec) + else return null + for (const candidate of [ + `${base}.ts`, + `${base}.tsx`, + path.join(base, "index.ts"), + path.join(base, "index.tsx"), + ]) { + if (existsSync(candidate)) return candidate + } + return null +} + +interface Hit { + /** The `"use client"` component that causes the download. */ + entry: string + /** How it gets there, entry first. */ + chain: string[] + parser: string +} + +function parsersReachableFromClientCode(): Hit[] { + const hits: Hit[] = [] + for (const entry of sourceFiles().filter(isClientEntry)) { + const seen = new Set() + const stack: [string, string[]][] = [[entry, [entry]]] + while (stack.length > 0) { + const [file, chain] = stack.pop()! + if (seen.has(file)) continue + seen.add(file) + if (file !== entry && isServerModule(file)) continue + for (const spec of staticImportsOf(file)) { + const parser = heavyParserIn(spec) + if (parser) { + hits.push({ + entry: path.relative(SRC, entry), + chain: chain.map((c) => path.relative(SRC, c)), + parser, + }) + continue + } + const next = resolveWithinApp(spec, file) + if (next) stack.push([next, [...chain, next]]) + } + } + } + return hits +} + +/** Every module a client entry statically pulls in, for the resolver's own test. */ +function modulesReachableFrom(entry: string): string[] { + const seen = new Set() + const stack = [entry] + while (stack.length > 0) { + const file = stack.pop()! + if (seen.has(file)) continue + seen.add(file) + if (file !== entry && isServerModule(file)) continue + for (const spec of staticImportsOf(file)) { + const next = resolveWithinApp(spec, file) + if (next) stack.push(next) + } + } + return [...seen].map((f) => path.relative(SRC, f)) +} + +describe("no page downloads a document parser to render itself", () => { + it("finds no client component reaching one at import time", () => { + // The chain, not just the file: a reviewer looking at `DocumentRow` — which + // names no parser anywhere in it — would otherwise have no idea why it had + // been reported. + const hits = parsersReachableFromClientCode() + + expect(hits.map((h) => `${h.parser} via ${h.chain.join(" -> ")}`)).toEqual([]) + }) + + it("still parses spreadsheets, just later, in both places that write one", () => { + // The other half, and the half a "no static import" rule alone would let + // somebody satisfy by deleting the feature. Both writers must still reach + // SheetJS dynamically. + // + // The overlay's import sits inside the `sheets` branch rather than merely + // inside an async function, and that is the point of it: nothing needs + // SheetJS to READ a workbook — the sheets arrive already parsed from the + // server — so a text document never fetches it and a spreadsheet only does + // at its first save. + for (const file of [ + "components/finance/BudgetUpload.tsx", + "components/documents/DocumentViewerOverlay.tsx", + ]) { + const source = read(path.join(SRC, file)) + + expect(source).toMatch(/await import\(["']xlsx["']\)/) + expect(source).not.toMatch(/^\s*import .* from ["']xlsx["']/m) + } + }) + + it("tells the uploader the truth when the parser itself will not load", () => { + // A chunk that fails to arrive is not a bad spreadsheet. `BudgetUpload` + // awaits the import OUTSIDE its parse `try` so the two cannot be confused, + // and the overlay treats the same failure as a failed SAVE — dirty and + // errored — rather than letting it reject out of `flush()` and close the + // overlay over edits that never left the browser. + expect(read(path.join(SRC, "components/finance/BudgetUpload.tsx"))).toMatch( + /Couldn't load the spreadsheet reader/, + ) + expect(read(path.join(SRC, "components/documents/DocumentViewerOverlay.tsx"))).toMatch( + /payload = await buildPayload\(\)/, + ) + }) +}) + +/** + * Everything above passes against a walk that found nothing — which is what a + * wrong SRC, a renamed extension, a broken resolver or a specifier this + * matcher does not recognise all produce, and any of them would report a clean + * bundle while measuring air. These are the checks that the instrument is on. + */ +describe("the scanner is measuring something", () => { + it("sees the app's client components", () => { + expect(sourceFiles().filter(isClientEntry).length).toBeGreaterThan(50) + }) + + it("recognises the import specifiers this repo actually writes", () => { + expect(heavyParserIn("xlsx")).toBe("xlsx") + expect(heavyParserIn("mammoth")).toBe("mammoth") + expect(heavyParserIn("pdfjs-dist/build/pdf.mjs")).toBe("pdfjs-dist") + // Not a false-positive machine: these must NOT match. + expect(heavyParserIn("@/lib/xlsx-helpers")).toBeNull() + expect(heavyParserIn("./xlsxish")).toBeNull() + }) + + it("would still find a parser where one genuinely is", () => { + // `api/documents/_lib/content.ts` imports xlsx, mammoth AND jszip, and is + // server-only. So it is live proof that the matcher fires on this repo's + // real import syntax — and, because it never appears as a hit above, proof + // that nothing in the browser reaches it. + const server = read(path.join(SRC, "app/api/documents/_lib/content.ts")) + const found = staticImportsOf(path.join(SRC, "app/api/documents/_lib/content.ts")) + .map(heavyParserIn) + .filter((p): p is string => p !== null) + + expect(server).toContain("xlsx") + expect(found.length).toBeGreaterThan(0) + }) + + it("resolves @/ imports, which is how it sees past a component that names nothing", () => { + // `DocumentRow` imports no parser and used to ship one, through exactly this + // edge. If `resolveWithinApp` stops resolving, this is what notices. + expect(modulesReachableFrom(path.join(SRC, "components/documents/DocumentRow.tsx"))).toContain( + "components/documents/DocumentViewerOverlay.tsx", + ) + }) + + it("stops at a server-action module, and that stop is not vacuous", () => { + // `BudgetUpload` imports `finance/actions.ts` for its server action. Next + // replaces that import with a network reference and ships none of the + // module, so following it would report the server's whole dependency tree + // as if it were in the browser. The boundary only helps if it is really + // there, so both halves are asserted. + const actions = path.join(SRC, "app/(app)/orgs/[slug]/finance/actions.ts") + + expect(isServerModule(actions)).toBe(true) + expect(isServerModule(path.join(SRC, "components/finance/BudgetUpload.tsx"))).toBe(false) + expect(modulesReachableFrom(path.join(SRC, "components/finance/BudgetUpload.tsx"))).toContain( + "app/(app)/orgs/[slug]/finance/actions.ts", + ) + }) +}) diff --git a/apps/web/src/components/documents/DocumentViewerOverlay.tsx b/apps/web/src/components/documents/DocumentViewerOverlay.tsx index 8bb17452..8878da6e 100644 --- a/apps/web/src/components/documents/DocumentViewerOverlay.tsx +++ b/apps/web/src/components/documents/DocumentViewerOverlay.tsx @@ -2,7 +2,6 @@ import { useCallback, useEffect, useRef, useState } from "react" import Link from "next/link" -import * as XLSX from "xlsx" import { Overlay } from "@/components/ui/Overlay" import { AlertCircle, @@ -151,13 +150,34 @@ export function DocumentViewerOverlay({ } }, [open, load]) - const buildPayload = useCallback((): SavePayload | null => { + /** + * The bytes to send, built from what is on screen. + * + * ── Why the spreadsheet engine is fetched here and not at the top ────────── + * + * SheetJS is 896 KB raw / 225 KB gzipped (measured on `node_modules/xlsx/ + * xlsx.mjs`, the entry a bundler takes). It used to be a top-level `import`, + * and this overlay is pulled in by `DocumentRow` — which is on the documents + * LIST — so opening a club's documents to read the filenames downloaded, + * parsed and evaluated a whole spreadsheet engine before the page was + * interactive, for every reader, whatever the documents happened to be. + * + * It is loaded inside the `sheets` branch specifically, not merely inside an + * async function. Nothing needs SheetJS to READ a workbook, or even to EDIT + * one: the sheets arrive already parsed by `api/documents/_lib/content.ts`, + * server-side. It is needed only to WRITE one back. So a text document never + * touches it, and a spreadsheet only touches it at the moment of the first + * save — by which point the person has been typing for at least + * `AUTOSAVE_MS`. + */ + const buildPayload = useCallback(async (): Promise => { const d = dataRef.current if (!d) return null if (d.content.kind === "text") { return { kind: "text", content: textRef.current } } if (d.content.kind === "sheets") { + const XLSX = await import("xlsx") const current = sheetsRef.current const norm = (rows: (string | number | null)[][]) => rows.map((r) => r.map((c) => (c === null ? "" : c))) @@ -177,7 +197,29 @@ export function DocumentViewerOverlay({ const doSave = useCallback(async () => { if (conflictRef.current) return - const payload = buildPayload() + /* + * Building the payload can now FAIL, and the failure has to be treated as a + * failed save rather than as no save. + * + * `buildPayload` fetches the spreadsheet engine on demand, so an offline + * laptop or a deploy whose chunk has rolled away throws here. Left + * unhandled that would reject out of `doSave`, and `flush()` — which the + * close and beforeunload paths await — would reject with it, so the overlay + * would close on a document whose last edits never left the browser and + * never said so. + * + * Marked dirty and errored, which is exactly what a failed POST does below: + * the 1.5s autosave is already a retry loop, and the second attempt gets + * the chunk from cache or fails visibly again. + */ + let payload: SavePayload | null + try { + payload = await buildPayload() + } catch { + dirtyRef.current = true + setStatus("error") + return + } if (!payload) return dirtyRef.current = false savingRef.current = true diff --git a/apps/web/src/components/finance/BudgetUpload.tsx b/apps/web/src/components/finance/BudgetUpload.tsx index 63592889..f00e5182 100644 --- a/apps/web/src/components/finance/BudgetUpload.tsx +++ b/apps/web/src/components/finance/BudgetUpload.tsx @@ -1,7 +1,6 @@ "use client" import { useRef, useState, useTransition } from "react" -import * as XLSX from "xlsx" // FileSpreadsheet/Check have no direct alias in the icon source; using the // closest existing FileText/CheckCircle (see notes). import { Upload, FileText as FileSpreadsheet, AlertCircle, CheckCircle as Check } from "@/components/ui/icons" @@ -64,9 +63,28 @@ function newUploadToken(): string { /** * Upload an Excel/CSV budget tracker and turn it into the dashboard. * - * Parsing happens in the browser (the xlsx dependency already ships for the - * document viewer), so we never store the raw file — only the clean rows are - * sent to the server, which re-validates and owns the write. + * Parsing happens in the browser, so we never store the raw file — only the + * clean rows are sent to the server, which re-validates and owns the write. + * + * ── Why the parser is imported inside the handler ─────────────────────────── + * + * SheetJS is 896 KB raw / 225 KB gzipped (measured: `node_modules/xlsx/ + * xlsx.mjs`, the `module` entry a bundler takes). As a top-level `import` it + * was fetched, parsed and evaluated by every visitor to /orgs//finance + * before the page became interactive — including the club members who can only + * READ the finance page and are never shown this control, and the treasurers + * who came to look at a total rather than upload anything. Nothing in this file + * touches XLSX at module scope; every use is inside `handleFile`, which is + * already `async` and already only runs once a person has picked a file. So the + * download can wait until then, and for almost every page view it never + * happens. + * + * The import is awaited OUTSIDE the parse `try` on purpose. Inside it, a failed + * chunk fetch — an offline laptop, a stale deploy whose chunk is gone — would + * be caught by the same `catch` that means "that spreadsheet is unreadable", + * and the uploader would be told their file was bad when their file is fine. + * That is the exact class of wrong sentence `newUploadToken` below was written + * to avoid. */ export function BudgetUpload({ slug, @@ -112,6 +130,13 @@ export function BudgetUpload({ async function handleFile(file: File) { setError(null) setDone(null) + let XLSX: typeof import("xlsx") + try { + XLSX = await import("xlsx") + } catch { + setError("Couldn't load the spreadsheet reader. Check your connection and try again.") + return + } try { const buf = await file.arrayBuffer() const wb = XLSX.read(buf, { type: "array" }) diff --git a/apps/web/src/components/shell/NotificationBell.tsx b/apps/web/src/components/shell/NotificationBell.tsx index 51536f35..6a324085 100644 --- a/apps/web/src/components/shell/NotificationBell.tsx +++ b/apps/web/src/components/shell/NotificationBell.tsx @@ -103,14 +103,45 @@ export function NotificationBell({ initialUnread = 0 }: { initialUnread?: number loadHistory() }, [loadHistory]) + /* + * The bell keeps itself current, and stops asking when nobody is looking. + * + * This used to be a bare `setInterval(refresh, 30_000)`. A tab left open on + * Tenure therefore issued an authenticated request every thirty seconds for + * the whole of a session — each one a dynamic invocation with a database read + * behind it — whether or not the tab was on screen. Nothing about a hidden + * tab needs a fresh badge: the moment it comes back, the `visibilitychange` + * and `focus` handlers below refresh it, so the count a person actually SEES + * is exactly as current as it was before. + * + * The guard is on the TICK rather than on the effect, deliberately. Tearing + * the interval down and building it back up on every visibility change makes + * the poll phase restart from zero each time, so a tab flicked back and forth + * would poll more often than one left alone. Skipping the tick leaves one + * timer running for the life of the mount and simply declines to spend a + * request on it. + * + * `visibilitychange` is listened for as well as `focus`, and they are not the + * same event: a background tab in a FOCUSED window becoming visible fires + * only `visibilitychange`, and a window regaining focus with the tab already + * visible fires only `focus`. `usePolling` in components/charts/hooks.ts + * already made both of these decisions; this is the same shape. + */ useEffect(() => { + const tick = () => { + if (document.visibilityState !== "hidden") refresh() + } refresh() - const id = setInterval(refresh, 30_000) - const onFocus = () => refresh() - window.addEventListener("focus", onFocus) + const id = setInterval(tick, 30_000) + const onVisible = () => { + if (document.visibilityState === "visible") refresh() + } + window.addEventListener("focus", onVisible) + document.addEventListener("visibilitychange", onVisible) return () => { clearInterval(id) - window.removeEventListener("focus", onFocus) + window.removeEventListener("focus", onVisible) + document.removeEventListener("visibilitychange", onVisible) } }, [refresh]) diff --git a/apps/web/src/components/shell/SideNav.tsx b/apps/web/src/components/shell/SideNav.tsx index 92d04b0c..07340b8a 100644 --- a/apps/web/src/components/shell/SideNav.tsx +++ b/apps/web/src/components/shell/SideNav.tsx @@ -1,7 +1,7 @@ "use client" import { useCallback, useEffect, useRef, useState } from "react" -import Link from "next/link" +import Link, { useLinkStatus } from "next/link" import { usePathname } from "next/navigation" import { Button as AriaButton, @@ -71,6 +71,44 @@ const TOOLTIP_CLASS = const ITEM_BASE = "nav-item group relative flex h-[32px] items-center gap-2.5 rounded-[8px] px-2.5 text-[13.5px] no-underline transition-colors" +/** + * "This one is loading", drawn inside the Link so `useLinkStatus` can see it. + * + * The nav's own highlight cannot do this job. `active` comes from + * `usePathname`, and the pathname does not change until the navigation COMMITS + * — which on these `force-dynamic` routes is the very thing being waited for. + * So between the click and the answer the nav still marks the page being left, + * and the entry just clicked shows nothing at all. `app/(app)/loading.tsx` now + * fills the content region during that window; this says which entry asked for + * it, which is the half the content region cannot express. + * + * A 2px pulse in the primary token plus a visually-hidden word, the same shape + * `charts/RangeFilter.tsx` already uses for the same problem — a mark that only + * pulses is a state a screen reader never hears. + * + * It is rendered ONLY in the Link branch below, and not in the `opensAssistant` + * button branch. That is not an oversight: the assistant entry opens a panel in + * this document and never navigates, so it has no link status to report and + * nothing to wait for. `useLinkStatus` outside a Link is a context read that + * returns the idle value, so putting it there would have been harmless and + * would also have been a promise of a state that can never arrive. + */ +function ItemPending() { + const { pending } = useLinkStatus() + if (!pending) return null + return ( + <> + Loading + + + ) +} + function ItemLink({ item, active, @@ -127,6 +165,7 @@ function ItemLink({ aria-current={active ? "page" : undefined} > {inner} + ) diff --git a/apps/web/src/components/shell/a-hidden-tab-stops-asking.test.tsx b/apps/web/src/components/shell/a-hidden-tab-stops-asking.test.tsx new file mode 100644 index 00000000..a7a71024 --- /dev/null +++ b/apps/web/src/components/shell/a-hidden-tab-stops-asking.test.tsx @@ -0,0 +1,189 @@ +/** + * @jest-environment jsdom + */ +import { act } from "react" +import { createRoot, type Root } from "react-dom/client" + +import { NotificationBell } from "./NotificationBell" + +/** + * A tab nobody is looking at was asking the server for news every 30 seconds. + * + * The bell mounts on EVERY page of the shell and polled on a bare + * `setInterval`. A Tenure tab left open behind somebody's coursework therefore + * issued an authenticated request every thirty seconds for the whole session — + * each one a dynamic invocation with a database read behind it — and none of + * them could change anything a person could see, because the person was not + * looking. `components/charts/hooks.ts` had already worked this out for the + * dashboards; the bell had not. + * + * ── What is easy to get wrong here, and is therefore what these test ──────── + * + * Not "does it skip a tick". Two neighbours of that: + * + * 1. The guard must not turn the poll OFF. A visible tab has to keep polling + * on the same cadence it always did, or the badge is now a number from + * whenever the page was opened. + * 2. Coming back has to cost exactly one request, and it has to happen on + * BOTH routes back — `visibilitychange` (a background tab surfacing inside + * a focused window) and `focus` (a window regaining focus with the tab + * already visible). Those are different events and neither implies the + * other; wiring only one leaves a stale badge for whichever half of + * "coming back" a person actually does. + * + * The phase point is tested too: the guard is on the TICK and not on the + * effect, so the interval is never rebuilt. An implementation that tore the + * timer down and stood it back up on each visibility change would restart the + * 30s phase every time, and a tab flicked back and forth would poll MORE than + * one left alone — the opposite of the intent, and invisible to a test that + * only counted requests in one steady state. + */ + +let container: HTMLDivElement +let root: Root +let calls: string[] +let visibility: DocumentVisibilityState + +/** Requests issued to the notifications API since the last reset. */ +const polls = () => calls.filter((u) => u.startsWith("/api/notifications")).length + +beforeAll(() => { + ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + // jsdom's visibilityState is a read-only getter, so the test drives it + // through a replacement descriptor rather than by assignment. + Object.defineProperty(document, "visibilityState", { + configurable: true, + get: () => visibility, + }) +}) + +beforeEach(() => { + jest.useFakeTimers() + calls = [] + visibility = "visible" + globalThis.fetch = jest.fn(async (input: RequestInfo | URL) => { + calls.push(String(input)) + return { + ok: true, + json: async () => ({ unread: 0, items: [] }), + } as Response + }) as unknown as typeof fetch + + container = document.createElement("div") + document.body.append(container) + root = createRoot(container) +}) + +afterEach(() => { + act(() => root.unmount()) + container.remove() + jest.useRealTimers() +}) + +/** Mount the bell and let its mount-time refresh settle. */ +async function mount() { + await act(async () => { + root.render() + }) +} + +/** Advance `ms` of wall clock and let anything it started settle. */ +async function elapse(ms: number) { + await act(async () => { + jest.advanceTimersByTime(ms) + }) +} + +/** Fire one of the two "the person is back" events. */ +async function comeBack(event: "visibilitychange" | "focus") { + visibility = "visible" + await act(async () => { + if (event === "focus") window.dispatchEvent(new Event("focus")) + else document.dispatchEvent(new Event("visibilitychange")) + }) +} + +it("still polls on its own while somebody is looking", async () => { + // The guard's most important property is the one it must NOT have. If this + // passes only because polling stopped altogether, the badge goes stale for + // everybody and the "saving" is the feature. + await mount() + expect(polls()).toBe(1) + + await elapse(30_000) + expect(polls()).toBe(2) + + await elapse(30_000) + expect(polls()).toBe(3) +}) + +it("asks for nothing while the tab is hidden", async () => { + await mount() + expect(polls()).toBe(1) + + visibility = "hidden" + await elapse(30_000 * 10) + + // Ten ticks' worth of wall clock, no requests. Before the guard this was ten. + expect(polls()).toBe(1) +}) + +it("catches up once — not once per skipped tick — when the tab comes back", async () => { + await mount() + visibility = "hidden" + await elapse(30_000 * 10) + expect(polls()).toBe(1) + + await comeBack("visibilitychange") + + // A queue of missed ticks would be the other way to get this wrong: a person + // returning after an hour does not owe the server 120 requests, they owe it + // one current answer. + expect(polls()).toBe(2) +}) + +it("also catches up on focus, which is the other way back", async () => { + // A window regaining focus with the tab already visible fires `focus` and NOT + // `visibilitychange`. Handling only the latter leaves an alt-tab back to a + // stale badge. + await mount() + visibility = "hidden" + await elapse(30_000 * 3) + + await comeBack("focus") + + expect(polls()).toBe(2) +}) + +it("resumes the ordinary cadence after coming back", async () => { + await mount() + visibility = "hidden" + await elapse(30_000 * 4) + await comeBack("visibilitychange") + const afterReturn = polls() + + await elapse(30_000) + + expect(polls()).toBe(afterReturn + 1) +}) + +it("does not restart the interval's phase each time the tab is revealed", async () => { + // The guard is on the tick, not on the effect. Reveal the tab at 10s, 20s and + // 25s; a rebuilt timer would push its next tick 30s past the last reveal and + // the 30s tick would never land. Each reveal costs its own catch-up request, + // so what is measured is the TICK arriving on the original schedule: three + // reveals plus the mount is four, and the tick at 30s makes five. + await mount() + + for (const at of [10_000, 10_000, 5_000]) { + visibility = "hidden" + await elapse(at) + await comeBack("visibilitychange") + } + expect(polls()).toBe(4) + + visibility = "visible" + await elapse(5_000) + + expect(polls()).toBe(5) +}) diff --git a/apps/web/src/lib/capability-registry/manifest.ts b/apps/web/src/lib/capability-registry/manifest.ts index 9c05e9eb..8df9b959 100644 --- a/apps/web/src/lib/capability-registry/manifest.ts +++ b/apps/web/src/lib/capability-registry/manifest.ts @@ -1,3 +1,5 @@ +import { cache } from "react" + import { modulesInAuditMetadata } from "./availability" import { db } from "@/lib/db" @@ -26,8 +28,33 @@ import { runUnscopedWidening } from "@/lib/tenancy/context" * tenant. [] means a manifest exists and declares no modules. The resolver * treats them differently on purpose: filtering against an absent manifest would * take a working product dark on the strength of a document that does not exist. + * + * ── Why it is `cache()`d ──────────────────────────────────────────────────── + * + * A tenant's manifest is one fact and every render asked for it twice. The app + * shell reads it in `(app)/layout.tsx` to decide which nav entries exist, and + * then each of the seven capability-gated surfaces underneath it — /connectors, + * /admin/metering, /reports, /reports/finance, and a club's memory and two + * handoff pages — asks again through `offeredTo`. Layout and page render in the + * SAME React pass, so `cache()` collapses the pair to one round trip, the same + * way `getUserContext` and `viewerTimeZone` already do. + * + * WHAT THIS DOES NOT COVER, stated because the absence is easy to misread as a + * fix: `react/cache` memoises against `ReactSharedInternals.A`, the async + * dispatcher that ONLY the RSC renderer installs. A Route Handler never runs + * through that renderer, so there the wrapper falls through to a direct call and + * `/api/ai/chat` still reads the manifest twice — once via `offeredTo`, once + * directly. That is a real remaining cost and it needs the two call sites to + * share a value, not a memo. + * + * Memoisation is per request, so a manifest published mid-request is still seen + * by the next one. `provisioning/reconcile.ts` does not read through here at + * all — it uses `declaredModulesNow(tx, …)` inside its own transaction — so + * there is no publish-then-read-back path this could stale. */ -export async function declaredModules(institutionId: string): Promise { +export const declaredModules = cache(async ( + institutionId: string, +): Promise => { const latest = await runUnscopedWidening( "control-plane", "a tenant's manifest is the input to capability availability", @@ -50,4 +77,4 @@ export async function declaredModules(institutionId: string): Promise Date: Tue, 25 Aug 2026 04:00:07 -0400 Subject: [PATCH 2/5] Claim the save before the first await, so a slow import cannot fork it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the independent review of this PR, which blocked on it. It is the most expensive defect in the queue today, because it loses a person's work quietly. `buildPayload` became async when the spreadsheet engine moved to a dynamic import. That put a network fetch — ~225 KB, first save of the session — between entering `doSave` and the lines that tell everyone else a save is running. For the length of that fetch `dirtyRef` was still true, `savingRef` still false and `savePromiseRef` still null. `flush()` is awaited by both the close path and the edit-to-view toggle, and it calls `doSave()` whenever `dirtyRef` is set. So a click inside that window could not see the save it was meant to wait for, and started a second one. Both POSTed the same `baseUpdatedAt`. Measured against a stub of the route's read-check and version compare-and-swap: main (buildPayload synchronous) close during save: POSTs=1 saved this PR before the fix close during save: POSTs=2 conflict The loser's 409 latches `conflictRef`, and the reader is told "Someone else saved a newer version" about a document nobody else touched. From that moment `scheduleSave` and `doSave` both early-return, so EVERY FURTHER KEYSTROKE IS DISCARDED WITH NO INDICATION, and closing the overlay loses them. On a budget spreadsheet. Two changes, and the ordering is the whole of it: · the claim — `dirtyRef = false`, `savingRef = true`, `setStatus("saving")` — now happens BEFORE any await, and the payload build moved inside the published promise. Nothing may yield between entering the function and `savePromiseRef.current = p`. · a second caller JOINS the save in flight instead of starting one, which is what `savePromiseRef` was always for. It also fixes the cosmetic half the review noted: the pill read "Unsaved changes" for the whole chunk download, because `setStatus("saving")` was behind the await. ONE DELIBERATE BEHAVIOUR CHANGE, stated rather than slipped in: `if (!payload) return` now runs with `dirtyRef` already cleared. `buildPayload` returns null only when no document is loaded or the content kind is unknown — neither is reachable while dirty, since you cannot edit a document that is not loaded — and re-marking it dirty would spin the 1.5s autosave against a payload that will never build. Also removed a docstring that described where this code used to be, and cited "the close and beforeunload paths". There is no `beforeunload` handler anywhere in the repository; the grep's only match was that comment. Zero now. The test asserts the invariant the fix rests on — no await before the claim — and says plainly that it is a source check and what that cannot cover. Verified against the version this PR shipped: 3 of its 4 cases fail, and the vacuity guard still passes. 376 tests pass across components. tsc 306, parity with this PR's head. --- .../documents/DocumentViewerOverlay.tsx | 68 +++++++++------ ...slow-import-does-not-fork-the-save.test.ts | 86 +++++++++++++++++++ 2 files changed, 130 insertions(+), 24 deletions(-) create mode 100644 apps/web/src/components/documents/a-slow-import-does-not-fork-the-save.test.ts diff --git a/apps/web/src/components/documents/DocumentViewerOverlay.tsx b/apps/web/src/components/documents/DocumentViewerOverlay.tsx index 8878da6e..5ffbf3ae 100644 --- a/apps/web/src/components/documents/DocumentViewerOverlay.tsx +++ b/apps/web/src/components/documents/DocumentViewerOverlay.tsx @@ -198,39 +198,59 @@ export function DocumentViewerOverlay({ const doSave = useCallback(async () => { if (conflictRef.current) return /* - * Building the payload can now FAIL, and the failure has to be treated as a - * failed save rather than as no save. + * AN IN-FLIGHT SAVE ANSWERS FOR THIS CALLER TOO. * - * `buildPayload` fetches the spreadsheet engine on demand, so an offline - * laptop or a deploy whose chunk has rolled away throws here. Left - * unhandled that would reject out of `doSave`, and `flush()` — which the - * close and beforeunload paths await — would reject with it, so the overlay - * would close on a document whose last edits never left the browser and - * never said so. - * - * Marked dirty and errored, which is exactly what a failed POST does below: - * the 1.5s autosave is already a retry loop, and the second attempt gets - * the chunk from cache or fails visibly again. + * `flush()` is awaited by both the close path and the edit->view toggle, + * and it calls `doSave()` whenever `dirtyRef` is set. Without this, a click + * during a save starts a SECOND one, and both POST the same + * `baseUpdatedAtRef` — the loser gets a 409 and latches `conflictRef`, + * telling the reader somebody else edited a document nobody else touched, + * after which every keystroke is discarded in silence. */ - let payload: SavePayload | null - try { - payload = await buildPayload() - } catch { - dirtyRef.current = true - setStatus("error") + if (savingRef.current && savePromiseRef.current) { + await savePromiseRef.current return } - if (!payload) return + /* + * CLAIMED BEFORE THE FIRST `await`, and that ordering is the whole fix. + * + * `buildPayload` became async when the spreadsheet engine moved to a + * dynamic import, which put a network fetch — ~225 KB, first save of the + * session — between entering this function and saying that a save is + * running. For the length of that fetch `dirtyRef` was still true, + * `savingRef` still false and `savePromiseRef` still null, so `flush()` + * could not see the save it was supposed to wait for. Measured against a + * stub of the route's compare-and-swap: two POSTs and a false conflict on + * both the close and the toggle path, where main produced one and `saved`. + * + * Nothing may `await` between here and the assignment of `savePromiseRef` + * below. The payload build has therefore moved INSIDE `p`. + */ dirtyRef.current = false savingRef.current = true setStatus("saving") const p = (async () => { try { - const res = await fetch(`/api/documents/${docId}/save`, { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ ...payload, baseUpdatedAt: baseUpdatedAtRef.current }), - }) + let payload: SavePayload | null + try { + payload = await buildPayload() + } catch { + // The chunk fetch failed — an offline laptop, or a deploy whose chunk + // has rolled away. Dirty and errored, which is what a failed POST + // does below: the 1.5s autosave is already a retry loop. + dirtyRef.current = true + setStatus("error") + return + } + // No document loaded, or a kind with nothing to send. Not reachable + // while dirty — you cannot edit a document that is not loaded — and + // deliberately NOT re-marked dirty, which would spin the autosave. + if (!payload) return + const res = await fetch(`/api/documents/${docId}/save`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ...payload, baseUpdatedAt: baseUpdatedAtRef.current }), + }) if (res.status === 409) { conflictRef.current = true setStatus("conflict") diff --git a/apps/web/src/components/documents/a-slow-import-does-not-fork-the-save.test.ts b/apps/web/src/components/documents/a-slow-import-does-not-fork-the-save.test.ts new file mode 100644 index 00000000..44d60d80 --- /dev/null +++ b/apps/web/src/components/documents/a-slow-import-does-not-fork-the-save.test.ts @@ -0,0 +1,86 @@ +import { readFileSync } from "node:fs" +import path from "node:path" + +/** + * NOTHING MAY `await` BETWEEN ENTERING `doSave` AND PUBLISHING THE SAVE. + * + * ── The defect this exists to prevent, which shipped once ─────────────────── + * + * `buildPayload` became async when the spreadsheet engine moved to a dynamic + * import. That put a network fetch — ~225 KB, first save of the session — + * between `doSave`'s entry and the three lines that tell everyone else a save + * is running: `dirtyRef = false`, `savingRef = true`, `savePromiseRef = p`. + * + * `flush()` is awaited by both the close path and the edit-to-view toggle, and + * it calls `doSave()` whenever `dirtyRef` is set. During that fetch `dirtyRef` + * was still true and `savingRef` still false, so `flush()` could not see the + * save it was meant to wait for and started a second one. Both POSTed the same + * `baseUpdatedAt`; the loser got a 409 and latched `conflictRef`. + * + * The reader is then told "Someone else saved a newer version" about a document + * nobody else touched — and from that moment `scheduleSave` and `doSave` both + * early-return, so every further keystroke is discarded WITH NO INDICATION, and + * closing the overlay loses them. On a budget spreadsheet. + * + * ── Why this is a source check and what that costs ───────────────────────── + * + * The property is about ORDERING INSIDE ONE FUNCTION, and it is invisible to a + * rendering test unless that test can hold the dynamic import open at exactly + * the right moment and click during it. What is asserted here is the invariant + * the fix rests on, stated where it can be read: no `await` before the claim. + * + * It is a text scan, so it is worth saying what it cannot do — it cannot tell + * that `savePromiseRef` is the promise `flush` awaits, only that nothing yields + * before it is set. The behavioural half is covered by the route's own + * compare-and-swap tests in `lib/documents/concurrent-save.itest.ts`. + */ + +const OVERLAY = path.resolve(__dirname, "DocumentViewerOverlay.tsx") +const src = readFileSync(OVERLAY, "utf8") + +/** `doSave`'s body, up to the line that publishes the in-flight promise. */ +function bodyBeforeTheClaim(): string { + const start = src.indexOf("const doSave = useCallback(async () => {") + expect(start).toBeGreaterThan(-1) + const claim = src.indexOf("savingRef.current = true", start) + expect(claim).toBeGreaterThan(start) + return src.slice(start, claim) +} + +/** With comments removed — an `await` written in prose is not one. */ +const withoutComments = (text: string) => + text.replace(/\/\*[\s\S]*?\*\//g, "").replace(/^[^\S\n]*\/\/.*$/gm, "") + +describe("a slow import does not fork the save", () => { + it("finds the overlay and its save, so this is not vacuously passing", () => { + expect(src.length).toBeGreaterThan(5_000) + expect(src).toContain("savePromiseRef") + expect(src).toContain("buildPayload") + }) + + it("does not await anything before it says a save is running", () => { + // THE INVARIANT. Every `await` above this line is a window in which + // `flush()` sees a dirty document and no save, and starts a second one. + const before = withoutComments(bodyBeforeTheClaim()) + const awaits = [...before.matchAll(/\bawait\b/g)].map((m) => + before.slice(Math.max(0, m.index! - 60), m.index! + 40).trim(), + ) + // One is allowed and only one: awaiting the save that is ALREADY running, + // which is the opposite of forking — it is how a second caller joins. + expect(awaits).toEqual(["await savePromiseRef.current"].map(expect.stringContaining)) + }) + + it("builds the payload inside the published promise, not before it", () => { + // The corollary. If the build is above the claim, the first assertion can + // still pass by someone hoisting only the flags and leaving the await. + const claim = src.indexOf("savingRef.current = true") + const build = src.indexOf("await buildPayload()") + expect(build).toBeGreaterThan(claim) + }) + + it("lets a second caller join the save in flight rather than start one", () => { + const before = withoutComments(bodyBeforeTheClaim()) + expect(before).toMatch(/if\s*\(\s*savingRef\.current\s*&&\s*savePromiseRef\.current\s*\)/) + expect(before).toContain("await savePromiseRef.current") + }) +}) From 7312c69401edff4fbf43918c542b072163803ace Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 19:40:28 -0400 Subject: [PATCH 3/5] Vary: Cookie, and an honest account of how much it closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer finding. Taken, and the severity stated accurately rather than inflated — because the reasoning that makes it small is itself the reason to add the header. `private` keeps the answer out of shared caches, which is what the existing comment is about and it is right. It expressly PERMITS the browser's own cache, whose key is the URL — so two people signed in one after another on the same machine share one entry for the same image URL, and the route where the entitlement is checked never runs. HOW BIG IS THAT, HONESTLY. Smaller than it sounds. A second viewer's browser only requests `/api/profile-image/` if a page RENDERS it, and a page renders it only when that viewer is entitled to see it — the same `sharesAnInstitution` and `canViewOrg` decision the route makes. The ordinary shared-computer case hands the second viewer bytes they were already allowed. WHAT IT DOES CLOSE, and this part is real: a viewer whose entitlement was REVOKED, on the same machine, inside the 300s window, where a back-navigation or a stale tab re-renders the URL. The browser answers from its own store and the revocation is never consulted. WHY ADD IT ANYWAY. It costs one header, and the argument that makes the leak unreachable is a claim about TODAY'S authorization model — that a page never renders an image its viewer may not see. That is true of the two routes that exist and nothing enforces it for the third. This codebase has watched premises like that go stale twice today already. `Cookie` rather than `Authorization`: both routes authenticate from the session cookie via `auth()`, so the cookie is what distinguishes one viewer from another. Three assertions, and the split matters: one on the constant, and one on EACH route's actual response — declared is not sent, and a constant nobody attaches is a value with no effect. Mutation-proved: removing `Vary` from the profile route alone fails the avatar case and leaves the club case passing, so the two are independently covered. Verified: jest 3 failed / 5,955 passed — the three pre-existing stale-client suites; this suite 10 -> 11. ESLint clean. tsc reads 306 here, and that is THIS BRANCH's baseline rather than the 307 I have been carrying from main — measured by stashing and re-running against the pristine branch, which also gives 306. Zero errors in any file this commit touches. Worth recording that the baseline is per-branch: #285 read 310 for a stale generated client, this one reads 306. Co-Authored-By: Claude Opus 5 (1M context) --- .../an-image-proxy-answer-is-reusable.test.ts | 12 ++++++- .../src/app/api/org-image/[orgId]/route.ts | 4 +-- .../app/api/profile-image/[userId]/route.ts | 4 +-- apps/web/src/lib/storage/image-proxy-cache.ts | 35 +++++++++++++++++++ 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts b/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts index c234dd84..ef64516f 100644 --- a/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts +++ b/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts @@ -31,7 +31,7 @@ * decides how long a departed member can still fetch the bytes — the presign * already answers that, and it says 600. */ -import { IMAGE_PROXY_CACHE_CONTROL } from "@/lib/storage/image-proxy-cache" +import { IMAGE_PROXY_CACHE_CONTROL, IMAGE_PROXY_VARY } from "@/lib/storage/image-proxy-cache" const state = { session: null as { user: { id: string } } | null, @@ -91,6 +91,8 @@ describe("the redirect is reusable", () => { expect(res.status).toBe(307) expect(res.headers.get("Cache-Control")).toBe(IMAGE_PROXY_CACHE_CONTROL) + // Declared is not sent: assert the response carries it, not just the const. + expect(res.headers.get("Vary")).toBe(IMAGE_PROXY_VARY) }) it("stamps the club-image redirect the same way, from the same constant", async () => { @@ -98,6 +100,8 @@ describe("the redirect is reusable", () => { expect(res.status).toBe(307) expect(res.headers.get("Cache-Control")).toBe(IMAGE_PROXY_CACHE_CONTROL) + // Declared is not sent: assert the response carries it, not just the const. + expect(res.headers.get("Vary")).toBe(IMAGE_PROXY_VARY) }) it("still sends the Location it was sending before", async () => { @@ -187,6 +191,12 @@ describe("the directive itself", () => { // on the other. A shared cache that stored the answer would hand it to // somebody who would have been refused. expect(IMAGE_PROXY_CACHE_CONTROL).toMatch(/\bprivate\b/) + // `private` keeps the answer out of SHARED caches and expressly permits the + // browser's own, whose key is the URL. Two people signed in one after + // another on one machine would share that entry, and the route — which is + // where the entitlement is checked — never runs. `Vary: Cookie` puts the + // session in the key, so a revoked viewer's browser has to ask again. + expect(IMAGE_PROXY_VARY).toBe("Cookie") expect(IMAGE_PROXY_CACHE_CONTROL).not.toMatch(/\bpublic\b|\bs-maxage\b|\bimmutable\b/) }) diff --git a/apps/web/src/app/api/org-image/[orgId]/route.ts b/apps/web/src/app/api/org-image/[orgId]/route.ts index db5e6b3e..51eb0774 100644 --- a/apps/web/src/app/api/org-image/[orgId]/route.ts +++ b/apps/web/src/app/api/org-image/[orgId]/route.ts @@ -4,7 +4,7 @@ import { db } from "@/lib/db" import { canViewOrg, getUserContext } from "@/lib/rbac" import { withTenantScope } from "@/lib/tenant-scope" import { documentViewUrl } from "@/lib/s3" -import { IMAGE_PROXY_CACHE_CONTROL } from "@/lib/storage/image-proxy-cache" +import { IMAGE_PROXY_CACHE_CONTROL, IMAGE_PROXY_VARY } from "@/lib/storage/image-proxy-cache" /** * Serves an uploaded club image by redirecting to a short-lived signed URL. @@ -38,6 +38,6 @@ export async function GET( // Same window, same reasoning, one constant. /orgs draws one of these per // club spine and re-ran this whole route for each of them on every load. // The 404 and the 403 above are left uncacheable deliberately. - return NextResponse.redirect(url, { headers: { "Cache-Control": IMAGE_PROXY_CACHE_CONTROL } }) + return NextResponse.redirect(url, { headers: { "Cache-Control": IMAGE_PROXY_CACHE_CONTROL, Vary: IMAGE_PROXY_VARY } }) }) } diff --git a/apps/web/src/app/api/profile-image/[userId]/route.ts b/apps/web/src/app/api/profile-image/[userId]/route.ts index 9cf69363..8a4d6a7d 100644 --- a/apps/web/src/app/api/profile-image/[userId]/route.ts +++ b/apps/web/src/app/api/profile-image/[userId]/route.ts @@ -3,7 +3,7 @@ import { auth } from "@/lib/auth" import { db } from "@/lib/db" import { sharesAnInstitution } from "@/lib/people/shared-institution" import { documentViewUrl } from "@/lib/s3" -import { IMAGE_PROXY_CACHE_CONTROL } from "@/lib/storage/image-proxy-cache" +import { IMAGE_PROXY_CACHE_CONTROL, IMAGE_PROXY_VARY } from "@/lib/storage/image-proxy-cache" /** * Serves an uploaded profile picture by redirecting to a short-lived signed URL. @@ -65,6 +65,6 @@ export async function GET( // carries it; the two 404s above stay uncacheable on purpose, because one of // them means "not yours to see" and access can be granted a second later. return NextResponse.redirect(await documentViewUrl(user.imageKey), { - headers: { "Cache-Control": IMAGE_PROXY_CACHE_CONTROL }, + headers: { "Cache-Control": IMAGE_PROXY_CACHE_CONTROL, Vary: IMAGE_PROXY_VARY }, }) } diff --git a/apps/web/src/lib/storage/image-proxy-cache.ts b/apps/web/src/lib/storage/image-proxy-cache.ts index cc819b41..25b3a14f 100644 --- a/apps/web/src/lib/storage/image-proxy-cache.ts +++ b/apps/web/src/lib/storage/image-proxy-cache.ts @@ -37,3 +37,38 @@ * after a seat is granted. */ export const IMAGE_PROXY_CACHE_CONTROL = "private, max-age=300" + +/** + * The other half of `private`, and worth being accurate about its size. + * + * `private` keeps the answer out of shared caches. It expressly PERMITS the + * browser's own cache, whose key is the URL — so two people signed in one after + * another on the same machine share one cache entry for the same image URL. + * + * ── How large is that, honestly ───────────────────────────────────────────── + * + * Smaller than it sounds, and the reason is worth writing down rather than + * asserting a leak. A second viewer's browser only requests + * `/api/profile-image/` if a page RENDERS it, and a page only renders it + * when that viewer is entitled to see it — the same `sharesAnInstitution` and + * `canViewOrg` decision the route makes. So the ordinary shared-computer case + * hands the second viewer bytes they were already allowed. + * + * The case it does close is narrow and real: a viewer whose entitlement was + * REVOKED, on the same machine, inside the 300s window, where a back-navigation + * or a stale tab re-renders the URL. Without `Vary`, the browser answers from + * its own store and the route never runs, so the revocation is not consulted. + * + * ── Why add it anyway ─────────────────────────────────────────────────────── + * + * It costs one header, and the argument that makes the leak unreachable is a + * claim about TODAY'S authorization model — that a page never renders an image + * the viewer may not see. That is exactly the kind of premise this codebase has + * watched go stale: it is true of the two routes that exist now, and nothing + * enforces it for the third. + * + * `Cookie` and not `Authorization`: these routes authenticate from the session + * cookie via `auth()`, so the cookie is what distinguishes one viewer from + * another. + */ +export const IMAGE_PROXY_VARY = "Cookie" From 60d2ec112997beed73143d0a4342e99c7515f7d3 Mon Sep 17 00:00:00 2001 From: Satvik Date: Tue, 25 Aug 2026 19:58:33 -0400 Subject: [PATCH 4/5] An edit typed during a save was dropped by the joiner The second caller into doSave() joined the save already in flight and returned as soon as it settled. That save had captured textRef/sheetsRef at its OWN start, so anything typed while it was in the air was never in its POST -- and the joiner returned as if it had been saved. requestClose reaches this path: a keystroke sets dirtyRef, flush() calls doSave(), doSave() joins, and requestClose then calls onOpenChange(false). The overlay closes with dirtyRef still true and no debounce pending, so the edits are gone and nothing on screen has said so. The edit-to-view toggle is the same path. Made the join a loop that re-checks after the awaited save settles, and falls through to save the newer text. It terminates on the two conditions the guard already had -- nothing left to save, or a latched conflict -- and both are pinned by tests. Mutation-proved: reverting while->if fails 3 cases; deleting the re-check fails the fall-through case. --- .../documents/DocumentViewerOverlay.tsx | 23 +++++++++++-- ...slow-import-does-not-fork-the-save.test.ts | 32 ++++++++++++++++++- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/apps/web/src/components/documents/DocumentViewerOverlay.tsx b/apps/web/src/components/documents/DocumentViewerOverlay.tsx index 5ffbf3ae..d296102c 100644 --- a/apps/web/src/components/documents/DocumentViewerOverlay.tsx +++ b/apps/web/src/components/documents/DocumentViewerOverlay.tsx @@ -207,9 +207,28 @@ export function DocumentViewerOverlay({ * telling the reader somebody else edited a document nobody else touched, * after which every keystroke is discarded in silence. */ - if (savingRef.current && savePromiseRef.current) { + /* + * A JOINER MUST RE-CHECK, and this is a `while` for that reason. + * + * Returning as soon as the running save settles loses every keystroke typed + * DURING it. That save captured `textRef`/`sheetsRef` at its own start, so + * those edits were never in its POST — and `requestClose` reaches here: + * a keystroke sets `dirtyRef`, `flush()` calls `doSave()`, `doSave()` joins, + * and `requestClose` then calls `onOpenChange(false)`. The overlay closes + * with `dirtyRef` still true and no debounce pending, so the edits are gone + * with nothing on screen having said so. The edit->view toggle is the same + * path. + * + * So a joiner falls THROUGH to perform a real save when the document is + * still dirty. It terminates: each iteration either joins a save that + * completes, or leaves the loop to do the save itself, and a latched + * conflict returns immediately. The condition is re-evaluated rather than + * checked once, because another save may have started while this one + * waited. + */ + while (savingRef.current && savePromiseRef.current) { await savePromiseRef.current - return + if (!dirtyRef.current || conflictRef.current) return } /* * CLAIMED BEFORE THE FIRST `await`, and that ordering is the whole fix. diff --git a/apps/web/src/components/documents/a-slow-import-does-not-fork-the-save.test.ts b/apps/web/src/components/documents/a-slow-import-does-not-fork-the-save.test.ts index 44d60d80..aa8efb81 100644 --- a/apps/web/src/components/documents/a-slow-import-does-not-fork-the-save.test.ts +++ b/apps/web/src/components/documents/a-slow-import-does-not-fork-the-save.test.ts @@ -80,7 +80,37 @@ describe("a slow import does not fork the save", () => { it("lets a second caller join the save in flight rather than start one", () => { const before = withoutComments(bodyBeforeTheClaim()) - expect(before).toMatch(/if\s*\(\s*savingRef\.current\s*&&\s*savePromiseRef\.current\s*\)/) + // `while`, not `if` — the case below is why that difference decides + // whether an edit survives. The join itself is unchanged. + expect(before).toMatch(/while\s*\(\s*savingRef\.current\s*&&\s*savePromiseRef\.current\s*\)/) expect(before).toContain("await savePromiseRef.current") }) + + it("makes the joiner fall through when the document is still dirty", () => { + // THE DEFECT. As an `if`, the joiner returned the moment the running save + // settled — and that save captured `textRef`/`sheetsRef` at its OWN start, + // so keystrokes typed during it were never in its POST. + // + // `requestClose` reaches this path: a keystroke sets `dirtyRef`, `flush()` + // calls `doSave()`, `doSave()` joins, and `requestClose` then calls + // `onOpenChange(false)`. The overlay closes with `dirtyRef` still true and + // no debounce pending, so those edits are gone with nothing on screen + // having said so. The edit-to-view toggle is the same path. + // + // As a `while` the joiner re-checks and falls through to save them. Two + // things make that terminate and both are asserted: it returns when there + // is nothing left to save, and it returns on a latched conflict. + const before = withoutComments(bodyBeforeTheClaim()) + expect(before).toMatch(/while\s*\(/) + expect(before).toMatch(/if\s*\(\s*!dirtyRef\.current\s*\|\|\s*conflictRef\.current\s*\)\s*return/) + }) + + it("still refuses outright once a conflict is latched", () => { + // Unchanged by the loop and worth pinning beside it: a joiner must not fall + // through into a save that the conflict guard exists to prevent. + const before = withoutComments(bodyBeforeTheClaim()) + expect(before.indexOf("if (conflictRef.current) return")).toBeLessThan( + before.indexOf("while ("), + ) + }) }) From b6fd364e05423ec843ce3f34456340ddc776f527 Mon Sep 17 00:00:00 2001 From: Satvik Date: Tue, 25 Aug 2026 20:05:35 -0400 Subject: [PATCH 5/5] Four reviewer findings: a doubled poll, a five-minute window, and two false comments image-proxy-cache: max-age 300 -> 60, and the documentation corrected. `Vary: Cookie` was described as closing the revocation case. It does not: it separates one VIEWER from another, but a person whose own seat is revoked still sends the same cookie, so the key is unchanged and they keep hitting their own entry until it goes stale. Only max-age bounds that. Of the three remedies available, two are not: no-store and revalidation both run the route on every image, which is the whole cost this removes, and a browser cache cannot be invalidated from the server (the ?v= stamp works for uploads only because the writer owns the URL). So the control is the length of the window. Nearly all of the saving is a page's own images plus an immediate back-navigation; the further four minutes bought little and were four minutes in which a revocation went unconsulted. The presign still sets the ceiling at 600s -- what changed is how much of it happens without anyone trying. NotificationBell: one return was costing two requests. A hidden tab in an UNFOCUSED window -- the ordinary alt-tab -- delivers visibilitychange AND focus for one gesture. Neither listener can be dropped, because each is the only one that fires in one of the other two cases, so the pair is coalesced by time: 1s, a thirtieth of the cadence, which can absorb an event delivered alongside another and nothing else. SideNav named app/(app)/loading.tsx as filling the content region. That file is deliberately absent -- a boundary there makes notFound() and redirect() answer 200, which is what this PR's own test forbids. So the nav mark is not a companion to a spinner; it is the only feedback there. The loading-boundary test compared path.relative() output, which is backslash-separated on Windows, against "/"-written expectations. CI is ubuntu-latest, so it would only ever fail on the one machine nobody else could reproduce it on. Mutation-proved: reverting each of the three code changes fails a case, and widening the coalescing window to 60s fails two. --- ...ding-boundary-swallows-the-refusal.test.ts | 14 ++++- .../an-image-proxy-answer-is-reusable.test.ts | 32 ++++++++--- .../app/api/profile-image/[userId]/route.ts | 2 +- .../src/components/shell/NotificationBell.tsx | 32 ++++++++++- apps/web/src/components/shell/SideNav.tsx | 11 ++-- .../shell/a-hidden-tab-stops-asking.test.tsx | 34 ++++++++++++ apps/web/src/lib/storage/image-proxy-cache.ts | 54 ++++++++++++++----- 7 files changed, 151 insertions(+), 28 deletions(-) diff --git a/apps/web/src/app/(app)/a-loading-boundary-swallows-the-refusal.test.ts b/apps/web/src/app/(app)/a-loading-boundary-swallows-the-refusal.test.ts index a719116e..cb612434 100644 --- a/apps/web/src/app/(app)/a-loading-boundary-swallows-the-refusal.test.ts +++ b/apps/web/src/app/(app)/a-loading-boundary-swallows-the-refusal.test.ts @@ -62,12 +62,22 @@ const APP_DIR = path.resolve(__dirname, "../../app") */ const PRE_EXISTING = ["(app)/reports"] +/** + * `path.relative` answers in the host's separator, so this walk yields + * `(app)\\reports` on Windows and `(app)/reports` everywhere else — while + * `PRE_EXISTING` and every assertion below are written with `/`. CI is + * ubuntu-latest, so the mismatch would never show there; it would show as a + * failure nobody else could reproduce on the one machine that runs Windows. + * Both walks below therefore state their answer in `/`, always. + */ +const posix = (p: string) => p.split(path.sep).join("/") + /** Directories holding a `loading.tsx`, relative to `app/`. */ function loadingBoundaries(): string[] { const out: string[] = [] const walk = (dir: string) => { const entries = readdirSync(dir) - if (entries.includes("loading.tsx")) out.push(path.relative(APP_DIR, dir) || "/") + if (entries.includes("loading.tsx")) out.push(posix(path.relative(APP_DIR, dir)) || "/") for (const entry of entries) { const child = path.join(dir, entry) if (statSync(child).isDirectory()) walk(child) @@ -86,7 +96,7 @@ function refusalsUnder(dir: string): string[] { if (statSync(child).isDirectory()) walk(child) else if (entry === "page.tsx") { const src = readFileSync(child, "utf8") - if (/\bnotFound\(\)|\bredirect\(/.test(src)) out.push(path.relative(APP_DIR, child)) + if (/\bnotFound\(\)|\bredirect\(/.test(src)) out.push(posix(path.relative(APP_DIR, child))) } } } diff --git a/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts b/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts index ef64516f..06524be4 100644 --- a/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts +++ b/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts @@ -22,14 +22,15 @@ * · the org route's 403, no; * · the 401, no. * - * A refusal cached for five minutes keeps refusing for five minutes after a - * seat is granted, which is a support ticket that reproduces for nobody. + * A cached refusal keeps refusing after a seat is granted, which is a support + * ticket that reproduces for nobody. * - * And it must stay `private` and stay inside the presign's own life. `private` - * is what stops a CDN holding one viewer's authorization decision and serving - * it to another; the 300s ceiling is what keeps this from being the thing that - * decides how long a departed member can still fetch the bytes — the presign - * already answers that, and it says 600. + * And it must stay `private` and stay well inside the presign's own life. + * `private` is what stops a CDN holding one viewer's authorization decision and + * serving it to another. The `max-age` is what decides how long a revocation + * goes unconsulted, because while the entry is fresh the route does not run at + * all — so it is asserted here as a NUMBER, not just as "the constant is the + * constant", which would pass at any value including five minutes or a day. */ import { IMAGE_PROXY_CACHE_CONTROL, IMAGE_PROXY_VARY } from "@/lib/storage/image-proxy-cache" @@ -85,6 +86,16 @@ const profile = (userId: string) => profileImage(request, { params: Promise.resolve({ userId }) }) const org = (orgId: string) => orgImage(request, { params: Promise.resolve({ orgId }) }) +describe("the window is short enough to be worth having", () => { + // These are about the value, not the plumbing. `max-age` is the only control + // over how long a revoked viewer keeps being served from their own cache + // without the route running: `no-store` and revalidation both defeat the + // purpose, and a browser cache cannot be invalidated from the server. + it("caches for a minute, not for five", () => { + expect(IMAGE_PROXY_CACHE_CONTROL).toBe("private, max-age=60") + }) +}) + describe("the redirect is reusable", () => { it("stamps the avatar redirect so a second look at the same page is free", async () => { const res = await profile("u2") @@ -195,7 +206,12 @@ describe("the directive itself", () => { // browser's own, whose key is the URL. Two people signed in one after // another on one machine would share that entry, and the route — which is // where the entitlement is checked — never runs. `Vary: Cookie` puts the - // session in the key, so a revoked viewer's browser has to ask again. + // session in the key, so the SECOND PERSON has to ask again. + // + // It does not do the other thing, and this comment used to say it did: one + // person whose own seat is revoked still sends the same cookie, so their + // key is unchanged and they keep hitting their own entry. Only `max-age` + // bounds that, which is what the case above is about. expect(IMAGE_PROXY_VARY).toBe("Cookie") expect(IMAGE_PROXY_CACHE_CONTROL).not.toMatch(/\bpublic\b|\bs-maxage\b|\bimmutable\b/) }) diff --git a/apps/web/src/app/api/profile-image/[userId]/route.ts b/apps/web/src/app/api/profile-image/[userId]/route.ts index 8a4d6a7d..2b5c3c95 100644 --- a/apps/web/src/app/api/profile-image/[userId]/route.ts +++ b/apps/web/src/app/api/profile-image/[userId]/route.ts @@ -60,7 +60,7 @@ export async function GET( const user = await db.user.findUnique({ where: { id: userId }, select: { imageKey: true } }) if (!user?.imageKey) return new NextResponse("Not found", { status: 404 }) - // Reusable for five minutes by THIS browser and nothing else — see + // Reusable for one minute by THIS browser and nothing else — see // IMAGE_PROXY_CACHE_CONTROL for why that widens nothing. Only the redirect // carries it; the two 404s above stay uncacheable on purpose, because one of // them means "not yours to see" and access can be granted a second later. diff --git a/apps/web/src/components/shell/NotificationBell.tsx b/apps/web/src/components/shell/NotificationBell.tsx index 6a324085..7b73370b 100644 --- a/apps/web/src/components/shell/NotificationBell.tsx +++ b/apps/web/src/components/shell/NotificationBell.tsx @@ -1,6 +1,6 @@ "use client" -import { useCallback, useEffect, useMemo, useState } from "react" +import { useCallback, useEffect, useMemo, useRef, useState } from "react" import Link from "next/link" import { Button, @@ -63,6 +63,14 @@ function groupByTime(items: NotificationItem[]): Group[] { * with the full history (grouped by time); the /notifications route still works * for deep links. */ +/** + * How close together two "the person is back" events must be to be one return. + * + * Sized to absorb the pair one alt-tab delivers, and nothing else: it is 1/30th + * of the poll cadence, so it can never swallow a tick or a later return. + */ +const PAIRED_RETURN_MS = 1_000 + export function NotificationBell({ initialUnread = 0 }: { initialUnread?: number }) { const [unread, setUnread] = useState(initialUnread) const [items, setItems] = useState([]) @@ -85,6 +93,9 @@ export function NotificationBell({ initialUnread = 0 }: { initialUnread?: number } }, []) + /** When the last return-triggered refresh went out — see the effect below. */ + const lastReturn = useRef(0) + const loadHistory = useCallback(async () => { try { const res = await fetch("/api/notifications?limit=100", { cache: "no-store" }) @@ -126,6 +137,19 @@ export function NotificationBell({ initialUnread = 0 }: { initialUnread?: number * only `visibilitychange`, and a window regaining focus with the tab already * visible fires only `focus`. `usePolling` in components/charts/hooks.ts * already made both of these decisions; this is the same shape. + * + * But the third case is the common one, and it fires BOTH: a hidden tab in + * an UNFOCUSED window — alt-tab away, alt-tab back — delivers + * `visibilitychange` and then `focus` for one single return, so an unguarded + * handler spends two requests where the person made one gesture. Neither + * listener can be dropped to fix it, because each is the only one that fires + * in one of the other two cases. So the pair is coalesced by time instead: + * the first of them refreshes, and the second, arriving inside + * `PAIRED_RETURN_MS`, is the same return and declines to ask again. + * + * That window is deliberately far shorter than the 30s cadence, so it can + * only ever absorb an event delivered alongside another; a genuine second + * return is seconds away at the very least and still costs its own catch-up. */ useEffect(() => { const tick = () => { @@ -134,7 +158,11 @@ export function NotificationBell({ initialUnread = 0 }: { initialUnread?: number refresh() const id = setInterval(tick, 30_000) const onVisible = () => { - if (document.visibilityState === "visible") refresh() + if (document.visibilityState !== "visible") return + const now = Date.now() + if (now - lastReturn.current < PAIRED_RETURN_MS) return + lastReturn.current = now + refresh() } window.addEventListener("focus", onVisible) document.addEventListener("visibilitychange", onVisible) diff --git a/apps/web/src/components/shell/SideNav.tsx b/apps/web/src/components/shell/SideNav.tsx index 07340b8a..12fbeefe 100644 --- a/apps/web/src/components/shell/SideNav.tsx +++ b/apps/web/src/components/shell/SideNav.tsx @@ -78,9 +78,14 @@ const ITEM_BASE = * `usePathname`, and the pathname does not change until the navigation COMMITS * — which on these `force-dynamic` routes is the very thing being waited for. * So between the click and the answer the nav still marks the page being left, - * and the entry just clicked shows nothing at all. `app/(app)/loading.tsx` now - * fills the content region during that window; this says which entry asked for - * it, which is the half the content region cannot express. + * and the entry just clicked shows nothing at all. + * + * There is deliberately no `app/(app)/loading.tsx` to fill the content region + * during that window — a route-level boundary there makes `notFound()` and + * `redirect()` answer 200, which is why it was withdrawn and why + * `a-loading-boundary-swallows-the-refusal.test.ts` now forbids it. So this + * mark is not a companion to a content-region spinner; for the whole of that + * window it is the ONLY thing on screen saying the click was received. * * A 2px pulse in the primary token plus a visually-hidden word, the same shape * `charts/RangeFilter.tsx` already uses for the same problem — a mark that only diff --git a/apps/web/src/components/shell/a-hidden-tab-stops-asking.test.tsx b/apps/web/src/components/shell/a-hidden-tab-stops-asking.test.tsx index a7a71024..284d1e3b 100644 --- a/apps/web/src/components/shell/a-hidden-tab-stops-asking.test.tsx +++ b/apps/web/src/components/shell/a-hidden-tab-stops-asking.test.tsx @@ -167,6 +167,40 @@ it("resumes the ordinary cadence after coming back", async () => { expect(polls()).toBe(afterReturn + 1) }) +it("counts a return that fires both events as one return", async () => { + // THE DEFECT. A hidden tab in an unfocused window is the ordinary alt-tab + // case, and coming back from it delivers `visibilitychange` AND `focus` for + // one gesture. Both handlers used to call refresh(), so one return cost two + // /api/notifications requests — each a dynamic invocation with a database + // read behind it, on every alt-tab, for every signed-in person. + await mount() + visibility = "hidden" + await elapse(30_000 * 2) + expect(polls()).toBe(1) + + await comeBack("visibilitychange") + await comeBack("focus") + + expect(polls()).toBe(2) +}) + +it("still charges a genuinely separate return its own catch-up", async () => { + // The control that keeps the fix above from becoming the bug. Coalescing by + // time is only safe while the window cannot reach the NEXT return, so this + // fails if the window is ever widened toward the cadence. + await mount() + visibility = "hidden" + await elapse(30_000) + await comeBack("focus") + expect(polls()).toBe(2) + + visibility = "hidden" + await elapse(30_000) + await comeBack("focus") + + expect(polls()).toBe(3) +}) + it("does not restart the interval's phase each time the tab is revealed", async () => { // The guard is on the tick, not on the effect. Reveal the tab at 10s, 20s and // 25s; a rebuilt timer would push its next tick 30s past the last reveal and diff --git a/apps/web/src/lib/storage/image-proxy-cache.ts b/apps/web/src/lib/storage/image-proxy-cache.ts index 25b3a14f..4ce35a7b 100644 --- a/apps/web/src/lib/storage/image-proxy-cache.ts +++ b/apps/web/src/lib/storage/image-proxy-cache.ts @@ -10,13 +10,37 @@ * threads issued twenty of them, and then the identical twenty again on the * next visit. * - * ── Why this does not widen any window ────────────────────────────────────── + * ── What this window is, and what actually bounds it ──────────────────────── * * What is cached is a redirect to a presigned URL that is ALREADY valid for - * 600s to whoever holds it (`documentViewUrl`, `expiresIn: 600`). The interval - * in which a viewer whose access just ended can still fetch those bytes is set - * by the presign, and is 600s with or without this header. 300s is half of it - * and cannot outlive it, so this adds no reachable second of exposure. + * 600s to whoever holds it (`documentViewUrl`, `expiresIn: 600`, s3.ts). So the + * CEILING on how long a viewer whose access just ended can still fetch those + * bytes is set by the presign and not by this header: they hold a URL S3 will + * honour until it expires, whatever Tenure answers in the meantime. + * + * What this header decides is a different and smaller thing — how much of that + * ceiling happens AUTOMATICALLY. While the entry is fresh the browser answers + * from its own store and the route never runs, so `auth`, `sharesAnInstitution` + * and `canViewOrg` are not consulted and a revocation inside the window is not + * seen. `Vary: Cookie` does not help here and it is important not to claim it + * does: it separates one viewer's entry from another's, but a viewer whose + * SEAT was revoked still carries the same cookie and so still hits their own + * entry (see IMAGE_PROXY_VARY below for what it does do). + * + * ── Why 60s ───────────────────────────────────────────────────────────────── + * + * Of the three ways to close that gap, two are not available here. `no-store` + * and revalidation both run the route on every image, which is the entire cost + * this exists to remove. Invalidating the cache key on revocation is not + * reachable either: the key is the URL, and nothing Tenure does can reach into + * a browser's store — the `?v=` stamp works for uploads only because the writer + * controls the URL, and a revocation does not. + * + * So the available control is the length of the window, and 60s is chosen + * deliberately over the 300s this shipped with. Practically all of the saving + * is a page's own images plus an immediate back-navigation, both of which + * happen inside a minute; the further four minutes bought little and were four + * minutes in which a revocation went unconsulted. * * `private` is load-bearing: the routes make a PER-VIEWER authorization * decision (`sharesAnInstitution`, `canViewOrg`), so no shared cache or CDN may @@ -33,10 +57,11 @@ * the response was meant to be cacheable all along. * * A 404 must NOT carry this. "Not found" here is also how both routes say "not - * yours to see", and caching a refusal would keep answering it for five minutes - * after a seat is granted. + * yours to see", and caching a refusal would keep answering it for a minute + * after a seat is granted — which is why the two 404s, the 403 and the 401 all + * carry no directive at all rather than a shorter one. */ -export const IMAGE_PROXY_CACHE_CONTROL = "private, max-age=300" +export const IMAGE_PROXY_CACHE_CONTROL = "private, max-age=60" /** * The other half of `private`, and worth being accurate about its size. @@ -54,10 +79,15 @@ export const IMAGE_PROXY_CACHE_CONTROL = "private, max-age=300" * `canViewOrg` decision the route makes. So the ordinary shared-computer case * hands the second viewer bytes they were already allowed. * - * The case it does close is narrow and real: a viewer whose entitlement was - * REVOKED, on the same machine, inside the 300s window, where a back-navigation - * or a stale tab re-renders the URL. Without `Vary`, the browser answers from - * its own store and the route never runs, so the revocation is not consulted. + * The case it closes is narrow and real, and it is worth stating precisely + * because the neighbouring case looks identical and is NOT closed. What `Vary` + * closes: two DIFFERENT people on one machine, one after another — without it + * they share a single entry keyed only by the URL, so the second is answered + * from the first's authorization decision. What it does not close: one person + * whose own entitlement is revoked mid-session. Their cookie is unchanged, so + * the key is unchanged, and they keep hitting their own entry until it goes + * stale. That case is bounded by `max-age` and by nothing else, which is what + * the 60s above is for. * * ── Why add it anyway ─────────────────────────────────────────────────────── *