add benchmark dashboard with static hosting and multi-pool support#200
Merged
Conversation
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Merged
commit: |
ianmacartney
changed the base branch from
ian/snapshot-query-prototype
to
graphite-base/200
ianmacartney
force-pushed
the
ian/scenario-dashboard
branch
from
78ac590 to
4fc490a
Compare
ianmacartney
force-pushed
the
graphite-base/200
branch
from
3369e6c to
461be2b
Compare
ianmacartney
force-pushed
the
ian/scenario-dashboard
branch
from
4fc490a to
4426e4f
Compare
ianmacartney
force-pushed
the
ian/scenario-dashboard
branch
from
4426e4f to
4a5eb79
Compare
ianmacartney
force-pushed
the
ian/scenario-dashboard
branch
from
4a5eb79 to
2d168f7
Compare
ianmacartney
force-pushed
the
ian/optimize
branch
from
36962e9 to
4ad9d7f
Compare
ianmacartney
force-pushed
the
ian/optimize
branch
from
4ad9d7f to
3df8a63
Compare
ianmacartney
force-pushed
the
ian/scenario-dashboard
branch
from
2d168f7 to
1b5dd6f
Compare
ianmacartney
force-pushed
the
graphite-base/200
branch
from
3df8a63 to
b067469
Compare
ianmacartney
force-pushed
the
ian/scenario-dashboard
branch
from
71e530a to
a8ad0dc
Compare
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive benchmark dashboard for the workpool example app. It adds backend infrastructure to support pool-aware scenario execution, creates a full React UI for running and comparing benchmark scenarios, enables static hosting of the dashboard, and updates all scenario implementations to support runtime pool selection. The dashboard enables users to launch workload scenarios, view run metrics with time-series charts, and compare runs side-by-side with computed delta metrics, while maintaining full state persistence and URL-shareable views. Sequence Diagram(s)sequenceDiagram
participant User as Browser User
participant Frontend as React Dashboard
participant ConvexAction as Convex (dashboard.runScenarios)
participant Scenarios as Scenario handlers
participant PoolA as Pool new
participant PoolB as Pool old
participant Backend as Backend work processors
participant DB as Database
User->>Frontend: Select scenario + pool + args
User->>Frontend: Click "Run"
Frontend->>ConvexAction: runScenarios(scenario, argsList)
ConvexAction->>Scenarios: Launch scenario 1 (pool selection)
activate Scenarios
Scenarios->>DB: Create run with pool field
Scenarios->>PoolA: configurePool(maxParallelism)
Scenarios->>PoolA: Enqueue tasks via pool
PoolA->>Backend: Tasks routed to new pool
Backend->>Backend: Execute work
Backend->>DB: Record task completion
deactivate Scenarios
ConvexAction->>Frontend: runId
Frontend->>Frontend: Fetch run metrics via listRuns/getRun
Frontend->>Frontend: Render history, throughput, latency CDF
Frontend-->>User: Display run results
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
example/convex/test/dashboard.ts (2)
6-9: ⚡ Quick winDuplicate helper function.
This
percentilefunction is identical to the one inrun.ts(line 147). Consider extracting it to a shared utility module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/convex/test/dashboard.ts` around lines 6 - 9, The percentile helper is duplicated (function percentile in this diff and the identical one in run.ts); extract it to a shared utility module (e.g., create/extend a utils file) and import it from both example/convex/test/dashboard.ts and run.ts, replacing the local implementations with an import of percentile; ensure the exported symbol name remains percentile and update any imports/exports accordingly so both files use the same implementation.
193-202: 💤 Low valueSimplify Promise.all assignment.
The destructuring
const [,] = await Promise.all([...])ignores all return values. Since the results aren't used, you can omit the assignment:♻️ Suggested simplification
- const [,] = await Promise.all([ + await Promise.all([ ctx.runAction(fn, { ...args, pool: "old" }), ctx.runAction(fn, { ...args, pool: "new" }), ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/convex/test/dashboard.ts` around lines 193 - 202, In runConcurrent's handler the line `const [,] = await Promise.all([ ... ])` discards results unnecessarily; replace it with just `await Promise.all([ ctx.runAction(fn, { ...args, pool: "old" }), ctx.runAction(fn, { ...args, pool: "new" }) ])` (or return that promise) so the Promise.all is awaited without an unused destructuring; locate this in the `runConcurrent` action handler where `fn` is derived from `internal.test.scenarios[scenario].default`.example/convex/test/run.ts (1)
107-145: ⚡ Quick winConsider extracting shared metrics computation logic.
The
metricsForRunquery duplicates computation logic found in themetricsquery (lines 153-227) and indashboard.getRun. Thepercentilehelper is also duplicated indashboard.ts. Consider extracting a shared helper function for run metrics computation to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/convex/test/run.ts` around lines 107 - 145, metricsForRun duplicates run metrics computation found in metrics and dashboard.getRun and also reimplements percentile; create a shared helper (e.g., computeRunMetrics) that accepts (run, tasks, runStatus) and returns the same payload shape (status, completedCount, taskCount, optional totalDurationMs, optional latency {p50,p95,p99,max}), and move the percentile helper into a common module used by all three callers; then replace the body in metricsForRun, metrics, and dashboard.getRun to call computeRunMetrics with the run and tasks (and use runStatus(ctx, run) as input) so all calculations are centralized and duplication removed.example/convex/test/scenarios/sustained.ts (1)
88-238: ⚡ Quick winConsider using run-scoped metrics for concurrent-run safety.
Similar to
overhead.ts, this scenario uses the globalmetricsquery (line 196) instead of the run-scopedmetricsForRun. Sincesustainedruns over 20 seconds with variable worker durations, there's a higher chance of overlapping with another scenario on a different pool.For consistency with
throughput.tsand to prevent cross-run status confusion:- metrics = (await ctx.runQuery(internal.test.run.metrics)) as Record< - string, - unknown - > | null; + metrics = (await ctx.runQuery(internal.test.run.metricsForRun, { + runId, + })) as Record<string, unknown> | null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/convex/test/scenarios/sustained.ts` around lines 88 - 238, The poll uses the global metrics query (internal.test.run.metrics) which can mix data across concurrent runs; change the query to the run-scoped metricsForRun (internal.test.run.metricsForRun) and pass the current runId when calling ctx.runQuery so the poll only observes this run's status and counts; update the variable handling to expect the same shape returned by metricsForRun and keep the existing status/completion checks and timeout logic intact (look for usages of metrics, poll loop, pollStart, and where ctx.runQuery(internal.test.run.metrics) is called).example/convex/test/scenarios/overhead.ts (1)
58-198: ⚡ Quick winConsider using run-scoped metrics for concurrent-run safety.
The scenario uses the global
metricsquery (line 164), whilethroughput.tswas updated to usemetricsForRun({ runId }). Sincerun.startnow permits concurrent runs on different pools, the globalmetricsquery could return the status of a different run if another scenario starts while this one is polling.For consistency with
throughput.tsand to prevent cross-run confusion, consider switching tometricsForRun:- metrics = (await ctx.runQuery(internal.test.run.metrics)) as Record< - string, - unknown - > | null; + metrics = (await ctx.runQuery(internal.test.run.metricsForRun, { + runId, + })) as Record<string, unknown> | null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/convex/test/scenarios/overhead.ts` around lines 58 - 198, The polling currently calls the global metrics query (ctx.runQuery(internal.test.run.metrics)) which can return another run's results; change it to query run-scoped metrics using internal.test.run.metricsForRun with the runId you created earlier (runId) inside the handler (the same handler that calls ctx.runMutation(internal.test.run.start)); replace the ctx.runQuery(internal.test.run.metrics) call with ctx.runQuery(internal.test.run.metricsForRun, { runId }) and keep the existing polling loop/pollTimeoutMs logic and type assertions for metrics.example/src/App.tsx (4)
342-347: 💤 Low valueConsider capitalizing "parameters" for consistency.
The summary text "parameters" is lowercase while other headings in the dashboard use title case. For visual consistency, consider "Parameters".
Suggested change
- <summary className="muted">parameters</summary> + <summary className="muted">Parameters</summary>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/src/App.tsx` around lines 342 - 347, Update the summary text to use title case for visual consistency: change the <summary className="muted"> element that currently contains "parameters" to "Parameters" in the JSX (the snippet around the details/summary block rendering run.parameters in App.tsx), leaving the surrounding markup and className unchanged.
460-460: 💤 Low valueType assertion is safe but could be removed.
The fallback chain
aPts[i]?.tMs ?? bPts[i]?.tMs ?? i * 500guarantees a number, making theas numberassertion unnecessary. TypeScript should infer the type correctly with the fallback.Suggested change
- const tMs = (aPts[i]?.tMs ?? bPts[i]?.tMs ?? i * 500) as number; + const tMs = aPts[i]?.tMs ?? bPts[i]?.tMs ?? i * 500;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/src/App.tsx` at line 460, The const tMs assignment uses an unnecessary type assertion; remove the trailing "as number" from the expression `const tMs = (aPts[i]?.tMs ?? bPts[i]?.tMs ?? i * 500)` so TypeScript can infer the number type from the fallback chain (refer to symbols aPts, bPts, tMs, and loop index i); no other changes needed.
471-479: ⚡ Quick winConsider merging CDF points by
mskey.The current approach creates separate points for each run's CDF data, potentially producing multiple points with the same
msvalue but different percentile fields. While Recharts handles this withconnectNulls, merging points bymsinto a Map would produce cleaner data and avoid relying on the chart library's gap-filling behavior.Refactor to merge points by ms
const cdfData = useMemo(() => { const aArr = cA ?? []; const bArr = cB ?? []; - const points: Array<{ ms: number; aPct?: number; bPct?: number }> = []; - aArr.forEach((p) => points.push({ ms: p.ms, aPct: p.pct })); - bArr.forEach((p) => points.push({ ms: p.ms, bPct: p.pct })); - points.sort((x, y) => x.ms - y.ms); - return points; + const map = new Map<number, { ms: number; aPct?: number; bPct?: number }>(); + aArr.forEach((p) => { + const entry = map.get(p.ms) ?? { ms: p.ms }; + entry.aPct = p.pct; + map.set(p.ms, entry); + }); + bArr.forEach((p) => { + const entry = map.get(p.ms) ?? { ms: p.ms }; + entry.bPct = p.pct; + map.set(p.ms, entry); + }); + return Array.from(map.values()).sort((x, y) => x.ms - y.ms); }, [cA, cB]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/src/App.tsx` around lines 471 - 479, The cdfData useMemo builds separate points for cA and cB causing duplicate ms entries; change the logic in the useMemo (cdfData) to merge points by ms: iterate cA and cB and insert/update a Map keyed by ms so each ms produces a single object with optional aPct and bPct fields, then convert the Map.values() to a sorted array before returning; update references to the local variables aArr, bArr, points inside that useMemo to perform Map-based merging instead of pushing duplicate point objects.
259-259: Re: N+1getRunqueries fromHistoryRow
HistoryRowtriggersapi.test.dashboard.getRunper row, butlistRunsintentionally omits computed metrics to avoid collecting/aggregating tasks for many runs on every history poll (seeexample/convex/test/dashboard.tscomment abovelistRuns).- Any reduction in query count would likely require a dedicated batched endpoint that computes metrics for multiple
runIds in one pass (grouping tasks byrunId), not a simple extension oflistRuns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/src/App.tsx` at line 259, HistoryRow currently calls api.test.dashboard.getRun per row causing N+1 queries because listRuns intentionally omits computed metrics; fix by removing per-row getRun calls and implement a batched endpoint (e.g., dashboard.getRunsMetrics or similar) that accepts an array of runIds and returns metrics grouped by runId, then update the component to fetch metrics once for all visible rows and map results into HistoryRow props instead of calling api.test.dashboard.getRun inside HistoryRow; ensure new endpoint computes metrics by grouping tasks by runId as described in example/convex/test/dashboard.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@example/convex/test/dashboard.ts`:
- Around line 96-102: The current eIdx computation silently clamps negative
indices (Math.max(0, ...)) when t.enqueuedAt < start; update the code around the
eIdx calculation to first detect this condition (check if t.enqueuedAt < start)
and surface it (e.g., console.warn/processLogger.warn or throw/assert with
identifying info from t such as an id or enqueuedAt) before performing the
clamping, then compute eIdx using the same Math.floor/min logic; keep the final
clamping to avoid crashes but ensure the warning/assertion makes the
data-integrity issue visible.
In `@example/convex/test/pool.ts`:
- Around line 45-62: The declared return type of enqueueFor is too narrow
because it always uses typeof enqueue/typeof enqueueBatch even though the "old"
branch returns enqueueOld/enqueueBatchOld; update enqueueFor's signature to
reflect both possibilities by either adding function overloads for
enqueueFor(kind: "new") and enqueueFor(kind: "old") returning distinct types, or
change the return type to a discriminated union that lists both shapes (one with
enqueueOne: typeof enqueue / enqueueMany: typeof enqueueBatch and one with
enqueueOne: typeof enqueueOld / enqueueMany: typeof enqueueBatchOld), and keep
ComponentApi/PoolKind the same so callers get correct type info for
enqueueOne/enqueueMany.
---
Nitpick comments:
In `@example/convex/test/dashboard.ts`:
- Around line 6-9: The percentile helper is duplicated (function percentile in
this diff and the identical one in run.ts); extract it to a shared utility
module (e.g., create/extend a utils file) and import it from both
example/convex/test/dashboard.ts and run.ts, replacing the local implementations
with an import of percentile; ensure the exported symbol name remains percentile
and update any imports/exports accordingly so both files use the same
implementation.
- Around line 193-202: In runConcurrent's handler the line `const [,] = await
Promise.all([ ... ])` discards results unnecessarily; replace it with just
`await Promise.all([ ctx.runAction(fn, { ...args, pool: "old" }),
ctx.runAction(fn, { ...args, pool: "new" }) ])` (or return that promise) so the
Promise.all is awaited without an unused destructuring; locate this in the
`runConcurrent` action handler where `fn` is derived from
`internal.test.scenarios[scenario].default`.
In `@example/convex/test/run.ts`:
- Around line 107-145: metricsForRun duplicates run metrics computation found in
metrics and dashboard.getRun and also reimplements percentile; create a shared
helper (e.g., computeRunMetrics) that accepts (run, tasks, runStatus) and
returns the same payload shape (status, completedCount, taskCount, optional
totalDurationMs, optional latency {p50,p95,p99,max}), and move the percentile
helper into a common module used by all three callers; then replace the body in
metricsForRun, metrics, and dashboard.getRun to call computeRunMetrics with the
run and tasks (and use runStatus(ctx, run) as input) so all calculations are
centralized and duplication removed.
In `@example/convex/test/scenarios/overhead.ts`:
- Around line 58-198: The polling currently calls the global metrics query
(ctx.runQuery(internal.test.run.metrics)) which can return another run's
results; change it to query run-scoped metrics using
internal.test.run.metricsForRun with the runId you created earlier (runId)
inside the handler (the same handler that calls
ctx.runMutation(internal.test.run.start)); replace the
ctx.runQuery(internal.test.run.metrics) call with
ctx.runQuery(internal.test.run.metricsForRun, { runId }) and keep the existing
polling loop/pollTimeoutMs logic and type assertions for metrics.
In `@example/convex/test/scenarios/sustained.ts`:
- Around line 88-238: The poll uses the global metrics query
(internal.test.run.metrics) which can mix data across concurrent runs; change
the query to the run-scoped metricsForRun (internal.test.run.metricsForRun) and
pass the current runId when calling ctx.runQuery so the poll only observes this
run's status and counts; update the variable handling to expect the same shape
returned by metricsForRun and keep the existing status/completion checks and
timeout logic intact (look for usages of metrics, poll loop, pollStart, and
where ctx.runQuery(internal.test.run.metrics) is called).
In `@example/src/App.tsx`:
- Around line 342-347: Update the summary text to use title case for visual
consistency: change the <summary className="muted"> element that currently
contains "parameters" to "Parameters" in the JSX (the snippet around the
details/summary block rendering run.parameters in App.tsx), leaving the
surrounding markup and className unchanged.
- Line 460: The const tMs assignment uses an unnecessary type assertion; remove
the trailing "as number" from the expression `const tMs = (aPts[i]?.tMs ??
bPts[i]?.tMs ?? i * 500)` so TypeScript can infer the number type from the
fallback chain (refer to symbols aPts, bPts, tMs, and loop index i); no other
changes needed.
- Around line 471-479: The cdfData useMemo builds separate points for cA and cB
causing duplicate ms entries; change the logic in the useMemo (cdfData) to merge
points by ms: iterate cA and cB and insert/update a Map keyed by ms so each ms
produces a single object with optional aPct and bPct fields, then convert the
Map.values() to a sorted array before returning; update references to the local
variables aArr, bArr, points inside that useMemo to perform Map-based merging
instead of pushing duplicate point objects.
- Line 259: HistoryRow currently calls api.test.dashboard.getRun per row causing
N+1 queries because listRuns intentionally omits computed metrics; fix by
removing per-row getRun calls and implement a batched endpoint (e.g.,
dashboard.getRunsMetrics or similar) that accepts an array of runIds and returns
metrics grouped by runId, then update the component to fetch metrics once for
all visible rows and map results into HistoryRow props instead of calling
api.test.dashboard.getRun inside HistoryRow; ensure new endpoint computes
metrics by grouping tasks by runId as described in
example/convex/test/dashboard.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: get-convex/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1be341a1-abee-4b39-a97f-1f426aec134e
⛔ Files ignored due to path filters (5)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**example/public/favicon.svgis excluded by!**/*.svgexample/public/og.pngis excluded by!**/*.pngexample/public/og.svgis excluded by!**/*.svgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
CONTRIBUTING.mdeslint.config.jsexample/README.mdexample/convex/convex.config.tsexample/convex/http.tsexample/convex/schema.tsexample/convex/staticHosting.tsexample/convex/test/dashboard.tsexample/convex/test/pool.tsexample/convex/test/run.tsexample/convex/test/scenarios/bigArgs.tsexample/convex/test/scenarios/bigContext.tsexample/convex/test/scenarios/bigReturnTypes.tsexample/convex/test/scenarios/burstyBatches.tsexample/convex/test/scenarios/overhead.tsexample/convex/test/scenarios/sustained.tsexample/convex/test/scenarios/throughput.tsexample/convex/test/work.tsexample/index.htmlexample/src/App.cssexample/src/App.tsxpackage.json
Comment on lines
+96
to
+102
| const eIdx = Math.max( | ||
| 0, | ||
| Math.min( | ||
| numBuckets - 1, | ||
| Math.floor((t.enqueuedAt - start) / bucketMs), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Defensive clamping may hide data integrity issues.
The Math.max(0, ...) at line 96 silently clamps negative bucket indices when t.enqueuedAt < start. This would occur if a task was enqueued before the run started, indicating a data integrity problem.
Consider adding a warning or assertion to surface this condition rather than silently accepting it:
🛡️ Suggested improvement
if (t.enqueuedAt !== undefined) {
+ if (t.enqueuedAt < start) {
+ console.warn(`Task ${t.workId} enqueuedAt (${t.enqueuedAt}) is before run start (${start})`);
+ }
const eIdx = Math.max(
0,
Math.min(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@example/convex/test/dashboard.ts` around lines 96 - 102, The current eIdx
computation silently clamps negative indices (Math.max(0, ...)) when
t.enqueuedAt < start; update the code around the eIdx calculation to first
detect this condition (check if t.enqueuedAt < start) and surface it (e.g.,
console.warn/processLogger.warn or throw/assert with identifying info from t
such as an id or enqueuedAt) before performing the clamping, then compute eIdx
using the same Math.floor/min logic; keep the final clamping to avoid crashes
but ensure the warning/assertion makes the data-integrity issue visible.
Comment on lines
+45
to
+62
| export function enqueueFor(kind: PoolKind): { | ||
| component: ComponentApi; | ||
| enqueueOne: typeof enqueue; | ||
| enqueueMany: typeof enqueueBatch; | ||
| } { | ||
| if (kind === "new") { | ||
| return { | ||
| component: components.testWorkpool, | ||
| enqueueOne: enqueue, | ||
| enqueueMany: enqueueBatch, | ||
| }; | ||
| } | ||
| return { | ||
| component: components.oldWorkpool, | ||
| enqueueOne: enqueueOld, | ||
| enqueueMany: enqueueBatchOld, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Return type doesn't accurately reflect the union of both branches.
The return type declares enqueueOne: typeof enqueue and enqueueMany: typeof enqueueBatch, but when kind === "old", the function returns enqueueOld and enqueueBatchOld from @convex-dev/workpool-old. TypeScript will provide incorrect type information for the "old" branch.
Consider using a conditional return type or union to accurately type both branches:
♻️ Suggested fix using function overloads
+export function enqueueFor(kind: "new"): {
+ component: ComponentApi;
+ enqueueOne: typeof enqueue;
+ enqueueMany: typeof enqueueBatch;
+};
+export function enqueueFor(kind: "old"): {
+ component: ComponentApi;
+ enqueueOne: typeof enqueueOld;
+ enqueueMany: typeof enqueueBatchOld;
+};
export function enqueueFor(kind: PoolKind): {
component: ComponentApi;
- enqueueOne: typeof enqueue;
- enqueueMany: typeof enqueueBatch;
+ enqueueOne: typeof enqueue | typeof enqueueOld;
+ enqueueMany: typeof enqueueBatch | typeof enqueueBatchOld;
} {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function enqueueFor(kind: PoolKind): { | |
| component: ComponentApi; | |
| enqueueOne: typeof enqueue; | |
| enqueueMany: typeof enqueueBatch; | |
| } { | |
| if (kind === "new") { | |
| return { | |
| component: components.testWorkpool, | |
| enqueueOne: enqueue, | |
| enqueueMany: enqueueBatch, | |
| }; | |
| } | |
| return { | |
| component: components.oldWorkpool, | |
| enqueueOne: enqueueOld, | |
| enqueueMany: enqueueBatchOld, | |
| }; | |
| } | |
| export function enqueueFor(kind: "new"): { | |
| component: ComponentApi; | |
| enqueueOne: typeof enqueue; | |
| enqueueMany: typeof enqueueBatch; | |
| }; | |
| export function enqueueFor(kind: "old"): { | |
| component: ComponentApi; | |
| enqueueOne: typeof enqueueOld; | |
| enqueueMany: typeof enqueueBatchOld; | |
| }; | |
| export function enqueueFor(kind: PoolKind): { | |
| component: ComponentApi; | |
| enqueueOne: typeof enqueue | typeof enqueueOld; | |
| enqueueMany: typeof enqueueBatch | typeof enqueueBatchOld; | |
| } { | |
| if (kind === "new") { | |
| return { | |
| component: components.testWorkpool, | |
| enqueueOne: enqueue, | |
| enqueueMany: enqueueBatch, | |
| }; | |
| } | |
| return { | |
| component: components.oldWorkpool, | |
| enqueueOne: enqueueOld, | |
| enqueueMany: enqueueBatchOld, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@example/convex/test/pool.ts` around lines 45 - 62, The declared return type
of enqueueFor is too narrow because it always uses typeof enqueue/typeof
enqueueBatch even though the "old" branch returns enqueueOld/enqueueBatchOld;
update enqueueFor's signature to reflect both possibilities by either adding
function overloads for enqueueFor(kind: "new") and enqueueFor(kind: "old")
returning distinct types, or change the return type to a discriminated union
that lists both shapes (one with enqueueOne: typeof enqueue / enqueueMany:
typeof enqueueBatch and one with enqueueOne: typeof enqueueOld / enqueueMany:
typeof enqueueBatchOld), and keep ComponentApi/PoolKind the same so callers get
correct type info for enqueueOne/enqueueMany.
a8ad0dc to
9451742
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example/convex/test/scenarios/burstyBatches.ts (1)
143-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse run-scoped metrics polling to avoid cross-run mixups.
Because pool-different runs can now overlap, polling
internal.test.run.metricsat Line 144 can read another run’s state. This can mark this scenario complete using the wrong run’s metrics. Query metrics byrunIdinstead.Suggested fix
- while (Date.now() - pollStart < timeout) { - metrics = (await ctx.runQuery(internal.test.run.metrics)) as Record< + while (Date.now() - pollStart < timeout) { + metrics = (await ctx.runQuery(internal.test.run.metricsForRun, { runId })) as Record< string, unknown > | null; if (metrics && metrics.status === "completed") break; await new Promise((resolve) => setTimeout(resolve, pollInterval)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/convex/test/scenarios/burstyBatches.ts` around lines 143 - 149, The polling loop in the while condition is calling ctx.runQuery with internal.test.run.metrics without specifying which run's metrics to query, which can read metrics from a different overlapping run and incorrectly mark the scenario as completed. Modify the ctx.runQuery call to pass the runId parameter to internal.test.run.metrics so that the metrics query is scoped to the current run only, preventing cross-run state mixups.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@example/convex/test/scenarios/burstyBatches.ts`:
- Around line 143-149: The polling loop in the while condition is calling
ctx.runQuery with internal.test.run.metrics without specifying which run's
metrics to query, which can read metrics from a different overlapping run and
incorrectly mark the scenario as completed. Modify the ctx.runQuery call to pass
the runId parameter to internal.test.run.metrics so that the metrics query is
scoped to the current run only, preventing cross-run state mixups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: get-convex/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4499bc8-9cdb-4b7b-9ec1-7166a63e4f3b
⛔ Files ignored due to path filters (5)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**example/public/favicon.svgis excluded by!**/*.svgexample/public/og.pngis excluded by!**/*.pngexample/public/og.svgis excluded by!**/*.svgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
CONTRIBUTING.mdeslint.config.jsexample/README.mdexample/convex/convex.config.tsexample/convex/http.tsexample/convex/schema.tsexample/convex/staticHosting.tsexample/convex/test/dashboard.tsexample/convex/test/pool.tsexample/convex/test/run.tsexample/convex/test/scenarios/bigArgs.tsexample/convex/test/scenarios/bigContext.tsexample/convex/test/scenarios/bigReturnTypes.tsexample/convex/test/scenarios/burstyBatches.tsexample/convex/test/scenarios/overhead.tsexample/convex/test/scenarios/sustained.tsexample/convex/test/scenarios/throughput.tsexample/convex/test/work.tsexample/index.htmlexample/src/App.cssexample/src/App.tsxpackage.json
✅ Files skipped from review due to trivial changes (3)
- eslint.config.js
- example/index.html
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (17)
- example/convex/schema.ts
- example/src/App.css
- example/convex/http.ts
- example/convex/convex.config.ts
- package.json
- example/convex/test/scenarios/bigContext.ts
- example/convex/test/scenarios/bigArgs.ts
- example/convex/staticHosting.ts
- example/convex/test/scenarios/bigReturnTypes.ts
- example/convex/test/work.ts
- example/convex/test/pool.ts
- example/src/App.tsx
- example/convex/test/scenarios/sustained.ts
- example/convex/test/scenarios/throughput.ts
- example/convex/test/run.ts
- example/convex/test/scenarios/overhead.ts
- example/convex/test/dashboard.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR replaces the placeholder example app with a full-featured benchmark dashboard for comparing workpool versions, and adds static hosting support so the dashboard can be deployed and served via Convex.
Dashboard UI (
example/src/App.tsx/App.css)A new React dashboard replaces the previous stub, with four tabs:
newpool,oldpool, or both sequentially, with editable JSON parameters and preset defaults per scenario.Multi-pool support
A new
test/pool.tsmodule centralizes pool selection logic (new=testWorkpool,old=oldWorkpool), exposingmakePool,enqueueFor,getComponent, andconfigurePoolhelpers. All scenarios (burstyBatches,throughput,overhead,sustained,bigArgs,bigContext,bigReturnTypes) now accept an optionalpoolparameter and route enqueue calls through the appropriate component. Therunsschema gains apoolfield to record which pool was used.The
run.startmutation allows concurrent runs when they target different pools, bypassing the "previous run still active" and "started too recently" guards when the pools differ.Dashboard queries (
test/dashboard.ts)New queries power the UI:
listRuns— lightweight run list without per-run task aggregation.getRun— full run detail including computed latency percentiles and total duration.throughputOverTime— time-bucketed completed/enqueued/in-flight counts.latencyCdf— thinned sorted latency array for CDF plotting.latestRunStatus— live status of the most recent run.New actions
runScenarios(sequential multi-run launcher with guard buffer) andrunConcurrent(fires both pools simultaneously) are exposed for the dashboard to call.Static hosting
The example app is wired up to
@convex-dev/static-hostingvia a newselfHostingcomponent,http.tsrouter, andstaticHosting.tsupload/deployment API. New npm scriptsbuild:dashboard,deploy:dashboard, anddeploy:dashboard:prodbuild and upload the Vite output.rechartsis added as a dependency for the chart components.