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..cb612434 --- /dev/null +++ b/apps/web/src/app/(app)/a-loading-boundary-swallows-the-refusal.test.ts @@ -0,0 +1,141 @@ +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"] + +/** + * `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(posix(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(posix(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 da7da867..0d8a4504 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..06524be4 --- /dev/null +++ b/apps/web/src/app/api/an-image-proxy-answer-is-reusable.test.ts @@ -0,0 +1,230 @@ +/** + * 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 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 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" + +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 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") + + 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 () => { + const res = await org("org1") + + 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 () => { + // 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/) + // `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 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/) + }) + + 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..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,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, IMAGE_PROXY_VARY } 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, 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 e4b7215d..2b5c3c95 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, IMAGE_PROXY_VARY } 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 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. + return NextResponse.redirect(await documentViewUrl(user.imageKey), { + headers: { "Cache-Control": IMAGE_PROXY_CACHE_CONTROL, Vary: IMAGE_PROXY_VARY }, + }) } 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 184eb571..54b26dc4 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, @@ -167,13 +166,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))) @@ -195,13 +215,82 @@ export function DocumentViewerOverlay({ if (conflictRef.current) return // Same bytes, same verdict. Editing clears this (see `scheduleSave`). if (rejectedRef.current) return - const payload = buildPayload() - if (!payload) return + /* + * AN IN-FLIGHT SAVE ANSWERS FOR THIS CALLER TOO. + * + * `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. + */ + /* + * 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 + // `rejectedRef` belongs in this list and it was not obvious. A rejection + // deliberately LEAVES `dirtyRef` true — the work really is unsaved, and + // `flush` and the beforeunload guard must keep saying so. So a joiner + // waiting on the save that got rejected would see dirty, fall through, + // and POST the identical bytes the server just refused. The two fixes are + // individually right and their combination is not, which no test on + // either branch could have caught. + if (!dirtyRef.current || conflictRef.current || rejectedRef.current) 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 { + 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" }, 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..34394687 --- /dev/null +++ b/apps/web/src/components/documents/a-slow-import-does-not-fork-the-save.test.ts @@ -0,0 +1,148 @@ +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()) + // `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]{0,80}?\)\s*return/) + }) + + it("does not let the joiner re-send bytes the server already refused", () => { + // FOUND BY A MERGE, not by either branch. The rejection guard and this loop + // are individually right and their combination was not. + // + // A 400 is a verdict on the CONTENT, so `rejectedRef` latches and the same + // bytes must not go again. But a rejection deliberately LEAVES `dirtyRef` + // true — the work really is unsaved, and `flush` and the beforeunload guard + // have to keep saying so. So a joiner waiting on the save that was rejected + // would wake, see dirty, fall through, and POST exactly what was refused. + // + // Neither branch could have caught it: the guard shipped before the loop + // existed, and the loop was written against a file that had no guard. + const before = withoutComments(bodyBeforeTheClaim()) + const loop = before.slice(before.indexOf("while (")) + expect(loop).toContain("rejectedRef.current") + // And the outright refusal still precedes the loop, so an already-latched + // rejection never reaches it at all. + // + // PRESENT FIRST, THEN ORDERED. `indexOf` answers -1 for something that is + // not there, and -1 is less than every real index — so the ordering + // assertion alone PASSES when the guard has been deleted. Caught by + // deleting it: all seven cases stayed green. + const guard = before.indexOf("if (rejectedRef.current) return") + const loopAt = before.indexOf("while (") + expect(guard).toBeGreaterThanOrEqual(0) + expect(loopAt).toBeGreaterThanOrEqual(0) + expect(guard).toBeLessThan(loopAt) + }) + + 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()) + // Same -1 hazard as the case above: assert presence before ordering. + const guard = before.indexOf("if (conflictRef.current) return") + const loopAt = before.indexOf("while (") + expect(guard).toBeGreaterThanOrEqual(0) + expect(loopAt).toBeGreaterThanOrEqual(0) + expect(guard).toBeLessThan(loopAt) + }) +}) diff --git a/apps/web/src/components/finance/BudgetUpload.tsx b/apps/web/src/components/finance/BudgetUpload.tsx index 21db0e52..4bbca2eb 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 988c6d5d..c0398369 100644 --- a/apps/web/src/components/shell/NotificationBell.tsx +++ b/apps/web/src/components/shell/NotificationBell.tsx @@ -171,6 +171,14 @@ function LoadPending() { * 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([]) @@ -246,6 +254,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 () => { setHistoryFailure(null) try { @@ -268,14 +279,62 @@ 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. + * + * 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 = () => { + 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") 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) 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..12fbeefe 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,49 @@ 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. + * + * 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 + * 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 +170,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..284d1e3b --- /dev/null +++ b/apps/web/src/components/shell/a-hidden-tab-stops-asking.test.tsx @@ -0,0 +1,223 @@ +/** + * @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("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 + // 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` 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 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 ─────────────────────────────────────────────────────── + * + * 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"