Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughThis PR introduces a new GitHub Actions workflow for performance budget validation using Playwright tests, updates frontend dependencies and configuration, migrates from Google Inter to a local font, extends Tailwind design tokens, and adds browser tracing instrumentation to the application. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export async function register(): Promise<void> { | ||
| await registerNodeInstrumentation(); | ||
| await registerBrowserInstrumentation(); |
There was a problem hiding this comment.
The new OpenTelemetry spans (e.g. price.quote and compositor.compose) rely on registerBrowserInstrumentation() to set up the web tracer and expose window.__paformFlushSpans. However, instrumentation.ts is only executed by Next.js on the server/edge runtimes. When register() runs, typeof window === 'undefined', so registerBrowserInstrumentation() immediately returns and no browser provider is ever installed. As a result, the spans emitted from client code are no-ops and the performance budgets that read from __paformFlushSpans will always see empty arrays. You’ll need to trigger the browser initialization from client code (e.g. in a root layout effect) instead of relying on the server instrumentation hook.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
frontend/tailwind.config.ts (1)
28-40: Naming inconsistency:pzds-vspds-prefix.The spacing tokens use
pzds-prefix while the font family usespds-prefix. Verify this is intentional or standardize to one prefix across all design tokens.Additionally, consider using Tailwind's default rem-based spacing instead of px units for better scalability and accessibility. For example:
spacing: { - 'pzds-0p5': '4px', - 'pzds-1': '8px', - 'pzds-1p5': '12px', - 'pzds-2': '16px', - 'pzds-3': '24px', - 'pzds-4': '32px', - 'pzds-5': '40px', - 'pzds-6': '48px', + 'pzds-0p5': '0.25rem', // 4px at default 16px base + 'pzds-1': '0.5rem', // 8px + 'pzds-1p5': '0.75rem', // 12px + 'pzds-2': '1rem', // 16px + 'pzds-3': '1.5rem', // 24px + 'pzds-4': '2rem', // 32px + 'pzds-5': '2.5rem', // 40px + 'pzds-6': '3rem', // 48px },frontend/tests/e2e/perf/configurator.perf.spec.ts (2)
204-212: Close contexts even when a run fails.If
measureRunthrows before the closing line executes, the context sticks around and continues consuming resources for the rest of the suite (and that noise can creep into later perf runs). Wrapping the setup intry/finallyguarantees we always tidy up.- const context = await browser.newContext({ ignoreHTTPSErrors: true }); - await registerDracoRoutes(context); - await registerPerformanceObservers(context); - results.push(await measureRun(context, url)); - await context.close(); + const context = await browser.newContext({ ignoreHTTPSErrors: true }); + try { + await registerDracoRoutes(context); + await registerPerformanceObservers(context); + results.push(await measureRun(context, url)); + } finally { + await context.close(); + }
223-235: Mirror the protective cleanup in warm runs.Same story here: if any warm iteration blows up before the explicit close, we retain a hot context and cache state that can leak into subsequent tests. A
try/finallykeeps the lifecycle well-behaved without changing the happy path.- const context = await browser.newContext({ ignoreHTTPSErrors: true }); - await registerDracoRoutes(context); - await registerPerformanceObservers(context); - await warmUpContext(context, url); - - const results: RunMetrics[] = []; - for (let index = 0; index < RUN_COUNT; index += 1) { - results.push(await measureRun(context, url)); - } - - await context.close(); - return results; + const context = await browser.newContext({ ignoreHTTPSErrors: true }); + try { + await registerDracoRoutes(context); + await registerPerformanceObservers(context); + await warmUpContext(context, url); + + const results: RunMetrics[] = []; + for (let index = 0; index < RUN_COUNT; index += 1) { + results.push(await measureRun(context, url)); + } + + return results; + } finally { + await context.close(); + }frontend/src/lib/telemetry.ts (1)
26-28: Minor: Redundant check and comparison could be simplified.Line 27 checks
key !== undefined, butObject.entriesnever produces undefined keys—this check is redundant. Line 28's comparison could uselocaleComparefor cleaner code.Apply this diff:
- const entries = Object.entries(value as Record<string, unknown>) - .filter(([key, val]) => typeof val !== 'undefined' && key !== undefined) - .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) + const entries = Object.entries(value as Record<string, unknown>) + .filter(([, val]) => typeof val !== 'undefined') + .sort(([a], [b]) => a.localeCompare(b))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/public/fonts/tesla/Universal-Sans-Display-Medium.woff2is excluded by!**/*.woff2frontend/public/fonts/tesla/Universal-Sans-Display-Regular.woff2is excluded by!**/*.woff2frontend/public/fonts/tesla/Universal-Sans-Text-Medium.woff2is excluded by!**/*.woff2frontend/public/fonts/tesla/Universal-Sans-Text-Regular.woff2is excluded by!**/*.woff2
📒 Files selected for processing (15)
.github/workflows/perf-budgets.yml(1 hunks)frontend/next.config.mjs(1 hunks)frontend/package.json(1 hunks)frontend/playwright.config.ts(1 hunks)frontend/src/api/client.ts(2 hunks)frontend/src/app/components/Viewer3D.tsx(4 hunks)frontend/src/app/layout.tsx(2 hunks)frontend/src/app/partcad/[model]/page.tsx(1 hunks)frontend/src/hooks/usePricePreview.ts(4 hunks)frontend/src/instrumentation.ts(1 hunks)frontend/src/lib/telemetry.ts(1 hunks)frontend/tailwind.config.ts(1 hunks)frontend/tests/e2e/perf/configurator.perf.spec.ts(1 hunks)frontend/tests/e2e/perf/perf-reporter.ts(1 hunks)tools/scripts/validate-budgets.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/src/instrumentation.ts (1)
frontend/src/api/client.ts (1)
request(97-120)
frontend/tests/e2e/perf/configurator.perf.spec.ts (3)
frontend/tools/perf/run-perf-budget.js (9)
page(148-148)context(147-147)browser(390-390)index(282-282)values(406-408)sorted(275-275)lower(283-283)upper(284-284)weight(288-288)scripts/publish-perf-metrics.mjs (1)
targetUrl(86-86)tools/scripts/validate-budgets.js (2)
metrics(49-49)metrics(58-58)
frontend/src/hooks/usePricePreview.ts (3)
frontend/src/lib/api/price.ts (3)
PricePreviewRequest(28-33)PricePreviewResponse(75-85)previewPrice(103-108)frontend/src/lib/telemetry.ts (1)
computeOptionsHash(45-45)frontend/src/api/client.ts (1)
ApiError(5-19)
frontend/src/app/components/Viewer3D.tsx (1)
frontend/src/lib/telemetry.ts (1)
computeOptionsHash(45-45)
frontend/tests/e2e/perf/perf-reporter.ts (2)
tools/scripts/validate-budgets.js (2)
path(3-3)fs(2-2)frontend/tools/perf/run-perf-budget.js (1)
parsed(30-30)
🪛 GitHub Check: CodeQL
.github/workflows/perf-budgets.yml
[warning] 12-79: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🔇 Additional comments (8)
frontend/src/app/partcad/[model]/page.tsx (2)
1-2: AI summary inconsistent with actual changes.The AI-generated summary mentions OpenTelemetry instrumentation and performance budgeting infrastructure, but the actual change is a simple import path adjustment following directory restructure. The PR objectives correctly describe relocating the partcad route under src/app.
2-2: Import path correctly updated for directory relocation.The additional
../level aligns with moving the partcad route deeper in the directory structure as described in the PR objectives.frontend/tailwind.config.ts (3)
1-45: AI summary inconsistency with actual changes.The AI-generated summary mentions "OpenTelemetry instrumentation and performance budgeting infrastructure" but the actual changes integrate Parviz design tokens into Tailwind configuration, which aligns with the PR objectives. This mismatch suggests the summary may have been generated from a different PR or context.
14-27: Color tokens look good.The Parviz design system color tokens are properly defined with valid hex values and follow a consistent naming convention.
38-40: Universal Sans font loading is properly configured via next/font.The verification confirms the font is correctly implemented:
next/font/localimports the font infrontend/src/app/layout.tsx, loads Universal Sans display and text variants (400/500 weights) frompublic/fonts/tesla/, and applies it globally to the root HTML element. The Tailwind config correctly references the font with appropriate fallbacks. No action required.frontend/src/instrumentation.ts (1)
204-209: Cache hit detection may not work reliably across all CDNs.The cache detection logic checks for
x-cacheorageheaders, but header naming and values vary by CDN provider. This may produce false negatives or positives depending on the infrastructure.Consider documenting which CDNs are supported or testing against your actual CDN configuration to ensure this works as expected.
.github/workflows/perf-budgets.yml (2)
1-80: Critical: PR description does not match the actual changes.The PR title and description mention "integrate parviz design tokens" and "Universal Sans font assets," but the files under review implement OpenTelemetry instrumentation and performance budgeting infrastructure. There is zero overlap between the stated objectives and the actual code changes.
Please verify:
- Are these the correct files for this PR?
- Is the PR description accurate?
- Should these changes be in a separate PR?
63-79: Well-designed error handling pattern.The workflow intentionally allows validation to fail (line 66), uploads artifacts for debugging (line 70), and then enforces the failure (line 78). This ensures test results are always available while still failing the build on budget violations.
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/instrumentation.ts (1)
253-256: Critical: Browser instrumentation is never initialized.This issue was flagged in previous reviews and remains unresolved. The
register()function is invoked by Next.js's instrumentation hook, which runs exclusively on the server or edge runtime—never in the browser. WhenregisterBrowserInstrumentation()executes,typeof window === 'undefined'evaluates totrue(line 140), causing an immediate return. Consequently:
- No
WebTracerProvideris instantiated in the browserwindow.__paformFlushSpansis never defined (line 242)- Client-side spans are no-ops
- Performance budgets relying on
__paformFlushSpanswill failYou must trigger browser initialization from client code (e.g., a root layout effect or client component) instead of relying on the server instrumentation hook.
🧹 Nitpick comments (1)
frontend/tests/e2e/perf/perf-reporter.ts (1)
42-50: Add error handling for file operations.File system operations can fail due to permissions, disk space, or other I/O issues. Without error handling, these failures will cause the reporter to crash silently, losing all collected metrics.
onEnd(_result: FullResult): void { if (this.metrics.size === 0) { return; } - fs.mkdirSync(path.dirname(this.outputFile), { recursive: true }); - const serializable = Object.fromEntries(this.metrics.entries()); - fs.writeFileSync(this.outputFile, JSON.stringify(serializable, null, 2), 'utf8'); + try { + fs.mkdirSync(path.dirname(this.outputFile), { recursive: true }); + const serializable = Object.fromEntries(this.metrics.entries()); + fs.writeFileSync(this.outputFile, JSON.stringify(serializable, null, 2), 'utf8'); + } catch (error) { + console.error('Failed to write performance metrics:', error); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/perf-budgets.yml(1 hunks)frontend/src/instrumentation.ts(1 hunks)frontend/tests/e2e/perf/perf-reporter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/perf-budgets.yml
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/instrumentation.ts (1)
frontend/src/api/client.ts (1)
request(97-120)
🪛 ast-grep (0.39.6)
frontend/src/instrumentation.ts
[warning] 177-177: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(originPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (12)
frontend/src/instrumentation.ts (11)
1-256: PR objectives don't match file content.The PR title and description state this PR integrates Parviz design tokens, relocates routes, and adds Universal Sans font assets. However, this file implements OpenTelemetry performance instrumentation with no connection to design tokens or fonts.
16-24: LGTM: Global type declarations are appropriate.The use of
varfor global scope augmentation is correct, and the Window interface extensions properly type the browser-side coordination state.
26-36: LGTM: Resource configuration is sound.The service metadata, version fallback, and environment detection chain are all appropriate.
38-47: LGTM: Span serialization is correct.The conversion from
ReadableSpantoSerializableSpanproperly handles timestamps and preserves all necessary trace data.
49-68: LGTM: WindowSpanExporter correctly implements buffering.The server-safety check and buffer append logic are appropriate for accumulating spans until they're flushed via
window.__paformFlushSpans.
70-82: LGTM: Sampling strategy is well-designed.The environment-aware configuration (always-on for dev, ratio-based for production) with parent propagation and proper validation is appropriate.
84-93: LGTM: Exporter URL resolution follows conventions.The scope-based environment variable precedence correctly respects Next.js's public variable constraints and OpenTelemetry's standard variable names.
95-137: LGTM: Node instrumentation setup is robust.The runtime guards, initialization deduplication, optional exporter, and graceful shutdown hooks are all well-implemented. The
NEXT_RUNTIME !== 'nodejs'check correctly excludes edge runtime, where Node-specific APIs may be unavailable.
174-180: LGTM: Security issue from previous review has been resolved.The allowlist approach correctly restricts trace header propagation to same-origin and trusted API namespaces, addressing the previous overly-permissive
/.*/pattern. TheescapeRegexfunction properly prevents ReDoS attacks.
188-237: LGTM: Asset type determination issue from previous review has been resolved.The logic now correctly determines asset type from file extension ('sprite' for images, 'model' for 3D assets, 'asset' as fallback) rather than hardcoding 'sprite'. The size and cache-hit extraction logic is sound, and error handling is appropriate.
242-247: LGTM: Flush implementation is correct.The copy-and-clear pattern safely returns buffered spans while resetting the buffer for new spans.
frontend/tests/e2e/perf/perf-reporter.ts (1)
15-20: Path resolution looks solid.The constructor correctly handles both absolute and relative paths, with a sensible default location.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
frontend/src/app/components/app-ready.tsx(1 hunks)frontend/src/instrumentation.ts(1 hunks)frontend/tests/e2e/perf/perf-reporter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/e2e/perf/perf-reporter.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/instrumentation.ts (1)
frontend/src/api/client.ts (1)
request(97-120)
frontend/src/app/components/app-ready.tsx (1)
scripts/__tests__/publish-perf-metrics.test.mjs (1)
module(71-71)
🪛 ast-grep (0.39.6)
frontend/src/instrumentation.ts
[warning] 177-177: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(originPattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
| const module = await import('@/instrumentation') | ||
| if (!cancelled && typeof module.initBrowserTracing === 'function') { | ||
| await module.initBrowserTracing() | ||
| } |
There was a problem hiding this comment.
Blocker: client bundle now pulls in Node-only OTEL SDK
Because AppReadyEffect is a Client Component, await import('@/instrumentation') is compiled for the browser. That file in turn (see frontend/src/instrumentation.ts, lines 106-110) lazily imports @opentelemetry/sdk-trace-node, which requires Node built-ins like fs, module, etc. When webpack builds the client bundle it will try to include that dependency and fail (Module not found: Can't resolve 'fs'), or, if forcibly shimmed, ship a large chunk of server-only code to the browser. Please split the browser initializer into a module that contains only web-safe imports (e.g., instrumentation/browser.ts) and have the server hooking keep the Node bits so the client never references sdk-trace-node. This needs to be fixed before merge.
1 participant
Summary
Testing