Skip to content

Commit ea4577e

Browse files
committed
perf(prefetch): finish the migration, delete the legacy helper, ratchet page graphs
Answers the question the previous commit left open: the tables list did not have to stay on HTTP. lib/table/service reached the executor through jobs/service -> rows/service -> workflow-columns, for one symbol. pendingDeleteMask is a delete-visibility SQL clause with no executor involvement, so it moves to its own leaf and that chain is cut. The tables prefetch now reads the data layer like every other one, and prefetch-internal-fetch.ts is deleted: nothing in the app calls its own API over HTTP during a server render any more. stripGroupDeps likewise moves to a leaf rather than being re-exported through workflow-columns, so its importers no longer pull the executor to get a pure projection. React Query mechanism fixes, all found by audit: - settings/[section] fired two prefetches without awaiting them. Only a settled query is dehydrated, so those were shipped mid-flight; a rejection hydrated into an error state retryOnMount: false never retries, leaving the panel broken for the session. Awaited now, and the pending-dehydration opt-in is removed since nothing streams. - The viewer profile was prefetched by both the layout and the settings page. Separate server QueryClients mean that was a real second read per request. - prefetchSubscriptionData was dead, and hand-rolled an unannotated raw fetch. - retry is scoped to the browser. Query core defaults it to 0 on the server; stating one value for both opted awaited prefetches into a retry backoff. The gcTime default is dropped entirely — 5 minutes is already the browser default, and setting it explicitly overrode the server's Infinity, leaving a live timer and payload per request. check:tool-registry-boundary now also ratchets per-page module counts against a committed baseline, attributing a regression to the import that caused it via a dominator tree. It caught a +444 regression in this branch by hand; it would have caught it in CI. Its import regex also missed bare side-effect imports, so `import '@/tools/registry'` could have slipped past it entirely. Prefetch guidance added to .claude/rules/sim-queries.md.
1 parent 73fbd49 commit ea4577e

22 files changed

Lines changed: 996 additions & 223 deletions

File tree

.agents/skills/tool-registry-boundary/SKILL.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,10 @@ If it fails, do not add the entry to an allowlist — there isn't one. Find the
6868

6969
Run it with `--verbose` to print per-route module counts, which is also the quickest way to see whether a change moved the graph.
7070

71+
The same command also ratchets those counts. `scripts/check-tool-registry-boundary.baseline.json` records each entry's module count plus its heaviest "gateway" modules (how many modules reach the graph *only* through each one). `--check` — what CI runs — fails when an entry exceeds its baseline by more than `max(25 modules, 2%)`, and names the gateway whose weight grew along with the import chain that reaches it. That catches the graph bloat the registry rule misses: the `listTables` prefetch that cost the Tables page 444 modules never touched `@/tools/registry`.
72+
73+
When the growth is deliberate (a real new feature on the page), re-record it with `--update-baseline` and commit the JSON. When an entry *shrinks*, the check says so and passes — re-record then too, or the win is silently spendable again.
74+
7175
## How to verify an edge actually got cut
7276

7377
Do not eyeball imports — the registry is reached through several redundant paths, so cutting one buys nothing while another survives. Walk the graph:

.claude/commands/tool-registry-boundary.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,10 @@ If it fails, do not add the entry to an allowlist — there isn't one. Find the
6767

6868
Run it with `--verbose` to print per-route module counts, which is also the quickest way to see whether a change moved the graph.
6969

70+
The same command also ratchets those counts. `scripts/check-tool-registry-boundary.baseline.json` records each entry's module count plus its heaviest "gateway" modules (how many modules reach the graph *only* through each one). `--check` — what CI runs — fails when an entry exceeds its baseline by more than `max(25 modules, 2%)`, and names the gateway whose weight grew along with the import chain that reaches it. That catches the graph bloat the registry rule misses: the `listTables` prefetch that cost the Tables page 444 modules never touched `@/tools/registry`.
71+
72+
When the growth is deliberate (a real new feature on the page), re-record it with `--update-baseline` and commit the JSON. When an entry *shrinks*, the check says so and passes — re-record then too, or the win is silently spendable again.
73+
7074
## How to verify an edge actually got cut
7175

7276
Do not eyeball imports — the registry is reached through several redundant paths, so cutting one buys nothing while another survives. Walk the graph:

.claude/rules/sim-queries.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,22 @@ const handler = useCallback(() => {
143143
}, [data])
144144
```
145145

146+
## Server prefetching
147+
148+
A server prefetch fills the *same* cache key a client hook fills, so it must be indistinguishable from a client fetch. Five rules:
149+
150+
1. **Read the data layer, never our own API over HTTP.** A server-to-server call to `/api/...` costs a round trip and a second authentication for data the process can already read. Where the route runs an application use case, call that same use case with a principal from the same auth policy the route declares — not a manager underneath it.
151+
2. **Match the wire shape the hook caches.** The hook's data is whatever `requestJson(contract, …)` produced, so the seed must equal it. Two traps: a contract field declared `z.coerce.date()` means the hook holds a `Date` where raw route JSON holds a string; a passthrough response schema (`z.custom`) means the hook caches route JSON *verbatim*, so seeding raw rows leaks `Date`s and server-only fields. When the route projects before responding, share that projection — have the route and the prefetch call one function.
152+
3. **Prove the viewer.** Data-layer reads carry no authorization; the route used to provide it. Resolve the viewer (`getWorkspaceHostContextForViewer`, already `cache`d by the layout so it costs nothing) and return early on failure, caching nothing — the client fetch then reaches the route for the real 403. Never widen what a viewer can see.
153+
4. **Always `await`.** Only a settled query is dehydrated, so an unawaited prefetch is silently dropped from the payload and the pane waterfalls anyway.
154+
5. **Don't repeat what the layout already seeded.** `getQueryClient()` builds a new client per server call, so a page re-seeding a layout key is a genuine second read — and `HydrationBoundary` defers an already-seen query to an effect, which SSR never runs, so it never reaches the server render either.
155+
156+
Reuse the hook's exported `staleTime` constant and its key factory; `dehydrate` carries neither options nor `staleTime`, and freshness is per-observer.
157+
158+
Seed with `setQueryData` only when the prefetch must be able to *decline* to create an entry (an empty list that has to fall through to a route's creation path). `prefetchQuery` and `ensureQueryData` always create one.
159+
160+
Keep prefetch imports light. A page prefetch's imports land in that route's server graph, so pulling a barrel to reach one function can drag thousands of modules behind it — `bun run check:tool-registry-boundary` gates this per page.
161+
146162
## Boundary Types
147163

148164
- Hooks import named type aliases from `@/lib/api/contracts/**` (e.g., `import { listEntitiesContract, type EntityList } from '@/lib/api/contracts/entities'`). Never write `z.input<...>` / `z.output<...>` in hooks, and never `import { z } from 'zod'` in client code.

.cursor/commands/tool-registry-boundary.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,10 @@ If it fails, do not add the entry to an allowlist — there isn't one. Find the
6363

6464
Run it with `--verbose` to print per-route module counts, which is also the quickest way to see whether a change moved the graph.
6565

66+
The same command also ratchets those counts. `scripts/check-tool-registry-boundary.baseline.json` records each entry's module count plus its heaviest "gateway" modules (how many modules reach the graph *only* through each one). `--check` — what CI runs — fails when an entry exceeds its baseline by more than `max(25 modules, 2%)`, and names the gateway whose weight grew along with the import chain that reaches it. That catches the graph bloat the registry rule misses: the `listTables` prefetch that cost the Tables page 444 modules never touched `@/tools/registry`.
67+
68+
When the growth is deliberate (a real new feature on the page), re-record it with `--update-baseline` and commit the JSON. When an entry *shrinks*, the check says so and passes — re-record then too, or the win is silently spendable again.
69+
6670
## How to verify an edge actually got cut
6771

6872
Do not eyeball imports — the registry is reached through several redundant paths, so cutting one buys nothing while another survives. Walk the graph:

apps/sim/app/_shell/providers/get-query-client.ts

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
1-
import { defaultShouldDehydrateQuery, isServer, QueryClient } from '@tanstack/react-query'
1+
import { isServer, QueryClient } from '@tanstack/react-query'
22
import { isDesktopApp } from '@/lib/desktop'
33

44
export function makeQueryClient() {
55
return new QueryClient({
66
defaultOptions: {
77
queries: {
88
staleTime: 30 * 1000,
9-
gcTime: 5 * 60 * 1000,
109
// The desktop app window lives for days, so cross-session changes —
1110
// an admin upgrading your org/workspace role, a workspace you were
1211
// auto-added to, seat/entitlement changes — would otherwise stay
@@ -18,16 +17,19 @@ export function makeQueryClient() {
1817
// frequent and noisy. Per-query overrides (e.g. useWorkspaceSchedules
1918
// pins this off) always win over this default.
2019
refetchOnWindowFocus: isDesktopApp(),
21-
retry: 1,
20+
/**
21+
* Query core already defaults retries to 0 on the server and 3 in the browser;
22+
* only the browser number is ours to change. Stating one value for both would
23+
* silently opt server prefetches into a retry, and because the layout awaits
24+
* them that spends a retry backoff of document latency on a read whose failure
25+
* the client recovers from on its own.
26+
*/
27+
retry: isServer ? 0 : 1,
2228
retryOnMount: false,
2329
},
2430
mutations: {
2531
retry: false,
2632
},
27-
dehydrate: {
28-
shouldDehydrateQuery: (query) =>
29-
defaultShouldDehydrateQuery(query) || query.state.status === 'pending',
30-
},
3133
},
3234
})
3335
}

apps/sim/app/workspace/[workspaceId]/lib/prefetch-internal-fetch.ts

Lines changed: 0 additions & 25 deletions
This file was deleted.

apps/sim/app/workspace/[workspaceId]/lib/prefetch.test.ts

Lines changed: 70 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ const {
1212
mockListFoldersForWorkspace,
1313
mockListInternalKnowledgeBases,
1414
mockListPinnedItemsForUser,
15-
mockPrefetchInternalJson,
15+
mockListTables,
1616
mockListWorkspaceFileFolders,
1717
mockListWorkspaceFilesWithShares,
1818
} = vi.hoisted(() => ({
@@ -23,7 +23,7 @@ const {
2323
mockListFoldersForWorkspace: vi.fn(),
2424
mockListInternalKnowledgeBases: vi.fn(),
2525
mockListPinnedItemsForUser: vi.fn(),
26-
mockPrefetchInternalJson: vi.fn(),
26+
mockListTables: vi.fn(),
2727
mockListWorkspaceFileFolders: vi.fn(),
2828
mockListWorkspaceFilesWithShares: vi.fn(),
2929
}))
@@ -46,8 +46,17 @@ vi.mock('@/lib/pinned-items/queries', () => ({
4646
vi.mock('@/lib/workspaces/permissions/utils', () => ({
4747
getWorkspaceMemberProfiles: mockGetWorkspaceMemberProfiles,
4848
}))
49-
vi.mock('@/app/workspace/[workspaceId]/lib/prefetch-internal-fetch', () => ({
50-
prefetchInternalJson: mockPrefetchInternalJson,
49+
vi.mock('@/lib/table/service', () => ({
50+
listTables: mockListTables,
51+
}))
52+
/**
53+
* `typeMetadataOf` is the one leaf of the real wire projection that reaches the
54+
* column-type registry, and through it every type module's icon and editor. Stub
55+
* that leaf only, so `toTableListItem`'s timestamp, `metadata`, and job
56+
* normalization stay under test rather than being mocked away wholesale.
57+
*/
58+
vi.mock('@/lib/table/column-types', () => ({
59+
typeMetadataOf: () => ({}),
5160
}))
5261
vi.mock('@/lib/api/server/routes', () => ({
5362
internalSessionAuth: { authenticate: mockAuthenticate },
@@ -90,7 +99,7 @@ describe('workspace list prefetches', () => {
9099
mockListWorkspaceFileFolders.mockResolvedValue([])
91100
mockListPinnedItemsForUser.mockResolvedValue([])
92101
mockGetWorkspaceMemberProfiles.mockResolvedValue([])
93-
mockPrefetchInternalJson.mockResolvedValue({ data: { tables: [] } })
102+
mockListTables.mockResolvedValue([])
94103
mockAuthenticate.mockResolvedValue({ kind: 'session', userId: USER_ID, sessionId: 'sess-1' })
95104
mockListInternalKnowledgeBases.mockResolvedValue({ knowledgeBases: [] })
96105
mockKnowledgePresenterList.mockReturnValue({ success: true, data: [] })
@@ -183,22 +192,67 @@ describe('workspace list prefetches', () => {
183192
})
184193

185194
describe('prefetchTables', () => {
195+
const TABLE_ROW = {
196+
id: 't-1',
197+
name: 'people',
198+
description: null,
199+
schema: { columns: [{ id: 'c1', name: 'name', type: 'string' }] },
200+
metadata: { columnWidths: { c1: 120 } },
201+
rowCount: 3,
202+
maxRows: 10_000,
203+
workspaceId: WORKSPACE_ID,
204+
folderId: null,
205+
createdBy: 'u-1',
206+
locks: {
207+
schemaLocked: false,
208+
insertLocked: false,
209+
updateLocked: false,
210+
deleteLocked: false,
211+
},
212+
archivedAt: null,
213+
createdAt: new Date('2026-01-01T00:00:00.000Z'),
214+
updatedAt: new Date('2026-01-02T00:00:00.000Z'),
215+
}
216+
217+
it('reads tables from the data layer', async () => {
218+
mockListTables.mockResolvedValue([TABLE_ROW])
219+
const client = makeClient()
220+
221+
await prefetchTables(client, WORKSPACE_ID, USER_ID)
222+
223+
expect(mockListTables).toHaveBeenCalledWith(WORKSPACE_ID, { scope: 'active' })
224+
})
225+
186226
/**
187-
* The tables list is the one read on this page still served over HTTP: `listTables` lives in
188-
* a module graph that reaches the executable tool registry, which
189-
* `check:tool-registry-boundary` refuses to let into a page graph.
227+
* `listTablesContract`'s response schema is a passthrough, so a client fetch caches the
228+
* route's JSON verbatim. Seeding the raw data-layer row would put `Date`s and the
229+
* server-only `metadata` field under a key the hook never sees them on.
190230
*/
191-
it('primes the exact key useTablesList reads and unwraps data.tables', async () => {
192-
const tables = [{ id: 't-1' }]
193-
mockPrefetchInternalJson.mockResolvedValue({ data: { tables } })
231+
it('seeds the wire shape a client fetch caches, not the raw data-layer row', async () => {
232+
mockListTables.mockResolvedValue([TABLE_ROW])
194233
const client = makeClient()
195234

196235
await prefetchTables(client, WORKSPACE_ID, USER_ID)
197236

198-
expect(mockPrefetchInternalJson).toHaveBeenCalledWith(
199-
`/api/table?workspaceId=${WORKSPACE_ID}&scope=active`
200-
)
201-
expect(client.getQueryData(tableKeys.list(WORKSPACE_ID, 'active'))).toEqual(tables)
237+
const [cached] = client.getQueryData(tableKeys.list(WORKSPACE_ID, 'active')) as Array<
238+
Record<string, unknown>
239+
>
240+
expect(cached.createdAt).toBe('2026-01-01T00:00:00.000Z')
241+
expect(cached.updatedAt).toBe('2026-01-02T00:00:00.000Z')
242+
expect(cached.archivedAt).toBeNull()
243+
expect(cached).not.toHaveProperty('metadata')
244+
expect(cached.jobStatus).toBeNull()
245+
expect(cached.jobRowsProcessed).toBe(0)
246+
})
247+
248+
it('caches no tables when the viewer cannot be proved', async () => {
249+
mockGetWorkspaceHostContextForViewer.mockResolvedValue(null)
250+
const client = makeClient()
251+
252+
await prefetchTables(client, WORKSPACE_ID, USER_ID)
253+
254+
expect(mockListTables).not.toHaveBeenCalled()
255+
expect(client.getQueryData(tableKeys.list(WORKSPACE_ID, 'active'))).toBeUndefined()
202256
})
203257
})
204258
describe('prefetchFilesBrowser', () => {
@@ -335,7 +389,7 @@ describe('workspace list prefetches', () => {
335389
const boom = new Error('500')
336390
mockListWorkspaceFilesWithShares.mockRejectedValue(boom)
337391
mockListFoldersForWorkspace.mockRejectedValue(boom)
338-
mockPrefetchInternalJson.mockRejectedValue(boom)
392+
mockListTables.mockRejectedValue(boom)
339393
mockListInternalKnowledgeBases.mockRejectedValue(boom)
340394
mockListPinnedItemsForUser.mockRejectedValue(boom)
341395
mockGetWorkspaceMemberProfiles.mockRejectedValue(boom)

apps/sim/app/workspace/[workspaceId]/prefetch.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -120,12 +120,11 @@ async function seedWorkspaceList(
120120
* seeds nothing, leaving the client fetch to reach `GET /api/workspaces`'
121121
* default-workspace creation path — the same outcome a rejecting `queryFn` used
122122
* to produce, without routing a normal state through the error channel. That
123-
* matters because `makeQueryClient` dehydrates pending queries and sets
124-
* `retryOnMount: false`: were this read ever deferred, its rejection would
125-
* hydrate the client query into an error state nothing retries, permanently
126-
* locking a brand-new viewer out of workspace creation. Seeding also skips the
127-
* `retry: 1` default, which previously ran the whole read a second time, a
128-
* retry delay later, purely to re-derive an outcome already known.
123+
* matters because only a settled query is dehydrated: an unawaited read would be
124+
* dropped from the payload entirely, so the switcher would waterfall on every
125+
* cold load rather than paint populated. Seeding also skips the `retry` default,
126+
* which previously ran the whole read a second time, a retry delay later, purely
127+
* to re-derive an outcome already known.
129128
*/
130129
export async function prefetchWorkspaceSidebar(
131130
queryClient: QueryClient,

apps/sim/app/workspace/[workspaceId]/settings/[section]/page.tsx

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import {
2525
} from '@/app/workspace/[workspaceId]/settings/navigation'
2626
import { resolveWorkspaceGroup } from '@/ee/access-control/utils/permission-check'
2727
import { isForkingAvailableForWorkspace } from '@/ee/workspace-forking/lib/lineage/authz'
28-
import { prefetchGeneralSettings, prefetchUserProfile } from './prefetch'
28+
import { prefetchGeneralSettings } from './prefetch'
2929
import { SettingsPage } from './settings'
3030

3131
interface WorkspaceSettingsSectionPageProps {
@@ -170,8 +170,14 @@ export default async function WorkspaceSettingsSectionPage({
170170
}
171171

172172
const queryClient = getQueryClient()
173-
void prefetchGeneralSettings(queryClient)
174-
void prefetchUserProfile(queryClient)
173+
/**
174+
* Awaited, not fired and forgotten. An unawaited prefetch is still `pending` when
175+
* `dehydrate` runs, so its rejection would hydrate the client query straight into an
176+
* error state that `retryOnMount: false` never retries — leaving the panel broken for
177+
* the rest of the session. The viewer's profile is already seeded by the workspace
178+
* layout under the same key, so it is not repeated here.
179+
*/
180+
await prefetchGeneralSettings(queryClient)
175181

176182
return (
177183
<HydrationBoundary state={dehydrate(queryClient)}>

0 commit comments

Comments
 (0)