-
Notifications
You must be signed in to change notification settings - Fork 3.8k
perf(tables): stop a table write refetching every loaded page in the tab that made it #6698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| /** | ||
| * @vitest-environment node | ||
| */ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { CLIENT_ID_HEADER, fingerprintClientId, readClientId } from '@/lib/api/client-id' | ||
|
|
||
| describe('readClientId', () => { | ||
| it('reads the sending tab id off the request', () => { | ||
| const request = new Request('https://sim.ai/api/table/t1/rows', { | ||
| headers: { [CLIENT_ID_HEADER]: 'tab-abc' }, | ||
| }) | ||
| expect(readClientId(request)).toBe('tab-abc') | ||
| }) | ||
|
|
||
| /** Absent must read as "unattributed" — the signal then makes every client refetch, as before. */ | ||
| it('is undefined when the caller sent no id', () => { | ||
| const request = new Request('https://sim.ai/api/table/t1/rows') | ||
| expect(readClientId(request)).toBeUndefined() | ||
| }) | ||
|
|
||
| /** | ||
| * The value is caller-controlled and is broadcast to every subscriber of the table, so an | ||
| * over-long one is dropped rather than fanned out. | ||
| */ | ||
| it('drops an over-long id instead of broadcasting it', () => { | ||
| const request = new Request('https://sim.ai/api/table/t1/rows', { | ||
| headers: { [CLIENT_ID_HEADER]: 'x'.repeat(65) }, | ||
| }) | ||
| expect(readClientId(request)).toBeUndefined() | ||
| }) | ||
| }) | ||
|
|
||
| /** | ||
| * Every subscriber of a table sees every broadcast, so what gets published must not be replayable. | ||
| * If the raw id travelled, a collaborator could read it off the stream, send it as their own | ||
| * header, and have their write attributed to someone else's tab — which would then suppress a | ||
| * refetch it genuinely needed and sit on stale rows. | ||
| */ | ||
| describe('fingerprintClientId', () => { | ||
| it('is stable for the same id, so a tab recognises its own broadcast', async () => { | ||
| expect(await fingerprintClientId('tab-abc')).toBe(await fingerprintClientId('tab-abc')) | ||
| }) | ||
|
|
||
| it('differs between tabs, so one tab never suppresses on another tab’s write', async () => { | ||
| expect(await fingerprintClientId('tab-abc')).not.toBe(await fingerprintClientId('tab-xyz')) | ||
| }) | ||
|
|
||
| it('does not reveal the id it was derived from', async () => { | ||
| const fingerprint = await fingerprintClientId('tab-abc') | ||
| expect(fingerprint).not.toContain('tab-abc') | ||
| // SHA-256 hex — knowing this cannot produce the header value that would match it. | ||
| expect(fingerprint).toMatch(/^[0-9a-f]{64}$/) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import { generateShortId } from '@sim/utils/id' | ||
|
|
||
| /** | ||
| * Header naming the browser tab that sent a request. | ||
| * | ||
| * Shared by the client that sets it and the route handlers that read it. An opaque correlation | ||
| * token, never an authorization input. | ||
| */ | ||
| export const CLIENT_ID_HEADER = 'x-sim-client-id' | ||
|
|
||
| /** | ||
| * Generated ids are {@link generateShortId} length; the ceiling is slack for that, not a format. | ||
| * Bounded because the value is caller-controlled and gets fanned out to every subscriber of a | ||
| * table — uncapped, one request could inflate every broadcast payload it triggers. | ||
| */ | ||
| const MAX_CLIENT_ID_LENGTH = 64 | ||
|
|
||
| let cachedClientId: string | undefined | ||
|
|
||
| /** | ||
| * An id for this browser tab, generated once per page load and not stable across reloads. | ||
| * | ||
| * Deliberately per-TAB rather than per-user or per-session: its only consumer compares it against | ||
| * the originator stamped on a broadcast, so two tabs belonging to the same user must not share one. | ||
| * A shared id would make the second tab ignore the first tab's edits and silently go stale. | ||
| * | ||
| * Returns `undefined` on the server, where there is no tab to identify. | ||
| */ | ||
| export function getClientId(): string | undefined { | ||
| if (typeof window === 'undefined') return undefined | ||
| cachedClientId ??= generateShortId() | ||
| return cachedClientId | ||
| } | ||
|
|
||
| /** | ||
| * The sending tab's id, as seen by a route handler. Absent for server-to-server callers, for any | ||
| * client that did not send one, and for an over-long value — all read as "unattributed", never as | ||
| * "not the actor". | ||
| * | ||
| * Untrusted, and never safe to broadcast as-is: see {@link fingerprintClientId}. | ||
| */ | ||
| export function readClientId(request: Request): string | undefined { | ||
| const raw = request.headers.get(CLIENT_ID_HEADER) | ||
| return raw && raw.length <= MAX_CLIENT_ID_LENGTH ? raw : undefined | ||
| } | ||
|
waleedlatif1 marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * One-way digest of a tab id, for naming the originator of a broadcast. | ||
| * | ||
| * The raw id must never travel on a broadcast. Every subscriber of a table sees every event, so a | ||
| * raw id would be observable by any collaborator, who could then replay it as their own | ||
| * `x-sim-client-id` — their write would be attributed to your tab, your tab would suppress its | ||
| * refetch, and it would sit on stale rows. Publishing the digest instead means matching it | ||
| * requires already knowing the id, which only the tab that generated it does. | ||
| * | ||
| * Web Crypto rather than `node:crypto` so one implementation serves both sides — the server | ||
| * stamping the event and the browser recognising its own — with no chance of the two disagreeing. | ||
| */ | ||
| export async function fingerprintClientId(clientId: string): Promise<string> { | ||
| const digest = await crypto.subtle.digest('SHA-256', new TextEncoder().encode(clientId)) | ||
| return Array.from(new Uint8Array(digest)) | ||
| .map((byte) => byte.toString(16).padStart(2, '0')) | ||
| .join('') | ||
| } | ||
|
|
||
| let cachedFingerprint: string | undefined | ||
|
|
||
| /** | ||
| * This tab's fingerprint, as it appears on a broadcast it caused. `undefined` on the server, and | ||
| * until the first digest resolves — callers must treat that as "not me" and take the normal path. | ||
| */ | ||
| export async function getClientFingerprint(): Promise<string | undefined> { | ||
| const clientId = getClientId() | ||
| if (!clientId) return undefined | ||
| cachedFingerprint ??= await fingerprintClientId(clientId) | ||
| return cachedFingerprint | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| /** | ||
| * @vitest-environment node | ||
| */ | ||
| import { readdir, readFile } from 'node:fs/promises' | ||
| import { join } from 'node:path' | ||
| import { describe, expect, it } from 'vitest' | ||
|
|
||
| /** | ||
| * `signalTableRowsChangedByActor` lets the acting tab skip its own refetch, which is only sound | ||
| * where that tab's mutation hook already applies the server's answer to every cached rows query. | ||
| * That invariant lives in `hooks/queries/tables.ts` — nothing in the type system ties it to the | ||
| * call site, so a well-meaning fourth call would silently strand that client on stale rows. | ||
| * | ||
| * This pins the allowlist. If you are here because it failed: adding a call means proving the | ||
| * calling route's client hook reconciles locally, then adding it below. Removing one is always safe. | ||
| */ | ||
| const ATTRIBUTED_CALL_SITES = [ | ||
| 'app/api/table/[tableId]/rows/route.ts', | ||
| 'app/api/table/[tableId]/rows/[rowId]/route.ts', | ||
| ] as const | ||
|
|
||
| const APP_ROOT = join(import.meta.dirname, '../..') | ||
| /** Declares the function; matching its own definition would say nothing about call sites. */ | ||
| const DECLARING_MODULE = 'lib/table/events.ts' | ||
|
|
||
| async function* walk(dir: string): AsyncGenerator<string> { | ||
| for (const entry of await readdir(dir, { withFileTypes: true })) { | ||
| if (entry.name === 'node_modules' || entry.name === '.next') continue | ||
| const full = join(dir, entry.name) | ||
| if (entry.isDirectory()) yield* walk(full) | ||
| else if (entry.name.endsWith('.ts') && !entry.name.includes('.test.')) yield full | ||
| } | ||
| } | ||
|
|
||
| describe('signalTableRowsChangedByActor call sites', () => { | ||
| it('is called only where the acting tab reconciles the write locally', async () => { | ||
| const callers: string[] = [] | ||
| for await (const file of walk(APP_ROOT)) { | ||
| const source = await readFile(file, 'utf8') | ||
| if (!source.includes('signalTableRowsChangedByActor(')) continue | ||
| const relative = file.slice(APP_ROOT.length + 1) | ||
| if (relative === DECLARING_MODULE) continue | ||
| callers.push(relative) | ||
| } | ||
|
|
||
| expect(callers.sort()).toEqual([...ATTRIBUTED_CALL_SITES].sort()) | ||
| }) | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.