Skip to content

Add 100-item limit to getSandboxesMetrics and batch client requests #290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
devin-ai-integration wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add 100-item limit to getSandboxesMetrics and batch client requests #290

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11c80b6
Add 100-item limit to getSandboxesMetrics and batch client requests
devin-ai-integration[bot] Apr 8, 2026
68b7f71
Use combine option on useQueries for referential stability and skip s…
devin-ai-integration[bot] Apr 8, 2026
64cd74d
Move MAX_SANDBOX_IDS_PER_REQUEST to shared sandboxes schemas module
devin-ai-integration[bot] Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/server/api/routers/sandboxes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export const sandboxesRouter = createTRPCRouter({
getSandboxesMetrics: sandboxesRepositoryProcedure
.input(
z.object({
sandboxIds: z.array(z.string()),
sandboxIds: z.array(z.string()).max(100),
})
)
.query(async ({ ctx, input }) => {
Expand Down
72 changes: 52 additions & 20 deletions src/features/dashboard/sandboxes/list/hooks/use-sandboxes-metrics.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
'use client'

import { useQuery } from '@tanstack/react-query'
import { useQueries } from '@tanstack/react-query'
import { useEffect, useMemo, useRef } from 'react'
import { SANDBOXES_METRICS_POLLING_MS } from '@/configs/intervals'
import type { Sandboxes } from '@/core/modules/sandboxes/models'
import type { ClientSandboxesMetrics } from '@/core/modules/sandboxes/models.client'
import { areStringArraysEqual } from '@/lib/utils/array'
import { useTRPC } from '@/trpc/client'
import { useDashboard } from '../../../context'
import { useSandboxMetricsStore } from '../stores/metrics-store'

const MAX_SANDBOX_IDS_PER_REQUEST = 100
Copy link
Copy Markdown
Member

@ben-fornefeld ben-fornefeld Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we put this in a shared constant inside the sandboxes core/module?

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — moved MAX_SANDBOX_IDS_PER_REQUEST to src/core/modules/sandboxes/schemas.ts and imported it in both the tRPC router and the client hook.


interface UseSandboxesMetricsProps {
sandboxes: Sandboxes
pollingIntervalMs?: number
Expand All @@ -31,6 +34,14 @@ function useStableSandboxIdsWhileScrolling(
return activeSandboxIdsRef.current
}

function chunkArray<T>(array: T[], size: number): T[][] {
const chunks: T[][] = []
for (let i = 0; i < array.length; i += size) {
chunks.push(array.slice(i, i + size))
}
return chunks
}

export function useSandboxesMetrics({
sandboxes,
pollingIntervalMs = SANDBOXES_METRICS_POLLING_MS,
Expand All @@ -54,28 +65,49 @@ export function useSandboxesMetrics({
const metricsRefetchInterval =
pollingIntervalMs > 0 ? pollingIntervalMs : false

const metricsQueryInput = useMemo(
() => ({
teamSlug: team.slug,
sandboxIds: activeSandboxIds,
}),
[activeSandboxIds, team.slug]
const sandboxIdChunks = useMemo(
() => chunkArray(activeSandboxIds, MAX_SANDBOX_IDS_PER_REQUEST),
[activeSandboxIds]
)

const { data } = useQuery(
trpc.sandboxes.getSandboxesMetrics.queryOptions(metricsQueryInput, {
enabled: shouldEnableMetricsQuery,
refetchInterval: metricsRefetchInterval,
refetchOnWindowFocus: false,
refetchOnMount: false,
refetchOnReconnect: true,
refetchIntervalInBackground: false,
})
)
const { metrics: mergedMetrics } = useQueries({
queries: sandboxIdChunks.map((chunk) =>
trpc.sandboxes.getSandboxesMetrics.queryOptions(
{
teamSlug: team.slug,
sandboxIds: chunk,
},
{
enabled: shouldEnableMetricsQuery,
refetchInterval: metricsRefetchInterval,
refetchOnWindowFocus: false,
refetchOnMount: false,
refetchOnReconnect: true,
refetchIntervalInBackground: false,
}
)
),
combine: (results) => {
const hasError = results.some((r) => r.isError)
if (hasError) {
return { metrics: undefined }
}

const merged: ClientSandboxesMetrics = {}
for (const result of results) {
if (result.data?.metrics) {
Object.assign(merged, result.data.metrics)
}
}
return {
metrics: Object.keys(merged).length > 0 ? merged : undefined,
}
},
})

useEffect(() => {
if (data?.metrics) {
setMetrics(data.metrics)
if (mergedMetrics) {
setMetrics(mergedMetrics)
}
}, [data, setMetrics])
}, [mergedMetrics, setMetrics])
}
Comment on lines +94 to 112
Copy link
Copy Markdown

@claude claude Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 When one or more batch chunks fail (e.g., a transient network error), only the successful chunks contribute to mergedMetrics, while the Zustand store's setMetrics merges the partial result into existing state ({ ...state.metrics, ...metrics }), leaving sandbox IDs from the failed chunk with their previous stale values indefinitely. The UI then silently displays a mixed-freshness table—some rows current, others outdated—with no error or loading indicator to alert the user. Before this PR a single useQuery meant failure was all-or-nothing; batching introduces a silent partial-staleness regression that should be addressed by either skipping setMetrics when any chunk has errored, or surfacing an error/staleness indicator for affected rows.

Extended reasoning...

What the bug is and how it manifests

The new batching logic issues N parallel chunk queries via useQueries. If chunk K fails (network timeout, server error, etc.), result.data for that chunk is undefined. The mergedMetrics memo at line 91-99 silently skips that chunk (if (result.data?.metrics)), so the sandbox IDs belonging to chunk K are absent from the merged object. setMetrics(mergedMetrics) is then called with only the successful chunks' data.

The specific code path

metrics-store.ts:28 implements setMetrics as:

const mergedMetrics = { ...state.metrics, ...metrics }

This is a merge, not a replace. Sandbox IDs from the failed chunk still exist as keys in state.metrics with their last-fetched values. Because the incoming metrics object omits them, the spread leaves those stale entries untouched. On the next render cycle (or next polling tick that also fails), they remain permanently stale.

Why existing code doesn't prevent it

The useEffect at line 101-105 only guards if (mergedMetrics)—which is truthy as long as at least one chunk succeeded. There is no check for whether any individual query has isError: true. No error state is propagated to the UI component. The polling interval will eventually heal the situation if chunk K starts succeeding again, but if the failure is persistent (e.g., a backend error for a specific subset of sandbox IDs), those rows will remain stale indefinitely.

Impact

Users with >100 active sandboxes (the only ones affected by this PR) will see a metrics table where some rows show up-to-date CPU/memory data and others silently show outdated data from an earlier fetch. There is no staleness badge, spinner, or error message. This is worse than the pre-PR behavior, where a single query failure left ALL rows consistently stale—at least then the polling retry would uniformly correct the situation.

How to fix

Option A (safest): Before calling setMetrics, check whether any result is in an error state:

const hasError = queryResults.some((r) => r.isError)
if (mergedMetrics && \!hasError) {
  setMetrics(mergedMetrics)
}

Option B (better UX): Surface an error/staleness indicator when hasError is true so users know that some metrics may be outdated. Option C: Replace setMetrics's merge strategy with a replace strategy keyed only to the currently active sandbox IDs, so stale entries for failed chunks are not retained.

Step-by-step proof

  1. User has 150 active sandboxes → chunk 1 = IDs 1-100, chunk 2 = IDs 101-150.
  2. Poll fires. Chunk 1 query succeeds, returns fresh metrics for IDs 1-100. Chunk 2 query fails (network error).
  3. mergedMetrics = { id1: fresh, id2: fresh, …, id100: fresh } (IDs 101-150 absent).
  4. setMetrics(mergedMetrics) runs: store becomes { ...state.metrics, id1: fresh, …, id100: fresh } → IDs 101-150 retain their previous stale values.
  5. UI renders: rows 1-100 show fresh metrics; rows 101-150 silently show metrics from the last successful fetch of chunk 2—potentially minutes or hours old—with no indication to the user.

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed — moved the merge logic into useQueries' combine option for referential stability, and added results.some((r) => r.isError) to return undefined when any chunk fails, preventing partial-staleness from being written to the store.

Comment on lines +77 to 112
Copy link
Copy Markdown

@claude claude Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The new useQueries call returns a new array reference on every render (without combine), so useMemo([queryResults]) recomputes mergedMetrics every render cycle—even when fetched data is unchanged. This causes useEffect to fire and setMetrics to be called on every render, which updates the Zustand store and re-renders all sandbox metric cells on every polling interval. The fix is to add a combine option to useQueries that merges results into a stable object, so downstream dependencies only update when data actually changes.

Extended reasoning...

The bug: useQueries from TanStack Query does not provide referential stability for the returned results array unless the combine option is used. On every render (including every polling cycle), queryResults is a brand-new array reference even if the underlying query data has not changed at all.

The cascade this triggers (lines 78–107):

  1. queryResults is a new array reference on every render
  2. useMemo(() => { ... }, [queryResults]) (line 94) uses Object.is comparison on its dependencies — it always sees a new queryResults and recomputes unconditionally
  3. The merge loop produces a new ClientSandboxesMetrics object, so mergedMetrics is a new object reference every render
  4. useEffect(() => { setMetrics(mergedMetrics) }, [mergedMetrics, setMetrics]) (line 103) fires on every render because mergedMetrics is always a new reference
  5. setMetrics in the Zustand store does const mergedMetrics = { ...state.metrics, ...metrics }, always producing a new state object, which notifies all store subscribers
  6. Every component calling useSandboxMetricsStore((s) => s.metrics.xxx) re-renders

Step-by-step proof with polling enabled:
Assume 10 sandboxes, pollingIntervalMs = 5000, data unchanged between polls.

  • T=0: first render, queryResults = [result0] (ref A). mergedMetrics computed (ref M1). setMetrics(M1) called.
  • T=5s: polling fires, React Query triggers a re-render. queryResults = [result0] (ref B, NEW). Object.is(A, B) === false → useMemo recomputes → mergedMetrics = new object ref M2. useEffect fires → setMetrics(M2). Zustand creates new state object. All metric cells re-render.
  • T=10s: same cycle repeats, even though data is bit-for-bit identical.

Why the previous code didn't have this problem: The old useQuery approach benefited from TanStack Query's structural sharing — if the fetched JSON is identical, data retains the same object reference. So useEffect([data]) only fired when data actually changed.

How to fix it: Use the combine option on useQueries to merge results into a stable derived value. TanStack Query will only call combine when query state actually changes, and the returned value is memoized with structural equality so the reference is stable:

const mergedMetrics = useQueries({
  queries: sandboxIdChunks.map((chunk) =>
    trpc.sandboxes.getSandboxesMetrics.queryOptions({ teamSlug: team.slug, sandboxIds: chunk }, { ... })
  ),
  combine: (results) => {
    const merged: ClientSandboxesMetrics = {}
    for (const result of results) {
      if (result.data?.metrics) Object.assign(merged, result.data.metrics)
    }
    return Object.keys(merged).length > 0 ? merged : undefined
  },
})

With this change, the useMemo and useEffect wrappers are no longer needed, and the Zustand store is only updated when metrics data actually changes.

Copy link
Copy Markdown
Member

@ben-fornefeld ben-fornefeld Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

devin, is this still valid?

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is no longer valid — the comment was on the first commit which used useMemo([queryResults]). The second commit (68b7f71) already addressed this by moving the merge logic into useQueries' combine option, which React Query memoizes internally. The current code (line 89-104) uses combine and no longer has the separate useMemo.

Loading