feat(blocks.product-list): implement product list block with grid plus table view #411
Conversation
|
👀 We've notified the reward creators here. |
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
packages/blocks/product-list/lint-staged.config.mjs (1)
1-4: lint‑staged setup is sound; consider typecheck cost on large changesRunning Prettier on staged JS/TS/CSS/SCSS plus a full
tsc --noEmiton any TS change is a solid safety net. On larger workspaces this can slow down commits a bit; if it ever becomes painful you could narrow the TypeScript check (e.g., dedicatedtsconfigor a lighter command) while keeping this structure. For now, this is perfectly fine to ship.packages/blocks/product-list/package.json (1)
1-55: Block manifest wiring looks correct; lint glob is slightly broader than necessaryThe exports map, build/dev scripts, and dependency sets all look consistent with other block packages and with the
dist/api-harmonizationlayout you’ve configured, including the dedicated./cliententry. Thelintscript’s pattern"{src,apps,libs,test}/**/*.ts"is a bit broader than this package needs (onlysrcexists here), but it’s harmless—ESLint will just receive a couple of unmatched globs. If you ever want to tighten it, you can safely scope it tosrc/**/*.tsfor slightly faster linting.packages/blocks/product-list/src/frontend/ProductList.server.tsx (1)
12-26: Consider logging errors before returning null.The server component correctly fetches data and passes it to the client component. However, the error handling silently returns
nullwithout logging. Consider adding error logging for debugging and monitoring purposes.Apply this diff to add error logging:
} catch (_error) { + console.error('Failed to fetch product list:', _error); return null; }packages/blocks/product-list/src/api-harmonization/product-list.mapper.ts (1)
5-25: Mapping logic is sound; consider reducingas unknown ascastingThe overall
mapProductListwiring (CMS block metadata + products list) looks correct and matches theProductListBlockmodel. The double cast onfiltersandtableis a reasonable escape hatch, but if/when CMS and harmonized models diverge it may hide structural mismatches. When you have time, consider tightening these types so the casts are either not needed or reduced to a single, well-documented conversion point.packages/blocks/product-list/src/api-harmonization/product-list.controller.ts (1)
12-21: Controller wiring is fine; consider adding an explicit return typeThe controller correctly delegates to
ProductListService.getProductListBlockwith typed headers and query. For clarity and future refactors, you may want to add an explicit return type (e.g.Observable<ProductListBlock>) togetProductListBlock, mirroring the service signature.packages/blocks/product-list/src/api-harmonization/product-list.service.ts (1)
18-37: Service flow is correct; tighten limit/offset handling and Rx chainThe overall flow (CMS block → derive params → Products list →
mapProductList) looks good. Two small improvements to consider:
- Replace
||with nullish coalescing so explicit0values can be honored and intent is clearer:- limit: query.limit || cms.pagination?.limit || 12, - offset: query.offset || 0, + limit: query.limit ?? cms.pagination?.limit ?? 12, + offset: query.offset ?? 0,
- Since there is only one upstream observable,
forkJoin([cms])could be simplified tocms.pipe(concatMap(...))for readability.packages/blocks/product-list/src/api-harmonization/index.ts (1)
1-8: Centralize theURLconstant to avoid duplicationThis index defines
export const URL = '/blocks/product-list';, and the client file (product-list.client.ts) also exports the same literal. To avoid accidental divergence later, consider having one canonical definition (e.g., keep it here and import/re-export it from the client) so both server and client always agree on the route.packages/framework/src/modules/cms/models/blocks/product-list.model.ts (1)
5-5: ProductKeys union collapses tostring, defeating type safety.The union
keyof Product | string | '__typename'effectively resolves to juststringbecausestringis a supertype that includes all string literals. This means the type narrowing attempted in the subsequent type aliases (lines 7-27) doesn't actually provide any compile-time type safety for theidfields.Consider one of these approaches:
Option 1 (if type safety is desired): Remove
| stringto restrict IDs to actual Product keys:-type ProductKeys = keyof Product | string | '__typename'; +type ProductKeys = keyof Product | '__typename';Option 2 (if flexibility is needed): If any string is intentionally allowed (e.g., for nested properties or computed fields), simplify to just
string:-type ProductKeys = keyof Product | string | '__typename'; +type ProductKeys = string;
📜 Review details
Configuration used: CodeRabbit 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 (34)
apps/api-harmonization/src/app.module.ts(2 hunks)apps/api-harmonization/src/modules/page/page.model.ts(2 hunks)apps/frontend/package.json(2 hunks)apps/frontend/src/blocks/renderBlocks.tsx(2 hunks)packages/blocks/product-list/.gitignore(1 hunks)packages/blocks/product-list/.prettierrc.mjs(1 hunks)packages/blocks/product-list/eslint.config.mjs(1 hunks)packages/blocks/product-list/lint-staged.config.mjs(1 hunks)packages/blocks/product-list/package.json(1 hunks)packages/blocks/product-list/src/api-harmonization/index.ts(1 hunks)packages/blocks/product-list/src/api-harmonization/product-list.client.ts(1 hunks)packages/blocks/product-list/src/api-harmonization/product-list.controller.ts(1 hunks)packages/blocks/product-list/src/api-harmonization/product-list.mapper.ts(1 hunks)packages/blocks/product-list/src/api-harmonization/product-list.model.ts(1 hunks)packages/blocks/product-list/src/api-harmonization/product-list.module.ts(1 hunks)packages/blocks/product-list/src/api-harmonization/product-list.request.ts(1 hunks)packages/blocks/product-list/src/api-harmonization/product-list.service.ts(1 hunks)packages/blocks/product-list/src/frontend/ProductList.client.tsx(1 hunks)packages/blocks/product-list/src/frontend/ProductList.renderer.tsx(1 hunks)packages/blocks/product-list/src/frontend/ProductList.server.tsx(1 hunks)packages/blocks/product-list/src/frontend/ProductList.types.ts(1 hunks)packages/blocks/product-list/src/frontend/index.ts(1 hunks)packages/blocks/product-list/src/sdk/index.ts(1 hunks)packages/blocks/product-list/src/sdk/product-list.ts(1 hunks)packages/blocks/product-list/tsconfig.api.json(1 hunks)packages/blocks/product-list/tsconfig.frontend.json(1 hunks)packages/blocks/product-list/tsconfig.json(1 hunks)packages/blocks/product-list/tsconfig.sdk.json(1 hunks)packages/framework/src/modules/cms/cms.model.ts(1 hunks)packages/framework/src/modules/cms/models/blocks/product-list.model.ts(1 hunks)packages/integrations/mocked/src/modules/cms/cms.service.ts(2 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts(1 hunks)packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts(8 hunks)packages/integrations/mocked/src/modules/cms/mappers/mocks/pages/product-list.page.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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-list/.prettierrc.mjspackages/blocks/product-list/eslint.config.mjs
📚 Learning: 2025-11-14T14:24:34.907Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: packages/blocks/ticket-summary/src/frontend/TicketSummary.types.ts:14-16
Timestamp: 2025-11-14T14:24:34.907Z
Learning: In the openselfservice repository, the pattern `Omit<PropsType, ''>` in TypeScript type definitions is intentionally used as a template for future prop omissions, even though it currently does nothing. This convention is preferred for consistency across blocks, making it easier to add omissions when needed later.
Applied to files:
packages/blocks/product-list/src/frontend/ProductList.types.ts
🧬 Code graph analysis (15)
packages/framework/src/modules/cms/cms.model.ts (2)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductListBlock(7-28)packages/framework/src/modules/cms/models/blocks/product-list.model.ts (1)
ProductListBlock(29-46)
packages/blocks/product-list/src/frontend/ProductList.renderer.tsx (5)
packages/blocks/product-list/src/frontend/index.ts (2)
ProductListRenderer(3-3)ProductList(2-2)packages/blocks/product-list/src/frontend/ProductList.types.ts (1)
ProductListRendererProps(14-16)packages/ui/src/components/Container/Container.tsx (1)
Container(42-62)packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductList(30-33)packages/blocks/product-list/src/frontend/ProductList.server.tsx (1)
ProductList(12-26)
packages/blocks/product-list/src/sdk/index.ts (2)
packages/framework/src/sdk.ts (1)
extendSdk(140-148)packages/blocks/product-list/src/sdk/product-list.ts (1)
productList(10-32)
apps/frontend/src/blocks/renderBlocks.tsx (3)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductList(30-33)packages/blocks/product-list/src/frontend/ProductList.server.tsx (1)
ProductList(12-26)packages/blocks/product-list/src/frontend/index.ts (1)
ProductList(2-2)
packages/integrations/mocked/src/modules/cms/cms.service.ts (3)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(1-4)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
mapProductListBlock(276-287)packages/integrations/mocked/src/utils/delay.ts (1)
responseDelay(4-7)
packages/blocks/product-list/src/sdk/product-list.ts (7)
packages/blocks/product-list/src/api-harmonization/index.ts (1)
URL(1-1)packages/blocks/product-list/src/api-harmonization/product-list.client.ts (1)
URL(1-1)packages/blocks/product-list/src/sdk/index.ts (1)
sdk(24-28)packages/blocks/product-list/src/api-harmonization/product-list.request.ts (1)
GetProductListBlockQuery(3-10)packages/framework/src/utils/models/headers.ts (1)
AppHeaders(1-6)packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductListBlock(7-28)packages/framework/src/modules/cms/models/blocks/product-list.model.ts (1)
ProductListBlock(29-46)
apps/api-harmonization/src/modules/page/page.model.ts (3)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (2)
ProductList(30-33)ProductListBlock(7-28)packages/blocks/product-list/src/frontend/ProductList.server.tsx (1)
ProductList(12-26)packages/blocks/product-list/src/frontend/index.ts (1)
ProductList(2-2)
packages/blocks/product-list/src/api-harmonization/index.ts (1)
packages/blocks/product-list/src/api-harmonization/product-list.client.ts (1)
URL(1-1)
packages/blocks/product-list/src/frontend/ProductList.server.tsx (4)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductList(30-33)packages/blocks/product-list/src/frontend/index.ts (1)
ProductList(2-2)packages/blocks/product-list/src/frontend/ProductList.types.ts (1)
ProductListProps(5-10)packages/blocks/product-list/src/sdk/index.ts (1)
sdk(24-28)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (2)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductListBlock(7-28)packages/framework/src/modules/cms/models/blocks/product-list.model.ts (1)
ProductListBlock(29-46)
packages/blocks/product-list/src/frontend/ProductList.types.ts (3)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductListBlock(7-28)packages/framework/src/modules/cms/models/blocks/product-list.model.ts (1)
ProductListBlock(29-46)packages/integrations/strapi-cms/generated/strapi.ts (1)
Omit(14-14)
apps/api-harmonization/src/app.module.ts (4)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductList(30-33)packages/blocks/product-list/src/frontend/ProductList.server.tsx (1)
ProductList(12-26)packages/blocks/product-list/src/frontend/index.ts (1)
ProductList(2-2)apps/api-harmonization/src/app.config.ts (1)
AppConfig(20-37)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (5)
packages/framework/src/modules/cms/models/blocks/product-list.model.ts (1)
ProductListBlock(29-46)packages/framework/src/utils/models/filters.ts (1)
Filters(1-10)packages/blocks/product-list/src/frontend/ProductList.server.tsx (1)
ProductList(12-26)packages/framework/src/utils/models/price.ts (1)
Price(3-7)packages/framework/src/utils/models/media.ts (1)
Media(1-7)
packages/blocks/product-list/src/api-harmonization/product-list.service.ts (5)
packages/integrations/mocked/src/modules/cms/cms.service.ts (1)
Injectable(42-195)packages/blocks/product-list/src/api-harmonization/product-list.request.ts (1)
GetProductListBlockQuery(3-10)packages/framework/src/utils/models/headers.ts (1)
AppHeaders(1-6)packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductListBlock(7-28)packages/blocks/product-list/src/api-harmonization/product-list.mapper.ts (1)
mapProductList(5-26)
packages/framework/src/modules/cms/models/blocks/product-list.model.ts (3)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (2)
Product(57-57)ProductListBlock(7-28)packages/framework/src/utils/models/filters.ts (3)
Filters(1-10)FilterSelect(20-27)FilterDateRange(38-48)packages/framework/src/modules/products/products.model.ts (1)
Products(26-26)
🔇 Additional comments (40)
packages/blocks/product-list/.gitignore (1)
1-57: Standard .gitignore configuration.This
.gitignorefile follows conventional patterns for a Node.js/TypeScript package and appropriately excludes build artifacts, dependencies, logs, environment files, IDE configs, and temporary data. The VSCode exceptions (lines 31-36) are sensible for sharing common editor configs in a monorepo setting.apps/frontend/package.json (1)
9-9: Cross‑platformpostbuildscript andshxdevDependency look goodSwitching to
shx cpis a nice win for Windows support, and addingshxas a devDependency keeps it scoped to tooling. Please just confirm that whatever runsnpm run buildin CI/containers installs devDependencies sopostbuilddoesn’t fail due to a missingshxbinary.Also applies to: 81-81
packages/blocks/product-list/tsconfig.json (1)
1-18: Root TS config and project references look coherentThis root
tsconfig.jsoncleanly extends the shared base and ties together the frontend, API-harmonization, and SDK configs viareferences, which fits the usual solution‑style pattern for blocks. Please run atsc --build tsconfig.jsonfrom this package once to ensure all three referenced configs build and emit into the expecteddistsubfolders without complaints.packages/blocks/product-list/.prettierrc.mjs (1)
1-25: Prettier overrides correctly mirror the three block surfacesThe override mapping of
api-harmonization,frontend, andsdkto the shared Prettier configs is consistent with the block’s folder layout and keeps formatting aligned across surfaces. Please ensure your local/editor Prettier setup is picking up this package‑level config (or that the monorepo root config is set to respect it) so contributors get the same formatting automatically.packages/blocks/product-list/eslint.config.mjs (1)
1-18: ESLint flat config is correctly scoped per surfaceUsing
defineConfigwith three entries that extend the API and frontend block configs for their respectivesrc/**subtrees is aligned with how other blocks are wired and with ESLint v9 flat‑config guidance. Based on learnings, this is the intended pattern. Please run the monorepo lint task once to confirm these new globs are picked up as expected.apps/api-harmonization/src/app.module.ts (1)
42-42: ProductList block registration matches existing block patternsImporting
@o2s/blocks.product-list/api-harmonizationand wiringProductList.Module.register(AppConfig)into the module imports is consistent with how the other blocks are integrated here. Please verify thatGET /api/blocks/product-listresponds correctly (ideally against the mocked CMS data and Products integration) on at least one of the new/products‑style CMS pages.Also applies to: 137-137
packages/blocks/product-list/tsconfig.api.json (1)
1-14: API-harmonization TS config aligns with the emitted dist layoutPointing
outDirat./dist/api-harmonizationand constrainingbaseUrl/includetosrc/api-harmonizationmatches the export paths you defined inpackage.json(e.g../api-harmonization). Withcompositeand declarations enabled, this should slot cleanly into the overall project references build. Please confirm that a build of this package producesdist/api-harmonization/api-harmonization/*.jsand.d.tsfiles as expected.packages/framework/src/modules/cms/cms.model.ts (1)
36-36: New ProductListBlock export is correctly wiredThe added
ProductListBlockexport matches the existing block export pattern and points at the expected./models/blocks/product-list.modelmodule. No further changes needed.apps/frontend/src/blocks/renderBlocks.tsx (1)
19-19: ProductListBlock registration in frontend renderer looks consistentThe new
ProductListimport and'ProductListBlock'case mirror existing block patterns and align with the updatedModules.Page.Model.Blocksunion. Props are forwarded viablockPropsas expected.Also applies to: 133-134
packages/blocks/product-list/src/api-harmonization/product-list.client.ts (1)
1-4: API harmonization client entry is minimal and correct
URLand theModel/Requestnamespace re-exports are set up as expected for a block client; this matches the usage pattern insdk/product-list.ts.packages/blocks/product-list/tsconfig.frontend.json (1)
1-22: Frontend tsconfig aligns with block structureThe extended frontend config, compiler options, and
includepaths (frontend, api-harmonization product-list files, SDK) look consistent and should support incremental builds without issues.packages/blocks/product-list/src/sdk/index.ts (1)
1-28: SDK bootstrap and block extension look correctThe SDK is correctly initialized with an environment-derived
API_URLand extended to exposeblocks.getProductListviaproductList(internalSdk). The intentional unusedModelsimport and env-based logger options are consistent with the established SDK pattern; just ensure the relevantNEXT_PUBLIC_API_URL*env vars are always set soAPI_URL!is safe.apps/api-harmonization/src/modules/page/page.model.ts (1)
18-18: ProductListBlock correctly added to page Blocks unionThe new ProductList import and inclusion of
ProductList.Model.ProductListBlock['__typename']in theBlocksunion cleanly register the block in the page model and keep typings in sync with the frontend switch on'ProductListBlock'.Also applies to: 77-77
packages/blocks/product-list/tsconfig.sdk.json (1)
1-19: SDK tsconfig is consistent with the API/SDK layoutExtending the shared API config, targeting
dist/sdk, and including both the SDK source and related api-harmonization files should integrate this SDK cleanly into the existing build graph.packages/blocks/product-list/src/frontend/ProductList.renderer.tsx (1)
1-28: Invalid use ofuseLocalein a server-side renderer; use thelocaleprop insteadAs written,
ProductListRendereris a Server Component (no'use client'directive) but callsuseLocalefromnext-intl, which is only valid in Client Components. Making this file a Client Component would then break, because Client Components cannot import the server component./ProductList.server.You already get
localeinrenderBlocks.tsxand pass it viablockProps, andProductListRendererPropsincludeslocale. The simplest fix is to treatProductListRendereras a pure Server Component, dropuseLocale, and use thelocaleprop:-import { useLocale } from 'next-intl'; -import React, { Suspense } from 'react'; +import React, { Suspense } from 'react'; @@ -import { ProductListRendererProps } from './ProductList.types'; - -export const ProductListRenderer: React.FC<ProductListRendererProps> = ({ id, accessToken, routing }) => { - const locale = useLocale(); +import { ProductListRendererProps } from './ProductList.types'; + +export const ProductListRenderer: React.FC<ProductListRendererProps> = ({ + id, + accessToken, + locale, + routing, +}) => { @@ - <ProductList id={id} accessToken={accessToken} locale={locale} routing={routing} /> + <ProductList id={id} accessToken={accessToken} locale={locale} routing={routing} /> </Suspense> ); };This keeps the component server-side, avoids hook misuse, and leverages the locale resolved in
renderBlocks.tsx.⛔ Skipped due to learnings
Learnt from: marcinkrasowski Repo: o2sdev/openselfservice PR: 348 File: packages/blocks/notification-summary/src/frontend/NotificationSummary.renderer.tsx:1-15 Timestamp: 2025-11-13T15:40:10.528Z Learning: In next-intl 3.0+, hook-style APIs like useLocale(), useTranslations(), and useFormatter() can be used in non-async Server Components without the 'use client' directive. The library provides server-optimized implementations automatically. Only async Server Components need to use the await-based APIs like getLocale() from next-intl/server.packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts (4)
39-39: LGTM!The import statement correctly brings in the product list page mocks for all three supported locales.
146-151: LGTM!The routing implementation correctly maps locale-specific slugs to their corresponding page objects, following the established pattern used for other resource types in this mapper.
278-278: LGTM!The product list pages are correctly added to all three locale-specific page arrays in
getAllPages, maintaining consistency across EN, DE, and PL locales.Also applies to: 300-300, 322-322
348-348: LGTM!The product list pages are correctly included in the alternative pages list for all three locales, ensuring proper i18n routing support.
Also applies to: 357-357, 366-366
packages/integrations/mocked/src/modules/cms/cms.service.ts (2)
19-19: LGTM!The import statement correctly brings in the product list block mapper.
192-194: LGTM!The
getProductListBlockmethod follows the established pattern used by other block methods in this service, correctly applyingresponseDelay()to simulate realistic API timing.packages/blocks/product-list/src/api-harmonization/product-list.request.ts (1)
1-10: LGTM!The query class correctly implements filtering, sorting, and pagination parameters while properly extending the CMS entry params pattern by omitting
locale(which is passed via headers).packages/integrations/mocked/src/modules/cms/mappers/mocks/pages/product-list.page.ts (3)
3-35: LGTM!The English product list page mock is correctly structured with appropriate SEO metadata, permissions, and a OneColumnTemplate containing the ProductListBlock.
37-69: LGTM!The German product list page mock correctly mirrors the English version with properly localized content.
71-103: LGTM!The Polish product list page mock correctly mirrors the English version with properly localized content.
packages/blocks/product-list/src/sdk/product-list.ts (1)
10-32: LGTM!The SDK implementation correctly follows the established factory pattern, properly merges headers including optional authorization, and makes a GET request with query parameters.
packages/blocks/product-list/src/frontend/ProductList.server.tsx (1)
8-10: LGTM!The dynamic import pattern correctly loads the client component with code splitting, following Next.js best practices for server/client component separation.
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (4)
3-92: LGTM!The English product list block mock is comprehensive, including all necessary fields for filters, pagination, table configuration, and localized labels.
94-183: LGTM!The German product list block mock correctly mirrors the English version with properly localized content.
185-274: LGTM!The Polish product list block mock correctly mirrors the English version with properly localized content.
276-287: LGTM!The mapper function correctly switches on locale with a sensible default fallback to English for unsupported locales.
packages/blocks/product-list/src/api-harmonization/product-list.module.ts (1)
10-31: Code follows established project convention—no changes needed.All block modules in the codebase consistently use the
_config: Framework.ApiConfigparameter pattern with the underscore prefix. This is a TypeScript convention indicating the parameter is intentionally unused. The parameter is kept for API consistency across all block modules.packages/blocks/product-list/src/frontend/index.ts (1)
1-5: Frontend barrel exports look consistentExports for
Client,Server,Renderer, andTypesalign with the corresponding implementation files and follow the existing block barrel pattern. No issues from a type or usage perspective.packages/blocks/product-list/src/api-harmonization/product-list.mapper.ts (1)
28-49: Product item mapping matches the model expectations
mapProductcorrectly projects the core fields (id,sku,name,description,shortDescription,price,image,tags,link) and derivestype/categorylabel/value pairs as required byProductItem. Reusingproduct.linkfor bothdetailsUrlandlinkis consistent and keeps the model flexible for table vs. card usage.packages/blocks/product-list/src/frontend/ProductList.types.ts (1)
5-16: Props surface matches component usage and repo conventions
ProductListProps,ProductListPureProps, andProductListRendererPropsline up with how the server/client/renderer components use them, and theOmit<ProductListProps, ''>pattern is consistent with the existing convention in this repo for future-proofing prop omissions. Based on learnings, this is the preferred style here.packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
7-57: API-harmonized models are consistent with CMS and frontend usage
ProductListBlock,ProductList, andProductItemalign well with the CMS block model and the way the client component consumes data (filters, table config, labels, noResults, and product fields). MakingnoResults.descriptionoptional here is a reasonable relaxation while still accepting the stricter CMS data.packages/blocks/product-list/src/frontend/ProductList.client.tsx (1)
39-55: Disregard this review comment – code is correct for React 19The codebase uses React 19.2.0, which has native support for async callbacks in
startTransition. The patternstartTransition(async () => {...})is valid, properly typed, and the recommended approach in React 19+.The review's premise that async callbacks violate TypeScript types is incorrect for React 19. No changes are needed; the code is correct as written.
Likely an incorrect or invalid review comment.
packages/framework/src/modules/cms/models/blocks/product-list.model.ts (3)
1-3: LGTM! Imports are correct.The imports properly reference the framework's products and utility models needed for the block definition.
7-27: Type alias structure is sound.The pattern of using
Omitand intersection types to specialize the genericDataTableandFilterstypes forProductentities is correctly implemented. The type aliases follow TypeScript best practices for type composition.
29-46: The mapping implementation is correct—no issues found.The verification confirms that the framework and API harmonization layers map correctly:
Products structure: The framework's
Productstype isPagination.Paginated<Product>, which hasdataandtotalproperties that map directly to the API's{ data: ProductItem[]; total: number }structure. The mapper correctly extracts both viatotal: products.totalanddata: products.data.map(...).Entity types: The framework's
Producttype is properly converted to the API'sProductItemtype through themapProduct()function, which handles the field transformation explicitly.The mapping logic at
packages/blocks/product-list/src/api-harmonization/product-list.mapper.tsis functioning as intended with no type compatibility issues.Likely an incorrect or invalid review comment.
Aqil-Ahmad
changed the title
marcinkrasowski
left a comment
marcinkrasowski
left a comment
There was a problem hiding this comment.
@Aqil-Ahmad a few comments, but nothing major, looks very good 👍
packages/blocks/product-list/src/frontend/ProductList.client.tsx
Outdated
Show resolved
Hide resolved
packages/framework/src/modules/cms/models/blocks/product-list.model.ts
Outdated
Show resolved
Hide resolved
packages/framework/src/modules/cms/models/blocks/product-list.model.ts
Outdated
Show resolved
Hide resolved
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts
Outdated
Show resolved
Hide resolved
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/blocks/product-list/src/frontend/ProductList.client.tsx (2)
27-55: Broaden filters state type to match actual filter payload.
initialFilters/filtersare typed from{ id, offset, limit }, buthandleFilteris also used for dynamic fields (e.g. sort, type, category) coming fromFiltersSectionand pagination. At runtime this works, but TypeScript loses those extra keys, which can make future changes brittle.Consider introducing an explicit
ProductListFilterstype that includesid, pagination fields, and dynamic filter keys, and use it forinitialFilters,filters, andhandleFilter:-type ViewMode = 'grid' | 'table'; +type ViewMode = 'grid' | 'table'; +type ProductListFilters = { + id: string; + offset: number; + limit: number; + // extend with sort, type, category, etc. as needed + // sort?: string; + // type?: string[]; + // category?: string[]; +}; @@ - const initialFilters = { + const initialFilters: ProductListFilters = { id: component.id, offset: 0, limit: component.pagination?.limit || 12, }; @@ - const [filters, setFilters] = useState(initialFilters); + const [filters, setFilters] = useState<ProductListFilters>(initialFilters); @@ - const handleFilter = (data: Partial<typeof initialFilters>) => { + const handleFilter = (data: Partial<ProductListFilters>) => {This keeps the API of
handleFilterthe same while making the types accurately reflect the shape of the filter object.
100-203: Always show filters/toggle; avoid gating by initialData length.Currently the whole filters + view toggle + pagination UI is hidden when
initialData.length === 0, and the user only sees the static “NoResults” block. If the initial payload is empty (e.g. because of default filters), this prevents users from ever changing filters or paging to find data.You already handle the no‑results case inside the
LoadingOverlaybased ondata.products.data.length, so you can simplify by removing the outerinitialData.length > 0guard and relying solely on the inner condition:- const initialData = component.products.data; - const [data, setData] = useState(component); + const [data, setData] = useState(component); @@ - return ( - <div className="w-full"> - {initialData.length > 0 ? ( - <div className="flex flex-col gap-6"> + return ( + <div className="w-full"> + <div className="flex flex-col gap-6"> @@ - <LoadingOverlay isActive={isPending}> - {data.products.data.length ? ( + <LoadingOverlay isActive={isPending}> + {data.products.data.length ? ( @@ - ) : ( - <div className="w-full flex flex-col gap-12 mt-6"> - <NoResults title={data.noResults.title} description={data.noResults.description} /> - - <Separator /> - </div> - )} - </LoadingOverlay> - </div> - ) : ( - <div className="w-full flex flex-col gap-12 mt-6"> - <NoResults title={data.noResults.title} description={data.noResults.description} /> - - <Separator /> - </div> - )} + ) : ( + <div className="w-full flex flex-col gap-12 mt-6"> + <NoResults title={data.noResults.title} description={data.noResults.description} /> + <Separator /> + </div> + )} + </LoadingOverlay> + </div> )} </div> );This keeps the current behavior for non‑empty results while allowing users to interact with filters and pagination even when the first load returns zero items.
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
55-96: Verify whether__typenameshould be present on filter items in mocked CMS data.Each filter item is initialized with
__typename: 'FilterSelect'. The framework CMS block models are normalized and avoid__typenameat the block level, with discriminators added later in the API‑harmonization layer.Please double‑check similar mappers (e.g.
cms.service-list.mapper.ts) to confirm whether filter items in CMS mocks are also expected to omit__typenameand rely on structural typing instead. If so, it would be better to remove__typenamehere for consistency with normalized CMS models and let the API‑harmonization layer add discriminators where needed.Based on learnings, CMS block models avoid
__typenamein the normalized layer.Also applies to: 160-200, 265-305
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/blocks/product-list/src/api-harmonization/product-list.mapper.ts(1 hunks)packages/blocks/product-list/src/api-harmonization/product-list.model.ts(1 hunks)packages/blocks/product-list/src/frontend/ProductList.client.tsx(1 hunks)packages/framework/src/modules/cms/models/blocks/product-list.model.ts(1 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/blocks/product-list/src/api-harmonization/product-list.mapper.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-list/src/frontend/ProductList.client.tsxpackages/blocks/product-list/src/api-harmonization/product-list.model.tspackages/framework/src/modules/cms/models/blocks/product-list.model.tspackages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts
📚 Learning: 2025-11-14T14:24:34.907Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 357
File: packages/blocks/ticket-summary/src/frontend/TicketSummary.types.ts:14-16
Timestamp: 2025-11-14T14:24:34.907Z
Learning: In the openselfservice repository, the pattern `Omit<PropsType, ''>` in TypeScript type definitions is intentionally used as a template for future prop omissions, even though it currently does nothing. This convention is preferred for consistency across blocks, making it easier to add omissions when needed later.
Applied to files:
packages/blocks/product-list/src/frontend/ProductList.client.tsx
🧬 Code graph analysis (3)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (5)
packages/framework/src/utils/models/filters.ts (1)
Filters(1-10)packages/framework/src/modules/products/products.model.ts (1)
Products(26-26)packages/integrations/strapi-cms/generated/strapi.ts (1)
Mapping(2142-2149)packages/framework/src/utils/models/price.ts (1)
Price(3-7)packages/framework/src/utils/models/media.ts (1)
Media(1-7)
packages/framework/src/modules/cms/models/blocks/product-list.model.ts (3)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (2)
ProductListBlock(7-29)Product(58-58)packages/integrations/strapi-cms/generated/strapi.ts (1)
Mapping(2142-2149)packages/framework/src/utils/models/filters.ts (1)
Filters(1-10)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (2)
packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductListBlock(7-29)packages/framework/src/modules/cms/models/blocks/product-list.model.ts (1)
ProductListBlock(5-22)
🔇 Additional comments (2)
packages/framework/src/modules/cms/models/blocks/product-list.model.ts (1)
5-22: CMS ProductListBlock model looks consistent with existing patterns.The block shape (table, fieldMapping, pagination, filters, labels, optional
detailsLabel) matches other CMS block models and correctly omits__typenamein the normalized framework layer, withnoResults.descriptionmarked optional.No changes needed here.
Based on learnings, the absence of
__typenamein this framework model is correct.packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
7-29: API-harmonized ProductList models align with framework and frontend usage.
ProductListBlockandProductIteminclude the expected fields (discriminator, filters, pagination, table, mapping, products, labels) and match the shapes consumed by the client component and CMS model snippets. I don’t see structural or typing issues here.Also applies to: 36-56
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts
Show resolved
Hide resolved
|
thank you @marcinkrasowski for the review! i've resolved most of the comments. but i will add the storybook in a separate commit. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/blocks/product-list/src/frontend/ProductList.client.stories.tsx (1)
19-303: Inline args are comprehensive; consider extracting fixtures and local imagesThe args object is nicely realistic (filters, pagination, table config, labels, products, alt text) and should exercise both grid and table views well.
To keep the story file lighter and easier to maintain, consider:
- Extracting the large
argsobject to a shared fixture (e.g.,productListMockArgs) that can be reused across stories/tests.- Optionally replacing external
raw.githubusercontent.comimage URLs with locally served static assets to avoid network flakiness in Storybook.Not required for correctness, but it would improve maintainability and resilience.
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
318-329: Consider narrowing thelocaletype for stronger type‑safety.
mapProductListBlockcurrently acceptslocale: stringbut only handles'en' | 'de' | 'pl'. Narrowing this to a string union (or a shared Locale type if available in the framework) will catch invalid locales at compile time.For example:
-export const mapProductListBlock = (locale: string): CMS.Model.ProductListBlock.ProductListBlock => { +export const mapProductListBlock = ( + locale: 'en' | 'de' | 'pl', +): CMS.Model.ProductListBlock.ProductListBlock => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/blocks/product-list/src/frontend/ProductList.client.stories.tsx(1 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts
🧬 Code graph analysis (1)
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (2)
packages/framework/src/modules/cms/models/blocks/product-list.model.ts (1)
ProductListBlock(5-22)packages/blocks/product-list/src/api-harmonization/product-list.model.ts (1)
ProductListBlock(7-29)
🪛 Gitleaks (8.29.1)
packages/blocks/product-list/src/frontend/ProductList.client.stories.tsx
[high] 18-18: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🔇 Additional comments (2)
packages/blocks/product-list/src/frontend/ProductList.client.stories.tsx (1)
1-13: Solid Storybook meta and typingUse of
satisfies Meta<typeof ProductListPure>plusStoryObj<typeof meta>is idiomatic and keeps the story strongly typed against the component props. No issues here.packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts (1)
55-95: Based on the web search results and the learnings provided, I can now assess the original review comment:The original review comment contains a fundamental misconception. The learnings you provided state that the framework layer (e.g.,
packages/framework/.../product-list.model.ts) should NOT include__typenamediscriminators—which is correct. However, the file under review is a mapper in the mocked integration layer (packages/integrations/mocked/.../cms.product-list.mapper.ts).Per GraphQL best practices (confirmed via web search), mappers and harmonization layers are the canonical place to add
__typenamefor discriminated unions. This is intentional and necessary for client-side type narrowing and GraphQL tooling.The original review misapplies the learnings to the wrong architectural layer.
Since the review comment is also incomplete (it ends abruptly with placeholder tags and lacks a coherent final recommendation), and since I cannot access the codebase to verify the secondary concerns (PL label mismatch, locale typing), here is my final assessment:
The
__typenamefield on filter items is correct for the mapper layer and follows GraphQL best practices for discriminated unions.The learnings correctly state that the framework layer should avoid
__typenamediscriminators to maintain normalized data models. However, mappers operate at the API harmonization layer, where__typenameis intentionally added to support client-side GraphQL type discrimination and tooling. This separation of concerns is by design: framework models are normalized; mappers add__typenamefor API consumption.The secondary concerns raised in the original comment (PL label consistency, locale typing narrowness) cannot be verified without codebase access.
Likely an incorrect or invalid review comment.
| args: { | ||
| locale: 'en', | ||
| accessToken: | ||
| 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSmFuZSBEb2UiLCJlbWFpbCI6ImphbmVAZXhhbXBsZS5jb20iLCJyb2xlIjoic2VsZnNlcnZpY2Vfb3JnX2FkbWluIiwiY3VzdG9tZXIiOnsiaWQiOiJjdXN0LTAwMSIsInJvbGVzIjpbInNlbGZzZXJ2aWNlX29yZ191c2VyIiwic2VsZnNlcnZpY2Vfb3JnX3VzZXIiLCJzZWxmc2VydmljZV9vcmdfYWRtaW4iXSwibmFtZSI6IkFjbWUgQ29ycG9yYXRpb24ifSwiaWF0IjoxNzU2MzI0ODI5fQ.i-ofzMm1drdeK2_-ORFrv4YZZwadD_P-URxF_cMlkV4', |
There was a problem hiding this comment.
Remove hard‑coded JWT access token from the repo
The accessToken prop currently embeds what looks like a full JWT, which is exactly what Gitleaks flagged. Even in Storybook, committing realistic tokens is a security/compliance risk and can confuse future scanners.
Replace this with a clearly fake placeholder or mock value (and rotate/revoke the real token if it ever existed):
- accessToken:
- 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSmFuZSBEb2UiLCJlbWFpbCI6ImphbmVAZXhhbXBsZS5jb20iLCJyb2xlIjoic2VsZnNlcnZpY2Vfb3JnX2FkbWluIiwiY3VzdG9tZXIiOnsiaWQiOiJjdXN0LTAwMSIsInJvbGVzIjpbInNlbGZzZXJ2aWNlX29yZ191c2VyIiwic2VsZnNlcnZpY2Vfb3JnX3VzZXIiLCJzZWxmc2VydmljZV9vcmdfYWRtaW4iXSwibmFtZSI6IkFjbWUgQ29ycG9yYXRpb24ifSwiaWF0IjoxNzU2MzI0ODI5fQ.i-ofzMm1drdeK2_-ORFrv4YZZwadD_P-URxF_cMlkV4',
+ // Storybook-only dummy token; not a real credential
+ accessToken: 'example-access-token',If the component doesn’t actually require a valid token for rendering in Storybook, you can also consider making the prop optional or using undefined here.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| args: { | |
| locale: 'en', | |
| accessToken: | |
| 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSmFuZSBEb2UiLCJlbWFpbCI6ImphbmVAZXhhbXBsZS5jb20iLCJyb2xlIjoic2VsZnNlcnZpY2Vfb3JnX2FkbWluIiwiY3VzdG9tZXIiOnsiaWQiOiJjdXN0LTAwMSIsInJvbGVzIjpbInNlbGZzZXJ2aWNlX29yZ191c2VyIiwic2VsZnNlcnZpY2Vfb3JnX3VzZXIiLCJzZWxmc2VydmljZV9vcmdfYWRtaW4iXSwibmFtZSI6IkFjbWUgQ29ycG9yYXRpb24ifSwiaWF0IjoxNzU2MzI0ODI5fQ.i-ofzMm1drdeK2_-ORFrv4YZZwadD_P-URxF_cMlkV4', | |
| args: { | |
| locale: 'en', | |
| // Storybook-only dummy token; not a real credential | |
| accessToken: 'example-access-token', |
🧰 Tools
🪛 Gitleaks (8.29.1)
[high] 18-18: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🤖 Prompt for AI Agents
In packages/blocks/product-list/src/frontend/ProductList.client.stories.tsx
around lines 15 to 18, the story embeds a real-looking JWT in the accessToken
arg; remove the hard-coded token and replace it with a non-sensitive placeholder
(e.g. 'FAKE_ACCESS_TOKEN') or set the prop to undefined, or update the
component/story to make accessToken optional for Storybook rendering; if this
token was ever valid ensure it is revoked/rotated outside of this change.
| SOFTWARE: 'Oprogramowanie', | ||
| TOOLS: 'Narzędzia', | ||
| HARDWARE: 'Sprzęt', | ||
| MEASUREMENT: 'Pomiar', | ||
| CLOUD: 'Chmura', | ||
| SUPPORT: 'Wsparcie', | ||
| SUBSCRIPTION: 'Subskrypcja', | ||
| WARRANTY: 'Gwarancja', | ||
| MAINTENANCE: 'Konserwacja', | ||
| TRAINING: 'Szkolenie', | ||
| }, |
There was a problem hiding this comment.
Align Polish MAINTENANCE category label between mapping and filters.
In the PL block, fieldMapping.category.MAINTENANCE is "Konserwacja", while the corresponding filter option label is "Utrzymanie". This is likely unintentional and may confuse users; make them consistent (similar to how DE SUBSCRIPTION was aligned).
For example, you could align the filter label with the mapping:
- { label: 'Utrzymanie', value: 'MAINTENANCE' },
+ { label: 'Konserwacja', value: 'MAINTENANCE' },Also applies to: 293-303
🤖 Prompt for AI Agents
In
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts
around lines 224-234 (and similarly review lines 293-303), the Polish labels for
the MAINTENANCE category are inconsistent between the fieldMapping (Konserwacja)
and the filter options (Utrzymanie); update the filter option label(s) to match
the mapping by changing "Utrzymanie" to "Konserwacja" wherever the PL filter
labels for MAINTENANCE appear so both mapping and filters use the same Polish
term.
2 participants
What does this PR do?
/fixes #332
/claim #332
Key Changes
Media (Loom or gif)
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.