Skip to content

Commit c49751b

Browse files
authored
perf(prefetch): stop calling our own API over the wire during server render (#6657)
* perf(prefetch): read the data layer instead of calling our own API over the wire Four server-render prefetches went out over HTTP to our own routes. With INTERNAL_API_BASE_URL unset in prod, getInternalApiBaseUrl() falls back to the public base URL, so each was RSC -> public HTTPS -> load balancer -> back into the app, awaited inside the render with a second round of auth. - /home fetched the workflow folder list that the workspace layout had already fetched, under the identical query key. Since getQueryClient() builds a new client per call on the server, the two never deduped: same data, twice a request, once directly and once over the wire. Dropped; the layout's entry already hydrates it. - /home cached raw route JSON under workspaceFilesKeys.list, while files/prefetch.ts seeds that same key from listWorkspaceFilesWithShares. The contract declares the date fields z.coerce.date(), so consumers hold Dates — a file record's type depended on which page the viewer landed on. Now reads the same function files/prefetch.ts does. - tables and knowledge folder reads now call listFoldersForWorkspace, matching the sidebar prefetch. These reads carry no authorization of their own, so each surface proves the viewer through getWorkspaceHostContextForViewer first and caches nothing when it fails, leaving the client fetch to reach the route for the real 403. Both it and getSession are cache()d and already resolved by the layout, so the proof costs no extra queries. Left on the wire, deliberately: the tables and knowledge lists, whose cached shape is the serialized wire shape, and pinned items and members, which have no exported data-layer function. * improvement(prefetch): skip the viewer proof when there is no session Passing an empty-string userId ran a real permission query that could only return null. Take an optional userId instead and skip straight to the unauthorized path, matching how the home prefetch is called. * perf(prefetch): finish removing self-HTTP prefetches and delete the legacy helper Converts the last four server-render prefetches that called our own API over HTTP, and deletes prefetch-internal-fetch.ts now that nothing imports it. - knowledge bases: runs the route's own listInternalKnowledgeBases use case with a principal from the same internalSessionAuth policy the route declares, then projects through the same presenter and contract. Not a bypass of the application boundary — the same path, called in-process. - tables: extracts the route's list projection into lib/table/wire.ts as toTableListItem, which the route and the prefetch now both call. This matters because listTablesContract's response schema is a passthrough z.custom, so a client fetch caches the route's JSON verbatim. Seeding listTables() directly would have put Date objects and the server-only metadata field under a key the hook never sees them on. - pinned items: extracts the route's inline query into lib/pinned-items/queries.ts as listPinnedItemsForUser, which the route now calls too. - workspace members: getWorkspaceMemberProfiles already existed; the prefetch calls it directly. normalizeColumn moves from app/api/table/utils.ts to lib/table/wire.ts with ten importers repointed. That also removes a pre-existing lib/* -> app/api/* boundary violation in lib/table/import-runner.ts. No response shape changes: the v1/v2 edits are import-path moves only. Every converted read proves the viewer first and caches nothing when that fails, so an unauthorized viewer's client fetch still reaches the route for the real 403. Authorization equivalence was checked by unfolding both paths to checkWorkspaceAccess rather than assumed. * improvement(prefetch): collapse the duplicated folder prefetch and unify the call shape - Extract prefetchResourceFolders. The same eight-line folder prefetch was written three times, varying only by resourceType, with the key, stale time and mapper kept in sync by hand. - Adopting it removes the conditional spread from the tables and knowledge prefetches. Tables can now early-return, matching prefetchFilesBrowser: prefetchResourceListChrome already self-guards on the same cached host context, so a null context meant the function did nothing either way. - Take userId as string | undefined everywhere and guard inside, so every prefetch module has one calling convention rather than two. - Export toWireTimestamp and use it for the create-table response's own copy of the same idiom, and drop a cast that the extraction made dead: the parameter is already TableDefinition, whose schema is TableSchema. - Read params and the session concurrently on the tables and knowledge pages, matching the files page, and drop TSDoc that restated each prefetch's own. * fix(prefetch): keep the tables list on its route and cut the executor edge Reading listTables from a page prefetch put the executable tool registry into the Tables page server graph — ~4,700 modules, which check:tool-registry-boundary rejects. lib/table/service reaches workflow-columns by several independent paths (directly, and through jobs/service and rows/service), so severing one edge is not enough; untangling that belongs in its own change. - The tables list goes back through GET /api/table, with the reason recorded so the next person does not repeat the attempt. Folders and chrome on that page stay on the data layer. - stripGroupDeps moves to its own leaf module. It is a pure projection over a WorkflowGroup, but living beside the group runtime meant every importer of lib/table/service paid for the executor to get it. Net effect on the Tables page graph: 2,186 modules to 1,742. * 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. * fix(prefetch): correct the extracted module's db imports and stale rationale Audit findings from the migration. - pending-delete-mask imported its schema tables from @sim/db rather than @sim/db/schema, which the module it came from was careful to split. The global test mocks are bound per-entrypoint and only the schema mock exports tables, so every suite that reaches pendingDeleteMask would have failed on a missing mock export. Restored to the original convention, and the same split applied to the new pinned-items queries module before it grows a test. - The settings prefetch and page justified awaiting with a mechanism this branch removed — pending queries being shipped with their promise. Only a settled query is dehydrated now, so an unawaited prefetch is dropped from the payload entirely. Same conclusion, correct reason, and no longer contradicting the rule this branch added. - Removed the doc block left orphaned above validateSchema when stripGroupDeps moved out of workflow-columns. Skill projections regenerated after trimming the boundary skill. * chore(table): drop a section separator comment Separators like these are non-TSDoc decoration that CLAUDE.md already rules out. This is the only one in a file this branch touches; the rest of the repo is swept separately. * chore: remove section separator comments CLAUDE.md already rules these out ("No ==== separators. No non-TSDoc comments"), but 546 of them had accumulated across 48 files. They decorate rather than explain, and they drift: a separator says "Validation" while the code beneath it moved elsewhere, as one in workflow-columns already had. Pure deletion — no source line was touched, and lines inside template literals were skipped so nothing in a generated string changed.
1 parent e8d278b commit c49751b

102 files changed

Lines changed: 1461 additions & 1165 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.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 against `check-tool-registry-boundary.baseline.json`. `--check` (what CI runs) fails when an entry exceeds its baseline by more than `max(25 modules, 2%)`, naming the import chain responsible. This catches bloat the registry rule misses — a prefetch importing `listTables` cost the Tables page 444 modules without ever touching `@/tools/registry`.
72+
73+
Re-record with `--update-baseline` and commit the JSON when growth is deliberate. A *shrink* passes but is reported — 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 against `check-tool-registry-boundary.baseline.json`. `--check` (what CI runs) fails when an entry exceeds its baseline by more than `max(25 modules, 2%)`, naming the import chain responsible. This catches bloat the registry rule misses — a prefetch importing `listTables` cost the Tables page 444 modules without ever touching `@/tools/registry`.
71+
72+
Re-record with `--update-baseline` and commit the JSON when growth is deliberate. A *shrink* passes but is reported — 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 against `check-tool-registry-boundary.baseline.json`. `--check` (what CI runs) fails when an entry exceeds its baseline by more than `max(25 modules, 2%)`, naming the import chain responsible. This catches bloat the registry rule misses — a prefetch importing `listTables` cost the Tables page 444 modules without ever touching `@/tools/registry`.
67+
68+
Re-record with `--update-baseline` and commit the JSON when growth is deliberate. A *shrink* passes but is reported — 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/api/pinned-items/route.ts

Lines changed: 3 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -2,40 +2,21 @@ import { db, pinnedItem } from '@sim/db'
22
import { createLogger } from '@sim/logger'
33
import { getPostgresErrorCode } from '@sim/utils/errors'
44
import { generateId } from '@sim/utils/id'
5-
import { and, eq, ne } from 'drizzle-orm'
65
import { type NextRequest, NextResponse } from 'next/server'
76
import {
87
createPinnedItemContract,
98
listPinnedItemsContract,
109
type PinnedItemApi,
11-
pinnedResourceTypeSchema,
1210
} from '@/lib/api/contracts'
1311
import { parseRequest } from '@/lib/api/server'
1412
import { getSession } from '@/lib/auth'
1513
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
16-
import { filterToActiveResources, pinnableResourceExists } from '@/lib/pinned-items/resources'
14+
import { listPinnedItemsForUser } from '@/lib/pinned-items/queries'
15+
import { pinnableResourceExists } from '@/lib/pinned-items/resources'
1716
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1817

1918
const logger = createLogger('PinnedItemsAPI')
2019

21-
/**
22-
* Narrows a stored row to the wire shape, dropping any row whose `resourceType` this build does
23-
* not recognise.
24-
*
25-
* `pinned_item.resource_type` is plain `text` — deliberately, so the set of pinnable kinds can
26-
* grow — while the contract is a closed enum. During a rolling deploy an older pod can therefore
27-
* read a pin a newer one wrote. Returning it would fail response validation and take the WHOLE
28-
* list down rather than the single row, so the unknown kind is skipped instead.
29-
*
30-
* `filterToActiveResources` already drops these as a side effect of not having a table to look
31-
* them up in; this makes the guarantee explicit and compiler-checked at the wire boundary.
32-
*/
33-
function toPinnedItemApi(row: typeof pinnedItem.$inferSelect): PinnedItemApi | null {
34-
const resourceType = pinnedResourceTypeSchema.safeParse(row.resourceType)
35-
if (!resourceType.success) return null
36-
return { ...row, resourceType: resourceType.data, pinnedAt: row.pinnedAt.toISOString() }
37-
}
38-
3920
/** Lists the session user's pinned items in a workspace, optionally filtered to one `resourceType`. */
4021
export const GET = withRouteHandler(async (request: NextRequest) => {
4122
const session = await getSession()
@@ -52,30 +33,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
5233
return NextResponse.json({ error: 'Access denied to this workspace' }, { status: 403 })
5334
}
5435

55-
const rows = await db
56-
.select()
57-
.from(pinnedItem)
58-
.where(
59-
and(
60-
eq(pinnedItem.userId, session.user.id),
61-
eq(pinnedItem.workspaceId, workspaceId),
62-
/**
63-
* A `workspace` pin stores `workspaceId === resourceId`, so it would otherwise
64-
* appear in this workspace's unscoped listing as a resource *inside* itself.
65-
* It is read from the workspace-list payload instead, so it is excluded here
66-
* rather than left for a future unscoped caller to mistake for a real resource.
67-
*/
68-
resourceType
69-
? eq(pinnedItem.resourceType, resourceType)
70-
: ne(pinnedItem.resourceType, 'workspace')
71-
)
72-
)
73-
74-
const activeRows = await filterToActiveResources(rows, workspaceId)
75-
76-
const pinnedItems = activeRows
77-
.map(toPinnedItemApi)
78-
.filter((item): item is PinnedItemApi => item !== null)
36+
const pinnedItems = await listPinnedItemsForUser(session.user.id, workspaceId, resourceType)
7937

8038
return NextResponse.json({ pinnedItems })
8139
})

apps/sim/app/api/table/[tableId]/columns/route.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,12 @@ vi.mock('@/lib/table/columns/service', () => ({
4949
updateColumnOptions: mockUpdateColumnOptions,
5050
updateColumnType: mockUpdateColumnType,
5151
}))
52+
vi.mock('@/lib/table/wire', () => ({
53+
normalizeColumn: (c: unknown) => c,
54+
}))
5255
vi.mock('@/app/api/table/utils', () => ({
5356
accessError: () => new Response('denied', { status: 403 }),
5457
checkAccess: mockCheckAccess,
55-
normalizeColumn: (c: unknown) => c,
5658
orchestrationOutcomeErrorResponse: (
5759
outcome: { error?: string; errorCode?: OrchestrationErrorCode },
5860
fallback: string

apps/sim/app/api/table/[tableId]/columns/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,10 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1313
import { addTableColumn, deleteColumn } from '@/lib/table'
1414
import { signalTableSchemaChanged } from '@/lib/table/events'
1515
import { performUpdateTableColumn } from '@/lib/table/orchestration'
16+
import { normalizeColumn } from '@/lib/table/wire'
1617
import {
1718
accessError,
1819
checkAccess,
19-
normalizeColumn,
2020
orchestrationOutcomeErrorResponse,
2121
rootErrorMessage,
2222
tableLockErrorResponse,

apps/sim/app/api/table/[tableId]/groups/route.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ vi.mock('@/lib/table/application/groups', () => ({
5454
updateTableGroupUseCase: mocks.useCases.update,
5555
}))
5656

57-
vi.mock('@/app/api/table/utils', () => ({
57+
vi.mock('@/lib/table/wire', () => ({
5858
normalizeColumn: vi.fn(),
5959
}))
6060

apps/sim/app/api/table/[tableId]/groups/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {
1212
} from '@/lib/table/application/groups'
1313
import { tableOperations } from '@/lib/table/application/operations'
1414
import type { TableDefinition } from '@/lib/table/types'
15-
import { normalizeColumn } from '@/app/api/table/utils'
15+
import { normalizeColumn } from '@/lib/table/wire'
1616

1717
const rateLimit = internalRateLimits.none({
1818
reason: 'Existing authenticated table group mutations have no request-rate policy',

0 commit comments

Comments
 (0)