(SP: 3) [SHOP] complete merch-ready PDP with multi-image gallery and size guidance#417
Merged
liudmylasovetovs merged 13 commits intodevelopfrom Mar 26, 2026
Merged
(SP: 3) [SHOP] complete merch-ready PDP with multi-image gallery and size guidance#417liudmylasovetovs merged 13 commits intodevelopfrom
liudmylasovetovs merged 13 commits intodevelopfrom
Conversation
liudmylasovetovs
requested review from
AM1007,
LesiaUKR and
ViktorSvertoka
as code owners
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaces single legacy product image with a full gallery: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant Form as ProductForm
participant API as Admin API
participant Parser as parseAdminProductPhotosForm
participant Service as Product Service
participant Cloudinary as Cloudinary
participant DB as Database
Admin->>Form: select files, set primary & order
Form->>API: POST/PATCH FormData (photoPlan, newImages, newImageUploadIds)
API->>Parser: parseAdminProductPhotosForm(formData)
Parser-->>API: { imagePlan, images } or error (photos)
API->>Service: createProduct/updateProduct(payload with imagePlan/images)
Service->>Cloudinary: upload new files
Cloudinary-->>Service: upload results (publicIds, urls)
Service->>DB: begin transaction
Service->>DB: insert/update/delete product_images & upsert prices
Service->>DB: update products.image_url / image_public_id from primary
DB-->>Service: commit
alt failure during uploads or txn
Service->>Cloudinary: destroy uploaded publicIds (cleanup)
Service-->>API: error (photos)
end
Service-->>API: return DbProduct with images[]
API-->>Form: 200 OK / error
sequenceDiagram
participant Client as Browser
participant Page as PDP
participant Service as getProductPageData
participant Commerce as Commerce resolver
participant Availability as getStorefrontAvailabilityState
participant Gallery as ProductGallery
participant Cart as AddToCartButton
Client->>Page: request product slug + locale
Page->>Service: getProductPageData(slug, locale)
Service->>Commerce: attempt commerceProduct resolution
Commerce-->>Service: ShopProduct | null
Service-->>Page: { product(display), commerceProduct, kind }
Page->>Availability: getStorefrontAvailabilityState(commerceProduct)
Availability-->>Page: availabilityState
Page->>Gallery: render with product.images
alt commerceProduct present
Page->>Cart: render AddToCartButton with commerceProduct + sizeGuide
else
Page-->>Client: show browse other products message
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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.
Reviewed commit: 8663088ade
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
frontend/lib/tests/shop/canonical-events-env.test.ts (2)
37-43: Redundant stubs: values already set inbeforeEach.Lines 38-39 stub
APP_ENVto'local'andNODE_ENVto'test', which are the exact same values established inbeforeEach. These calls can be removed without affecting test behavior.♻️ Remove redundant stubs
it('allows explicit disable only in non-production runtime', () => { - vi.stubEnv('APP_ENV', 'local'); - vi.stubEnv('NODE_ENV', 'test'); process.env.SHOP_CANONICAL_EVENTS_DUAL_WRITE = 'off'; expect(isCanonicalEventsDualWriteEnabled()).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/canonical-events-env.test.ts` around lines 37 - 43, The test "allows explicit disable only in non-production runtime" contains redundant environment stubs: remove the vi.stubEnv('APP_ENV', 'local') and vi.stubEnv('NODE_ENV', 'test') lines since those values are already set in the beforeEach; keep the SHOP_CANONICAL_EVENTS_DUAL_WRITE = 'off' stub and the expect(isCanonicalEventsDualWriteEnabled()).toBe(false) assertion unchanged.
9-25: Consider usingvi.stubEnvconsistently for all environment variables.The setup mixes
vi.stubEnv()(forAPP_ENV,NODE_ENV) with directprocess.envmanipulation (forSHOP_CANONICAL_EVENTS_DUAL_WRITE). While functional, usingvi.stubEnv()for all would simplify cleanup and improve consistency.♻️ Suggested refactor for consistency
beforeEach(() => { - for (const key of ENV_KEYS) { - previousEnv[key] = process.env[key]; - } vi.stubEnv('APP_ENV', 'local'); vi.stubEnv('NODE_ENV', 'test'); - delete process.env.SHOP_CANONICAL_EVENTS_DUAL_WRITE; + vi.unstubAllEnvs(); // Clear any prior stubs first + vi.stubEnv('APP_ENV', 'local'); + vi.stubEnv('NODE_ENV', 'test'); + // Ensure SHOP_CANONICAL_EVENTS_DUAL_WRITE is unset by not stubbing it }); afterEach(() => { vi.unstubAllEnvs(); - for (const key of ENV_KEYS) { - const prev = previousEnv[key]; - if (prev === undefined) delete process.env[key]; - else process.env[key] = prev; - } });Note: If you need to test the "unset" state, you may still need manual deletion since
vi.stubEnv(key, undefined)behavior may differ from a truly absent variable. The current approach works—this is purely a readability suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/canonical-events-env.test.ts` around lines 9 - 25, Update the test setup to consistently use vi.stubEnv instead of direct process.env manipulation: in the beforeEach block (alongside APP_ENV and NODE_ENV) call vi.stubEnv('SHOP_CANONICAL_EVENTS_DUAL_WRITE', undefined) or omit the key via vi.stubEnv to represent an absent value (so cleanup is handled by vi.unstubAllEnvs in afterEach), and keep restoration logic using ENV_KEYS and previousEnv unchanged; this targets the beforeEach/afterEach helpers and the SHOP_CANONICAL_EVENTS_DUAL_WRITE handling to improve consistency with vi.stubEnv/vi.unstubAllEnvs.frontend/lib/shop/size-guide.ts (1)
31-40: Consider hardening shared guide data as read-only.
APPAREL_SIZE_GUIDE_ROWS/ exported guide content are shared references; making them immutable would prevent accidental mutation side effects in callers/tests.Also applies to: 133-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/shop/size-guide.ts` around lines 31 - 40, APPAREL_SIZE_GUIDE_ROWS and the exported APPAREL_SIZE_GUIDE_CONTENT are mutable shared objects; make them immutable to prevent accidental mutations by callers/tests by returning/declaring them as read-only and freezing their values: update the declarations for APPAREL_SIZE_GUIDE_ROWS and APPAREL_SIZE_GUIDE_CONTENT (and any related exported constants at the other location) to use readonly types (e.g., ReadonlyArray<ApparelSizeGuideChartRow> / Readonly<Record<Locale, ApparelSizeGuide>>) and apply a runtime freeze (Object.freeze) to the arrays/objects so both TypeScript and runtime enforce immutability, referencing the symbols APPAREL_SIZE_GUIDE_ROWS, APPAREL_SIZE_GUIDE_CONTENT, ApparelSizeGuideChartRow and ApparelSizeGuide to locate the changes.frontend/app/api/shop/admin/products/[id]/route.ts (1)
407-449: Consider parsing photos only after form validation succeeds.Currently
parseAdminProductPhotosForm(line 408) executes before checking!parsed.ok(line 411). If form parsing fails, photo parsing was unnecessary work.This is a minor inefficiency, not a bug.
♻️ Proposed reorder
const parsed = parseAdminProductForm(formData, { mode: 'update' }); - const parsedPhotos = parseAdminProductPhotosForm(formData, { - mode: 'update', - }); if (!parsed.ok) { // ... existing error handling } + const parsedPhotos = parseAdminProductPhotosForm(formData, { + mode: 'update', + }); if (!parsedPhotos.ok) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/admin/products/`[id]/route.ts around lines 407 - 449, The code calls parseAdminProductPhotosForm before confirming parseAdminProductForm succeeded, doing unnecessary work when the main form is invalid; change the order so parseAdminProductForm(...) is called first and you only call parseAdminProductPhotosForm(...) after checking parsed.ok is true (use parsed.ok and parsed.error checks), preserving the same log/response behavior (logWarn keys 'admin_product_update_invalid_payload' and 'admin_product_update_invalid_photos' and the returned noStoreJson responses) and the same metadata variables (baseMeta, productIdForLog, startedAtMs).frontend/lib/tests/shop/admin-product-sale-contract.test.ts (1)
77-88: Consider usingmockClear()instead ofmockReset()to avoid re-implementing the mock.The mock implementation in
beforeEach(lines 78-88) duplicates the hoisted default (lines 15-23). UsingmockClear()preserves the implementation while resetting call history.♻️ Proposed simplification
beforeEach(() => { vi.stubEnv('ENABLE_ADMIN_API', 'true'); - parseAdminProductFormMock.mockReset(); - parseAdminProductPhotosFormMock.mockReset(); - parseAdminProductPhotosFormMock.mockImplementation( - (formData: FormData) => ({ - ok: true, - data: { - imagePlan: [{ uploadId: 'legacy-image', isPrimary: true }], - images: [ - { uploadId: 'legacy-image', file: formData.get('image') as File }, - ], - }, - }) - ); + parseAdminProductFormMock.mockClear(); + parseAdminProductPhotosFormMock.mockClear(); productsServiceMock.createProduct.mockReset();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-product-sale-contract.test.ts` around lines 77 - 88, The test currently calls parseAdminProductPhotosFormMock.mockReset() and then re-defines the implementation in the beforeEach, duplicating the hoisted default; change mockReset() to mockClear() so the existing mock implementation (parseAdminProductPhotosFormMock.mockImplementation defined earlier) is preserved and only the call history is cleared, and remove the duplicated mockImplementation block in the beforeEach (or keep it only if you intend to override the hoisted implementation) so parseAdminProductPhotosFormMock is not re-implemented unnecessarily.frontend/lib/tests/shop/product-gallery-interaction.test.ts (1)
34-94: Good test coverage for core gallery interaction.The test correctly validates:
- Primary image renders first
- Thumbnail
aria-pressedstates reflect selection- Click interaction updates the main image
Consider adding edge-case tests for robustness:
- Single image (should hide or disable thumbnail strip)
- Empty images array (error boundary or fallback)
- Keyboard navigation (
Enter/Spaceon thumbnails)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/product-gallery-interaction.test.ts` around lines 34 - 94, Add three edge-case tests to product-gallery-interaction.test.ts for ProductGallery: (1) a "single image" test that renders ProductGallery with one image and asserts the thumbnail strip is hidden/disabled and the main image displays the single src; (2) an "empty images" test that renders with an empty images array and asserts the component shows the fallback UI or an accessible error boundary (e.g., a placeholder image or aria-live message); and (3) a "keyboard navigation" test that focuses a thumbnail and triggers Enter/Space key events to assert the main image swaps and aria-pressed toggles on ProductGallery thumbnails. Use the same query patterns (screen.getByRole('img'...) and screen.getByRole('button'...)) and the existing class/aria assertions to match behavior.frontend/lib/tests/shop/admin-product-photo-management.test.ts (1)
228-243: Make the second-row ID assertion non-tautological.
id: imageRows[1]!.idjust echoes the actual value back into the expected object, so that field can never fail. If this test is meant to prove a new row was inserted, assert that ID independently.🧪 Proposed tightening
+ expect(imageRows[1]!.id).not.toBe(primaryImage.id); + expect(imageRows[1]!.id).not.toBe(secondaryImage.id); expect(imageRows).toEqual([ { id: secondaryImage.id, imageUrl: 'https://example.com/p2.png', imagePublicId: 'products/p2', @@ }, { - id: imageRows[1]!.id, + id: expect.any(String), imageUrl: 'https://example.com/p3.png', imagePublicId: 'products/p3', sortOrder: 1, isPrimary: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-product-photo-management.test.ts` around lines 228 - 243, The expected object for imageRows currently uses a tautological id assertion (id: imageRows[1]!.id); update the test in admin-product-photo-management.test.ts to assert the second row's id independently—e.g., verify imageRows[1] is defined and its id is a non-empty string or matches the inserted record id, or assert it is not equal to secondaryImage.id—by replacing id: imageRows[1]!.id with a concrete assertion (like expect(imageRows[1]!.id).toBeDefined()/toMatch(/^[\w-]+$/)/not.toEqual(secondaryImage.id)) and keep the other expected fields (imageUrl, imagePublicId, sortOrder, isPrimary) the same.frontend/lib/services/products/mutations/update.ts (2)
47-59: Consider removing unnecessary type casts.The
imagesandimagePlanfields are already defined inProductUpdateInput, so the(input as any)casts are unnecessary and reduce type safety. You could access them directly or use a type guard.♻️ Suggested improvement
- const requestedUploads = - Array.isArray((input as any).images) && (input as any).images.length > 0 - ? ((input as any).images as Array<{ uploadId: string; file: File }>) - : []; + const requestedUploads = + Array.isArray(input.images) && input.images.length > 0 + ? input.images + : []; - const hasExplicitPhotoPlan = - Array.isArray((input as any).imagePlan) && - (input as any).imagePlan.length > 0; + const hasExplicitPhotoPlan = + Array.isArray(input.imagePlan) && + input.imagePlan.length > 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/products/mutations/update.ts` around lines 47 - 59, The three variables legacyImage, requestedUploads, and hasExplicitPhotoPlan use unnecessary (input as any) casts which weaken type safety; update the code to treat the function parameter as ProductUpdateInput (or add a type guard/narrowing) and access input.image, input.images, and input.imagePlan directly, keeping the existing runtime checks (instanceof File, Array.isArray, length checks) to preserve behavior while restoring proper typing.
356-372: Duplicated retained/removed image calculation.The logic to compute
retainedExistingIdsandremovedImagesis duplicated from inside the transaction (lines 259-274). SinceresolvedPhotoPlanandexistingImagesare both available before the transaction, consider computing these once before the transaction and reusing them.♻️ Extract shared computation
+ let retainedExistingIds: Set<string> | undefined; + let removedImages: typeof existingImages | undefined; + + if (resolvedPhotoPlan) { + retainedExistingIds = new Set( + resolvedPhotoPlan + .filter( + (item): item is Extract<typeof item, { source: 'existing' }> => + item.source === 'existing' + ) + .map(item => item.imageId) + ); + removedImages = existingImages.filter( + image => !retainedExistingIds!.has(image.id) + ); + } // Then use retainedExistingIds and removedImages in both the transaction and cleanup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/products/mutations/update.ts` around lines 356 - 372, Compute retainedExistingIds and removedImages once before the DB transaction and reuse those variables inside the transaction block instead of re-deriving them twice: extract the logic that builds retainedExistingIds from resolvedPhotoPlan (filtering items with source === 'existing' and mapping imageId) and the removedImages filter from existingImages into a shared pre-transaction section, then reference those identifiers inside the transaction code paths (the same retainedExistingIds and removedImages variables) to avoid duplication.frontend/lib/tests/shop/product-images-contract.test.ts (1)
161-171: Consider adding type definitions for the test input.The
as anycast at line 171 suggests the test input shape doesn't matchProductInput. While this works, creating a properly typed test helper or adjusting the input to match the expected type would improve test maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/product-images-contract.test.ts` around lines 161 - 171, The test is using an unsafe cast ("as any") when calling createProduct; replace this by providing a properly typed object matching the ProductInput (or the specific input type used by createProduct) — either update the literal to conform to ProductInput (e.g., ensure fields and types for image, prices, badge, etc. match the type) or add a small test helper/fixture typed as ProductInput and call createProduct(productFixture). Locate the createProduct call in product-images-contract.test.ts and remove the "as any" cast, using the typed fixture or corrected literal so TS enforces the expected shape.frontend/lib/services/products/images.ts (1)
116-146: Consider filtering byisPrimaryin the query.The function fetches all images and then iterates to find the primary. Since there's a partial unique index on
is_primary, you could filter directly in the query for better efficiency on products with many images.♻️ Suggested optimization
export async function getPrimaryProductImageRow( productId: string, options?: { db?: ProductImagesReader } ): Promise<ProductImage | null> { const executor = options?.db ?? db; - const rows = await executor + const [row] = await executor .select({ id: productImages.id, productId: productImages.productId, imageUrl: productImages.imageUrl, imagePublicId: productImages.imagePublicId, sortOrder: productImages.sortOrder, isPrimary: productImages.isPrimary, createdAt: productImages.createdAt, updatedAt: productImages.updatedAt, }) .from(productImages) - .where(eq(productImages.productId, productId)) + .where( + and( + eq(productImages.productId, productId), + eq(productImages.isPrimary, true) + ) + ) .orderBy( asc(productImages.sortOrder), asc(productImages.createdAt), asc(productImages.id) - ); + ) + .limit(1); - for (const row of rows) { - const parsed = productImageSchema.parse(row); - if (parsed.isPrimary) return parsed; - } - - return null; + return row ? productImageSchema.parse(row) : null; }Note: You'll need to import
andfromdrizzle-orm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/products/images.ts` around lines 116 - 146, The query in getPrimaryProductImageRow currently selects all rows then loops to find the primary; change the query to filter for the primary in the DB by adding a condition for productImages.isPrimary (e.g., eq(productImages.isPrimary, true)) combined with the existing productId predicate (use and(...) if needed), import and from drizzle-orm, then parse the single returned row with productImageSchema and return it (or null if none); keep using options?.db ?? db as the executor and preserve the ordering/fallback logic.frontend/lib/shop/data.ts (1)
86-101: Minor redundancy in view model construction.The
toProductPageDisplayProductis called at line 89-98, thentoProductPageViewModelis called which rebuilds the same display product fromcommerceProduct(lines 193-202). Consider either callingtoProductPageViewModeldirectly without pre-buildingproduct, or simplifying the flow.♻️ Simplified approach
if (mapped) { - return toProductPageViewModel({ + return { kind: 'available', - product: toProductPageDisplayProduct({ - id: mapped.id, - slug: mapped.slug, - name: mapped.name, - image: mapped.image, - images: mapped.images, - primaryImage: mapped.primaryImage, - description: mapped.description, - badge: mapped.badge ?? 'NONE', - }), + product: toProductPageDisplayProduct(mapped), commerceProduct: mapped, - }); + }; }This would require updating
toProductPageDisplayProductto acceptShopProductdirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/shop/data.ts` around lines 86 - 101, The current code builds a display product with toProductPageDisplayProduct and then passes both that product and commerceProduct into toProductPageViewModel, causing redundant reconstruction; fix by removing the pre-built product and either (A) call toProductPageViewModel({ kind: 'available', commerceProduct: mapped }) and let toProductPageViewModel construct the display product internally, or (B) change toProductPageDisplayProduct to accept a ShopProduct (mapped) and have toProductPageViewModel call toProductPageDisplayProduct(mapped) itself; update references to toProductPageDisplayProduct, toProductPageViewModel, and the mapped/commerceProduct usage accordingly so the display product is created in one place only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/`[locale]/admin/shop/products/_components/ProductForm.tsx:
- Around line 176-185: The current branch in ProductForm that returns a
synthetic image object when fromInitial.imageUrl exists creates imageId:
'legacy-image', which later gets serialized into the photoPlan submit path and
rejected by the server (it expects real UUID-backed product_images); fix by
keeping legacy-only images off the "existing" submission path or by backfilling
real image rows before render: update the logic in ProductForm (the branch that
checks fromInitial.imageUrl and produces the 'legacy-image' object) to either
(A) return a placeholder that marks source as 'legacy' and excludes imageId /
existing-image serialization so the submit routine skips converting it into an
existing photo reference, or (B) instead populate initialValues.images with
actual product_images (UUID imageId and previewUrl) before the form mounts so
photoPlan receives valid UUID-backed imageIds; ensure the submit serialization
(photoPlan path) recognizes and only serializes images with real UUID imageId
values.
In `@frontend/app/`[locale]/admin/shop/products/[id]/edit/page.tsx:
- Around line 38-43: The catch is too broad—replace the empty catch that treats
any error from getAdminProductByIdWithPrices(...) as a 404 with targeted
handling: catch only the domain error (e.g., ProductNotFoundError) and call
notFound() for that case, but let other errors propagate (or rethrow/log and
throw) so infrastructure issues surface; update the try/catch around
getAdminProductByIdWithPrices to check error instanceof ProductNotFoundError (or
error.name) and rethrow or throw the original error for non-matching cases.
In `@frontend/app/`[locale]/shop/products/[slug]/page.tsx:
- Around line 73-77: The ProductGallery instance persists its internal
selectedIndex across navigations because it lacks a stable React key; fix by
giving the ProductGallery component a key that uniquely identifies the product
(e.g., product.id or product.slug) so a new instance is mounted for each product
change — update the ProductGallery invocation (productName/images/badgeLabel) to
include key={product.id || product.slug}.
In `@frontend/components/shop/ProductCard.tsx`:
- Around line 100-109: The availability text in ProductCard.tsx uses
role="status" for unavailable items which creates an unnecessary live region;
update the <p> element logic (the element that checks availabilityState) to only
set aria-label when availabilityState === 'available_to_order' and remove the
role:'status' branch so unavailable state renders as plain static text (no role
or live-region attributes).
In `@frontend/db/queries/shop/products.ts`:
- Around line 125-145: The fallback path getPublicProductBaseBySlug is returning
legacy products.image_url and hardcoding primaryImage undefined, which bypasses
gallery resolution; update getPublicProductBaseBySlug to call the same resolver
used by mapRowToDbProduct (resolveProductImages) and map the resolved fields
(imageUrl, imagePublicId, images, primaryImage) into the returned DbProduct (or
pass the resulting row through mapRowToDbProduct) so unavailable products get
the same gallery-derived images as getPublicProductBySlug; alternatively, if
intentional, add a clear comment in getPublicProductBaseBySlug explaining why
gallery resolution is intentionally skipped.
In `@frontend/lib/services/products/mutations/delete.ts`:
- Around line 10-48: The deleteProduct transaction now gathers and returns
Cloudinary publicIds and performs asset destruction itself, but the admin-create
audit rollback still calls destroyProductImage(inserted.imagePublicId) after
deleteProduct(inserted.id), causing the primary image to be deleted twice;
update the admin-create rollback to stop calling destroyProductImage for the
product primary image (or otherwise remove the duplicate call) so cleanup
happens only in deleteProduct, or alternatively move asset cleanup entirely out
of deleteProduct and document the contract change; look for callers named
deleteProduct and destroyProductImage and the admin-create audit rollback logic
to remove the extra destroyProductImage invocation or adjust the cleanup
responsibility consistently.
In `@frontend/lib/services/products/photo-plan.ts`:
- Around line 51-52: Reject requests that contain duplicate uploadIds before
converting uploads into a Map: in the function that builds uploadsById (and the
similar block at lines 111-121), detect duplicate upload.uploadId values in the
uploads array (e.g., by tracking seen IDs) and throw or return a validation
error when a duplicate is found rather than silently keeping the last one; do
this check before calling new Map(...) so you fail fast on malformed input
referencing the same uploadId multiple times.
In `@frontend/lib/tests/shop/size-guide-content.test.ts`:
- Around line 36-47: Add a negative test asserting that products which are not
apparel but still have a non-empty sizes array do NOT return apparel size-guide
content: call getApparelSizeGuideForProduct with a product object that includes
sizes (e.g. sizes: ['S']) and an explicit non-apparel indicator used by your
code (e.g. isApparel: false, category: 'accessory' or type: 'non-apparel'—match
the property the implementation checks) and assert it returns null (toBeNull()).
Reference getApparelSizeGuideForProduct and APPAREL_SIZE_GUIDE_CONTENT when
adding this case.
---
Nitpick comments:
In `@frontend/app/api/shop/admin/products/`[id]/route.ts:
- Around line 407-449: The code calls parseAdminProductPhotosForm before
confirming parseAdminProductForm succeeded, doing unnecessary work when the main
form is invalid; change the order so parseAdminProductForm(...) is called first
and you only call parseAdminProductPhotosForm(...) after checking parsed.ok is
true (use parsed.ok and parsed.error checks), preserving the same log/response
behavior (logWarn keys 'admin_product_update_invalid_payload' and
'admin_product_update_invalid_photos' and the returned noStoreJson responses)
and the same metadata variables (baseMeta, productIdForLog, startedAtMs).
In `@frontend/lib/services/products/images.ts`:
- Around line 116-146: The query in getPrimaryProductImageRow currently selects
all rows then loops to find the primary; change the query to filter for the
primary in the DB by adding a condition for productImages.isPrimary (e.g.,
eq(productImages.isPrimary, true)) combined with the existing productId
predicate (use and(...) if needed), import and from drizzle-orm, then parse the
single returned row with productImageSchema and return it (or null if none);
keep using options?.db ?? db as the executor and preserve the ordering/fallback
logic.
In `@frontend/lib/services/products/mutations/update.ts`:
- Around line 47-59: The three variables legacyImage, requestedUploads, and
hasExplicitPhotoPlan use unnecessary (input as any) casts which weaken type
safety; update the code to treat the function parameter as ProductUpdateInput
(or add a type guard/narrowing) and access input.image, input.images, and
input.imagePlan directly, keeping the existing runtime checks (instanceof File,
Array.isArray, length checks) to preserve behavior while restoring proper
typing.
- Around line 356-372: Compute retainedExistingIds and removedImages once before
the DB transaction and reuse those variables inside the transaction block
instead of re-deriving them twice: extract the logic that builds
retainedExistingIds from resolvedPhotoPlan (filtering items with source ===
'existing' and mapping imageId) and the removedImages filter from existingImages
into a shared pre-transaction section, then reference those identifiers inside
the transaction code paths (the same retainedExistingIds and removedImages
variables) to avoid duplication.
In `@frontend/lib/shop/data.ts`:
- Around line 86-101: The current code builds a display product with
toProductPageDisplayProduct and then passes both that product and
commerceProduct into toProductPageViewModel, causing redundant reconstruction;
fix by removing the pre-built product and either (A) call
toProductPageViewModel({ kind: 'available', commerceProduct: mapped }) and let
toProductPageViewModel construct the display product internally, or (B) change
toProductPageDisplayProduct to accept a ShopProduct (mapped) and have
toProductPageViewModel call toProductPageDisplayProduct(mapped) itself; update
references to toProductPageDisplayProduct, toProductPageViewModel, and the
mapped/commerceProduct usage accordingly so the display product is created in
one place only.
In `@frontend/lib/shop/size-guide.ts`:
- Around line 31-40: APPAREL_SIZE_GUIDE_ROWS and the exported
APPAREL_SIZE_GUIDE_CONTENT are mutable shared objects; make them immutable to
prevent accidental mutations by callers/tests by returning/declaring them as
read-only and freezing their values: update the declarations for
APPAREL_SIZE_GUIDE_ROWS and APPAREL_SIZE_GUIDE_CONTENT (and any related exported
constants at the other location) to use readonly types (e.g.,
ReadonlyArray<ApparelSizeGuideChartRow> / Readonly<Record<Locale,
ApparelSizeGuide>>) and apply a runtime freeze (Object.freeze) to the
arrays/objects so both TypeScript and runtime enforce immutability, referencing
the symbols APPAREL_SIZE_GUIDE_ROWS, APPAREL_SIZE_GUIDE_CONTENT,
ApparelSizeGuideChartRow and ApparelSizeGuide to locate the changes.
In `@frontend/lib/tests/shop/admin-product-photo-management.test.ts`:
- Around line 228-243: The expected object for imageRows currently uses a
tautological id assertion (id: imageRows[1]!.id); update the test in
admin-product-photo-management.test.ts to assert the second row's id
independently—e.g., verify imageRows[1] is defined and its id is a non-empty
string or matches the inserted record id, or assert it is not equal to
secondaryImage.id—by replacing id: imageRows[1]!.id with a concrete assertion
(like
expect(imageRows[1]!.id).toBeDefined()/toMatch(/^[\w-]+$/)/not.toEqual(secondaryImage.id))
and keep the other expected fields (imageUrl, imagePublicId, sortOrder,
isPrimary) the same.
In `@frontend/lib/tests/shop/admin-product-sale-contract.test.ts`:
- Around line 77-88: The test currently calls
parseAdminProductPhotosFormMock.mockReset() and then re-defines the
implementation in the beforeEach, duplicating the hoisted default; change
mockReset() to mockClear() so the existing mock implementation
(parseAdminProductPhotosFormMock.mockImplementation defined earlier) is
preserved and only the call history is cleared, and remove the duplicated
mockImplementation block in the beforeEach (or keep it only if you intend to
override the hoisted implementation) so parseAdminProductPhotosFormMock is not
re-implemented unnecessarily.
In `@frontend/lib/tests/shop/canonical-events-env.test.ts`:
- Around line 37-43: The test "allows explicit disable only in non-production
runtime" contains redundant environment stubs: remove the vi.stubEnv('APP_ENV',
'local') and vi.stubEnv('NODE_ENV', 'test') lines since those values are already
set in the beforeEach; keep the SHOP_CANONICAL_EVENTS_DUAL_WRITE = 'off' stub
and the expect(isCanonicalEventsDualWriteEnabled()).toBe(false) assertion
unchanged.
- Around line 9-25: Update the test setup to consistently use vi.stubEnv instead
of direct process.env manipulation: in the beforeEach block (alongside APP_ENV
and NODE_ENV) call vi.stubEnv('SHOP_CANONICAL_EVENTS_DUAL_WRITE', undefined) or
omit the key via vi.stubEnv to represent an absent value (so cleanup is handled
by vi.unstubAllEnvs in afterEach), and keep restoration logic using ENV_KEYS and
previousEnv unchanged; this targets the beforeEach/afterEach helpers and the
SHOP_CANONICAL_EVENTS_DUAL_WRITE handling to improve consistency with
vi.stubEnv/vi.unstubAllEnvs.
In `@frontend/lib/tests/shop/product-gallery-interaction.test.ts`:
- Around line 34-94: Add three edge-case tests to
product-gallery-interaction.test.ts for ProductGallery: (1) a "single image"
test that renders ProductGallery with one image and asserts the thumbnail strip
is hidden/disabled and the main image displays the single src; (2) an "empty
images" test that renders with an empty images array and asserts the component
shows the fallback UI or an accessible error boundary (e.g., a placeholder image
or aria-live message); and (3) a "keyboard navigation" test that focuses a
thumbnail and triggers Enter/Space key events to assert the main image swaps and
aria-pressed toggles on ProductGallery thumbnails. Use the same query patterns
(screen.getByRole('img'...) and screen.getByRole('button'...)) and the existing
class/aria assertions to match behavior.
In `@frontend/lib/tests/shop/product-images-contract.test.ts`:
- Around line 161-171: The test is using an unsafe cast ("as any") when calling
createProduct; replace this by providing a properly typed object matching the
ProductInput (or the specific input type used by createProduct) — either update
the literal to conform to ProductInput (e.g., ensure fields and types for image,
prices, badge, etc. match the type) or add a small test helper/fixture typed as
ProductInput and call createProduct(productFixture). Locate the createProduct
call in product-images-contract.test.ts and remove the "as any" cast, using the
typed fixture or corrected literal so TS enforces the expected shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1e52cee-06cc-49aa-9d2b-290003b83879
📒 Files selected for processing (43)
frontend/app/[locale]/admin/shop/products/[id]/edit/page.tsxfrontend/app/[locale]/admin/shop/products/_components/ProductForm.tsxfrontend/app/[locale]/shop/products/[slug]/page.tsxfrontend/app/api/shop/admin/products/[id]/route.tsfrontend/app/api/shop/admin/products/route.tsfrontend/components/shop/AddToCartButton.tsxfrontend/components/shop/CartProvider.tsxfrontend/components/shop/ProductCard.tsxfrontend/components/shop/ProductGallery.tsxfrontend/db/queries/shop/products.tsfrontend/db/schema/shop.tsfrontend/drizzle/0033_marvelous_arclight.sqlfrontend/drizzle/meta/0033_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/lib/admin/parseAdminProductForm.tsfrontend/lib/services/products/admin/queries.tsfrontend/lib/services/products/images.tsfrontend/lib/services/products/mapping.tsfrontend/lib/services/products/mutations/create.tsfrontend/lib/services/products/mutations/delete.tsfrontend/lib/services/products/mutations/toggle.tsfrontend/lib/services/products/mutations/update.tsfrontend/lib/services/products/photo-plan.tsfrontend/lib/shop/availability.tsfrontend/lib/shop/data.tsfrontend/lib/shop/size-guide.tsfrontend/lib/tests/shop/admin-product-canonical-audit-phase5.test.tsfrontend/lib/tests/shop/admin-product-create-atomic-phasec.test.tsfrontend/lib/tests/shop/admin-product-patch-price-config-error-contract.test.tsfrontend/lib/tests/shop/admin-product-photo-management.test.tsfrontend/lib/tests/shop/admin-product-sale-contract.test.tsfrontend/lib/tests/shop/canonical-events-env.test.tsfrontend/lib/tests/shop/intl-quote-domain-phase2.test.tsfrontend/lib/tests/shop/product-availability-view-model.test.tsfrontend/lib/tests/shop/product-gallery-interaction.test.tsfrontend/lib/tests/shop/product-gallery-view-model.test.tsfrontend/lib/tests/shop/product-images-contract.test.tsfrontend/lib/tests/shop/size-guide-content.test.tsfrontend/lib/types/shop.tsfrontend/lib/validation/shop.tsfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/drizzle/0034_melodic_pet_avengers.sql`:
- Around line 1-30: This migration introduces an unrelated table
ai_learned_terms (and its FK constraint ai_learned_terms_user_id_users_id_fk)
into a storefront release; remove that DDL from this migration and either move
the CREATE TABLE and DO $$... ALTER TABLE block into a separate migration PR
targeted at the AI/knowledge-base workstream or revert this file to only include
storefront-related changes; ensure the separate migration preserves the table
name ai_learned_terms, the UNIQUE constraint ai_learned_terms_user_term_uniq,
and the foreign key definition when you create the new dedicated migration.
- Around line 1-27: The migration mixes unqualified and schema-qualified names
causing potential search_path issues; make the table and all references
schema-consistent by qualifying the created table and ALTER TABLE targets with
the public schema. Specifically, update the CREATE TABLE to create
"public"."ai_learned_terms" (instead of unqualified "ai_learned_terms") and
ensure the ALTER TABLE, the conrelid check (currently comparing to
'public.ai_learned_terms'::regclass), and the constraint name/FOREIGN KEY
reference all target the same "public"."ai_learned_terms" and the referenced
"public"."users" table so the constraint creation and existence check operate on
the exact same schema-qualified relation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a331310c-d35f-4bce-b234-40c3a19f16f4
📒 Files selected for processing (6)
frontend/drizzle/0034_melodic_pet_avengers.sqlfrontend/drizzle/meta/0034_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.json
✅ Files skipped from review due to trivial changes (3)
- frontend/drizzle/meta/_journal.json
- frontend/messages/pl.json
- frontend/messages/uk.json
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/messages/en.json
…allery tests, and migration SQL consistency
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/lib/shop/data.ts (1)
189-214: Tighten the available-case contract here.The available branch ignores
data.productand rebuilds it fromcommerceProduct, so callers now construct aproductobject that is immediately discarded. Making this helper derive the display model fromcommerceProductonly will remove that dead mapping and reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/shop/data.ts` around lines 189 - 214, The available branch of toProductPageViewModel rebuilds a display product from commerceProduct fields and ignores data.product, creating dead mapping; change the available branch to derive the display model directly from data.commerceProduct (e.g. call toProductPageDisplayProduct(data.commerceProduct)) instead of reconstructing the object, preserving the badge default (badge ?? 'NONE') as needed, and update any type/signature of toProductPageDisplayProduct if necessary so it accepts the commerceProduct shape rather than a rebuilt object.frontend/lib/tests/shop/admin-product-photo-management.test.ts (1)
250-252: Assert the exact Cloudinary deletions, not just one matching call.
toHaveBeenCalledWith(...)still passes ifupdateProduct(...)deletes the removed image and one of the retained images, so this test would miss an over-eager cleanup regression. Add a call-count check or assert the full call list here.✅ Tighten the assertion
+ expect(cloudinaryMocks.destroyProductImage).toHaveBeenCalledTimes(1); expect(cloudinaryMocks.destroyProductImage).toHaveBeenCalledWith( primaryImage.imagePublicId );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-product-photo-management.test.ts` around lines 250 - 252, The test currently only asserts that cloudinaryMocks.destroyProductImage was called with primaryImage.imagePublicId, which can miss extra unintended deletions; update the test for the updateProduct flow (the test invoking updateProduct(...)) to assert the exact calls by either checking call count (expect(cloudinaryMocks.destroyProductImage).toHaveBeenCalledTimes(1)) and that the single call matches primaryImage.imagePublicId, or assert the full calls array (expect(cloudinaryMocks.destroyProductImage.mock.calls).toEqual([[primaryImage.imagePublicId]]) ) so you verify no retained images were also deleted.frontend/app/[locale]/admin/shop/products/_components/ProductForm.tsx (1)
360-365: Rehydrating the form leaks superseded blob previews.New preview URLs are revoked on explicit remove and on final unmount, but the edit hydration path replaces
photoswithout revoking any blob URLs already held in the previous state. Because the unmount cleanup only sees the latestphotosRef.current, previews created for an earlier edit session can stay allocated for the rest of the page lifetime.Also applies to: 369-381, 427-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/admin/shop/products/_components/ProductForm.tsx around lines 360 - 365, When hydrating/editing you replace photos state via setPhotos(ensureUiPhotos(...)) without revoking any blob URLs held by the previous photos, leaking object URLs; before calling setPhotos (and in the other hydrate/update sites that call ensureUiPhotos like the other edit paths), iterate the existing photosRef.current and call URL.revokeObjectURL for any photo.preview/blob URL (or invoke the same helper that revokes previews), then update photos and photosRef.current with the new ensureUiPhotos result so the older previews are released and only the current previews remain tracked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/`[locale]/admin/shop/products/_components/ProductForm.tsx:
- Around line 174-200: buildPhotoPlanSubmission currently drops legacy photos
leading to destructive edits when legacy + non-legacy photos are mixed; add a
guard that detects if any photo.source === 'legacy' and also any photo.source
!== 'legacy' and, when mixed, block the submission path (e.g., return {
photoPlan: undefined, newPhotos: [] } or throw a specific MigrationRequired
error) so the UI can show a migration message instead of sending a partial
photoPlan; implement the same mixed-state guard in the other photo-plan
builders/handlers referenced (near the other ranges called out: 224-232,
487-489, 557-568) and ensure the UI uses this sentinel to display the migration
prompt from ensureUiPhotos-produced legacy items.
In `@frontend/lib/shop/data.ts`:
- Around line 83-102: The code currently converts a validation failure from
mapToShopProduct into a not_found response; instead, when
mapToShopProduct(dbProduct) returns falsy you should not return { kind:
'not_found' } but surface a distinct invalid-data outcome (or throw) so real
data/merch regressions aren't hidden. Update the block handling mapped (the
result of mapToShopProduct) inside the getPublicProductBySlug flow to either
throw a descriptive Error (e.g., new Error(`Invalid product data for slug
${slug}`)) or return a dedicated view-model like { kind: 'invalid_data', reason:
'invalid_gallery_or_db_payload' } (include slug/id in the message/state) and
keep the existing logging performed by mapToShopProduct.
---
Nitpick comments:
In `@frontend/app/`[locale]/admin/shop/products/_components/ProductForm.tsx:
- Around line 360-365: When hydrating/editing you replace photos state via
setPhotos(ensureUiPhotos(...)) without revoking any blob URLs held by the
previous photos, leaking object URLs; before calling setPhotos (and in the other
hydrate/update sites that call ensureUiPhotos like the other edit paths),
iterate the existing photosRef.current and call URL.revokeObjectURL for any
photo.preview/blob URL (or invoke the same helper that revokes previews), then
update photos and photosRef.current with the new ensureUiPhotos result so the
older previews are released and only the current previews remain tracked.
In `@frontend/lib/shop/data.ts`:
- Around line 189-214: The available branch of toProductPageViewModel rebuilds a
display product from commerceProduct fields and ignores data.product, creating
dead mapping; change the available branch to derive the display model directly
from data.commerceProduct (e.g. call
toProductPageDisplayProduct(data.commerceProduct)) instead of reconstructing the
object, preserving the badge default (badge ?? 'NONE') as needed, and update any
type/signature of toProductPageDisplayProduct if necessary so it accepts the
commerceProduct shape rather than a rebuilt object.
In `@frontend/lib/tests/shop/admin-product-photo-management.test.ts`:
- Around line 250-252: The test currently only asserts that
cloudinaryMocks.destroyProductImage was called with primaryImage.imagePublicId,
which can miss extra unintended deletions; update the test for the updateProduct
flow (the test invoking updateProduct(...)) to assert the exact calls by either
checking call count
(expect(cloudinaryMocks.destroyProductImage).toHaveBeenCalledTimes(1)) and that
the single call matches primaryImage.imagePublicId, or assert the full calls
array
(expect(cloudinaryMocks.destroyProductImage.mock.calls).toEqual([[primaryImage.imagePublicId]])
) so you verify no retained images were also deleted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd115060-85c8-46a3-9a05-660a1f6661bd
📒 Files selected for processing (19)
frontend/app/[locale]/admin/shop/products/[id]/edit/page.tsxfrontend/app/[locale]/admin/shop/products/_components/ProductForm.tsxfrontend/app/[locale]/shop/products/[slug]/page.tsxfrontend/app/api/shop/admin/products/[id]/route.tsfrontend/app/api/shop/admin/products/route.tsfrontend/components/shop/ProductCard.tsxfrontend/components/shop/ProductGallery.tsxfrontend/db/queries/shop/products.tsfrontend/drizzle/0034_melodic_pet_avengers.sqlfrontend/lib/services/products/photo-plan.tsfrontend/lib/shop/data.tsfrontend/lib/tests/shop/admin-edit-product-page-error-handling.test.tsfrontend/lib/tests/shop/admin-product-create-atomic-phasec.test.tsfrontend/lib/tests/shop/admin-product-photo-management.test.tsfrontend/lib/tests/shop/admin-product-sale-contract.test.tsfrontend/lib/tests/shop/canonical-events-env.test.tsfrontend/lib/tests/shop/product-gallery-interaction.test.tsfrontend/lib/tests/shop/product-images-contract.test.tsfrontend/lib/tests/shop/product-photo-plan-fixes.test.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/drizzle/0034_melodic_pet_avengers.sql
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/lib/tests/shop/admin-product-create-atomic-phasec.test.ts
- frontend/lib/tests/shop/canonical-events-env.test.ts
- frontend/lib/services/products/photo-plan.ts
- frontend/lib/tests/shop/admin-product-sale-contract.test.ts
- frontend/app/api/shop/admin/products/[id]/route.ts
- frontend/lib/tests/shop/product-gallery-interaction.test.ts
- frontend/components/shop/ProductGallery.tsx
- frontend/components/shop/ProductCard.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/[locale]/admin/shop/products/_components/ProductForm.tsx (1)
677-684: Consider extending field checks for photo-related errors.The server (
parseAdminProductForm.ts) can return errors withfieldvalues of'photoPlan','newImages', or'newImageUploadIds'in addition to'photos'and'image'. Currently, these would fall through to the generic error display rather than appearing in the dedicated photo error section.♻️ Suggested improvement
if ( data.code === 'IMAGE_UPLOAD_FAILED' || data.code === 'IMAGE_REQUIRED' || data.field === 'image' || - data.field === 'photos' + data.field === 'photos' || + data.field === 'photoPlan' || + data.field === 'newImages' || + data.field === 'newImageUploadIds' ) { setImageError(data.error ?? 'Failed to update product photos'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/admin/shop/products/_components/ProductForm.tsx around lines 677 - 684, Update the conditional that sets image errors in ProductForm.tsx so photo-related server fields also route to the photo error UI: extend the check in the if inside the submit/response handler (the block that calls setImageError(...)) to include data.field values 'photoPlan', 'newImages', and 'newImageUploadIds' in addition to 'image' and 'photos' and the existing codes 'IMAGE_UPLOAD_FAILED'/'IMAGE_REQUIRED', ensuring photo-specific errors are displayed via setImageError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/`[locale]/admin/shop/products/_components/ProductForm.tsx:
- Around line 677-684: Update the conditional that sets image errors in
ProductForm.tsx so photo-related server fields also route to the photo error UI:
extend the check in the if inside the submit/response handler (the block that
calls setImageError(...)) to include data.field values 'photoPlan', 'newImages',
and 'newImageUploadIds' in addition to 'image' and 'photos' and the existing
codes 'IMAGE_UPLOAD_FAILED'/'IMAGE_REQUIRED', ensuring photo-specific errors are
displayed via setImageError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04b4f9bd-d52d-46b2-9ef2-d9ee3e545f83
📒 Files selected for processing (5)
frontend/app/[locale]/admin/shop/products/_components/ProductForm.tsxfrontend/lib/shop/data.tsfrontend/lib/tests/shop/admin-product-photo-management.test.tsfrontend/lib/tests/shop/product-gallery-view-model.test.tsfrontend/lib/tests/shop/product-photo-plan-fixes.test.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/lib/tests/shop/admin-product-photo-management.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/lib/tests/shop/product-gallery-view-model.test.ts
- frontend/lib/tests/shop/product-photo-plan-fixes.test.ts
ViktorSvertoka
approved these changes
Mar 26, 2026
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
2 participants
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.
Description
This PR completes the Phase 2 P0 merch-ready storefront work for product presentation on the Shop PDP.
The main goal of this phase was to make product detail pages feel production-ready for merch:
support multiple product photos, render an interactive primary-first gallery, improve storefront availability messaging, and add localized size guidance near the size selector for apparel products.
Before this work, the storefront and admin flows still depended on a single-image-oriented product contract, PDP image rendering was static, and apparel products had size chips without practical size-help. This PR closes those gaps with a production-grade multi-image foundation, admin photo management, PDP gallery interaction, clearer availability copy, and a localized size-guide entry point.
Related Issue
Issue: #<issue_number>
Changes
Database Changes (if applicable)
How Has This Been Tested?
Validation performed:
Screenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Bug Fixes
Tests