Skip to content

Commit 220fc37

Browse files
committed
fix(settings): tighten intent and execution boundaries
1 parent f0227a4 commit 220fc37

16 files changed

Lines changed: 214 additions & 237 deletions

File tree

.claude/rules/sim-settings-pages.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,10 +104,11 @@ Adding a new settings page:
104104
2. Render the component inside the shell's `effectiveSection` switch in
105105
`settings/[section]/settings.tsx`.
106106
3. Build the component body inside `<SettingsPanel>` — no shell, no title block.
107-
4. When the initial body depends on server data, export shared React Query options for both the
108-
mounted consumer and the settings intent warmer. Warm only authorized destinations, preserve
109-
the current section during the transition, and follow the failure-recovery rules in
110-
`sim-react-performance.md`; never render temporary default data that will be replaced after load.
107+
4. When a real second consumer or server boundary needs it, extract client-safe React Query options;
108+
otherwise keep them with the hook. Approved intent warmers reuse those exact options and must keep
109+
`check-tool-registry-boundary` green. Warm only authorized destinations, preserve the current
110+
section during the transition, and follow `sim-react-performance.md` recovery rules; never render
111+
temporary default data that will be replaced after load.
111112

112113
## Text-scale tokens (no literal pixel sizes)
113114

apps/sim/app/workspace/[workspaceId]/settings/components/settings-empty-state/settings-empty-state.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ export function SettingsEmptyState({
4545
)
4646
}
4747

48-
/** Canonical recoverable error state for settings queries. */
4948
export function SettingsQueryErrorState({
5049
error,
5150
fallback,

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

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,7 @@ describe('useWorkflowExecution cancellation', () => {
417417
describe('useWorkflowExecution attachment uploads', () => {
418418
beforeEach(() => {
419419
vi.clearAllMocks()
420+
mockEndScopedExecution.mockReset().mockReturnValue(true)
420421
terminalStoreState._hasHydrated = false
421422
executionStoreState.workflowExecutions.set('workflow-1', idleExecution)
422423
executionStoreState.getWorkflowExecution.mockReturnValue(idleExecution)
@@ -562,18 +563,23 @@ describe('useWorkflowExecution attachment uploads', () => {
562563
it('does not let an overlapping run without lifecycle ownership end the active run', async () => {
563564
const persistenceExecution = {}
564565
let resolveActiveRun: (() => void) | undefined
566+
let markExecutionStarted: (() => void) | undefined
567+
const executionStarted = new Promise<void>((resolve) => {
568+
markExecutionStarted = resolve
569+
})
565570
mockBeginScopedExecution.mockReturnValueOnce(persistenceExecution)
566-
mockExecute.mockImplementationOnce(
567-
() =>
568-
new Promise<void>((resolve) => {
569-
resolveActiveRun = resolve
570-
})
571-
)
571+
mockExecute.mockImplementationOnce(() => {
572+
markExecutionStarted?.()
573+
return new Promise<void>((resolve) => {
574+
resolveActiveRun = resolve
575+
})
576+
})
572577
const { result, unmount } = renderWorkflowExecutionHook()
573578

574579
let activeRun: unknown
575580
await act(async () => {
576581
activeRun = await result().handleRunWorkflow({ input: 'active run' })
582+
await executionStarted
577583
})
578584

579585
executionStoreState.getWorkflowExecution.mockReturnValue({
@@ -586,7 +592,11 @@ describe('useWorkflowExecution attachment uploads', () => {
586592
})
587593

588594
expect(mockBeginScopedExecution).toHaveBeenCalledTimes(1)
595+
expect(mockExecute).toHaveBeenCalledTimes(1)
589596
expect(mockEndScopedExecution).not.toHaveBeenCalled()
597+
expect(executionStoreState.setCurrentExecutionId).not.toHaveBeenCalled()
598+
expect(executionStoreState.setIsDebugging).not.toHaveBeenCalled()
599+
expect(executionStoreState.setActiveBlocks).not.toHaveBeenCalled()
590600

591601
await act(async () => {
592602
resolveActiveRun?.()
@@ -599,6 +609,35 @@ describe('useWorkflowExecution attachment uploads', () => {
599609
unmount()
600610
})
601611

612+
it('rejects overlapping block runs before starting another execution', async () => {
613+
executionStoreState.getWorkflowExecution.mockReturnValue({
614+
...idleExecution,
615+
isExecuting: true,
616+
})
617+
const startCandidate = {
618+
blockId: 'start',
619+
block: workflowBlocks.start,
620+
path: 'legacy-starter',
621+
}
622+
mockResolveStartCandidates.mockReturnValue([startCandidate])
623+
624+
const { result, unmount } = renderWorkflowExecutionHook()
625+
626+
await act(async () => {
627+
await result().handleRunUntilBlock('start', 'workflow-1')
628+
await result().handleRunFromBlock('start', 'workflow-1')
629+
})
630+
631+
expect(mockBeginScopedExecution).not.toHaveBeenCalled()
632+
expect(mockExecute).not.toHaveBeenCalled()
633+
expect(mockExecuteFromBlock).not.toHaveBeenCalled()
634+
expect(executionStoreState.setCurrentExecutionId).not.toHaveBeenCalled()
635+
expect(executionStoreState.setIsDebugging).not.toHaveBeenCalled()
636+
expect(executionStoreState.setActiveBlocks).not.toHaveBeenCalled()
637+
638+
unmount()
639+
})
640+
602641
it('adopts and finishes persistence ownership created before the hook mounted', async () => {
603642
const persistenceExecution = {}
604643
terminalStoreState._hasHydrated = true

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

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -443,7 +443,7 @@ export function useWorkflowExecution() {
443443
const getCurrentExecutionId = useExecutionStore((s) => s.getCurrentExecutionId)
444444
const rawSetIsExecuting = useExecutionStore((s) => s.setIsExecuting)
445445

446-
const startExecution = useCallback(
446+
const tryStartExecution = useCallback(
447447
(workflowId: string): ConsolePersistenceExecution | undefined => {
448448
const wasExecuting = useExecutionStore.getState().getWorkflowExecution(workflowId).isExecuting
449449
if (wasExecuting) return undefined
@@ -753,9 +753,11 @@ export function useWorkflowExecution() {
753753
return
754754
}
755755

756+
const persistenceExecution = tryStartExecution(activeWorkflowId)
757+
if (!persistenceExecution) return
758+
756759
// Reset execution result and set execution state
757760
setExecutionResult(null)
758-
const persistenceExecution = startExecution(activeWorkflowId)
759761

760762
// Set debug mode only if explicitly requested
761763
if (enableDebug) {
@@ -1060,7 +1062,7 @@ export function useWorkflowExecution() {
10601062
currentWorkflow,
10611063
toggleConsole,
10621064
getVariablesByWorkflowId,
1063-
startExecution,
1065+
tryStartExecution,
10641066
finishOwnedExecution,
10651067
setIsDebugging,
10661068
setDebugContext,
@@ -2079,7 +2081,9 @@ export function useWorkflowExecution() {
20792081
}
20802082
}
20812083

2082-
const persistenceExecution = startExecution(workflowId)
2084+
const persistenceExecution = tryStartExecution(workflowId)
2085+
if (!persistenceExecution) return
2086+
20832087
const runOwnerId = generateId()
20842088
runFromBlockOwnerRef.current = runOwnerId
20852089
const executionIdRef = { current: '' }
@@ -2330,7 +2334,7 @@ export function useWorkflowExecution() {
23302334
clearLastExecutionSnapshot,
23312335
getCurrentExecutionId,
23322336
setCurrentExecutionId,
2333-
startExecution,
2337+
tryStartExecution,
23342338
finishOwnedExecution,
23352339
setActiveBlocks,
23362340
setBlockRunStatus,
@@ -2356,10 +2360,11 @@ export function useWorkflowExecution() {
23562360
return
23572361
}
23582362

2359-
logger.info('Starting run-until-block execution', { workflowId, stopAfterBlockId: blockId })
2363+
const persistenceExecution = tryStartExecution(workflowId)
2364+
if (!persistenceExecution) return
23602365

2366+
logger.info('Starting run-until-block execution', { workflowId, stopAfterBlockId: blockId })
23612367
setExecutionResult(null)
2362-
const persistenceExecution = startExecution(workflowId)
23632368

23642369
const executionId = generateId()
23652370
try {
@@ -2378,7 +2383,7 @@ export function useWorkflowExecution() {
23782383
return errorResult
23792384
}
23802385
},
2381-
[activeWorkflowId, setExecutionResult, startExecution]
2386+
[activeWorkflowId, setExecutionResult, tryStartExecution]
23822387
)
23832388

23842389
useEffect(() => {
@@ -2543,7 +2548,7 @@ export function useWorkflowExecution() {
25432548
activated = true
25442549
setCurrentExecutionId(reconnectWorkflowId, capturedExecutionId)
25452550
reconnectPersistenceExecution =
2546-
startExecution(reconnectWorkflowId) ??
2551+
tryStartExecution(reconnectWorkflowId) ??
25472552
consolePersistence.adoptScopedExecution(reconnectWorkflowId)
25482553
activationOwnsPersistence = Boolean(reconnectPersistenceExecution)
25492554
if (fromEventId === 0) {

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-sidebar/settings-query-warmers.test.ts

Lines changed: 17 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ vi.mock('@/lib/api/client/request', () => ({
1313
}))
1414

1515
import { warmSettingsSectionQuery } from '@/app/workspace/[workspaceId]/w/components/sidebar/components/settings-sidebar/settings-query-warmers'
16-
import { apiKeysQueryOptions } from '@/hooks/queries/api-key-list'
1716
import { workspaceCredentialListQueryOptions } from '@/hooks/queries/utils/fetch-workspace-credentials'
1817

1918
let queryClient: QueryClient
@@ -25,16 +24,16 @@ describe('settings query warmers', () => {
2524
defaultOptions: { queries: { retry: false, retryOnMount: false } },
2625
})
2726
mockRequestJson.mockImplementation((contract: { path: string }) => {
28-
if (contract.path === '/api/mcp/servers' || contract.path === '/api/mcp/workflow-servers') {
29-
return Promise.resolve({ data: { servers: [] } })
30-
}
31-
if (contract.path === '/api/workspaces/[id]/sandboxes') {
32-
return Promise.resolve({ sandboxes: [], entitled: true, strategy: 'prebuilt' })
33-
}
3427
if (contract.path === '/api/credentials') {
3528
return Promise.resolve({ credentials: [] })
3629
}
37-
return Promise.resolve({ keys: [] })
30+
if (
31+
contract.path === '/api/billing' ||
32+
contract.path === '/api/organizations/[id]/billing-summary'
33+
) {
34+
return Promise.resolve({})
35+
}
36+
throw new Error(`Unexpected settings warmer contract: ${contract.path}`)
3837
})
3938
})
4039

@@ -43,28 +42,11 @@ describe('settings query warmers', () => {
4342
vi.clearAllMocks()
4443
})
4544

46-
it('warms only the approved first-content list for each section', async () => {
47-
expect(warmSettingsSectionQuery(queryClient, personalContext, 'apikeys')).toBe(true)
48-
expect(warmSettingsSectionQuery(queryClient, personalContext, 'sandboxes')).toBe(true)
49-
expect(warmSettingsSectionQuery(queryClient, personalContext, 'byok')).toBe(true)
50-
expect(warmSettingsSectionQuery(queryClient, personalContext, 'mcp')).toBe(true)
45+
it('warms only first-content data already present in the shared sidebar graph', async () => {
5146
expect(warmSettingsSectionQuery(queryClient, personalContext, 'secrets')).toBe(true)
52-
expect(warmSettingsSectionQuery(queryClient, personalContext, 'workflow-mcp-servers')).toBe(
53-
true
54-
)
5547

56-
await vi.waitFor(() => expect(mockRequestJson).toHaveBeenCalledTimes(7))
57-
expect(mockRequestJson.mock.calls.map(([contract]) => contract.path)).toEqual(
58-
expect.arrayContaining([
59-
'/api/workspaces/[id]/api-keys',
60-
'/api/users/me/api-keys',
61-
'/api/workspaces/[id]/sandboxes',
62-
'/api/workspaces/[id]/byok-keys',
63-
'/api/mcp/servers',
64-
'/api/mcp/workflow-servers',
65-
'/api/credentials',
66-
])
67-
)
48+
await vi.waitFor(() => expect(mockRequestJson).toHaveBeenCalledTimes(1))
49+
expect(mockRequestJson.mock.calls[0][0].path).toBe('/api/credentials')
6850
expect(
6951
mockRequestJson.mock.calls.find(([contract]) => contract.path === '/api/credentials')?.[1]
7052
).toEqual(
@@ -73,6 +55,13 @@ describe('settings query warmers', () => {
7355
})
7456

7557
it('does not warm broad settings data', () => {
58+
expect(warmSettingsSectionQuery(queryClient, personalContext, 'apikeys')).toBe(false)
59+
expect(warmSettingsSectionQuery(queryClient, personalContext, 'sandboxes')).toBe(false)
60+
expect(warmSettingsSectionQuery(queryClient, personalContext, 'byok')).toBe(false)
61+
expect(warmSettingsSectionQuery(queryClient, personalContext, 'mcp')).toBe(false)
62+
expect(warmSettingsSectionQuery(queryClient, personalContext, 'workflow-mcp-servers')).toBe(
63+
false
64+
)
7665
expect(warmSettingsSectionQuery(queryClient, personalContext, 'custom-tools')).toBe(false)
7766

7867
expect(mockRequestJson).not.toHaveBeenCalled()
@@ -100,15 +89,6 @@ describe('settings query warmers', () => {
10089
)
10190
})
10291

103-
it('deduplicates a successful API-key warm with the eventual consumer', async () => {
104-
warmSettingsSectionQuery(queryClient, personalContext, 'apikeys')
105-
await vi.waitFor(() => expect(mockRequestJson).toHaveBeenCalledTimes(2))
106-
107-
await queryClient.fetchQuery(apiKeysQueryOptions('workspace-1', 'combined'))
108-
109-
expect(mockRequestJson).toHaveBeenCalledTimes(2)
110-
})
111-
11292
it('keeps the Secrets warmer and consumer on mount-recoverable shared options', () => {
11393
const options = workspaceCredentialListQueryOptions('workspace-1', 'env_workspace')
11494

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-sidebar/settings-query-warmers.ts

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,9 @@
11
import type { QueryClient } from '@tanstack/react-query'
22
import type { SettingsSection } from '@/app/workspace/[workspaceId]/settings/navigation'
3-
import { apiKeysQueryOptions } from '@/hooks/queries/api-key-list'
4-
import { byokKeysQueryOptions } from '@/hooks/queries/byok-key-list'
5-
import { mcpServersQueryOptions } from '@/hooks/queries/mcp-server-list'
63
import { organizationBillingSummaryOptions } from '@/hooks/queries/organization-billing-summary'
7-
import { getSandboxListQueryOptions } from '@/hooks/queries/sandbox-list'
84
import { subscriptionDataQueryOptions } from '@/hooks/queries/subscription-data'
95
import { workspaceCredentialListQueryOptions } from '@/hooks/queries/utils/fetch-workspace-credentials'
106
import { prefetchQueryOnIntent } from '@/hooks/queries/utils/prefetch-query-on-intent'
11-
import { workflowMcpServersQueryOptions } from '@/hooks/queries/workflow-mcp-server-list'
127

138
const SETTINGS_QUERY_WARMERS: Partial<
149
Record<SettingsSection, (queryClient: QueryClient, context: SettingsQueryWarmContext) => void>
@@ -18,16 +13,6 @@ const SETTINGS_QUERY_WARMERS: Partial<
1813
queryClient,
1914
workspaceCredentialListQueryOptions(workspaceId, 'env_workspace')
2015
),
21-
apikeys: (queryClient, { workspaceId }) =>
22-
prefetchQueryOnIntent(queryClient, apiKeysQueryOptions(workspaceId, 'combined')),
23-
sandboxes: (queryClient, { workspaceId }) =>
24-
prefetchQueryOnIntent(queryClient, getSandboxListQueryOptions(workspaceId)),
25-
byok: (queryClient, { workspaceId }) =>
26-
prefetchQueryOnIntent(queryClient, byokKeysQueryOptions(workspaceId)),
27-
mcp: (queryClient, { workspaceId }) =>
28-
prefetchQueryOnIntent(queryClient, mcpServersQueryOptions(workspaceId)),
29-
'workflow-mcp-servers': (queryClient, { workspaceId }) =>
30-
prefetchQueryOnIntent(queryClient, workflowMcpServersQueryOptions(workspaceId)),
3116
billing: (queryClient, { billingOrganizationId }) => {
3217
if (billingOrganizationId) {
3318
prefetchQueryOnIntent(queryClient, organizationBillingSummaryOptions(billingOrganizationId))
@@ -42,7 +27,7 @@ export interface SettingsQueryWarmContext {
4227
billingOrganizationId: string | null
4328
}
4429

45-
/** Starts only the first-content query explicitly approved for a settings section. */
30+
/** Starts approved first-content data within the workspace graph's enforced module budget. */
4631
export function warmSettingsSectionQuery(
4732
queryClient: QueryClient,
4833
context: SettingsQueryWarmContext,

apps/sim/hooks/queries/byok-key-list.ts

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

0 commit comments

Comments
 (0)