Skip to content

Commit b654bc1

Browse files
authored
improvement(navigation): eliminate hidden workspace requests (#6979)
# Conflicts: # apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/deploy.tsx
1 parent ab35ff4 commit b654bc1

11 files changed

Lines changed: 429 additions & 50 deletions

File tree

.agents/skills/react-query-best-practices/SKILL.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ Read these before analyzing:
3535
- Every query must have an explicit `staleTime` (default 0 is almost never correct), assigned from a named exported constant — never an inline numeric literal. A server-side prefetch hydrating the same query key must import and reuse that constant instead of restating the number
3636
- `keepPreviousData` / `placeholderData` only on variable-key queries (where params change), never on static keys
3737
- Use `enabled` to prevent queries from running without required params
38+
- Warm data for hover/focus intent with `queryClient.prefetchQuery` and shared `queryOptions`; never temporarily enable a mounted hidden observer, which can remain active after focus restoration and refetch data for closed UI
39+
- When gating a query by view or modal state, move every consumer to the active query too: imperative refresh/pagination, loading and error feedback, and data-derived controls must never read a disabled query or placeholder data from a previous key
3840
- Compose caller-controlled `enabled` options with required-param guards (`Boolean(id) && (options?.enabled ?? true)`). Never spread options after an internal guard, because `{ enabled: true }` can silently re-enable an invalid request.
3941
- A disabled query can still report `isPending: true`. Aggregate loading state only for queries that are applicable/enabled, or an optional query can hold the whole surface in a permanent loading state.
4042
- Deferred authorization or policy queries must fail closed. Do not give pending/error data the same fallback as a successfully loaded unrestricted policy; disable guarded actions until the policy query succeeds.

apps/sim/app/workspace/[workspaceId]/logs/logs.tsx

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,7 @@ export default function Logs() {
240240

241241
const viewMode = useFilterStore((s) => s.viewMode)
242242
const setViewMode = useFilterStore((s) => s.setViewMode)
243+
const isDashboardView = viewMode === 'dashboard'
243244

244245
const [{ selectedLogId, isSidebarOpen }, dispatch] = useReducer(logSelectionReducer, {
245246
selectedLogId: null,
@@ -277,7 +278,7 @@ export default function Logs() {
277278
const isSidebarOpenRef = useRef(false)
278279
const shouldScrollIntoViewRef = useRef(false)
279280
const resourceTableRef = useRef<ResourceTableHandle>(null)
280-
const logsRefetchRef = useRef<() => void>(() => {})
281+
const activeViewRefetchRef = useRef<() => void>(() => {})
281282
const activeLogRefetchRef = useRef<() => void>(() => {})
282283
const activeLogTabRef = useRef<string>('overview')
283284
const logsQueryRef = useRef({ isFetching: false, hasNextPage: false, fetchNextPage: () => {} })
@@ -316,6 +317,7 @@ export default function Logs() {
316317
)
317318

318319
const selectedDetailQuery = useLogDetail(selectedLogId ?? undefined, workspaceId, {
320+
enabled: isSidebarOpen,
319321
refetchInterval,
320322
})
321323

@@ -352,6 +354,7 @@ export default function Logs() {
352354
)
353355

354356
const logsQuery = useLogsList(workspaceId, logFilters, {
357+
enabled: !isDashboardView || isSidebarOpen,
355358
refetchInterval: isLive ? LIVE_REFRESH_INTERVAL_MS : false,
356359
})
357360

@@ -370,6 +373,7 @@ export default function Logs() {
370373
)
371374

372375
const dashboardStatsQuery = useDashboardStats(workspaceId, dashboardFilters, {
376+
enabled: isDashboardView,
373377
refetchInterval: isLive ? LIVE_REFRESH_INTERVAL_MS : false,
374378
})
375379

@@ -394,7 +398,14 @@ export default function Logs() {
394398
selectedLogIndexRef.current = selectedLogIndex
395399
selectedLogIdRef.current = selectedLogId
396400
isSidebarOpenRef.current = isSidebarOpen
397-
logsRefetchRef.current = logsQuery.refetch
401+
activeViewRefetchRef.current = () => {
402+
if (isDashboardView) {
403+
void dashboardStatsQuery.refetch()
404+
}
405+
if (!isDashboardView || isSidebarOpen) {
406+
void logsQuery.refetch()
407+
}
408+
}
398409
activeLogRefetchRef.current = selectedDetailQuery.refetch
399410
logsQueryRef.current = {
400411
isFetching: logsQuery.isFetching,
@@ -641,22 +652,25 @@ export default function Logs() {
641652

642653
const handleRefresh = useCallback(() => {
643654
triggerVisualRefresh()
644-
logsRefetchRef.current()
645-
if (selectedLogIdRef.current) {
655+
activeViewRefetchRef.current()
656+
if (selectedLogIdRef.current && isSidebarOpenRef.current) {
646657
activeLogRefetchRef.current()
647658
}
648659
}, [triggerVisualRefresh])
649660

650-
const prevIsFetchingRef = useRef(logsQuery.isFetching)
661+
const activeViewIsFetching = isDashboardView
662+
? dashboardStatsQuery.isFetching || (isSidebarOpen && logsQuery.isFetching)
663+
: logsQuery.isFetching
664+
const prevIsFetchingRef = useRef(activeViewIsFetching)
651665
useEffect(() => {
652666
const wasFetching = prevIsFetchingRef.current
653-
const isFetching = logsQuery.isFetching
667+
const isFetching = activeViewIsFetching
654668
prevIsFetchingRef.current = isFetching
655669

656670
if (isLive && !wasFetching && isFetching) {
657671
triggerVisualRefresh()
658672
}
659-
}, [logsQuery.isFetching, isLive, triggerVisualRefresh])
673+
}, [activeViewIsFetching, isLive, triggerVisualRefresh])
660674

661675
const handleExport = useCallback(async () => {
662676
setIsExporting(true)
@@ -777,8 +791,6 @@ export default function Logs() {
777791
setPreviewLogId(null)
778792
}
779793

780-
const isDashboardView = viewMode === 'dashboard'
781-
782794
const rows: ResourceRow[] = useMemo(
783795
() =>
784796
logs.map((log) => {
@@ -1135,14 +1147,17 @@ export default function Logs() {
11351147
)
11361148

11371149
const refreshIcon = isVisuallyRefreshing ? SpinningRefreshCw : RefreshCw
1150+
const hasExportableLogs = isDashboardView
1151+
? !dashboardStatsQuery.isPlaceholderData && (dashboardStatsQuery.data?.totalRuns ?? 0) > 0
1152+
: !logsQuery.isPlaceholderData && logs.length > 0
11381153

11391154
const headerActions = useMemo<ResourceAction[]>(
11401155
() => [
11411156
{
11421157
text: 'Export',
11431158
icon: Download,
11441159
onSelect: handleExport,
1145-
disabled: !userPermissions.canEdit || isExporting || logs.length === 0,
1160+
disabled: !userPermissions.canEdit || isExporting || !hasExportableLogs,
11461161
},
11471162
{
11481163
text: 'Refresh',
@@ -1170,7 +1185,7 @@ export default function Logs() {
11701185
handleExport,
11711186
userPermissions.canEdit,
11721187
isExporting,
1173-
logs.length,
1188+
hasExportableLogs,
11741189
]
11751190
)
11761191

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { act } from 'react'
5+
import { createRoot, type Root } from 'react-dom/client'
6+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
const mocks = vi.hoisted(() => ({
9+
pathname: '/workspace/workspace-1/tables',
10+
workspaceId: 'workspace-1' as string | undefined,
11+
searchOpen: false,
12+
useProviderModels: vi.fn(() => ({
13+
data: undefined,
14+
isLoading: false,
15+
isFetching: false,
16+
error: null,
17+
})),
18+
setProviderModels: vi.fn(),
19+
setProviderLoading: vi.fn(),
20+
setOpenRouterModelInfo: vi.fn(),
21+
}))
22+
23+
vi.mock('@sim/logger', () => ({
24+
createLogger: () => ({ error: vi.fn(), warn: vi.fn() }),
25+
}))
26+
27+
vi.mock('next/navigation', () => ({
28+
useParams: () => ({ workspaceId: mocks.workspaceId }),
29+
usePathname: () => mocks.pathname,
30+
}))
31+
32+
vi.mock('@/hooks/queries/providers', () => ({
33+
useProviderModels: mocks.useProviderModels,
34+
}))
35+
36+
vi.mock('@/providers/utils', () => ({
37+
updateBasetenProviderModels: vi.fn(),
38+
updateFireworksProviderModels: vi.fn(),
39+
updateLiteLLMProviderModels: vi.fn(),
40+
updateOllamaCloudProviderModels: vi.fn(),
41+
updateOllamaProviderModels: vi.fn(),
42+
updateOpenRouterProviderModels: vi.fn(),
43+
updateTogetherProviderModels: vi.fn(),
44+
updateVLLMProviderModels: vi.fn(),
45+
}))
46+
47+
vi.mock('@/stores/modals/search/store', () => ({
48+
useSearchModalStore: (selector: (state: { isOpen: boolean }) => unknown) =>
49+
selector({ isOpen: mocks.searchOpen }),
50+
}))
51+
52+
vi.mock('@/stores/providers', () => ({
53+
useProvidersStore: (
54+
selector: (state: {
55+
setProviderModels: typeof mocks.setProviderModels
56+
setProviderLoading: typeof mocks.setProviderLoading
57+
setOpenRouterModelInfo: typeof mocks.setOpenRouterModelInfo
58+
}) => unknown
59+
) =>
60+
selector({
61+
setProviderModels: mocks.setProviderModels,
62+
setProviderLoading: mocks.setProviderLoading,
63+
setOpenRouterModelInfo: mocks.setOpenRouterModelInfo,
64+
}),
65+
}))
66+
67+
import { ProviderModelsLoader } from '@/app/workspace/[workspaceId]/providers/provider-models-loader'
68+
69+
let root: Root
70+
71+
function renderLoader() {
72+
act(() => {
73+
root.render(<ProviderModelsLoader />)
74+
})
75+
}
76+
77+
function expectEveryProviderEnabled(enabled: boolean) {
78+
expect(mocks.useProviderModels).toHaveBeenCalledTimes(9)
79+
for (const call of mocks.useProviderModels.mock.calls) {
80+
expect(call[2]).toEqual({ enabled })
81+
}
82+
}
83+
84+
describe('ProviderModelsLoader request gating', () => {
85+
beforeEach(() => {
86+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
87+
root = createRoot(document.createElement('div'))
88+
mocks.pathname = '/workspace/workspace-1/tables'
89+
mocks.workspaceId = 'workspace-1'
90+
mocks.searchOpen = false
91+
})
92+
93+
afterEach(() => {
94+
act(() => root.unmount())
95+
vi.clearAllMocks()
96+
})
97+
98+
it.each(['tables', 'knowledge', 'files', 'logs', 'settings'])(
99+
'defers every provider catalog on the %s route',
100+
(route) => {
101+
mocks.pathname = `/workspace/workspace-1/${route}`
102+
renderLoader()
103+
104+
expectEveryProviderEnabled(false)
105+
}
106+
)
107+
108+
it.each(['home', 'w/workflow-1', 'chat/chat-1'])(
109+
'loads every provider catalog on the %s route',
110+
(route) => {
111+
mocks.pathname = `/workspace/workspace-1/${route}`
112+
renderLoader()
113+
114+
expectEveryProviderEnabled(true)
115+
}
116+
)
117+
118+
it('loads every provider catalog when global search opens on a resource route', () => {
119+
mocks.searchOpen = true
120+
renderLoader()
121+
122+
expectEveryProviderEnabled(true)
123+
})
124+
125+
it('does not create an empty-workspace route prefix', () => {
126+
mocks.workspaceId = undefined
127+
mocks.searchOpen = true
128+
renderLoader()
129+
130+
expectEveryProviderEnabled(false)
131+
})
132+
})

apps/sim/app/workspace/[workspaceId]/providers/provider-models-loader.tsx

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import { useEffect } from 'react'
44
import { createLogger } from '@sim/logger'
5-
import { useParams } from 'next/navigation'
5+
import { useParams, usePathname } from 'next/navigation'
66
import { useProviderModels } from '@/hooks/queries/providers'
77
import {
88
updateBasetenProviderModels,
@@ -14,15 +14,37 @@ import {
1414
updateTogetherProviderModels,
1515
updateVLLMProviderModels,
1616
} from '@/providers/utils'
17+
import { useSearchModalStore } from '@/stores/modals/search/store'
1718
import { type ProviderName, useProvidersStore } from '@/stores/providers'
1819

1920
const logger = createLogger('ProviderModelsLoader')
2021

21-
function useSyncProvider(provider: ProviderName, workspaceId?: string) {
22+
function shouldLoadProviderModels(
23+
pathname: string | null,
24+
workspaceId: string | undefined,
25+
isSearchModalOpen: boolean
26+
): boolean {
27+
if (!workspaceId) return false
28+
if (isSearchModalOpen) return true
29+
30+
const workspaceBase = `/workspace/${workspaceId}`
31+
return (
32+
pathname === workspaceBase ||
33+
pathname === `${workspaceBase}/home` ||
34+
pathname === `${workspaceBase}/w` ||
35+
pathname?.startsWith(`${workspaceBase}/w/`) === true ||
36+
pathname === `${workspaceBase}/chat` ||
37+
pathname?.startsWith(`${workspaceBase}/chat/`) === true
38+
)
39+
}
40+
41+
function useSyncProvider(provider: ProviderName, enabled: boolean, workspaceId?: string) {
2242
const setProviderModels = useProvidersStore((state) => state.setProviderModels)
2343
const setProviderLoading = useProvidersStore((state) => state.setProviderLoading)
2444
const setOpenRouterModelInfo = useProvidersStore((state) => state.setOpenRouterModelInfo)
25-
const { data, isLoading, isFetching, error } = useProviderModels(provider, workspaceId)
45+
const { data, isLoading, isFetching, error } = useProviderModels(provider, workspaceId, {
46+
enabled,
47+
})
2648

2749
useEffect(() => {
2850
setProviderLoading(provider, isLoading || isFetching)
@@ -68,16 +90,19 @@ function useSyncProvider(provider: ProviderName, workspaceId?: string) {
6890

6991
export function ProviderModelsLoader() {
7092
const params = useParams()
93+
const pathname = usePathname()
7194
const workspaceId = params?.workspaceId as string | undefined
95+
const isSearchModalOpen = useSearchModalStore((state) => state.isOpen)
96+
const shouldLoad = shouldLoadProviderModels(pathname, workspaceId, isSearchModalOpen)
7297

73-
useSyncProvider('base')
74-
useSyncProvider('ollama')
75-
useSyncProvider('ollama-cloud', workspaceId)
76-
useSyncProvider('vllm')
77-
useSyncProvider('litellm')
78-
useSyncProvider('openrouter')
79-
useSyncProvider('fireworks', workspaceId)
80-
useSyncProvider('together', workspaceId)
81-
useSyncProvider('baseten', workspaceId)
98+
useSyncProvider('base', shouldLoad)
99+
useSyncProvider('ollama', shouldLoad)
100+
useSyncProvider('ollama-cloud', shouldLoad, workspaceId)
101+
useSyncProvider('vllm', shouldLoad)
102+
useSyncProvider('litellm', shouldLoad)
103+
useSyncProvider('openrouter', shouldLoad)
104+
useSyncProvider('fireworks', shouldLoad, workspaceId)
105+
useSyncProvider('together', shouldLoad, workspaceId)
106+
useSyncProvider('baseten', shouldLoad, workspaceId)
82107
return null
83108
}

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/deploy-modal.tsx

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,9 +140,14 @@ export function DeployModal({
140140
const userPermissions = useUserPermissionsContext()
141141
const canManageWorkspaceKeys = userPermissions.canAdmin
142142
const { config: permissionConfig, isPublicApiDisabled } = usePermissionConfig()
143-
const { data: apiKeysData, isLoading: isLoadingKeys } = useApiKeys(workflowWorkspaceId || '')
143+
const { data: apiKeysData, isLoading: isLoadingKeys } = useApiKeys(
144+
workflowWorkspaceId || '',
145+
'combined',
146+
{ enabled: open }
147+
)
144148
const { data: workspaceSettingsData, isLoading: isLoadingSettings } = useWorkspaceSettings(
145-
workflowWorkspaceId || ''
149+
workflowWorkspaceId || '',
150+
{ enabled: open }
146151
)
147152
const apiKeyWorkspaceKeys = apiKeysData?.workspaceKeys || []
148153
const apiKeyPersonalKeys = apiKeysData?.personalKeys || []
@@ -170,7 +175,9 @@ export function DeployModal({
170175
refetch: refetchChatInfo,
171176
} = useChatDeploymentInfo(workflowId, { enabled: open })
172177

173-
const { data: mcpServers = [] } = useWorkflowMcpServers(workflowWorkspaceId || '')
178+
const { data: mcpServers = [] } = useWorkflowMcpServers(workflowWorkspaceId || '', {
179+
enabled: open,
180+
})
174181
const hasMcpServers = mcpServers.length > 0
175182

176183
const deployMutation = useDeployWorkflow()

0 commit comments

Comments
 (0)