Skip to content

Commit d3561e7

Browse files
committed
improvement(queries): fix a second row-cache collision, and make the memoized workspace read actually dedupe
1 parent 0dd8f7c commit d3561e7

30 files changed

Lines changed: 232 additions & 221 deletions

File tree

apps/sim/app/api/auth/oauth/token/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
118118
const auth = await checkSessionOrInternalAuth(request, { requireWorkflowId: false })
119119
const result = await resolveCredentialToken(auth, {
120120
requestId,
121-
credentialId: credentialId ?? '',
121+
credentialId,
122122
workflowId: workflowId ?? undefined,
123123
scopes,
124124
impersonateEmail,

apps/sim/app/api/copilot/checkpoints/revert/route.test.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,11 @@ describe('Copilot Checkpoints Revert API Route', () => {
4949

5050
authMockFns.mockGetSession.mockResolvedValue(null)
5151

52+
/** Authorization is the route's workflow read, so an allowed result always carries one. */
5253
workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
5354
allowed: true,
5455
status: 200,
56+
workflow: { id: 'b2c3d4e5-f6a7-4b89-a0d1-e2f3a4b5c6d7', workspaceId: 'ws-123' },
5557
})
5658

5759
mockGetAccessibleCopilotChat.mockResolvedValue({ id: 'chat-123', userId: 'user-123' })
@@ -201,7 +203,12 @@ describe('Copilot Checkpoints Revert API Route', () => {
201203
}
202204

203205
queueTableRows(schemaMock.workflowCheckpoints, [mockCheckpoint])
204-
queueTableRows(schemaMock.workflow, [])
206+
/** Authorization performs the workflow read, so a missing workflow surfaces through it. */
207+
workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValueOnce({
208+
allowed: false,
209+
status: 404,
210+
workflow: null,
211+
})
205212

206213
const req = new NextRequest('http://localhost:3000/api/copilot/checkpoints/revert', {
207214
method: 'POST',
@@ -214,6 +221,7 @@ describe('Copilot Checkpoints Revert API Route', () => {
214221
expect(response.status).toBe(404)
215222
const responseData = await response.json()
216223
expect(responseData.error).toBe('Workflow not found')
224+
expect(mockSaveWorkflowNormalizedState).not.toHaveBeenCalled()
217225
})
218226

219227
it('should return 401 when workflow belongs to different user', async () => {
@@ -237,6 +245,7 @@ describe('Copilot Checkpoints Revert API Route', () => {
237245
workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValueOnce({
238246
allowed: false,
239247
status: 403,
248+
workflow: { id: 'b2c3d4e5-f6a7-4b89-a0d1-e2f3a4b5c6d7', workspaceId: 'ws-123' },
240249
})
241250

242251
const req = new NextRequest('http://localhost:3000/api/copilot/checkpoints/revert', {
@@ -562,8 +571,9 @@ describe('Copilot Checkpoints Revert API Route', () => {
562571
}
563572

564573
dbChainMockFns.where.mockReturnValueOnce(Promise.resolve([mockCheckpoint]))
565-
dbChainMockFns.where.mockReturnValueOnce(
566-
Promise.reject(new Error('Database error during workflow lookup'))
574+
/** Authorization performs the workflow read, so a failed lookup surfaces through it. */
575+
workflowAuthzMockFns.mockAuthorizeWorkflowByWorkspacePermission.mockRejectedValueOnce(
576+
new Error('Database error during workflow lookup')
567577
)
568578

569579
const req = new NextRequest('http://localhost:3000/api/copilot/checkpoints/revert', {

apps/sim/app/api/copilot/checkpoints/revert/route.ts

Lines changed: 6 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,7 @@
11
import { db } from '@sim/db'
2-
import { workflowCheckpoints, workflow as workflowTable } from '@sim/db/schema'
2+
import { workflowCheckpoints } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
4-
import {
5-
authorizeWorkflowByWorkspacePermission,
6-
WorkflowLockedError,
7-
} from '@sim/platform-authz/workflow'
4+
import { authorizeWorkflowByWorkspacePermission } from '@sim/platform-authz/workflow'
85
import { and, eq } from 'drizzle-orm'
96
import { type NextRequest, NextResponse } from 'next/server'
107
import { revertCopilotCheckpointContract } from '@/lib/api/contracts/copilot'
@@ -68,21 +65,15 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
6865
return createNotFoundResponse('Checkpoint not found or access denied')
6966
}
7067

71-
const workflowData = await db
72-
.select()
73-
.from(workflowTable)
74-
.where(eq(workflowTable.id, checkpoint.workflowId))
75-
.then((rows) => rows[0])
76-
77-
if (!workflowData) {
78-
return createNotFoundResponse('Workflow not found')
79-
}
80-
68+
/** Authorization already loads the workflow, so its absence is the not-found signal. */
8169
const authorization = await authorizeWorkflowByWorkspacePermission({
8270
workflowId: checkpoint.workflowId,
8371
userId,
8472
action: 'write',
8573
})
74+
if (!authorization.workflow) {
75+
return createNotFoundResponse('Workflow not found')
76+
}
8677
if (!authorization.allowed) {
8778
return createUnauthorizedResponse()
8879
}
@@ -144,22 +135,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
144135
)
145136
}
146137

147-
/**
148-
* A locked workflow used to surface here as a non-OK PUT response, so it
149-
* still resolves to the same revert failure rather than the generic
150-
* outer-catch message. Every other throw keeps propagating, matching the
151-
* old transport-error path.
152-
*/
153138
const saveResult = await saveWorkflowNormalizedState({
154139
requestId: tracker.requestId,
155140
workflowId: checkpoint.workflowId,
156141
userId,
157142
state: parsedState.data,
158-
}).catch((error) => {
159-
if (error instanceof WorkflowLockedError) {
160-
return { success: false as const, status: error.status, error: error.message }
161-
}
162-
throw error
163143
})
164144

165145
if (!saveResult.success) {

apps/sim/app/api/workflows/[id]/state/route.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -110,11 +110,6 @@ export const PUT = withRouteHandler(
110110
const parsed = await parseRequest(putWorkflowNormalizedStateContract, request, context)
111111
if (!parsed.success) return parsed.response
112112

113-
// Note: prior versions cross-checked that each variable's `workflowId`
114-
// equalled the path param. The write contract does not carry `workflowId`
115-
// per variable (the path param is the source of truth), so the check
116-
// is unreachable and was removed.
117-
118113
const result = await saveWorkflowNormalizedState({
119114
requestId,
120115
workflowId,

apps/sim/app/invite/[id]/invite.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import { InviteLayout, InviteStatusCard } from '@/app/invite/components'
1515
import { useInvitationDetails } from '@/hooks/queries/invitations'
1616
import { organizationKeys } from '@/hooks/queries/organization'
1717
import { refreshSessionQuery } from '@/hooks/queries/session'
18-
import { subscriptionKeys } from '@/hooks/queries/subscription'
18+
import { subscriptionKeys } from '@/hooks/queries/utils/subscription-keys'
1919
import { workspaceKeys } from '@/hooks/queries/workspace'
2020

2121
const logger = createLogger('InviteById')

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

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -515,8 +515,8 @@ describe('workspace list prefetches', () => {
515515

516516
/**
517517
* The file list is seeded on every workspace route, so it is the one entry whose size
518-
* scales with a workspace's content on routes that never read it. It is read one row
519-
* past the budget so the overflow is detectable.
518+
* scales with a workspace's content on routes that never read it. The budget is passed
519+
* down rather than applied here, so the read can stop before the share join.
520520
*/
521521
it('seeds the file list, bounded by the document payload budget', async () => {
522522
const files = [{ id: 'file-1', name: 'a.txt' }]
@@ -526,24 +526,19 @@ describe('workspace list prefetches', () => {
526526
await prefetchWorkspaceSidebar(client, WORKSPACE_ID, USER_ID, HOST_CONTEXT, null)
527527

528528
expect(mockListWorkspaceFilesWithShares).toHaveBeenCalledWith(WORKSPACE_ID, 'active', {
529-
limit: WORKSPACE_FILE_SEED_MAX + 1,
529+
maxRows: WORKSPACE_FILE_SEED_MAX,
530530
})
531531
expect(client.getQueryData(workspaceFilesKeys.list(WORKSPACE_ID, 'active'))).toEqual(files)
532532
})
533533

534534
/**
535535
* The load-bearing half of the budget: a workspace over it seeds NOTHING rather than the
536-
* prefix it read. The sidebar search filters this list client-side and the Files browser
537-
* renders it as the workspace's files, so a truncated seed would silently hide files —
538-
* the client fetch must reach the route for the complete list instead.
536+
* prefix that was read. The sidebar search filters this list client-side and the Files
537+
* browser renders it as the workspace's files, so a truncated seed would silently hide
538+
* files — the client fetch must reach the route for the complete list instead.
539539
*/
540540
it('seeds nothing when the workspace exceeds the budget', async () => {
541-
mockListWorkspaceFilesWithShares.mockResolvedValue(
542-
Array.from({ length: WORKSPACE_FILE_SEED_MAX + 1 }, (_, index) => ({
543-
id: `file-${index}`,
544-
name: `${index}.txt`,
545-
}))
546-
)
541+
mockListWorkspaceFilesWithShares.mockResolvedValue(null)
547542
const client = makeClient()
548543

549544
await prefetchWorkspaceSidebar(client, WORKSPACE_ID, USER_ID, HOST_CONTEXT, null)

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -101,11 +101,11 @@ async function seedWorkspaceList(
101101
* thousands of files would otherwise push more than a megabyte of HTML ahead of first
102102
* paint on the logs, settings, and editor routes that never read it.
103103
*
104-
* The read is capped at one row past the budget purely to detect the overflow. A
105-
* workspace above it seeds NOTHING rather than a prefix: the sidebar search filters this
106-
* list client-side and the Files browser renders it as the workspace's files, so a
107-
* truncated seed would silently hide files. Those workspaces fetch the complete list
108-
* from the route instead — which for a list that large is also the cheaper first paint.
104+
* A workspace above the budget seeds NOTHING rather than a prefix: the sidebar search
105+
* filters this list client-side and the Files browser renders it as the workspace's
106+
* files, so a truncated seed would silently hide files. Those workspaces fetch the
107+
* complete list from the route instead — which for a list that large is also the
108+
* cheaper first paint.
109109
*/
110110
export const WORKSPACE_FILE_SEED_MAX = 300
111111

@@ -123,15 +123,15 @@ export const WORKSPACE_FILE_SEED_MAX = 300
123123
* workspace exceeds {@link WORKSPACE_FILE_SEED_MAX}: `prefetchQuery` always creates one,
124124
* and a partial one would be read as the whole list.
125125
*
126-
* The shape comes from the same contract-parsed reader `GET /api/workspaces/[id]/files`
127-
* responds with, so a seeded entry is identical to what the client hook would cache.
126+
* Parsed through the same response contract `GET /api/workspaces/[id]/files` validates
127+
* against, so a seeded entry is identical to what the client hook would cache.
128128
*/
129129
async function seedWorkspaceFiles(queryClient: QueryClient, workspaceId: string): Promise<void> {
130130
try {
131131
const files = await listWorkspaceFilesWithShares(workspaceId, 'active', {
132-
limit: WORKSPACE_FILE_SEED_MAX + 1,
132+
maxRows: WORKSPACE_FILE_SEED_MAX,
133133
})
134-
if (files.length > WORKSPACE_FILE_SEED_MAX) return
134+
if (!files) return
135135
queryClient.setQueryData(workspaceFilesKeys.list(workspaceId, 'active'), files)
136136
} catch (error) {
137137
/** Optimization only: the client fetch reaches the route instead. Logged so drift between

apps/sim/app/workspace/[workspaceId]/upgrade/hooks/use-upgrade-state.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ import type { WorkspaceHostContext } from '@/lib/api/contracts/workspaces'
99
import { useSubscriptionUpgrade } from '@/lib/billing/client/upgrade'
1010
import { CREDIT_TIERS } from '@/lib/billing/constants'
1111
import { getPlanTierCredits, isEnterprise, isFree, isPro, isTeam } from '@/lib/billing/plan-helpers'
12-
import { subscriptionKeys } from '@/hooks/queries/subscription'
12+
import { invalidateWorkspaceUsage } from '@/hooks/queries/utils/invalidate-usage'
13+
import { subscriptionKeys } from '@/hooks/queries/utils/subscription-keys'
1314
import { workspaceHostKeys } from '@/hooks/queries/workspace-host'
14-
import { invalidateWorkspaceUsage } from '@/hooks/queries/workspace-usage'
1515

1616
const PRO_TIER = CREDIT_TIERS[0]
1717
const MAX_TIER = CREDIT_TIERS[1]
@@ -94,15 +94,17 @@ export function useUpgradeState({
9494
/**
9595
* A non-redirect plan switch settles server-side immediately, so every read that
9696
* describes the plan has to be refetched — the host context the page renders from,
97-
* the subscription/usage reads the billing surfaces share, and the workspace credit
98-
* availability that drives the credits chip and the run gate.
97+
* the subscription/usage reads the billing surfaces share, the proration invoice the
98+
* switch just produced, and the workspace credit availability that drives the credits
99+
* chip and the run gate.
99100
*/
100101
const refreshBillingState = useCallback(
101102
() =>
102103
Promise.all([
103104
queryClient.invalidateQueries({ queryKey: workspaceHostKeys.detail(workspaceId) }),
104105
queryClient.invalidateQueries({ queryKey: subscriptionKeys.users() }),
105106
queryClient.invalidateQueries({ queryKey: subscriptionKeys.usage() }),
107+
queryClient.invalidateQueries({ queryKey: subscriptionKeys.invoicesAll() }),
106108
invalidateWorkspaceUsage(queryClient),
107109
]),
108110
[queryClient, workspaceId]

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-wand.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { wandGenerateStreamContract } from '@/lib/api/contracts'
1010
import { readSSEStream } from '@/lib/core/utils/sse'
1111
import { shouldStripCodeFences, stripCodeFences } from '@/lib/wand/strip-code-fences'
1212
import type { GenerationType } from '@/blocks/types'
13-
import { scheduleUsageRefresh } from '@/hooks/queries/workspace-usage'
13+
import { scheduleUsageRefresh } from '@/hooks/queries/utils/invalidate-usage'
1414
import { useSettingsNavigation } from '@/hooks/use-settings-navigation'
1515
import { useWorkflowRegistry } from '@/stores/workflows/registry/store'
1616

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,8 @@ import type { SerializableExecutionState } from '@/executor/execution/types'
6161
import type { BlockLog, BlockState, ExecutionResult, StreamingExecution } from '@/executor/types'
6262
import { hasExecutionResult } from '@/executor/utils/errors'
6363
import { coerceValue } from '@/executor/utils/start-block'
64+
import { scheduleUsageRefresh } from '@/hooks/queries/utils/invalidate-usage'
6465
import { getWorkflows } from '@/hooks/queries/utils/workflow-cache'
65-
import { scheduleUsageRefresh } from '@/hooks/queries/workspace-usage'
6666
import {
6767
isExecutionStreamHttpError,
6868
SSEEventHandlerError,

0 commit comments

Comments
 (0)