feat: benchmark page added for proof generation #13
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new benchmarking feature for proof generation workflows. It adds a /benchmark route with navigation integration, a useBenchmark React hook managing benchmark state and orchestration, and a BenchmarkPage component providing file upload, configuration, and detailed results visualization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BenchmarkPage
participant useBenchmark
participant SDK as zk-email SDK
participant Circuit as Noir Circuit
User->>BenchmarkPage: Select EML file & enter command
BenchmarkPage->>useBenchmark: runBenchmark(file, command, numRuns)
useBenchmark->>useBenchmark: Validate inputs
useBenchmark->>useBenchmark: Parse EML file
useBenchmark->>SDK: Dynamically import components
useBenchmark->>SDK: Initialize blueprint & prover
useBenchmark->>Circuit: Load local circuit
loop For each run (1 to numRuns)
useBenchmark->>SDK: Generate proof (measure time)
SDK-->>useBenchmark: Proof + proof generation time
useBenchmark->>SDK: Verify proof (measure time)
SDK-->>useBenchmark: Verification result + verification time
useBenchmark->>useBenchmark: Record run metrics
useBenchmark->>BenchmarkPage: Update progress (currentRun/totalRuns)
BenchmarkPage->>User: Display live progress
end
useBenchmark->>useBenchmark: Calculate summary statistics
useBenchmark->>BenchmarkPage: Provide summary (avg/min/max times)
BenchmarkPage->>User: Display detailed results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In @src/features/benchmark/useBenchmark.ts:
- Around line 4-23: The module-level polyfills in useBenchmark.ts (the
globalThis assignments and __proc handling) should be moved to a dedicated
function (e.g., applyBrowserPolyfills) in a new src/polyfills.ts that imports
Buffer as BufferPolyfill, guards with a (globalThis as any).__POLYFILLS_APPLIED
flag, performs the same assignments (numberIsNaN, Buffer, process,
__proc.env/NODE_ENV, __proc.version/versions.node) and sets the flag, then
remove the polyfill block from useBenchmark.ts and call applyBrowserPolyfills()
once from the application entry (e.g., main.tsx) before other imports so
polyfills are applied only once and not at arbitrary module import time.
- Around line 139-142: The comment claiming we "Try to determine if error
occurred during proof or verification" is inaccurate because proofTimeMs and
verificationTimeMs are both hard-coded to 0; either update the comment to state
that timing is not determined and these values default to 0, or actually measure
the phases by recording timestamps around the proof generation and verification
steps (e.g., capture start and end times in the proof generation function and
before/after the verify function, then set proofTimeMs and verificationTimeMs
accordingly) so the variables reflect real durations and the comment becomes
truthful; locate and update the code that sets proofTimeMs/verificationTimeMs in
useBenchmark.ts and the surrounding proof/verification calls to implement the
chosen fix.
- Around line 81-85: The SDK base URL is hardcoded in the initZkEmail call
inside useBenchmark (and also in useTwitterProof); change it to read from a
configuration/env var (e.g., process.env.VITE_ZK_EMAIL_BASE_URL or an app
config) with a fallback to "https://dev-conductor.zk.email" for local dev, and
update the initZkEmail invocation in useBenchmark (and mirror the same change
where initZkEmail is called in useTwitterProof) so the baseUrl is configurable
across environments.
🧹 Nitpick comments (6)
src/features/benchmark/useBenchmark.ts (3)
85-85: Consider making the blueprint ID configurable.The blueprint ID
"benceharomi/X_HANDLE@v2"is hardcoded. If this benchmarking tool might be used with different blueprints in the future, consider accepting it as a parameter or configuration option.📝 Example: parameterized blueprint
export function useBenchmark() { + const [blueprintId, setBlueprintId] = useState("benceharomi/X_HANDLE@v2"); // ... other state const runBenchmark = useCallback( - async (emlFile: File, command: string, numRuns: number) => { + async (emlFile: File, command: string, numRuns: number, customBlueprintId?: string) => { // ... - const blueprint = await sdk.getBlueprint("benceharomi/X_HANDLE@v2"); + const blueprint = await sdk.getBlueprint(customBlueprintId || blueprintId);
56-214: Consider adding cleanup for in-progress benchmarks.The benchmark loop has no cancellation mechanism. If a user navigates away or the component unmounts during a long-running benchmark, the async operations will continue executing in the background.
🔄 Recommended approach using AbortController
export function useBenchmark() { const [isRunning, setIsRunning] = useState(false); + const abortControllerRef = useRef<AbortController | null>(null); // ... other state const runBenchmark = useCallback( async (emlFile: File, command: string, numRuns: number) => { + // Cancel any existing benchmark + abortControllerRef.current?.abort(); + abortControllerRef.current = new AbortController(); + const signal = abortControllerRef.current.signal; + setIsRunning(true); setError(null); // ... try { // ... validation // Run benchmark multiple times for (let i = 0; i < numRuns; i++) { + if (signal.aborted) { + throw new Error("Benchmark cancelled"); + } setCurrentRun(i + 1); // ... benchmark logic } } catch (e) { + if (signal.aborted) { + console.log("Benchmark cancelled by user"); + return; + } const msg = e instanceof Error ? e.message : String(e); console.error("Benchmark error:", e); setError(msg); } finally { setIsRunning(false); + abortControllerRef.current = null; } }, [] ); const reset = useCallback(() => { + abortControllerRef.current?.abort(); setIsRunning(false); // ... other resets }, []);Also add cleanup in a useEffect:
useEffect(() => { return () => { abortControllerRef.current?.abort(); }; }, []);
163-187: Summary metrics default to 0 when all runs fail.When there are no successful results (
times.length === 0), all aggregate metrics (average, min, max) default to 0. This could be misleading in the UI, as 0ms would suggest instant execution rather than complete failure.Consider returning
null,undefined, orInfinityfor these metrics when there are no successful runs, allowing the UI to display "N/A" or similar:💡 Example implementation
const averageTimeMs = times.length > 0 ? times.reduce((sum, t) => sum + t, 0) / times.length - : 0; + : null; -const minTimeMs = times.length > 0 ? Math.min(...times) : 0; -const maxTimeMs = times.length > 0 ? Math.max(...times) : 0; +const minTimeMs = times.length > 0 ? Math.min(...times) : null; +const maxTimeMs = times.length > 0 ? Math.max(...times) : null;Update the type definitions:
export type BenchmarkSummary = { results: BenchmarkResult[]; - averageTimeMs: number; - minTimeMs: number; - maxTimeMs: number; + averageTimeMs: number | null; + minTimeMs: number | null; + maxTimeMs: number | null; // ... same for proof and verification timessrc/pages/BenchmarkPage.tsx (3)
23-28: Minor optimization: useEffect runs unnecessarily on mount.The effect runs on mount even when
fileisnull, though theif (file)guard prevents any action. This is harmless but slightly inefficient.🔧 Optional optimization
You could add
fileto the condition to make the intent clearer:useEffect(() => { if (file) { reset(); } -}, [file, reset]); +}, [file]);Since
resetis stable (wrapped inuseCallbackwith no dependencies), it's safe to omit from the dependency array, though linters might complain. Alternatively, keep it as-is—the current code is correct.
200-204: Input validation might cause unexpected UX.The manual validation in
onChangeprevents state updates for invalid values (≤0 or >100), but the HTML input attributesminandmax(lines 206-207) already provide browser-level validation. The combination might cause the input to briefly show an invalid value before React reconciliation reverts it.💡 Simplified approach
Consider removing the manual validation and relying on HTML5 validation:
<input id="num-runs" type="number" className="input" value={numRuns} onChange={(e) => { const value = parseInt(e.target.value, 10); - if (!isNaN(value) && value > 0 && value <= 100) { + if (!isNaN(value)) { setNumRuns(value); } }} min={1} max={100}The
min/maxattributes will prevent form submission with invalid values, and users can still type freely. Alternatively, add validation feedback in the UI if you want to prevent state updates for out-of-range values.
76-588: Extensive inline styles could be extracted to CSS.The component contains significant inline styling throughout. While functional, extracting these to CSS classes or a styled-components approach would improve maintainability and reusability.
This is a large refactor and can be deferred. For now, the inline styles work correctly. Consider creating a separate stylesheet or using CSS modules when you have time to refactor the styling approach across the application.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/App.tsxsrc/components/NavBar.tsxsrc/features/benchmark/useBenchmark.tssrc/pages/BenchmarkPage.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/BenchmarkPage.tsx (3)
src/features/benchmark/useBenchmark.ts (1)
useBenchmark(49-233)src/components/NavBar.tsx (1)
NavBar(4-33)src/components/ThemeToggle.tsx (1)
ThemeToggle(3-29)
src/App.tsx (1)
src/pages/BenchmarkPage.tsx (1)
BenchmarkPage(7-589)
🔇 Additional comments (3)
src/App.tsx (1)
5-12: LGTM! Route integration follows existing patterns.The new
/benchmarkroute is properly integrated and follows the same structure as the existing routes.src/components/NavBar.tsx (1)
24-29: LGTM! Navigation link properly integrated.The Benchmark link is correctly placed and uses appropriate styling with the
nav-ctaclass.src/pages/BenchmarkPage.tsx (1)
7-588: Well-structured benchmark UI with comprehensive results display.The component provides a complete user experience for benchmarking:
- Intuitive file upload with drag-and-drop support
- Clear input validation and disabled states during execution
- Detailed results breakdown including per-run metrics
- Good separation of concerns using the
useBenchmarkhook
| // Browser polyfills for libs expecting Node-like globals | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (globalThis as any).numberIsNaN ??= Number.isNaN; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (globalThis as any).Buffer ??= BufferPolyfill; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (globalThis as any).process ??= { env: {} }; | ||
| // Ensure NODE_ENV is a string. Some deps call process.env.NODE_ENV.slice() | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const __proc: any = (globalThis as any).process; | ||
| __proc.env ??= {}; | ||
| if (typeof __proc.env.NODE_ENV !== "string") { | ||
| __proc.env.NODE_ENV = "development"; | ||
| } | ||
| // Some libs call process.version.slice or read versions.node | ||
| __proc.version ??= "v18.0.0"; | ||
| __proc.versions ??= {}; | ||
| if (typeof __proc.versions.node !== "string") { | ||
| __proc.versions.node = "18.0.0"; | ||
| } |
There was a problem hiding this comment.
Polyfills could affect other modules unintentionally.
These polyfills are applied at module scope, meaning they execute as soon as this module is imported anywhere in the application. This could have unintended side effects on other parts of the codebase that might not expect these globals to be modified.
Consider one of these approaches:
- Move polyfills to a dedicated initialization module imported once at the app entry point
- Apply polyfills conditionally only when needed
- Scope the SDK usage to an isolated environment (e.g., Web Worker) if feasible
♻️ Recommended approach: dedicated polyfill module
Create a new file src/polyfills.ts:
import { Buffer as BufferPolyfill } from "buffer";
export function applyBrowserPolyfills() {
// Only apply once
if ((globalThis as any).__POLYFILLS_APPLIED) return;
(globalThis as any).numberIsNaN ??= Number.isNaN;
(globalThis as any).Buffer ??= BufferPolyfill;
(globalThis as any).process ??= { env: {} };
const __proc: any = (globalThis as any).process;
__proc.env ??= {};
if (typeof __proc.env.NODE_ENV !== "string") {
__proc.env.NODE_ENV = "development";
}
__proc.version ??= "v18.0.0";
__proc.versions ??= {};
if (typeof __proc.versions.node !== "string") {
__proc.versions.node = "18.0.0";
}
(globalThis as any).__POLYFILLS_APPLIED = true;
}Then call applyBrowserPolyfills() once in your app's entry point (e.g., main.tsx) before rendering, and remove the polyfill code from this file.
🤖 Prompt for AI Agents
In @src/features/benchmark/useBenchmark.ts around lines 4 - 23, The module-level
polyfills in useBenchmark.ts (the globalThis assignments and __proc handling)
should be moved to a dedicated function (e.g., applyBrowserPolyfills) in a new
src/polyfills.ts that imports Buffer as BufferPolyfill, guards with a
(globalThis as any).__POLYFILLS_APPLIED flag, performs the same assignments
(numberIsNaN, Buffer, process, __proc.env/NODE_ENV,
__proc.version/versions.node) and sets the flag, then remove the polyfill block
from useBenchmark.ts and call applyBrowserPolyfills() once from the application
entry (e.g., main.tsx) before other imports so polyfills are applied only once
and not at arbitrary module import time.
| const sdk = initZkEmail({ | ||
| baseUrl: "https://dev-conductor.zk.email", | ||
| logging: { enabled: false, level: "info" }, | ||
| }); | ||
| const blueprint = await sdk.getBlueprint("benceharomi/X_HANDLE@v2"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for dev-conductor references and surrounding context
rg -n "dev-conductor" --type ts --type tsx -C 3Repository: zkemail/ens-dashboard
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check for environment variable usage patterns related to base URLs or SDK configuration
rg -n "baseUrl|BASE_URL|process.env" src/features/benchmark/ --type ts --type tsx -C 2Repository: zkemail/ens-dashboard
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Look for other SDK initialization patterns to see if URLs are typically hardcoded or configurable
rg -n "initZkEmail|baseUrl" --type ts --type tsx -C 2 | head -50Repository: zkemail/ens-dashboard
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for dev-conductor references
rg -n "dev-conductor" -C 3Repository: zkemail/ens-dashboard
Length of output: 1334
🏁 Script executed:
#!/bin/bash
# Check for environment variable usage in the benchmark directory
rg -n "baseUrl|BASE_URL|process.env" src/features/benchmark/ -C 2Repository: zkemail/ens-dashboard
Length of output: 1511
🏁 Script executed:
#!/bin/bash
# Look for all references to initZkEmail to see configuration patterns
rg -n "initZkEmail" -C 3Repository: zkemail/ens-dashboard
Length of output: 1765
Use environment variables or configuration for the SDK base URL instead of hardcoding the development endpoint.
The dev-conductor.zk.email endpoint is hardcoded in multiple files (src/features/benchmark/useBenchmark.ts and src/features/twitter/useTwitterProof.ts). This development URL should be configurable to support different environments (development, staging, production). Consider extracting it to an environment variable (e.g., VITE_ZK_EMAIL_BASE_URL) and using a fallback to the development endpoint for local development.
🤖 Prompt for AI Agents
In @src/features/benchmark/useBenchmark.ts around lines 81 - 85, The SDK base
URL is hardcoded in the initZkEmail call inside useBenchmark (and also in
useTwitterProof); change it to read from a configuration/env var (e.g.,
process.env.VITE_ZK_EMAIL_BASE_URL or an app config) with a fallback to
"https://dev-conductor.zk.email" for local dev, and update the initZkEmail
invocation in useBenchmark (and mirror the same change where initZkEmail is
called in useTwitterProof) so the baseUrl is configurable across environments.
| // Try to determine if error occurred during proof or verification | ||
| // If we don't have proof time, assume it failed during proof generation | ||
| const proofTimeMs = 0; | ||
| const verificationTimeMs = 0; |
There was a problem hiding this comment.
Misleading comment: error timing is not determined.
The comment states "Try to determine if error occurred during proof or verification", but the code doesn't actually attempt this determination—it simply sets both proofTimeMs and verificationTimeMs to 0. This could confuse future maintainers.
🔧 Suggested fix
Either update the comment to reflect the actual behavior:
-// Try to determine if error occurred during proof or verification
-// If we don't have proof time, assume it failed during proof generation
+// Error occurred before timing could be captured; both times unavailable
const proofTimeMs = 0;
const verificationTimeMs = 0;Or, if you want to actually track which phase failed, capture the proof time before verification:
try {
// Generate proof (measure separately)
const proofStartTime = performance.now();
const proof = await prover.generateProof(text, externalInputs, {
noirWasm,
});
const proofEndTime = performance.now();
const proofTimeMs = proofEndTime - proofStartTime;
// Verify proof (measure separately)
const verificationStartTime = performance.now();
await blueprint.verifyProof(proof, { noirWasm });
const verificationEndTime = performance.now();
const verificationTimeMs =
verificationEndTime - verificationStartTime;
// ... success case
} catch (e) {
const endTime = performance.now();
const timeMs = endTime - startTime;
const msg = e instanceof Error ? e.message : String(e);
+ // If proof completed, error was during verification
+ let proofTimeMs = 0;
+ let verificationTimeMs = 0;
+ // You'd need to track whether proof succeeded to set these properly📝 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.
| // Try to determine if error occurred during proof or verification | |
| // If we don't have proof time, assume it failed during proof generation | |
| const proofTimeMs = 0; | |
| const verificationTimeMs = 0; | |
| // Error occurred before timing could be captured; both times unavailable | |
| const proofTimeMs = 0; | |
| const verificationTimeMs = 0; |
🤖 Prompt for AI Agents
In @src/features/benchmark/useBenchmark.ts around lines 139 - 142, The comment
claiming we "Try to determine if error occurred during proof or verification" is
inaccurate because proofTimeMs and verificationTimeMs are both hard-coded to 0;
either update the comment to state that timing is not determined and these
values default to 0, or actually measure the phases by recording timestamps
around the proof generation and verification steps (e.g., capture start and end
times in the proof generation function and before/after the verify function,
then set proofTimeMs and verificationTimeMs accordingly) so the variables
reflect real durations and the comment becomes truthful; locate and update the
code that sets proofTimeMs/verificationTimeMs in useBenchmark.ts and the
surrounding proof/verification calls to implement the chosen fix.
zkfriendly
left a comment
zkfriendly
left a comment
There was a problem hiding this comment.
Amazing. tried it and all worked as expected.
Maybe one note is to add a note explaining what type of email Is needed for the benchmark. I used it with x reset password email. But would be nice say if any email works or a specific one.
Summary by CodeRabbit
New Features
/benchmark✏️ Tip: You can customize this high-level summary in your review settings.