feat: add product-details block #488
Conversation
WalkthroughAdds two new block packages — product-details and recommended-products — with API modules/controllers/services, SDKs, frontend renderer/client/server components, UI components (gallery/carousel), CMS mappers/mock data, tsconfigs, tooling, stories, tests, and app registrations. Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant Renderer as Frontend Renderer
participant ServerComp as Server Component
participant SDK as Block SDK
participant API as Block API (Nest)
participant Service as Block Service
participant CMS as CMS Service
participant Products as Products Service
Browser->>Renderer: request product page (id, locale)
Renderer->>ServerComp: render (id, locale)
ServerComp->>SDK: sdk.blocks.getProductDetails({id}, query, headers)
SDK->>API: HTTP GET /blocks/product-details/{id}
API->>Service: getProductDetails(id, query, headers)
par Parallel fetch
Service->>CMS: getProductDetailsBlock({id, locale})
Service->>Products: getProduct({id})
end
CMS-->>Service: cms block
Products-->>Service: product data
Service->>Service: mapProductDetails(...)
Service-->>API: ProductDetailsBlock / 404
API-->>SDK: JSON response
SDK-->>ServerComp: data
ServerComp->>Renderer: render client with data
Renderer->>Browser: final UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (2)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
…ment and null handling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
3-37: Identical placeholder implementation.This mapper is identical to the Contentful implementation. See the review comment on
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tsfor details.
🧹 Nitpick comments (17)
packages/blocks/product-details/vitest.config.mjs (1)
1-1: LGTM! Standard config re-export pattern.The file correctly imports and re-exports the shared Vitest configuration, which is a common and appropriate pattern for maintaining consistent testing setup across blocks in a monorepo.
Optionally, consider splitting the import and export statements across multiple lines for better readability:
-import { config } from '@o2s/vitest-config/block'; export default config; +import { config } from '@o2s/vitest-config/block'; + +export default config;packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
322-393: Consider aligning image URLs with the established pattern and removing redundancy.The new mock product uses
unsplash.comURLs for images, while all other mock products consistently useraw.githubusercontent.comURLs. External URLs may introduce stability concerns (URL changes, attribution requirements, network availability) and break the established pattern for mock data.Additionally, the
imagefield (lines 330-335) and the first item in theimagesarray (lines 337-342) share the same URL. This redundancy can be eliminated.🔎 Suggested improvements
Option 1: Use consistent githubusercontent.com URLs
Replace unsplash URLs with githubusercontent URLs matching the pattern used by other products.Option 2: Remove image field redundancy
If keeping the current URLs, consider whether bothimageandimages[0]need the same URL. Based on the api-harmonization layer (external_code_snippet_1),imageserves as a fallback whenimagesis undefined, so having both defined with identical content may be unnecessary.Option 3: Verify icon names
Ensure the icon string references inkeySpecs('Calendar', 'CheckCircle', 'Fuel', 'Settings') match the actual icon library being used in the frontend.packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
10-15: Consider simplifying the images type.The conditional type logic for the
imagesfield is complex and may be difficult to maintain. Since the API harmonization layer likely uses a mapper to ensure images are always populated, consider documenting this requirement or simplifying the type.Alternative approach
If the mapper guarantees images are always present, you could simplify to:
export type Product = Products.Model.Product & { badges?: Badge[]; - images: - | NonNullable<Products.Model.Product['images']> - | (Products.Model.Product['image'] extends undefined ? never : [Products.Model.Product['image']]); + images: Media.Media[]; };And ensure the mapper always populates this field from either
imagesorimage.packages/blocks/product-details/src/api-harmonization/product-details.service.spec.ts (1)
1-62: Consider adding behavior tests for service methods.The test setup correctly instantiates the service with mocked dependencies following NestJS testing patterns. However, the tests only verify that services are defined without testing any actual behavior of
ProductDetailsService.Consider adding tests that verify:
- How
ProductDetailsServiceusescmsService.getProductDetailsBlock- How it integrates with
productsServicemethods- Error handling scenarios
- Data transformation logic
This would provide better coverage and confidence in the service implementation.
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
3-18: Placeholder implementation - consider following the mocked CMS pattern for locale support.The mapper currently returns static English labels regardless of locale. When implementing proper Contentful mapping, reference the mocked CMS implementation (
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts) which demonstrates locale-aware label switching.Note: This comment also applies to the identical Strapi implementation in
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts.packages/blocks/product-details/src/sdk/index.ts (1)
16-19: Consider fixing logger type definitions instead of suppressing errors.The
@ts-expect-errordirectives indicate missing type definitions forlevelandformatin the logger configuration. If this is a recurring pattern, consider updating the logger type definitions in@o2s/framework/sdkto include these properties.packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
3-37: Placeholder implementation - identical to Strapi mapper.This mapper is duplicated in
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tswith identical structure. When implementing proper Contentful mapping, consider whether shared stub data could be extracted to reduce duplication.Note: Unlike product-details, there's no locale-aware mocked implementation yet to use as a reference for this block type.
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
3-17: Acknowledge placeholder implementation.The mapper currently returns a hardcoded structure with a TODO for proper Strapi mapping. While this is acceptable for initial integration, ensure the TODO is tracked so that proper CMS field mapping is implemented before production use.
Do you want me to open an issue to track the Strapi mapping implementation?
packages/integrations/strapi-cms/src/modules/cms/cms.service.ts (1)
450-456: Consider adding caching for consistency.Unlike most other block methods in this service (which use
getCachedBlock),getProductListBlockandgetProductDetailsBlockreturn observables directly without caching. This divergence from the established pattern may lead to unnecessary repeated mapper calls.🔎 Proposed refactor to add caching
getProductListBlock(options: CMS.Request.GetCmsEntryParams) { - return of(mapProductListBlock(options.locale)); + const key = `product-list-component-${options.id}-${options.locale}`; + return this.getCachedBlock(key, () => of(mapProductListBlock(options.locale))); } getProductDetailsBlock(options: CMS.Request.GetCmsEntryParams) { - return of(mapProductDetailsBlock(options.locale)); + const key = `product-details-component-${options.id}-${options.locale}`; + return this.getCachedBlock(key, () => of(mapProductDetailsBlock(options.locale))); }packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
32-35: Consider improving error handling by providing user feedback.Returning
nullon error silently hides the component without any user-visible indication. Consider rendering an error state component or using an error boundary pattern to provide feedback.🔎 Suggested improvement
} catch (error) { console.error('Error fetching ProductDetails block', error); - return null; + return ( + <div className="text-destructive p-4"> + Unable to load product details + </div> + ); }packages/blocks/product-details/src/api-harmonization/product-details.service.ts (1)
34-40: Consider fetching more items to ensure 4 popular offers after filtering.Fetching exactly 4 items at
limit: 4may result in fewer than 4 popular offers after filtering out the current product. Consider fetching a few extra (e.g.,limit: 5) to account for the exclusion.🔎 Suggested improvement
const popularOffers = this.productsService.getProductList({ - limit: 4, + limit: 5, offset: 0, locale, });packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (2)
245-245: Address TODO: Recommended products rendering is incomplete.The
popularOfferssection renders a placeholder<h3>TODO: Recommended products</h3>instead of actual product cards. This should be implemented before merging or tracked as a follow-up issue.Would you like me to help implement the recommended products grid, or should I open a new issue to track this task?
219-228: Consider memoizing the action button click handler.If
actionButton.onClickis a new function reference on each render, it could cause unnecessary re-renders. Consider wrapping it withuseCallbackif performance becomes a concern.Also applies to: 270-279
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (1)
38-43: Consider making the action button's icon and variant configurable.The
icon: 'MessageCircle'andvariant: 'default'are hardcoded. If different product types require different icons or styling, consider adding these fields to the CMS labels.packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (1)
106-161: Consider extracting common mock data to constants.The
popularOffersarray and other data structures are duplicated across multiple stories (e.g., lines 106-161, 285-323). While this duplication makes each story self-contained, it increases maintenance burden when product data needs updating.Consider extracting frequently reused data to constants:
const MOCK_POPULAR_OFFERS = [ { id: 'PRD-005', name: 'Cordless Angle Grinder', /* ... */ }, { id: 'PRD-006', name: 'Laser Measurement', /* ... */ }, { id: 'PRD-007', name: 'Cordless Drill Driver', /* ... */ }, ]; const DEFAULT_ROUTING = { locales: ['en', 'pl'], defaultLocale: 'en', pathnames: { /* ... */ }, };Then reference them in stories:
popularOffers: MOCK_POPULAR_OFFERS.packages/ui/src/components/ProductGallery/ProductGallery.tsx (2)
62-80: LGTM: Proper keyboard and scroll handling.The ESC key handling and body scroll locking follow the standard pattern for modal components. The cleanup function correctly restores the original state.
For more robust scroll locking (especially with nested modals or multiple components), consider using a dedicated library like
react-remove-scrollorbody-scroll-lock-upgradethat handles edge cases like iOS Safari and nested locks.
87-128: LGTM: Well-implemented main gallery.The Swiper configuration is comprehensive with proper null checks (line 105 checking both existence and
destroyedstate). The conditional navigation opacity (lines 94-96) provides good UX. Image optimization withpriorityflag and responsivesizesis properly implemented.Line 111 uses array index as key. While acceptable for static image galleries, consider using stable identifiers (e.g.,
image.urlor a unique ID) if images might be reordered dynamically:-<SwiperSlide key={index}> +<SwiperSlide key={image.url}>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
-
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
.gitignoreapps/api-harmonization/package.jsonapps/api-harmonization/src/app.module.tsapps/api-harmonization/src/modules/page/page.model.tsapps/frontend/package.jsonapps/frontend/src/blocks/renderBlocks.tsxpackages/blocks/product-details/.gitignorepackages/blocks/product-details/.prettierrc.mjspackages/blocks/product-details/eslint.config.mjspackages/blocks/product-details/lint-staged.config.mjspackages/blocks/product-details/package.jsonpackages/blocks/product-details/src/api-harmonization/index.tspackages/blocks/product-details/src/api-harmonization/product-details.client.tspackages/blocks/product-details/src/api-harmonization/product-details.controller.tspackages/blocks/product-details/src/api-harmonization/product-details.mapper.tspackages/blocks/product-details/src/api-harmonization/product-details.model.tspackages/blocks/product-details/src/api-harmonization/product-details.module.tspackages/blocks/product-details/src/api-harmonization/product-details.request.tspackages/blocks/product-details/src/api-harmonization/product-details.service.spec.tspackages/blocks/product-details/src/api-harmonization/product-details.service.tspackages/blocks/product-details/src/api-harmonization/product-details.url.tspackages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/blocks/product-details/src/frontend/ProductDetails.renderer.tsxpackages/blocks/product-details/src/frontend/ProductDetails.server.tsxpackages/blocks/product-details/src/frontend/ProductDetails.types.tspackages/blocks/product-details/src/frontend/index.tspackages/blocks/product-details/src/sdk/index.tspackages/blocks/product-details/src/sdk/product-details.tspackages/blocks/product-details/tsconfig.api.jsonpackages/blocks/product-details/tsconfig.frontend.jsonpackages/blocks/product-details/tsconfig.jsonpackages/blocks/product-details/tsconfig.sdk.jsonpackages/blocks/product-details/vitest.config.mjspackages/framework/src/modules/cms/cms.model.tspackages/framework/src/modules/cms/cms.service.tspackages/framework/src/modules/cms/models/blocks/product-details.model.tspackages/framework/src/modules/products/products.model.tspackages/integrations/contentful-cms/src/modules/cms/cms.service.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tspackages/integrations/mocked/src/modules/cms/cms.service.tspackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/mocked/src/modules/products/products.mapper.tspackages/integrations/strapi-cms/src/modules/cms/cms.service.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tspackages/ui/src/components/ProductGallery/ProductGallery.stories.tsxpackages/ui/src/components/ProductGallery/ProductGallery.tsxpackages/ui/src/components/ProductGallery/ProductGallery.types.tspackages/ui/src/components/ProductGallery/index.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/blocks/product-details/src/api-harmonization/product-details.request.tspackages/framework/src/modules/cms/cms.service.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tspackages/blocks/product-details/tsconfig.sdk.jsonpackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/blocks/product-details/package.jsonpackages/integrations/contentful-cms/src/modules/cms/cms.service.tsapps/frontend/src/blocks/renderBlocks.tsxpackages/blocks/product-details/src/api-harmonization/product-details.mapper.tsapps/api-harmonization/src/modules/page/page.model.tspackages/framework/src/modules/cms/models/blocks/product-details.model.tspackages/blocks/product-details/src/frontend/ProductDetails.types.tspackages/framework/src/modules/cms/cms.model.tspackages/blocks/product-details/tsconfig.api.jsonpackages/integrations/mocked/src/modules/cms/cms.service.tspackages/blocks/product-details/src/api-harmonization/product-details.model.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.tspackages/framework/src/modules/products/products.model.tspackages/blocks/product-details/tsconfig.jsonpackages/integrations/strapi-cms/src/modules/cms/cms.service.ts
📚 Learning: 2025-11-14T14:34:25.489Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: packages/blocks/ticket-summary/eslint.config.mjs:1-18
Timestamp: 2025-11-14T14:34:25.489Z
Learning: ESLint v9+ provides `defineConfig` as an export from 'eslint/config' module, which supports an `extends` array property in flat config objects for composing configurations.
Applied to files:
packages/blocks/product-details/lint-staged.config.mjspackages/blocks/product-details/.prettierrc.mjspackages/blocks/product-details/eslint.config.mjs
🧬 Code graph analysis (20)
packages/framework/src/modules/cms/cms.service.ts (3)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(45-51)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(13-16)
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (4)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(42-53)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(45-51)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(13-16)
packages/ui/src/components/ProductGallery/ProductGallery.types.ts (1)
packages/ui/src/components/ProductGallery/index.ts (2)
ImageItem(2-2)ProductGalleryProps(2-2)
packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
mapProductListBlock(3-37)
packages/blocks/product-details/lint-staged.config.mjs (1)
packages/blocks/product-details/.prettierrc.mjs (1)
config(8-23)
packages/integrations/contentful-cms/src/modules/cms/cms.service.ts (5)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
mapProductListBlock(3-37)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
mapProductListBlock(3-37)packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)
packages/ui/src/components/ProductGallery/ProductGallery.tsx (3)
packages/ui/src/components/ProductGallery/index.ts (2)
ProductGallery(1-1)ProductGalleryProps(2-2)packages/ui/src/components/ProductGallery/ProductGallery.types.ts (1)
ProductGalleryProps(8-16)packages/ui/src/lib/utils.ts (1)
cn(5-7)
packages/blocks/product-details/src/sdk/index.ts (2)
packages/framework/src/sdk.ts (1)
extendSdk(140-148)packages/blocks/product-details/src/sdk/product-details.ts (1)
productDetails(8-31)
apps/frontend/src/blocks/renderBlocks.tsx (2)
packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
ProductDetails(12-36)packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetails(2-2)
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (2)
packages/framework/src/modules/cms/models/blocks/product-details.model.ts (2)
ProductDetailsBlock(13-16)Labels(3-11)apps/api-harmonization/src/modules/page/page.model.ts (1)
Labels(43-43)
packages/framework/src/modules/cms/models/blocks/product-details.model.ts (2)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (2)
Labels(35-43)ProductDetailsBlock(45-51)apps/api-harmonization/src/modules/page/page.model.ts (1)
Labels(43-43)
packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (4)
packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetails(2-2)packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsProps(5-12)packages/blocks/product-details/src/sdk/index.ts (1)
sdk(24-28)apps/frontend/src/i18n/routing.ts (1)
routing(8-18)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (2)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(45-51)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(13-16)
packages/framework/src/modules/cms/cms.model.ts (2)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
ProductDetailsBlock(45-51)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
ProductDetailsBlock(13-16)
packages/integrations/mocked/src/modules/products/products.mapper.ts (2)
packages/framework/src/modules/products/products.model.ts (2)
Products(43-43)Product(19-41)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Product(10-15)
packages/integrations/mocked/src/modules/cms/cms.service.ts (5)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(42-53)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)packages/integrations/mocked/src/utils/delay.ts (1)
responseDelay(4-7)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (3)
packages/framework/src/modules/products/products.model.ts (2)
Product(19-41)Products(43-43)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (2)
Labels(3-11)ProductDetailsBlock(13-16)apps/api-harmonization/src/modules/page/page.model.ts (1)
Labels(43-43)
packages/framework/src/modules/products/products.model.ts (2)
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Product(10-15)packages/framework/src/utils/models/media.ts (1)
Media(1-7)
packages/integrations/strapi-cms/src/modules/cms/cms.service.ts (3)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
mapProductListBlock(3-37)packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
mapProductDetailsBlock(3-18)
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (2)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
ProductDetailsPure(19-289)packages/blocks/product-details/src/frontend/index.ts (1)
ProductDetailsPure(1-1)
🔇 Additional comments (61)
.gitignore (1)
36-36: LGTM!The addition of "agent-os" is appropriately placed alongside other IDE/editor exclusions and follows the existing .gitignore conventions.
packages/blocks/product-details/.gitignore (1)
1-57: LGTM!This is a well-structured and comprehensive .gitignore file with all standard patterns for a TypeScript/Node.js package. The selective VSCode allowlist (lines 33-36) is a good practice to preserve shared team configs. No concerns.
packages/blocks/product-details/eslint.config.mjs (1)
1-18: LGTM! Clean ESLint configuration following established patterns.The configuration correctly applies layer-specific linting rules: API config for the harmonization layer and frontend config for both the UI and SDK layers. The structure aligns with the project's ESLint v9+ flat config conventions.
Based on learnings, the
defineConfigwithextendsarrays is the correct approach for this repository.packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
407-407: LGTM!The array extension correctly includes the new mock product and will work seamlessly with the existing mapping functions.
packages/blocks/product-details/.prettierrc.mjs (1)
1-25: LGTM! Well-structured Prettier configuration for multi-layer architecture.The configuration correctly applies layer-specific formatting rules using overrides. This aligns well with the product-details block's architecture spanning API harmonization, frontend, and SDK layers.
packages/blocks/product-details/src/api-harmonization/product-details.url.ts (1)
1-1: LGTM!The URL constant follows the established pattern for block endpoints and will be correctly consumed by the client, SDK, and controller layers.
packages/blocks/product-details/lint-staged.config.mjs (1)
1-3: LGTM!The configuration correctly imports and re-exports the shared base lint-staged config, maintaining consistency with other blocks in the repository.
packages/blocks/product-details/tsconfig.api.json (1)
1-14: LGTM!The TypeScript configuration correctly extends the base API config and enables the necessary decorator support for NestJS-based API harmonization modules.
packages/blocks/product-details/tsconfig.sdk.json (1)
1-20: LGTM!The SDK configuration correctly includes both the SDK source files and the specific API harmonization dependencies it requires (client, model, request, url).
packages/blocks/product-details/tsconfig.frontend.json (1)
1-23: LGTM!The frontend configuration properly extends the base frontend config and includes the necessary API harmonization and SDK dependencies for type-safe frontend development.
packages/blocks/product-details/tsconfig.json (1)
1-18: LGTM!The main TypeScript configuration correctly orchestrates the three specialized build configurations (frontend, API, SDK) using project references for efficient multi-project builds.
packages/framework/src/modules/products/products.model.ts (2)
8-17: LGTM!The new type definitions for key specifications and detailed specifications are clear and well-structured, providing the necessary flexibility for product detail rendering.
25-40: LGTM!The Product model extensions are well-designed with all new fields being optional, ensuring backward compatibility while supporting the enhanced product details block functionality.
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (4)
5-8: LGTM!The Badge type provides a clean interface for product badges with appropriate variant options.
17-25: LGTM!The ProductSummary type provides an appropriate simplified view of product data for card/list displays.
35-43: LGTM!The Labels type provides comprehensive localization support for the product details block UI.
45-51: LGTM!The ProductDetailsBlock correctly includes the
__typenamediscriminator at the API harmonization layer, consistent with the established architectural pattern.Based on learnings, the
__typenamefield is appropriately placed in the API harmonization layer for discriminated unions.apps/api-harmonization/package.json (1)
49-49: LGTM!The dependency addition follows the workspace protocol pattern used by all other block packages in this file.
packages/framework/src/modules/cms/cms.model.ts (1)
47-47: LGTM!The export alias follows the established pattern for CMS block models. The framework layer correctly maintains normalized data models without
__typenamediscriminators, which are added at the API harmonization layer as confirmed by the relevant code snippets.Based on learnings, the framework layer correctly excludes
__typenamewhich is added inpackages/blocks/product-details/src/api-harmonization/product-details.model.ts.apps/frontend/src/blocks/renderBlocks.tsx (2)
20-20: LGTM!The import follows the established pattern for block imports in this file.
146-147: LGTM!The rendering case follows the established pattern used by other blocks in this switch statement. The ProductDetails.Renderer component is properly integrated with error handling in place at the server component level.
packages/blocks/product-details/src/api-harmonization/product-details.module.ts (1)
1-20: LGTM!The module structure follows NestJS best practices. The provider alias pattern (lines 13-16) correctly maps the
Products.Servicetoken to the framework implementation, enabling dependency injection flexibility while maintaining a clean module boundary.apps/api-harmonization/src/modules/page/page.model.ts (2)
19-19: LGTM!The import statement follows the established pattern for block API harmonization imports.
82-82: LGTM!The addition to the
Blocksunion type follows the established pattern. TheProductDetailsBlock['__typename']correctly extracts the type discriminator for the union, consistent with other block types in this discriminated union.apps/api-harmonization/src/app.module.ts (1)
43-43: LGTM!The import statement follows the established pattern for block imports.
packages/blocks/product-details/src/sdk/index.ts (1)
9-11: Verify API_URL environment variable handling.The API_URL resolution uses a non-null assertion (line 14:
apiUrl: API_URL!), which will cause runtime errors if all three environment variables are undefined. Ensure that at leastNEXT_PUBLIC_API_URLis always defined in your environment configuration.packages/blocks/product-details/src/api-harmonization/product-details.request.ts (1)
1-9: Verify the need foridin both Params and Query types.Both
GetProductDetailsBlockParamsandGetProductDetailsBlockQueryinclude anidfield. Since the params are typically used for URL path segments (as seen in the SDK implementation:url: \${URL}/${params.id}`), havingidin the query type appears redundant. Confirm whether the queryid` serves a distinct purpose or should be removed.packages/integrations/contentful-cms/src/modules/cms/cms.service.ts (1)
561-567: LGTM! Consistent with existing block patterns.The new methods follow the established pattern used by other block getters in this service (e.g.,
getServiceDetailsBlock,getResourceDetailsBlock). The implementation correctly delegates to the respective mapper functions and returns an Observable.packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts (1)
42-53: LGTM! Good reference implementation for locale handling.This mapper demonstrates the proper pattern for locale-aware block mapping with appropriate fallback to English. The implementation can serve as a reference for the Contentful and Strapi mappers when they implement proper CMS integration.
packages/blocks/product-details/src/api-harmonization/index.ts (1)
1-7: LGTM! Standard barrel export pattern.The index file provides a clean public API surface for the product-details block's API harmonization layer. All necessary exports are present and follow established conventions.
packages/integrations/mocked/src/modules/cms/cms.service.ts (1)
29-29: LGTM! Consistent implementation.The import and method implementation follow the established pattern for other block types in this service. The method correctly delegates to the mapper and applies
responseDelay()for mock behavior.Also applies to: 211-213
packages/blocks/product-details/src/frontend/index.ts (1)
1-3: LGTM! Clean public API surface.The barrel export pattern with semantic aliases (Client/Server/Renderer) provides a clear and stable public API for consumers. This approach maintains good separation of concerns between client-side, server-side, and rendering logic.
packages/blocks/product-details/src/api-harmonization/product-details.controller.ts (2)
14-25: LGTM! Well-structured NestJS controller.The controller follows NestJS best practices with proper dependency injection, decorators, and delegation to the service layer. The implementation is consistent with other block controllers in the codebase.
17-18: Empty roles array is consistent with the codebase authorization pattern.The empty roles array in the product-details endpoint is intentional. Multiple similar endpoints across the blocks use the same pattern (product-list, article, category, quick-links, faq, etc.), distinguishing "public" endpoints from restricted ones that require specific roles like ORG_USER or ORG_ADMIN. This endpoint correctly follows the established design.
packages/framework/src/modules/cms/models/blocks/product-details.model.ts (1)
1-16: LGTM! Correct framework layer implementation.The model correctly follows the framework layer pattern by omitting the
__typenamediscriminator (which belongs in the API harmonization layer). The structure with requiredlabelsfield and optionaltitlefield aligns with the established pattern for block models.Based on learnings: Framework layer models should not include
__typename.packages/blocks/product-details/src/frontend/ProductDetails.renderer.tsx (1)
1-41: LGTM! Well-structured renderer component.The renderer properly implements:
- Locale resolution with fallback (
propLocale || localeFromHook)- Suspense boundary with informative loading fallback
- Clean prop forwarding to the underlying
ProductDetailscomponentThe implementation follows React best practices and provides good user experience during loading states.
packages/blocks/product-details/src/api-harmonization/product-details.client.ts (1)
1-4: LGTM! Clean barrel export pattern.The file establishes a clear public API surface by aggregating related exports into logical namespaces (Model, Request) and re-exporting the URL constant. This approach provides a clean interface for API consumers.
packages/blocks/product-details/src/sdk/product-details.ts (1)
8-31: LGTM!The SDK wrapper follows the established patterns in the codebase. The header merging order is correct (defaults → custom → auth), and the conditional authorization header is properly structured.
packages/framework/src/modules/cms/cms.service.ts (1)
131-138: LGTM!The new abstract methods follow the established pattern of existing block methods in the service. Based on learnings, the framework layer correctly returns
CMS.Model.*types without__typenamediscriminators, which are added at the API harmonization layer.packages/blocks/product-details/src/frontend/ProductDetails.server.tsx (1)
27-31: Note: Suspense here wraps the dynamic import, not data fetching.Since data is fetched with
awaitbefore rendering, theSuspensefallback only appears during the dynamic component chunk load, not during data fetching. This is correct but worth noting for clarity.packages/blocks/product-details/src/api-harmonization/product-details.service.ts (1)
41-77: LGTM on the overall service structure.The use of
forkJoinfor parallel data fetching is appropriate, and the conditional branching for popular offers is clean. TheNotFoundExceptionhandling ensures proper HTTP responses when products are missing.packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
1-289: Overall structure is solid, but the component is large.The component handles responsive layouts well with proper breakpoint handling. Consider extracting sub-components (e.g.,
ProductCarousel,ProductSpecs,MobileCTA) in a future refactor to improve maintainability.packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (2)
33-45: LGTM! The__typenamediscriminator is correctly added at the API harmonization layer.Based on learnings, the framework layer maintains normalized models without
__typename, and it's appropriately added here in the API harmonization mapper for discriminated union support.
14-18: Same type cast concern as in the service.The
tag.variant as Model.Badge['variant']cast here mirrors the same pattern in the service. Ensure the source data'svariantvalues align with the expected union type.packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
1-23: LGTM!The type definitions are well-structured. Good use of intersection types for
ProductDetailsPurePropsto combine component props with the API model, and appropriate use ofReturnType<typeof defineRouting>for type-safe routing.packages/blocks/product-details/package.json (1)
43-43: No action required — TypeScript version is current.TypeScript 5.9.3 is consistent with the current stable release (TypeScript 5.9 as of December 2025) and is intentional.
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (2)
1-12: LGTM! Well-structured Storybook setup.The imports, meta configuration, and type definitions follow Storybook best practices with proper type safety using the
satisfiesoperator.
182-428: Excellent story coverage!The story variants effectively demonstrate different component states:
- Discount scenarios with promotional badges
- Locale variations
- Optional features (popularOffers, actionButton)
- Minimal data edge cases
- Different button variants (link vs. onClick, variant styles)
The use of
console.login onClick handlers (lines 167, 329, 417) is appropriate for Storybook demonstrations, allowing developers to verify interactions in the browser console.packages/ui/src/components/ProductGallery/ProductGallery.stories.tsx (3)
1-17: LGTM: Clean Storybook setup.The imports, meta configuration, and type definitions follow Storybook best practices. The use of
satisfiesensures type safety while maintaining type inference.
19-49: LGTM: Well-organized sample data.The sample image generation using color-coded placeholders provides clear visual differentiation. The data sets effectively cover various gallery scenarios (single, few, many images).
51-96: LGTM: Comprehensive story coverage.The stories effectively demonstrate the component's flexibility across different scenarios (default, many images, without thumbnails, single image). The JSDoc comments clearly explain each story's purpose and expected behavior.
packages/ui/src/components/ProductGallery/index.ts (1)
1-2: LGTM: Clean barrel exports.The barrel export pattern is correctly implemented, properly separating type exports from value exports for optimal tree-shaking.
packages/ui/src/components/ProductGallery/ProductGallery.types.ts (1)
1-16: LGTM: Well-designed type definitions.The interfaces are clean and properly typed. Extending
Omit<SwiperProps, 'children'>is the right approach since the component manages its own children. ThehasPriorityprop supports image loading optimization, which is a good performance practice.packages/ui/src/components/ProductGallery/ProductGallery.tsx (8)
1-28: LGTM: Proper client component setup.The component is correctly configured as a client component with appropriate imports and default prop values. The use of
Readonly<ProductGalleryProps>enforces immutability as expected in React.
29-34: LGTM: Clean state management.The state variables appropriately track the main gallery, thumbnails, and lightbox Swiper instances. Proper null handling is set up for Swiper instance types.
36-45: LGTM: Efficient module configuration.The use of
useMemowith conditional module inclusion is the right approach. Always enabling A11y and Keyboard modules ensures good accessibility support.
51-60: LGTM: Well-optimized callback handlers.Both handlers properly use
useCallbackwith correct dependencies. The null check formainSwiperinhandleOpenLightboxprevents potential errors.
82-85: LGTM: Proper guard clause.The early return for empty or missing images prevents unnecessary rendering and potential errors.
130-163: LGTM: Clean thumbnail implementation.The thumbnail gallery correctly renders only when needed (multiple images and thumbnails enabled). The hover interaction and active state styling provide good visual feedback.
165-249: LGTM: Well-structured lightbox implementation.The lightbox overlay properly handles user interactions:
- Click-to-close on overlay (line 169)
- Click event propagation stopped on content (line 181)
- Close button with proper ARIA label (line 175)
- Synchronized
initialSlidefrom main gallery (line 190)- Proper null checking for thumbs Swiper (line 194)
The implementation provides a smooth fullscreen viewing experience with keyboard navigation support.
47-49: No action needed—slideToLoop()is the correct Swiper API method for this use case.The Swiper v12 API documentation confirms that
slideToLoop(index, speed)is the standard method specifically designed for loop mode navigation. It jumps to the slide whose realIndex matches the passed index and handles duplicated looped slides automatically. The second parameter (0) correctly specifies instant navigation (0ms duration). This is the appropriate method choice here and no changes are required.Likely an incorrect or invalid review comment.
packages/blocks/product-details/src/api-harmonization/product-details.model.ts
Show resolved
Hide resolved
packages/blocks/product-details/src/api-harmonization/product-details.service.ts
Outdated
Show resolved
Hide resolved
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx
Show resolved
Hide resolved
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
40-42: Accessibility concern: Duplicate<h1>elements still present.This mobile
<h1>(line 41) and the desktop<h1>(line 140) both exist in the DOM simultaneously, violating accessibility guidelines. Consider using a single<h1>with responsive styling, or applyingaria-hidden="true"to the visually hidden duplicate.
🧹 Nitpick comments (5)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (5)
113-133: Consider making icon names configurable.The icon names "FileText" (line 119) and "MapPin" (line 127) are hardcoded. If these may need to vary by tenant or configuration, consider moving them to the labels or configuration object.
166-199: Consider extracting action button rendering to reduce duplication.The action button rendering logic (href vs onClick, icon handling) is duplicated between the desktop sidebar (lines 170-196) and mobile footer (lines 227-244). Consider extracting this into a helper component or render function.
🔎 Example refactor approach
Create a helper function above the component return:
const renderActionButton = (mobile = false) => { if (!actionButton) return null; const buttonSize = mobile ? 'default' : 'lg'; const showIcon = !mobile && actionButton.icon; const buttonContent = ( <> {showIcon && <DynamicIcon name={actionButton.icon} size={20} className="mr-2" />} {actionButton.label} </> ); const buttonProps = { variant: actionButton.variant || 'default', size: buttonSize, className: 'w-full', }; if (actionButton.href) { return ( <Button {...buttonProps} asChild> <LinkComponent href={actionButton.href}> {buttonContent} </LinkComponent> </Button> ); } return ( <Button {...buttonProps} onClick={actionButton.onClick}> {buttonContent} </Button> ); };Then use
{renderActionButton()}in desktop and{renderActionButton(true)}in mobile.
204-215: TODO: Recommended products section is incomplete.The popular offers section is scaffolded but shows a TODO placeholder (line 212). The
popularOffersdata is available but not rendered.Would you like me to generate an implementation for the recommended products section, or should I open an issue to track this task?
220-220: Asymmetric margin on mobile footer content.The footer content uses
ml-auto mr-4, creating asymmetric horizontal margins. If this is intentional for visual alignment, consider adding a comment. Otherwise, consider usingmx-autoor symmetric padding.
249-249: Magic number: Spacer height should match footer height.The spacer uses
h-32(8rem/128px) to prevent content from being hidden by the fixed footer. Consider using a CSS variable or constant that matches the actual footer height to maintain consistency if the footer size changes.🔎 Potential approach
Define a constant at the top of the file:
const MOBILE_FOOTER_HEIGHT = 'h-32'; // or 128px if using inline stylesThen reference it for both the spacer calculation and potentially for calculating the footer's actual height, or document why h-32 was chosen.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (5)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsPureProps(14-14)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Badge(5-8)packages/ui/src/components/DynamicIcon/DynamicIcon.tsx (1)
DynamicIcon(8-42)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)packages/framework/src/utils/models/price.ts (1)
Price(3-7)
⏰ 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: deploy-preview
🔇 Additional comments (3)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (3)
62-81: LGTM: Key specs section.The conditional rendering, responsive grid layout, and optional icon/value handling are well-implemented. The DynamicIcon component properly handles missing icons by returning null.
83-111: LGTM: Description and specifications sections.Both sections use proper conditional rendering, semantic HTML headings, and responsive layouts. The RichText component appropriately handles the description content.
159-164: Price component already gracefully handles missing or invalid price data.The Price component includes explicit null/undefined handling (returns
nullwhen price is falsy), properly types the price prop as optional, and the usePriceService includes fallback mechanisms for edge cases like unknown currencies. No changes are needed.
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (1)
66-68: Consider: className relies on Swiper internal DOM structure.The className uses deeply nested Swiper-specific selectors (
[&_.swiper-slide],[&_.swiper-pagination-bullet], etc.) which couples the styling to Swiper's internal DOM structure. If the Carousel component changes its underlying implementation or Swiper updates its markup, these selectors may break.Consider either:
- Exposing styling props on the Carousel component itself for better encapsulation
- Documenting this dependency on Swiper's DOM structure
- Adding integration tests to catch breaking changes in the Carousel/Swiper structure
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.types.tspackages/ui/src/components/ProductCarousel/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsx (2)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (1)
ProductCarousel(15-88)packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (1)
ProductSummaryItem(19-27)
⏰ 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: deploy-preview
🔇 Additional comments (6)
packages/ui/src/components/ProductCarousel/index.ts (1)
1-2: LGTM! Clean barrel exports.The index file follows standard patterns by re-exporting both the component and its public types, enabling consumers to import from a single entry point.
packages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsx (1)
11-16: LGTM! MockLinkComponent is appropriate for stories.The mock implementation correctly satisfies the LinkComponent interface for Storybook demonstration purposes.
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (2)
25-27: LGTM! Appropriate early return for edge case.The guard clause correctly handles the empty products scenario, preventing unnecessary rendering.
29-42: LGTM! Clean conditional rendering logic.The header section renders appropriately based on the presence of title, description, or action, with responsive layout support.
packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (2)
19-27: LGTM! Well-structured ProductSummaryItem interface.The interface appropriately defines required fields (id, name, link) and optional fields (description, image, price, badges) for product summary data.
29-32: LGTM! ProductBadge interface is clean and well-typed.The variant options align with common UI badge patterns and the interface is appropriately simple.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (1)
16-16: Remove__typenamefrom story args (duplicate concern).As previously flagged, the
__typenamefield should not be included in story args for a UI component. Based on learnings,__typenameis a discriminator used at the API harmonization layer, not in frontend component props.🔎 Suggested fix
export const Default: Story = { args: { - __typename: 'ProductDetailsBlock', id: 'PRD-015',Based on learnings,
__typenamediscriminators are added at the API harmonization layer, not in framework or frontend UI components.packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (1)
84-84: Reminder: carouselConfig can still override critical props.This issue was previously flagged but remains unfixed. The
carouselConfigspread after critical props allows users to overrideslides,slidesPerView,breakpoints, and other essential configuration, potentially breaking the component.Please refer to the previous review comment on lines 45-85 for detailed solutions (either reorder the spread or restrict the type with
Omit).
🧹 Nitpick comments (2)
packages/blocks/product-details/src/api-harmonization/product-details.service.spec.ts (1)
52-62: Consider adding behavioral tests beyond existence checks.The current tests only verify that the service and its dependencies are defined. While this confirms the module wiring is correct, it doesn't validate the actual behavior of
ProductDetailsService.Consider adding tests that:
- Call the
getProductDetailsmethod with sample inputs- Verify that CMS and Products services are invoked with correct parameters
- Test error scenarios (e.g., product not found, CMS service failures)
- Validate data transformation and mapping logic
Would you like me to generate sample test cases for the
getProductDetailsmethod?packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (1)
39-45: Hardcoded actionButton properties.The
actionButtonuses hardcoded values forvariant('default') andicon('MessageCircle'). While this may be intentional for consistency, consider whether these should be configurable via the CMS to support different use cases or localization needs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/blocks/product-details/src/api-harmonization/product-details.mapper.tspackages/blocks/product-details/src/api-harmonization/product-details.model.tspackages/blocks/product-details/src/api-harmonization/product-details.service.spec.tspackages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/framework/src/modules/cms/models/blocks/product-details.model.tspackages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.tspackages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
- packages/framework/src/modules/cms/models/blocks/product-details.model.ts
- packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts
- packages/integrations/strapi-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
- packages/ui/src/components/ProductCarousel/ProductCarousel.stories.tsx
- packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
- packages/integrations/contentful-cms/src/modules/cms/mappers/blocks/cms.product-details.mapper.ts
- packages/blocks/product-details/src/api-harmonization/product-details.model.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/product-details/src/api-harmonization/product-details.mapper.ts
📚 Learning: 2025-11-28T14:07:04.474Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/blocks/product-list/src/frontend/ProductList.client.stories.tsx:15-18
Timestamp: 2025-11-28T14:07:04.474Z
Learning: Mock JWT tokens with placeholder data (e.g., Jane Doe, Acme Corporation) used in Storybook story files (.stories.tsx) for demo/test purposes are acceptable and do not need to be replaced with non-JWT placeholders.
Applied to files:
packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx
🧬 Code graph analysis (2)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (4)
packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (1)
ProductCarouselProps(8-17)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (1)
ProductCard(16-94)packages/ui/src/lib/utils.ts (1)
cn(5-7)
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (4)
packages/framework/src/modules/products/products.model.ts (2)
Products(43-43)Product(19-41)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (5)
Product(10-15)ProductSummary(17-25)ProductDetailsBlock(46-52)Badge(5-8)Labels(35-44)packages/framework/src/modules/cms/models/blocks/product-details.model.ts (2)
ProductDetailsBlock(14-17)Labels(3-12)apps/api-harmonization/src/modules/page/page.model.ts (1)
Labels(43-43)
⏰ 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). (2)
- GitHub Check: deploy-preview
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/blocks/product-details/src/api-harmonization/product-details.service.spec.ts (2)
1-6: LGTM! Imports are appropriate.The imports are well-organized and include all necessary testing utilities for NestJS with Vitest.
8-50: Test setup is well-structured.The testing module configuration properly mocks the CMS.Service and Products.Service dependencies. The mock data for
getProductDetailsBlockincludes comprehensive label properties.Note: The
Products.Servicemocks (getProduct,getProductList) currently have no return values. If you add behavioral tests that invoke these methods, you'll need to define appropriate return values using.mockReturnValue()or.mockResolvedValue().packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx (3)
1-12: LGTM! Standard Storybook setup.The imports and meta configuration follow Storybook best practices with proper TypeScript typing.
203-215: LGTM! Good story variant demonstrating secondary button.The story correctly extends the Default args and demonstrates the secondary button variant. The console.log in the onClick handler is appropriate for Storybook demonstrations.
217-222: LGTM! Good story variant demonstrating optional button state.The story correctly demonstrates the case where no action button is provided by explicitly setting it to
undefined.packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (1)
1-27: LGTM - Clean component setup and guard clause.The imports, prop destructuring, and early return for empty products are well implemented.
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (2)
1-9: LGTM!The imports and function signature are well-structured and correctly typed.
23-32: No issue found. The CMS model's type system guarantees required fields.The
Labelstype defines four required fields, and the mapper correctly assumes they're provided. The CMS model declarationlabels!: Labels(with definite assignment assertion) guarantees that thelabelsproperty always contains a fully formedLabelsobject with all required fields populated. Direct field access without null checks is appropriate here and is enforced by TypeScript's type system.
packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/blocks/product-details/src/frontend/ProductDetails.client.tsx:
- Around line 18-27: ProductDetailsPure currently destructures product from
component and immediately accesses product.keySpecs, which will throw if product
is null/undefined; update the component to defensively handle a missing product
by either returning early (e.g., if (!product) return null or a placeholder) or
by using optional chaining/defaults (e.g., const keySpecs = product?.keySpecs ??
[]), and ensure any other accesses to product within ProductDetailsPure are
similarly guarded or use a safe fallback.
- Around line 202-203: In ProductDetails.client.tsx update the bottom fixed
div's utility classes: replace the invalid Tailwind class "z-100" with a valid
scale value like "z-50", and change the inner container's "max-w-7xl ml-auto
mr-4" to symmetric spacing such as "max-w-7xl mx-auto px-4" to ensure consistent
centering and horizontal padding.
🧹 Nitpick comments (4)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (4)
47-55: Consider using stable keys for badge rendering.The badges are mapped using
indexas the key. While this works if the badge array is static, consider using a more stable identifier (e.g.,badge.idor a combination ofbadge.labelandbadge.variant) if badges can be reordered or modified dynamically.♻️ Alternative approach with stable keys
If badges have unique labels:
- {product.badges.map((badge, index) => ( - <li key={index}> + {product.badges.map((badge) => ( + <li key={`${badge.variant}-${badge.label}`}> <Badge variant={badge.variant}>{badge.label}</Badge> </li> ))}
60-79: Consider using stable keys for spec items.Similar to the badges, key specs are mapped using
indexas the key. If specs can be dynamically modified, consider using a more stable identifier.
162-195: Consider extracting action button rendering logic.The action button rendering logic is duplicated between the desktop sidebar (lines 162-195) and mobile bottom bar (lines 200-234). While the mobile version intentionally omits the icon, the core href/onClick logic is repeated.
♻️ Proposed refactor: Extract common logic
Consider creating a helper function or component:
const renderActionButton = (options: { actionButton: typeof actionButton; LinkComponent: typeof LinkComponent; variant?: string; size: 'lg' | 'default'; showIcon?: boolean; className?: string; }) => { const { actionButton, LinkComponent, variant, size, showIcon = true, className } = options; if (!actionButton) return null; const buttonContent = ( <> {showIcon && actionButton.icon && ( <DynamicIcon name={actionButton.icon} size={20} className="mr-2" /> )} {actionButton.label} </> ); const buttonProps = { variant: actionButton.variant || variant || 'default', size, className, }; if (actionButton.href) { return ( <Button {...buttonProps} asChild> <LinkComponent href={actionButton.href}>{buttonContent}</LinkComponent> </Button> ); } return ( <Button {...buttonProps} onClick={actionButton.onClick}> {buttonContent} </Button> ); };Then use it in both places with different options.
Also applies to: 200-234
232-232: Consider matching spacer height to actual bottom bar height.The fixed spacer height of
h-32(8rem/128px) may not match the actual height of the mobile bottom bar, which varies based on its content (price display + button + padding). This could result in either excessive whitespace or content being hidden.💡 Alternative approaches
Option 1: Calculate the actual height and use a CSS variable:
// Use a ref to measure the bar height dynamically const bottomBarRef = useRef<HTMLDivElement>(null); const [barHeight, setBarHeight] = useState(0); useEffect(() => { if (bottomBarRef.current) { setBarHeight(bottomBarRef.current.offsetHeight); } }, [actionButton]);Option 2: Use
pb-32(padding-bottom) on the parent container instead of a separate spacer div, which is more semantic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
⏰ 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). (2)
- GitHub Check: deploy-preview
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (3)
1-16: LGTM!The imports are well-organized and appropriate for a client-side React component. The 'use client' directive is correctly placed at the top of the file.
81-109: LGTM!The description and detailed specifications sections are well-structured with proper conditional rendering and semantic HTML usage. The
asChildpattern ensures proper heading hierarchy for accessibility.
18-237: Overall component structure is solid.The component demonstrates good practices:
- Semantic HTML with proper list elements (
ul/li) for accessibility- Responsive design with appropriate mobile and desktop layouts
- Proper use of
hasPriorityprop for LCP optimization via gallery preloading- Type-safe with TypeScript
The main areas for improvement have been flagged in separate comments above.
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/blocks/product-details/src/frontend/ProductDetails.client.tsx:
- Line 188: The Tailwind class "z-100" on the mobile footer div generates no CSS
because the project doesn't define that utility; replace it with the arbitrary
value syntax "z-[100]" in the same class string on the div (the element
rendering in ProductDetails.client.tsx that has className "lg:hidden fixed
bottom-0 left-0 right-0 bg-background border-t border-border p-4 shadow-lg
z-100") so the element receives the intended z-index.
🧹 Nitpick comments (8)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (3)
47-47: Consider reducing the bottom margin.The
mb-12(3rem/48px) creates significant space below each product card, likely to prevent pagination bullets from overlapping. Consider whether this spacing could be reduced or managed through padding on the carousel container for a more compact layout.Alternative approach
If the margin is solely for pagination clearance, you might refactor to use carousel container padding instead:
- <div key={product.id} className="h-full mb-12"> + <div key={product.id} className="h-full">And adjust the carousel className:
className={cn( - '[&_.swiper-slide]:h-auto [&_.swiper-pagination]:bottom-0 [&_.swiper-pagination-bullet]:bg-primary [&_.swiper-pagination-bullet-active]:bg-primary [&_.swiper-pagination-bullet]:opacity-100 [&_.swiper-pagination-bullet]:w-2.5 [&_.swiper-pagination-bullet]:h-2.5', + '[&_.swiper-slide]:h-auto [&_.swiper]:pb-12 [&_.swiper-pagination]:bottom-0 [&_.swiper-pagination-bullet]:bg-primary [&_.swiper-pagination-bullet-active]:bg-primary [&_.swiper-pagination-bullet]:opacity-100 [&_.swiper-pagination-bullet]:w-2.5 [&_.swiper-pagination-bullet]:h-2.5', carouselClassName, )}However, verify this spacing is appropriate for your design system before changing.
70-73: Consider extracting the complex className for maintainability.The className string on line 71 contains multiple Swiper-specific selectors that make it difficult to read and modify. Consider extracting this to a constant or breaking it into a more maintainable structure.
Refactoring suggestion
+const SWIPER_PAGINATION_STYLES = [ + '[&_.swiper-slide]:h-auto', + '[&_.swiper-pagination]:bottom-0', + '[&_.swiper-pagination-bullet]:bg-primary', + '[&_.swiper-pagination-bullet-active]:bg-primary', + '[&_.swiper-pagination-bullet]:opacity-100', + '[&_.swiper-pagination-bullet]:w-2.5', + '[&_.swiper-pagination-bullet]:h-2.5', +].join(' '); + export const ProductCarousel: React.FC<ProductCarouselProps> = ({ // ... props }) => { // ... earlier code <Carousel // ... other props className={cn( - '[&_.swiper-slide]:h-auto [&_.swiper-pagination]:bottom-0 [&_.swiper-pagination-bullet]:bg-primary [&_.swiper-pagination-bullet-active]:bg-primary [&_.swiper-pagination-bullet]:opacity-100 [&_.swiper-pagination-bullet]:w-2.5 [&_.swiper-pagination-bullet]:h-2.5', + SWIPER_PAGINATION_STYLES, carouselClassName, )}
66-87: Note: Redundant configuration between base props and breakpoints.Lines 66-67 set
slidesPerView={1}andspaceBetween={16}, which are then overridden by the0breakpoint in lines 75-77. While this works correctly (breakpoints take precedence), it creates redundancy. Consider either removing the base values or documenting that breakpoints override them.Simplification option
Remove the redundant base values since breakpoints cover all screen sizes:
<Carousel slides={...} - slidesPerView={1} - spaceBetween={16} showNavigation={true} showPagination={true}The
breakpointsobject starting at0will handle the default case.packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
24-24:createNavigationis called on every render, which may impact performance.
createNavigation(routing)creates a new navigation context on each render. Consider memoizing this or moving it outside the component ifroutingis stable.♻️ Suggested refactor
export const ProductDetailsPure: React.FC<ProductDetailsPureProps> = ({ locale, routing, hasPriority, ...component }) => { - const { Link: LinkComponent } = createNavigation(routing); + const { Link: LinkComponent } = React.useMemo(() => createNavigation(routing), [routing]); const { product, labels, actionButton } = component;packages/blocks/product-details/src/api-harmonization/product-details.model.ts (2)
10-15: Compleximagestype may not reflect runtime behavior accurately.The conditional type logic on lines 12-14 is hard to reason about. The mapper handles this at runtime with
product.images || (product.image ? [product.image] : []), which can result in an empty array[]— but the current type definition doesn't account for that.Consider simplifying to match actual runtime possibilities:
♻️ Suggested simplification
export type Product = Products.Model.Product & { badges?: Badge[]; - images: - | NonNullable<Products.Model.Product['images']> - | (Products.Model.Product['image'] extends undefined ? never : [Products.Model.Product['image']]); + images: NonNullable<Products.Model.Product['images']> | Products.Model.Product['image'][]; };
5-22: Consider extracting the sharedvariantunion type.
Badge.variantandActionButton.variantuse the same union. Extracting to a shared type would improve maintainability.♻️ Suggested refactor
+export type ButtonVariant = 'default' | 'secondary' | 'destructive' | 'outline'; + export type Badge = { label: string; - variant: 'default' | 'secondary' | 'destructive' | 'outline'; + variant: ButtonVariant; }; // ... export type ActionButton = { label: string; href: string; - variant?: 'default' | 'secondary' | 'destructive' | 'outline'; + variant?: ButtonVariant; icon?: string; };packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (2)
13-17: Unsafe type cast fortag.variantcould cause runtime issues.The cast
as Model.Badge['variant']assumestag.variantis always one of the expected values. If the source data contains an unexpected variant, this could lead to incorrect UI rendering.♻️ Suggested fix with validation
+const VALID_BADGE_VARIANTS = ['default', 'secondary', 'destructive', 'outline'] as const; + +const isValidBadgeVariant = (variant: unknown): variant is Model.Badge['variant'] => + VALID_BADGE_VARIANTS.includes(variant as Model.Badge['variant']); + export const mapProductDetails = ( product: Products.Model.Product, cms: CMS.Model.ProductDetailsBlock.ProductDetailsBlock, ): Model.ProductDetailsBlock => { const mappedProduct: Model.Product = { ...product, images: product.images || (product.image ? [product.image] : []), badges: product.tags?.map((tag) => ({ label: tag.label, - variant: tag.variant as Model.Badge['variant'], + variant: isValidBadgeVariant(tag.variant) ? tag.variant : 'default', })) || [],
35-43: Hardcoded icon'MessageCircle'may limit flexibility.The action button icon is hardcoded. If different product types need different icons, consider making this configurable via CMS labels or product data.
♻️ Optional: Make icon configurable
actionButton: labels.actionButtonLabel && product.link ? { label: labels.actionButtonLabel, href: product.link, variant: 'default', - icon: 'MessageCircle', + icon: cms.labels.actionButtonIcon || 'MessageCircle', } : undefined,This would require adding
actionButtonIcon?: stringto the CMS Labels type.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/blocks/product-details/src/api-harmonization/product-details.mapper.tspackages/blocks/product-details/src/api-harmonization/product-details.model.tspackages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsxpackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/blocks/product-details/src/frontend/ProductDetails.client.stories.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
📚 Learning: 2025-11-26T11:57:00.632Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
Applied to files:
packages/blocks/product-details/src/api-harmonization/product-details.mapper.tspackages/blocks/product-details/src/api-harmonization/product-details.model.ts
🧬 Code graph analysis (2)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (4)
packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (1)
ProductCarouselProps(8-17)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (1)
ProductCard(16-94)packages/ui/src/lib/utils.ts (1)
cn(5-7)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (5)
packages/blocks/product-details/src/frontend/ProductDetails.types.ts (1)
ProductDetailsPureProps(13-13)packages/ui/src/components/ProductGallery/ProductGallery.tsx (1)
ProductGallery(19-252)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Badge(5-8)packages/ui/src/components/DynamicIcon/DynamicIcon.tsx (1)
DynamicIcon(8-42)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)
⏰ 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: deploy-preview
🔇 Additional comments (7)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (4)
1-24: LGTM! Clean component structure and imports.The 'use client' directive is correctly placed for React 19 Server Components architecture, and all imports are properly organized.
25-27: LGTM! Appropriate guard clause.The early return correctly prevents rendering an empty carousel when no products are available.
32-42: LGTM! Well-structured header section.The nested conditionals ensure the header wrapper only renders when needed, and the Typography variant="h2" provides proper semantic structure.
54-61: Verify link rendering behavior.The link object is only created when
linkDetailsLabelis provided. This means a product with aproduct.linkbut nolinkDetailsLabelwon't render a clickable link. Confirm this is the intended UX—some users might expect the product card itself or the product name to be clickable even without an explicit detail link.packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
29-211: Well-structured responsive layout with proper semantic HTML.The component demonstrates good practices:
- Proper heading hierarchy (h1, h2) with
asChildpattern- Semantic list elements for badges and specs
- Mobile-first responsive design with appropriate breakpoints
- Sticky sidebar for desktop and fixed bottom CTA for mobile
- Conditional rendering for optional fields
packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
33-38: Correct placement of__typenamediscriminator.Per project conventions, the
__typenamefield is correctly added at the API harmonization layer for discriminated unions, rather than in the framework layer. Based on learnings, this is the expected pattern.packages/blocks/product-details/src/api-harmonization/product-details.mapper.ts (1)
5-46: Mapper logic is clean with appropriate fallbacks.The mapping function handles optional fields well with sensible defaults (
|| []for arrays). The conditionalactionButtoncreation based on bothactionButtonLabelandproduct.linkprevents incomplete button states.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ui/src/components/ProductCarousel/ProductCarousel.tsx:
- Around line 54-61: The link object is created when linkDetailsLabel is truthy
even if product.link is undefined, producing a broken link; update the
conditional to only build the link object when both linkDetailsLabel and a
non-empty product.link exist (check product.link !== undefined && product.link
!== '' or use a truthy check) before passing it to ProductCard so url is never
undefined and the component receives either a valid { label, url } or undefined.
🧹 Nitpick comments (1)
packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
518-543: Consider using stable image URLs instead of picsum.photos.The
picsum.photosservice returns random images on each request (especially with?random=Nparams), which can cause:
- Inconsistent UI appearance across page loads/refreshes
- Potentially flaky visual regression tests
- Dependency on external service availability
Other mock products use stable GitHub raw URLs from the repository. Consider using the same approach for consistency:
💡 Suggested change
image: { - url: 'https://picsum.photos/1200/800', + url: 'https://raw.githubusercontent.com/o2sdev/openselfservice/refs/heads/main/packages/integrations/mocked/public/images/empty.jpg', width: 1200, height: 800, alt: 'Premium Industrial Machine XL-2000', }, images: [ { - url: 'https://picsum.photos/1200/800?random=1', + url: 'https://raw.githubusercontent.com/o2sdev/openselfservice/refs/heads/main/packages/integrations/mocked/public/images/empty.jpg', alt: 'Industrial Machine Front View', width: 1200, height: 800, }, // ... similar changes for other images ],
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/integrations/mocked/src/modules/products/products.mapper.tspackages/ui/src/components/Cards/ProductCard/ProductCard.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 411
File: packages/framework/src/modules/cms/models/blocks/product-list.model.ts:29-46
Timestamp: 2025-11-26T11:57:00.632Z
Learning: In the framework layer (packages/framework/src/modules/cms/models/blocks/*.model.ts), block classes like ProductListBlock should NOT include explicit __typename discriminators. The __typename field is added at the API harmonization layer (packages/blocks/*/src/api-harmonization/*.model.ts) where it's needed for discriminated unions. The framework maintains normalized data models without these discriminators.
🧬 Code graph analysis (2)
packages/integrations/mocked/src/modules/products/products.mapper.ts (2)
packages/framework/src/modules/products/products.model.ts (2)
Products(42-42)Product(19-40)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Product(10-15)
packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (4)
packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (1)
ProductCarouselProps(8-17)packages/ui/src/components/RichText/RichText.tsx (1)
RichText(48-252)packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (1)
ProductCard(16-94)packages/ui/src/lib/utils.ts (1)
cn(5-7)
🔇 Additional comments (7)
packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (2)
30-30: LGTM!The use of
shrink-0(Tailwind v3 canonical utility) is cleaner and equivalent toflex-shrink-0. Good modernization.
44-44: LGTM!Using
flex-1instead ofh-fullis the correct approach within a flex container. This ensures the content section grows to fill available space using proper flex semantics rather than relying on percentage-based height.packages/integrations/mocked/src/modules/products/products.mapper.ts (3)
3-508: Mock product data enhancements look good.The expanded product data with
subtitle,keySpecs,detailedSpecs, andlocationfields aligns well with theProducts.Model.Producttype definition. The rich HTML descriptions with semantic structure (<h3>,<ul>,<li>) will render nicely in the product-details block.Note: All products 1-11 share the same SKU
ABC-12345-S-BL. This is fine for mock data but worth keeping in mind if any future logic depends on SKU uniqueness.
582-595: LGTM!The
MOCK_PRODUCTSarray is correctly updated to includeMOCK_PRODUCT_12.
597-693: Mapping functions remain correct and type-safe.The
mapProduct,mapProducts, andmapRelatedProductsfunctions are unchanged and will correctly pass through the new product fields (subtitle,keySpecs,detailedSpecs,location) to consumers, maintaining type safety viaProducts.Model.Product.packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (2)
66-88: Props spread order allows full config override.The
{...carouselConfig}spread at line 88 is positioned after the explicit prop declarations (slidesPerView,spaceBetween,showNavigation,showPagination,className,breakpoints). This means any values incarouselConfigwill override the defaults.If this is intentional for maximum flexibility, this is fine. If certain props should remain fixed (e.g., always show navigation), consider spreading
carouselConfigbefore those props or selectively merging.
15-92: Well-structured carousel component.The component follows good patterns:
- Defensive guard for empty products
- Clean conditional header rendering
- Proper use of
product.idas key- Responsive breakpoints configuration
- Flexible customization via
carouselConfigandcarouselClassName
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/integrations/mocked/src/modules/cms/mappers/cms.header.mapper.ts:
- Around line 170-180: The NavigationGroup for the German header currently has a
NavigationItem with label 'Shop' pointing to '/products', which is inconsistent
with other German paths; update the URL for the NavigationItem (the object with
__typename: 'NavigationItem' and label: 'Shop' inside the NavigationGroup with
title: 'Shop') to '/produkte' so the DE header uses localized paths
consistently.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/integrations/mocked/src/modules/cms/mappers/cms.header.mapper.ts
🔇 Additional comments (2)
packages/integrations/mocked/src/modules/cms/mappers/cms.header.mapper.ts (2)
76-86: LGTM!The Shop navigation group for the EN locale is correctly structured and follows the existing pattern.
264-274: LGTM!The Polish locale correctly follows the localization pattern with translated title, label, and URL path.
packages/integrations/mocked/src/modules/cms/mappers/cms.header.mapper.ts
Show resolved
Hide resolved
…lti-language support
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
85-100: Critical: Mutating the original mock data array.
datais an alias toproductsSource, which points directly toMOCK_PRODUCTS_EN/DE/PL. Callingdata.sort()mutates the original mock array in place, corrupting the data order for all subsequent calls with different sort parameters.🐛 Proposed fix
Create a copy before sorting:
- const products: Products.Model.Product[] = productsSource; + const products: Products.Model.Product[] = [...productsSource]; if (!products.length) { return { data: [], total: 0, }; } - const data = products; + const data = [...products];Alternatively, a single spread is sufficient if you remove the intermediate
productsvariable:- const products: Products.Model.Product[] = productsSource; - - if (!products.length) { + if (!productsSource.length) { return { data: [], total: 0, }; } - const data = products; + const data = [...productsSource];
🤖 Fix all issues with AI agents
In @packages/integrations/mocked/src/modules/resources/mock/products.mock.ts:
- Around line 295-297: MOCK_PRODUCT_7 has an incorrect category value; update
its category field (the "category" property in the MOCK_PRODUCT_7 object) from
'TRAINING' to a more accurate value such as 'CALIBRATION' or 'SERVICES' so the
product "PrecisionPro Calibration™ – Ensuring Accuracy for Industrial Equipment"
is correctly classified; locate MOCK_PRODUCT_7 in products.mock.ts and change
the category string accordingly.
- Around line 614-615: Update the misleading comment above the default export:
replace the line claiming "not used, but kept for reference" with a brief,
accurate description stating that MOCK_PRODUCTS is the default export used by
the resources module (referenced in resources.mapper.ts), while the
locale-specific constants MOCK_PRODUCTS_EN, MOCK_PRODUCTS_DE, and
MOCK_PRODUCTS_PL are intended for the products module; keep the export as-is but
change the comment to reflect that purpose.
🧹 Nitpick comments (4)
packages/integrations/mocked/src/modules/products/products.mapper.ts (2)
7-13: Extract locale source selection into a helper function.The locale-to-source mapping logic is duplicated three times. Consider extracting it into a reusable helper to improve maintainability and ensure consistency.
♻️ Suggested refactor
+const getProductsSource = (locale?: string): Products.Model.Product[] => { + if (locale === 'pl') { + return MOCK_PRODUCTS_PL; + } else if (locale === 'de') { + return MOCK_PRODUCTS_DE; + } + return MOCK_PRODUCTS_EN; +}; + export const mapProduct = (id: string, locale?: string): Products.Model.Product => { - let productsSource = MOCK_PRODUCTS_EN; - if (locale === 'pl') { - productsSource = MOCK_PRODUCTS_PL; - } else if (locale === 'de') { - productsSource = MOCK_PRODUCTS_DE; - } + const productsSource = getProductsSource(locale);Then use
getProductsSource(locale)inmapProductsandmapRelatedProductsas well.Also applies to: 25-30, 78-83
44-67: Consider extracting common sorting logic.The sorting logic in
mapProducts(lines 44-67) andmapRelatedProducts(lines 96-119) is nearly identical. Extracting a shared sort helper would reduce duplication and simplify future maintenance.Also applies to: 96-119
packages/integrations/mocked/src/modules/resources/mock/products.mock.ts (2)
5-5: Duplicate SKU values across products.11 of 12 products share the identical SKU
'ABC-12345-S-BL'. SKUs typically represent unique Stock Keeping Units. Consider assigning unique SKUs to improve mock data realism and ensure better test coverage for SKU-based lookups.Also applies to: 50-50, 99-99, 144-144, 189-189, 234-234, 279-279, 328-328, 377-377, 422-422, 467-467
524-543: Consider addingimagesarrays to more products for better test coverage.Only
MOCK_PRODUCT_12has animagesarray. Given that the PR adds gallery/carousel UI components, having more products with multiple images would improve test coverage for those features.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/integrations/mocked/src/modules/products/products.mapper.tspackages/integrations/mocked/src/modules/products/products.service.tspackages/integrations/mocked/src/modules/resources/mock/products.mock.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/integrations/mocked/src/modules/products/products.service.ts (2)
packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
mapProduct(7-20)packages/integrations/mocked/src/utils/delay.ts (1)
responseDelay(4-7)
packages/integrations/mocked/src/modules/products/products.mapper.ts (2)
packages/integrations/mocked/src/modules/resources/mock/products.mock.ts (3)
MOCK_PRODUCTS_EN(599-602)MOCK_PRODUCTS_PL(609-612)MOCK_PRODUCTS_DE(604-607)packages/framework/src/modules/products/products.request.ts (1)
GetProductListQuery(4-9)
packages/integrations/mocked/src/modules/resources/mock/products.mock.ts (3)
packages/framework/src/modules/products/products.model.ts (2)
Products(42-42)Product(19-40)packages/blocks/product-details/src/api-harmonization/product-details.model.ts (1)
Product(10-15)packages/integrations/mocked/src/modules/products/products.mapper.ts (3)
MOCK_PRODUCTS_EN(5-5)MOCK_PRODUCTS_DE(5-5)MOCK_PRODUCTS_PL(5-5)
🔇 Additional comments (2)
packages/integrations/mocked/src/modules/products/products.service.ts (1)
15-17: LGTM!The locale parameter is correctly passed to enable locale-aware product mapping. This aligns well with the updated
mapProductsignature.packages/integrations/mocked/src/modules/resources/mock/products.mock.ts (1)
582-615: Well-structured locale-aware mock data generation.The pattern of defining a base array and mapping it to locale-specific versions with appropriate link transformations is clean and maintainable. This approach makes it easy to add new locales or modify the base data without duplication.
packages/integrations/mocked/src/modules/resources/mock/products.mock.ts
Show resolved
Hide resolved
packages/integrations/mocked/src/modules/resources/mock/products.mock.ts
Show resolved
Hide resolved
…mendedProducts components
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/blocks/recommended-products/src/frontend/RecommendedProducts.renderer.tsx (1)
19-30: Error handling is already in place server-side, but consider an error boundary for better UX.The
RecommendedProductsserver component (lines 18-39 of RecommendedProducts.server.tsx) wraps the data-fetch call in a try-catch block that returnsnullon error, preventing uncaught exceptions from propagating. However, this results in a silent failure where the component disappears entirely. Adding an error boundary around this renderer would allow displaying a user-friendly error message or fallback UI instead of the component vanishing, improving the user experience for data-fetch failures.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/blocks/product-details/src/frontend/ProductDetails.renderer.tsxpackages/blocks/recommended-products/src/frontend/RecommendedProducts.renderer.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/blocks/product-details/src/frontend/ProductDetails.renderer.tsx
- packages/blocks/product-details/src/frontend/ProductDetails.client.tsx
⏰ 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: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/blocks/recommended-products/src/frontend/RecommendedProducts.renderer.tsx (1)
10-31: LGTM! Clean implementation of the Suspense-wrapped renderer.The component follows good patterns:
key={id}on Suspense correctly resets the loading state when the block id changes.- Locale fallback logic is sound, and
useLocale()is called unconditionally per React hooks rules.- Props are cleanly forwarded to the server component.
What does this PR do?
Related Ticket(s)
Key Changes
How to test
Media (Loom or gif)
Summary by CodeRabbit
New Features
UI Components
Localization
Tests & Stories
Chores
✏️ Tip: You can customize this high-level summary in your review settings.