[FEAT] Frontend Pivot: 2D Viewer and PZ UX#621
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
WalkthroughRemoved 3D viewer, related pages and tests; added a 2D layered sprite viewer and browser OpenTelemetry instrumentation; moved styling from KDS/CSS to Tailwind and updated global colors; adjusted pages, imports, and package.json scripts/dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant Page as ClientPage
participant V2 as Viewer2D
participant L as LayerImage
participant OT as OTEL
U->>Page: navigate to configurator
activate Page
Page->>OT: beginViewerCompose(context)
activate OT
Page->>V2: render (Suspense -> Spinner fallback)
activate V2
V2->>V2: load manifest & sort layers
par load layers
V2->>L: request base layer image
L-->>V2: onLoadingComplete
V2->>L: request cabinets layer
L-->>V2: onLoadingComplete
V2->>L: request countertop layer
L-->>V2: onLoadingComplete
V2->>L: request hardware layer
L-->>V2: onLoadingComplete
end
V2->>OT: composeHandle.end(success, durationMs)
deactivate V2
OT->>OT: record duration, set status, end span
deactivate OT
deactivate Page
U->>U: view composed sprite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Comment |
![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".
| requestId, | ||
| optionsHash: computeOptionsHash({ elevation, finish, hardware, countertop, rev: manifest.rev }), | ||
| layerCount: layers.length, | ||
| }) | ||
| setLoadedMap({}) |
There was a problem hiding this comment.
The effect that runs on every configurator change resets loadedMap to an empty object but the Image elements reuse the same layer.key when a layer’s asset doesn’t change (e.g. the base structure when only hardware is toggled). Because those images are not remounted, their onLoadingComplete handler never fires again, so isLoaded stays false and they render with opacity-0 indefinitely after the first configuration change. The result is that any layer whose key remains constant disappears from the viewer after adjusting other options. Consider preserving load state for persistent keys or force those layers to remount when you wipe the map.
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 (2)
frontend/src/components/PricingFooter.tsx (1)
36-36: Hardcoded "Due Today" amount should be configurable.The "$250" deposit amount is hardcoded. Consider making this configurable via the store or props for flexibility.
frontend/src/lib/sprite-manifest.ts (1)
34-39: Consider removing unused 'lighting' from SpriteLayer union.The lighting layer has been removed from the manifest (line 7 in the type definition still includes
'lighting'in theSpriteLayerunion), but it's no longer used in themanifestForfunction.If the lighting layer is permanently removed, apply this diff to clean up the type:
export type SpriteLayer = | 'base' | 'cabinets' | 'hardware' | 'countertop' | 'backsplash' - | 'lighting'
📜 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 (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
frontend/package.json(0 hunks)frontend/public/sprites/kitchen/hero/layers.json(1 hunks)frontend/src/app/components/Viewer3D.tsx(0 hunks)frontend/src/app/components/__tests__/viewer3d.test.tsx(0 hunks)frontend/src/app/configurator/ClientPage.tsx(1 hunks)frontend/src/app/configurator/page.tsx(1 hunks)frontend/src/app/globals.css(1 hunks)frontend/src/app/layout.tsx(1 hunks)frontend/src/app/providers.tsx(1 hunks)frontend/src/components/ConfiguratorPanel.tsx(3 hunks)frontend/src/components/GalleryAffordances.css(0 hunks)frontend/src/components/KitchenLayoutShell.tsx(0 hunks)frontend/src/components/OrderSummaryCard.tsx(1 hunks)frontend/src/components/PricingFooter.tsx(1 hunks)frontend/src/components/Viewer2D.tsx(2 hunks)frontend/src/lib/sprite-manifest.ts(1 hunks)frontend/src/lib/sprite-preloader.ts(0 hunks)frontend/src/otel/viewer2d-instrumentation.ts(1 hunks)frontend/src/styles/kds.css(0 hunks)frontend/tests/e2e/layout.spec.ts(1 hunks)
💤 Files with no reviewable changes (7)
- frontend/src/components/GalleryAffordances.css
- frontend/src/components/KitchenLayoutShell.tsx
- frontend/src/lib/sprite-preloader.ts
- frontend/src/styles/kds.css
- frontend/package.json
- frontend/src/app/components/Viewer3D.tsx
- frontend/src/app/components/tests/viewer3d.test.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/components/Viewer2D.tsx (3)
frontend/src/lib/sprite-manifest.ts (1)
manifestFor(24-41)frontend/src/otel/viewer2d-instrumentation.ts (1)
beginViewerCompose(20-49)frontend/src/lib/telemetry.ts (1)
computeOptionsHash(45-45)
frontend/src/app/configurator/ClientPage.tsx (3)
frontend/src/lib/otel-route.ts (1)
initOtelRoute(20-38)frontend/src/components/Viewer2D.tsx (1)
Viewer2D(21-118)frontend/src/components/PricingFooter.tsx (1)
PricingFooter(10-51)
frontend/src/components/ConfiguratorPanel.tsx (1)
frontend/src/components/OrderSummaryCard.tsx (1)
OrderSummaryCard(10-38)
frontend/src/app/configurator/page.tsx (1)
frontend/src/app/configurator/ClientPage.tsx (1)
ClientPage(19-47)
🔇 Additional comments (10)
frontend/src/app/layout.tsx (1)
51-51: LGTM!The background color
#f4f4f4correctly matches the--background-rgb: 244, 244, 244value set inglobals.css, ensuring consistency across the light theme update.frontend/src/app/providers.tsx (2)
11-12: LGTM!The brand color update from pink/purple to blue/black aligns with the broader design system pivot documented in the PR objectives.
18-19: LGTM!The explicit hex values match the CSS custom properties in
globals.css:#f4f4f4corresponds to--background-rgb: 244, 244, 244and#171a20to--foreground-rgb: 23, 26, 32.frontend/tests/e2e/layout.spec.ts (2)
6-16: LGTM!The test correctly validates the configurator panel width of 448px, which matches the
md:w-[448px]class inClientPage.tsx, and verifies the pricing footer's static positioning.
18-24: LGTM!The sprite rendering test provides basic coverage by verifying that at least one image is present in the viewer section, validating the 2D sprite compositor is functioning.
frontend/src/components/PricingFooter.tsx (1)
7-8: LGTM!The financing constants are clearly defined and align with the displayed financing terms.
frontend/src/lib/sprite-manifest.ts (2)
28-29: LGTM!The sprite size and base URL updates align with the new hero-based asset structure defined in
layers.json.
32-32: LGTM!Including the
elevationparameter in the revision string provides better cache invalidation when elevation changes.frontend/src/app/globals.css (1)
6-7: Legacy color variables successfully removed with no remaining references.Verification confirms that
--vector-pinkand--vector-purpleare not referenced anywhere in the codebase. Comprehensive searches across all file types (CSS, JavaScript, TypeScript, JSX, TSX) returned no matches for either direct usage or string-based references.frontend/src/app/configurator/page.tsx (1)
1-1: No SSR issues found—the changes are safe and follow modern Next.js patterns.The verification reveals no SSR incompatibilities:
- All components (ClientPage, Viewer2D, PricingFooter, ConfiguratorPanel) are properly marked with
'use client'initOtelRoute()has a proper guard:if (typeof window === 'undefined') return- No unguarded browser API usage detected
- The store uses
'use client'and zustand with no direct browser APIsThe current approach using static imports with
'use client'directives is the modern, idiomatic Next.js 13+ pattern and is architecturally superior to the previousnext/dynamicwithssr: false. The'use client'directive explicitly marks components as client-only, eliminating SSR concerns without relying on code-splitting tricks.Likely an incorrect or invalid review comment.
| { | ||
| "rev": "hero-r1", | ||
| "size": { "w": 1600, "h": 900 }, | ||
| "layers": [ | ||
| { "key": "base:structure", "src": "/sprites/kitchen/hero/base.webp", "z": 0 }, | ||
| { "key": "cabinets:{finish}", "src": "/sprites/kitchen/hero/cabinets_{finish}.webp", "z": 10 }, | ||
| { "key": "countertop:{countertop}", "src": "/sprites/kitchen/hero/countertop_{countertop}.webp", "z": 20 }, | ||
| { "key": "hardware:{hardware}", "src": "/sprites/kitchen/hero/hardware_{hardware}.webp", "z": 30 } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify usage of layers.json manifest.
This static JSON manifest defines the sprite layer structure, but it's unclear if and how it's consumed by the application. The sprite-manifest.ts file programmatically generates manifests with the same structure, suggesting this file might be unused or serve as documentation/reference.
Run the following script to check for references to this layers.json file:
🏁 Script executed:
#!/bin/bash
# Description: Check for imports or fetch calls to layers.json
# Search for imports or references to layers.json
rg -n --type=ts --type=tsx --type=js 'layers\.json'
# Search for fetch/require of the file
rg -n --type=ts --type=tsx --type=js '/sprites/kitchen/hero/layers'Length of output: 55
I'll run improved shell scripts to search for references to this file more broadly:
🏁 Script executed:
#!/bin/bash
# Search for layers.json references without file type restrictions
echo "=== Searching for 'layers.json' ==="
rg -n 'layers\.json'
echo -e "\n=== Searching for sprite path pattern ==="
rg -n '/sprites/kitchen/hero/layers'
echo -e "\n=== Looking for sprite-manifest.ts ==="
fd -type f -name 'sprite-manifest.ts'Length of output: 289
🏁 Script executed:
#!/bin/bash
# Check for any fetch or import patterns that might load JSON manifests
echo "=== Searching for dynamic imports/fetch patterns ==="
rg -n 'fetch.*json|import.*json|require.*json' --type=js --type=ts
echo -e "\n=== Searching for sprite references in general ==="
rg -n 'sprites.*kitchen.*hero' --type=js --type=tsLength of output: 533
🏁 Script executed:
#!/bin/bash
# If sprite-manifest.ts exists, examine its content
echo "=== Checking sprite-manifest.ts location ==="
fd -type f -name '*manifest*' | head -20Length of output: 215
🏁 Script executed:
#!/bin/bash
# Examine sprite-manifest.ts to understand manifest generation
echo "=== Content of sprite-manifest.ts ==="
cat frontend/src/lib/sprite-manifest.ts
echo -e "\n=== Searching for any build scripts that might reference layers.json ==="
find . -type f \( -name '*.json' -o -name '*.js' -o -name '*.ts' \) -path '*/build/*' -o -path '*/scripts/*' | head -10
echo -e "\n=== Check if layers.json is in any build or config files ==="
rg -n 'layers.json' . 2>/dev/null || echo "No results"Length of output: 1548
🏁 Script executed:
#!/bin/bash
# Find all manifest files to understand the structure
echo "=== All files with 'manifest' in name ==="
find . -type f -name '*manifest*' 2>/dev/null | grep -v node_modules | head -20
echo -e "\n=== Check models/manifest.json ==="
cat frontend/public/models/manifest.json 2>/dev/null | head -30 || echo "File not found or not accessible"Length of output: 916
Remove unused layers.json—it's dead code.
Verification confirms that frontend/public/sprites/kitchen/hero/layers.json is not referenced anywhere in the codebase. The application generates sprite manifests dynamically via sprite-manifest.ts:manifestFor(), which produces the identical structure programmatically with parameterized values. This static file serves no purpose and can be safely deleted.
🤖 Prompt for AI Agents
frontend/public/sprites/kitchen/hero/layers.json (lines 1-10): this static
layers.json is dead code and duplicates the programmatic manifest produced by
sprite-manifest.ts:manifestFor(); delete the file from the repo (remove it from
git), ensure no imports or references remain (search for any remaining usages)
and, if your build/static asset manifest lists this file, remove it from that
list so it is not packaged or deployed.
| <button | ||
| key={option} | ||
| type="button" | ||
| className="kds-cta" | ||
| style={{ | ||
| background: isActive ? '#fff' : 'var(--kds-pill-bg)', | ||
| color: isActive ? '#000' : 'var(--kds-fg)', | ||
| }} | ||
| className={`rounded-full border px-pzds-2 py-[10px] text-sm font-medium transition-colors ${ | ||
| isActive | ||
| ? 'border-pzds-black bg-pzds-black text-pzds-white shadow-[0_18px_36px_rgba(0,0,0,0.18)]' | ||
| : 'border-pzds-grey-50 bg-transparent text-pzds-grey-20 hover:border-pzds-grey-30 hover:text-pzds-black' | ||
| }`} | ||
| onClick={() => onChange(option)} |
There was a problem hiding this comment.
Expose the active state via ARIA for the option pills.
These buttons act as toggle choices but never announce which one is selected. Screen readers need the state exposed (e.g., aria-pressed={isActive} or converting this block into a proper radiogroup) to let users understand and change the selection. Please add the ARIA state (and, if you go the radiogroup route, the corresponding group semantics) so the control meets WCAG 4.1.2.
Apply this diff, or an equivalent radiogroup refactor, to expose the state:
- <button
+ <button
key={option}
type="button"
className={`rounded-full border px-pzds-2 py-[10px] text-sm font-medium transition-colors ${
isActive
? 'border-pzds-black bg-pzds-black text-pzds-white shadow-[0_18px_36px_rgba(0,0,0,0.18)]'
: 'border-pzds-grey-50 bg-transparent text-pzds-grey-20 hover:border-pzds-grey-30 hover:text-pzds-black'
}`}
+ aria-pressed={isActive}
onClick={() => onChange(option)}
>📝 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.
| <button | |
| key={option} | |
| type="button" | |
| className="kds-cta" | |
| style={{ | |
| background: isActive ? '#fff' : 'var(--kds-pill-bg)', | |
| color: isActive ? '#000' : 'var(--kds-fg)', | |
| }} | |
| className={`rounded-full border px-pzds-2 py-[10px] text-sm font-medium transition-colors ${ | |
| isActive | |
| ? 'border-pzds-black bg-pzds-black text-pzds-white shadow-[0_18px_36px_rgba(0,0,0,0.18)]' | |
| : 'border-pzds-grey-50 bg-transparent text-pzds-grey-20 hover:border-pzds-grey-30 hover:text-pzds-black' | |
| }`} | |
| onClick={() => onChange(option)} | |
| <button | |
| key={option} | |
| type="button" | |
| className={`rounded-full border px-pzds-2 py-[10px] text-sm font-medium transition-colors ${ | |
| isActive | |
| ? 'border-pzds-black bg-pzds-black text-pzds-white shadow-[0_18px_36px_rgba(0,0,0,0.18)]' | |
| : 'border-pzds-grey-50 bg-transparent text-pzds-grey-20 hover:border-pzds-grey-30 hover:text-pzds-black' | |
| }`} | |
| aria-pressed={isActive} | |
| onClick={() => onChange(option)} | |
| > |
🤖 Prompt for AI Agents
frontend/src/components/ConfiguratorPanel.tsx around lines 33 to 41: the option
buttons do not expose selection state to assistive tech; add ARIA state by
setting aria-pressed={isActive} on each button (or, if you prefer a radiogroup
pattern, wrap the options in an element with role="radiogroup" and give each
option role="radio" with aria-checked={isActive} and a keyboard/space/arrow
handling consistent with radio semantics). Ensure the active visual classes
remain unchanged and update any onKey handlers as needed so the ARIA state
always reflects the current selection.
| const { totalLabel, monthlyLabel } = useMemo(() => { | ||
| if (priceCents == null) { | ||
| return { | ||
| totalLabel: '—', | ||
| monthlyLabel: '—', | ||
| } | ||
| } | ||
| const total = priceCents / 100 | ||
| const monthly = total / FINANCE_TERM_MONTHS | ||
| return { | ||
| totalLabel: `$${total.toLocaleString(undefined, { minimumFractionDigits: 0 })}`, | ||
| monthlyLabel: `$${monthly.toLocaleString(undefined, { minimumFractionDigits: 0 })}/mo`, | ||
| } | ||
| }, [priceCents]) |
There was a problem hiding this comment.
Fix incorrect monthly payment calculation.
The monthly payment calculation at line 21 incorrectly divides the total by the number of months without accounting for interest. This produces a significantly lower payment estimate than a proper amortized loan calculation would show.
For a loan with principal P, monthly rate r, and n payments, the correct formula is:
monthly = P × [r(1+r)^n] / [(1+r)^n - 1]
With 3.99% APR (r = 0.0399/12 ≈ 0.003325), the true monthly payment will be noticeably higher than total/72.
Apply this diff to fix the calculation:
const { totalLabel, monthlyLabel } = useMemo(() => {
if (priceCents == null) {
return {
totalLabel: '—',
monthlyLabel: '—',
}
}
const total = priceCents / 100
- const monthly = total / FINANCE_TERM_MONTHS
+ const monthlyRate = APR / 100 / 12
+ const n = FINANCE_TERM_MONTHS
+ const monthly = total * (monthlyRate * Math.pow(1 + monthlyRate, n)) / (Math.pow(1 + monthlyRate, n) - 1)
return {
totalLabel: `$${total.toLocaleString(undefined, { minimumFractionDigits: 0 })}`,
monthlyLabel: `$${monthly.toLocaleString(undefined, { minimumFractionDigits: 0 })}/mo`,
}
}, [priceCents])Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
frontend/src/components/PricingFooter.tsx around lines 13 to 26: the current
monthly payment uses total / FINANCE_TERM_MONTHS which ignores interest; replace
it with the amortized loan formula: set principal = priceCents / 100,
monthlyRate = FINANCE_APR / 12 (use the APR constant used elsewhere or add
FINANCE_APR = 0.0399), n = FINANCE_TERM_MONTHS, then compute monthly = principal
* (r * (1 + r) ** n) / ((1 + r) ** n - 1) (handle r === 0 fallback to principal
/ n), format monthlyLabel with toLocaleString and “/mo” as before, and keep the
null price handling unchanged.
| <Image | ||
| key={layer.key} | ||
| src={layer.url} | ||
| alt={layer.key} | ||
| fill | ||
| priority={index === 0} | ||
| className={`${layerBaseClass} ${isLoaded ? 'opacity-100' : 'opacity-0'}`} | ||
| style={{ zIndex: layer.z }} | ||
| onLoadingComplete={() => handleLayerComplete(layer.key)} | ||
| /> |
There was a problem hiding this comment.
End the compositor span when a layer fails to load.
If any sprite layer errors, onLoadingComplete never fires, so composeHandleRef.current stays open forever and the compositor.compose span never ends. That breaks telemetry and makes subsequent traces misleading. Please add an error handler that records the failure and closes the span.
One way to address it:
- {layers.map((layer, index) => {
+ {layers.map((layer, index) => {
const isLoaded = Boolean(loadedMap[layer.key])
return (
<Image
key={layer.key}
src={layer.url}
alt={layer.key}
fill
priority={index === 0}
className={`${layerBaseClass} ${isLoaded ? 'opacity-100' : 'opacity-0'}`}
style={{ zIndex: layer.z }}
onLoadingComplete={() => handleLayerComplete(layer.key)}
+ onError={() => {
+ composeHandleRef.current?.end({ error: new Error(`failed to load layer ${layer.key}`) })
+ composeHandleRef.current = null
+ }}
/>
)
})}If you prefer to keep the handler reusable, factor it into a dedicated handleLayerError helper next to handleLayerComplete.
📝 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.
| <Image | |
| key={layer.key} | |
| src={layer.url} | |
| alt={layer.key} | |
| fill | |
| priority={index === 0} | |
| className={`${layerBaseClass} ${isLoaded ? 'opacity-100' : 'opacity-0'}`} | |
| style={{ zIndex: layer.z }} | |
| onLoadingComplete={() => handleLayerComplete(layer.key)} | |
| /> | |
| <Image | |
| key={layer.key} | |
| src={layer.url} | |
| alt={layer.key} | |
| fill | |
| priority={index === 0} | |
| className={`${layerBaseClass} ${isLoaded ? 'opacity-100' : 'opacity-0'}`} | |
| style={{ zIndex: layer.z }} | |
| onLoadingComplete={() => handleLayerComplete(layer.key)} | |
| onError={() => { | |
| composeHandleRef.current?.end({ error: new Error(`failed to load layer ${layer.key}`) }) | |
| composeHandleRef.current = null | |
| }} | |
| /> |
🤖 Prompt for AI Agents
In frontend/src/components/Viewer2D.tsx around lines 102 to 111, the Image lacks
an error handler so if a layer fails to load onLoadingComplete never fires and
composeHandleRef.current remains open; add an onError prop that calls a new
helper (e.g. handleLayerError) next to handleLayerComplete which records the
failure (log/telemetry) and ensures composeHandleRef.current is ended/closed and
any per-layer state is updated so the compositor span is always finished even on
load errors.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @shayancoin. * #621 (comment) The following files were modified: * `frontend/src/app/configurator/ClientPage.tsx` * `frontend/src/app/configurator/page.tsx` * `frontend/src/app/layout.tsx` * `frontend/src/components/OrderSummaryCard.tsx` * `frontend/src/components/PricingFooter.tsx` * `frontend/src/components/Viewer2D.tsx` * `frontend/src/otel/viewer2d-instrumentation.ts`
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/instrumentation.ts (2)
127-132: Consider simplifying the request URL extraction.The nested ternary for extracting
requestUrlis verbose. Consider a more readable approach:- const requestUrl = - typeof request === 'string' - ? request - : request && typeof (request as { url?: string }).url === 'string' - ? ((request as { url: string }).url) - : ''; + const requestUrl = typeof request === 'string' + ? request + : (request as { url?: string })?.url ?? '';
183-192: Consider consolidating duplicate initialization logic.Both
register()andinitBrowserTracing()perform identical operations by callingregisterBrowserInstrumentation(). This duplication could lead to confusion about which function to use.If both functions are required for different entry points, consider adding documentation clarifying their intended usage contexts. Otherwise, one could delegate to the other or be marked as deprecated:
export async function register(): Promise<void> { await initBrowserTracing(); } /** * Initialize browser-side OpenTelemetry tracing. * Call this from browser contexts to set up instrumentation. */ export async function initBrowserTracing(): Promise<void> { if (typeof window === 'undefined') { return; } await registerBrowserInstrumentation(); }frontend/src/app/kitchens/vvd/view/[file]/page.tsx (1)
1-9: Consider whether 'use client' directive is necessary here.The file uses
dynamic()withssr: false(line 6) which already ensures client-side-only rendering. The'use client'directive on line 1 may be redundant in this case, as the dynamic import boundary handles the client/server split.If the intent is to make this entire page a client component, the
'use client'directive is appropriate. However, if the goal is only to lazy-load the viewer client-side, you could remove the directive and keep just the dynamic import:-'use client' - import dynamic from 'next/dynamic' const ViewerPage = dynamic(() => import('./viewer-client'), { ssr: false, }) export default ViewerPageThis would allow the page wrapper to remain a Server Component while only the viewer loads client-side.
📜 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 (16)
frontend/next-env.d.ts(1 hunks)frontend/next.config.mjs(0 hunks)frontend/package.json(1 hunks)frontend/playwright.config.ts(1 hunks)frontend/src/app/(shop)/product/[handle]/page.tsx(1 hunks)frontend/src/app/components/app-ready.tsx(1 hunks)frontend/src/app/dealer/actions.ts(5 hunks)frontend/src/app/kitchen-configurator/[sku]/page.tsx(1 hunks)frontend/src/app/kitchens/vvd/page.tsx(2 hunks)frontend/src/app/kitchens/vvd/view/[file]/page.tsx(1 hunks)frontend/src/app/kitchens/vvd/view/[file]/viewer-client.tsx(1 hunks)frontend/src/app/partcad/[model]/page.tsx(0 hunks)frontend/src/app/server-actions/dealerQuotes.ts(2 hunks)frontend/src/instrumentation.ts(5 hunks)frontend/src/workers/compositor.worker.ts(1 hunks)frontend/tests/e2e/perf/perf-reporter.ts(2 hunks)
💤 Files with no reviewable changes (2)
- frontend/next.config.mjs
- frontend/src/app/partcad/[model]/page.tsx
✅ Files skipped from review due to trivial changes (2)
- frontend/src/workers/compositor.worker.ts
- frontend/next-env.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/package.json
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/app/kitchens/vvd/view/[file]/page.tsx (1)
frontend/src/app/kitchens/vvd/view/[file]/viewer-client.tsx (1)
ViewerPage(7-28)
frontend/src/app/kitchens/vvd/view/[file]/viewer-client.tsx (1)
frontend/src/components/Viewer2D.tsx (1)
Viewer2D(21-118)
frontend/src/app/kitchen-configurator/[sku]/page.tsx (4)
frontend/src/app/(shop)/product/[handle]/page.tsx (1)
dynamic(4-4)frontend/src/app/kitchens/vvd/page.tsx (1)
dynamic(5-5)frontend/src/app/configurator/quote/complete/page.tsx (1)
dynamic(9-9)frontend/src/app/configurator/ClientPage.tsx (1)
ClientPage(19-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test-perf
🔇 Additional comments (14)
frontend/tests/e2e/perf/perf-reporter.ts (1)
3-3: LGTM! Correct type fix for Playwright Reporter API.The change correctly updates the
onBeginsignature to useSuiteinstead ofTestCase, aligning with Playwright's Reporter API specification whereonBeginreceives the root test suite.Also applies to: 23-23
frontend/src/app/kitchen-configurator/[sku]/page.tsx (1)
1-6: Clean handoff to the new configurator surface.Importing
ClientPagehere and marking the routeforce-dynamicmirrors the other updated configurator pages, so this page will opt out of caching and defer all UI to the new 2D viewer layout cleanly. Nice tidy removal of the old 3D plumbing.frontend/src/app/server-actions/dealerQuotes.ts (2)
25-27: LGTM! Correct Next.js 15 async cookies() migration.The function signature and
cookies()call have been properly updated to handle the asynchronous API introduced in Next.js 15.
53-53: LGTM! Properly awaits the async function.The call to
createDealerServerClient()correctly awaits the now-asynchronous function.frontend/src/app/dealer/actions.ts (4)
56-57: LGTM! Correct Next.js 15 async cookies() migration.The function properly handles the asynchronous
cookies()API with the appropriate async signature and await keyword.
106-107: LGTM! Properly propagates async changes.The function correctly awaits the now-asynchronous
extractAccessTokenFromCookie()call.
158-158: LGTM! Call site correctly updated.The call to
requireDealerToken()properly awaits the async function.
200-200: LGTM! All call sites correctly updated.Both
approveDealerQuoteandgetQuoteDownloadUrlproperly await the asyncrequireDealerToken()function.Also applies to: 238-238
frontend/playwright.config.ts (1)
25-37: LGTM! Cleaner reporter processing logic.The refactored approach using
normalizeReportermakes the reporter processing more consistent and maintainable. The non-null checks ensure only valid normalized reporters are added to the array.frontend/src/app/components/app-ready.tsx (1)
45-47: LGTM! Improved variable naming.The renaming from
moduletoinstrumentationModuleimproves code clarity and avoids potential confusion with the reservedmoduleidentifier in Node.js contexts.frontend/src/app/kitchens/vvd/page.tsx (1)
3-3: LGTM! Upgraded to Next.js Image component.The migration from standard
imgto Next.jsImagecomponent enables automatic image optimization, lazy loading, and better performance. The addition of the requiredheightprop completes the API requirements.Also applies to: 75-75
frontend/src/instrumentation.ts (1)
82-82: LGTM! Simplified exporter URL resolution.The refactored
resolveExporterUrlis more concise and directly returns the environment variable without unnecessary complexity.frontend/src/app/kitchens/vvd/view/[file]/viewer-client.tsx (2)
7-28: LGTM! Clean 2D viewer implementation.The migration from 3D Canvas to the
Viewer2Dcomponent is well-structured. The file parameter extraction properly handles both single values and arrays, and the conditional footer messaging provides clear user feedback.
18-21: Verify footer messaging aligns with product roadmap.The footer states "Detailed 3D model ... is not yet available" (line 20), which implies 3D models may be added in the future. Ensure this messaging aligns with the product roadmap and doesn't create misleading expectations if 3D support is not planned.
| export const dynamic = 'force-dynamic'; | ||
|
|
||
| export default async function Page({params}: {params: {handle: string}}) { | ||
| export default async function Page({params}: any) { |
There was a problem hiding this comment.
Replace any with proper typing and handle async params.
Using any removes type safety and masks a critical issue: in Next.js 15, params is asynchronous and must be awaited. The current synchronous access on line 11 will fail.
Apply this diff to use proper typing and await params:
-export default async function Page({params}: any) {
+export default async function Page({params}: {params: Promise<{handle: string}>}) {
+ const {handle} = await params;
+
if (!process.env.NEXT_PUBLIC_SHOPIFY_STORE_DOMAIN || !process.env.NEXT_PUBLIC_SHOPIFY_STOREFRONT_API_TOKEN) {
return <p>Connect Shopify</p>;
}
- const data = await shopify.storefront.query(ProductQueryDocument, {variables: {handle: params.handle}});
+ const data = await shopify.storefront.query(ProductQueryDocument, {variables: {handle}});Alternatively, use Next.js 15.5's PageProps helper type if available in your project.
Based on library documentation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/app/(shop)/product/[handle]/page.tsx around line 6, the function
parameter uses `any` and accesses `params` synchronously even though Next.js 15
makes `params` async; replace `any` with a proper Page props type (e.g., Next.js
PageProps or an explicit interface with params.handle as string) and await the
params before using them (for example, accept the props object, await
props.params to extract handle) so type safety is restored and the runtime error
is avoided.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Testing