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.
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.
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.
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.
…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.
…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.
3 participants
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.