Enhance RichText with format detection and add product sorting #472
Conversation
WalkthroughEnhances RichText with HTML vs Markdown detection and safe HTML rendering, adds sorting to mocked product mappers, exposes a new sort request field and an extra implemented interface on product list query, removes product-type filter in mocked CMS mapper, and tweaks ProductCard and ProductList loading UI. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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
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)
401-426: Sorting mutates the globalMOCK_PRODUCTSarray.Unlike
mapProductswhich correctly creates a copy with[...filteredProducts], this function assignsdata = productsas a direct reference. Callingdata.sort()mutatesMOCK_PRODUCTSin place, causing non-deterministic behavior across all subsequent calls.Apply this diff to create a copy before sorting:
- const data = products; + const data = [...products];
🧹 Nitpick comments (3)
packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
362-380: Consider extracting the sorting logic into a reusable helper.The sorting logic in
mapProducts(lines 362-380) is duplicated inmapRelatedProducts(lines 407-425). Extracting it into a shared helper function would improve maintainability and ensure consistent behavior.Example helper:
const sortProducts = ( products: Products.Model.Product[], sort: string ): Products.Model.Product[] => { const [field, order] = sort.split('_'); const isAscending = order === 'ASC'; return [...products].sort((a, b) => { if (field === 'name') { return isAscending ? a.name.localeCompare(b.name) : b.name.localeCompare(a.name); } else if (field === 'price') { return isAscending ? a.price.value - b.price.value : b.price.value - a.price.value; } const aValue = a[field as keyof Products.Model.Product]; const bValue = b[field as keyof Products.Model.Product]; if (typeof aValue === 'string' && typeof bValue === 'string') { return isAscending ? aValue.localeCompare(bValue) : bValue.localeCompare(aValue); } else if (typeof aValue === 'number' && typeof bValue === 'number') { return isAscending ? aValue - bValue : bValue - aValue; } return 0; }); };packages/ui/src/components/Cards/ProductCard/ProductCard.tsx (1)
60-63: Verifyoverflow-ellipsisclass and clamping behaviorUsing
RichTextwithline-clamp-3for the description is a good fit for product cards. Please double‑check thatoverflow-ellipsisis a valid utility in your Tailwind/config (commonlytext-ellipsis/truncateare used) so the intended truncation actually applies.packages/ui/src/components/RichText/RichText.tsx (1)
12-29: Content format detection is fine, but JSDoc slightly overstates behaviorThe HTML/Markdown detection via a simple HTML tag regex is reasonable for this use case. The JSDoc mentions detecting Markdown syntax, but the implementation only checks for HTML tags and otherwise defaults to
'markdown'; consider tightening the comment so it matches the actual logic.
📜 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 (10)
.changeset/cute-eagles-cheer.md(1 hunks)packages/blocks/product-list/src/api-harmonization/product-list.request.ts(1 hunks)packages/blocks/product-list/src/frontend/ProductList.renderer.tsx(1 hunks)packages/configs/integrations/package.json(1 hunks)packages/framework/src/modules/products/products.request.ts(1 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts(0 hunks)packages/integrations/mocked/src/modules/products/products.mapper.ts(2 hunks)packages/ui/src/components/Cards/ProductCard/ProductCard.tsx(1 hunks)packages/ui/src/components/RichText/RichText.tsx(3 hunks)packages/ui/src/components/RichText/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.product-list.mapper.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-13T15:40:10.528Z
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.
Applied to files:
packages/blocks/product-list/src/frontend/ProductList.renderer.tsx
📚 Learning: 2025-12-03T12:34:20.707Z
Learnt from: marcinkrasowski
Repo: o2sdev/openselfservice PR: 419
File: apps/frontend/package.json:52-52
Timestamp: 2025-12-03T12:34:20.707Z
Learning: In the openselfservice repository, the application does not use Next.js cache components (use cache/cacheComponents), so next-intl compatibility limitations related to those experimental features are not a concern.
Applied to files:
packages/blocks/product-list/src/frontend/ProductList.renderer.tsx
📚 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/api-harmonization/product-list.request.ts
🧬 Code graph analysis (2)
packages/blocks/product-list/src/api-harmonization/product-list.request.ts (3)
packages/framework/src/modules/cms/cms.request.ts (1)
GetCmsEntryParams(5-8)packages/framework/src/modules/products/products.model.ts (1)
Products(26-26)packages/framework/src/modules/products/products.request.ts (1)
GetProductListQuery(4-9)
packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
packages/framework/src/modules/products/products.model.ts (1)
Products(26-26)
🪛 ast-grep (0.40.0)
packages/ui/src/components/RichText/RichText.tsx
[warning] 82-82: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
packages/ui/src/components/RichText/RichText.tsx
[error] 83-83: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ 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 (8)
packages/blocks/product-list/src/frontend/ProductList.renderer.tsx (1)
16-18: Simplified loading fallback looks goodThe new single-wrapper +
Loadingskeleton keeps behavior intact while reducing layout complexity; no functional or UX regressions are apparent here.packages/framework/src/modules/products/products.request.ts (1)
4-9: LGTM!The addition of
sort?: stringtoGetProductListQueryis consistent with the existingsortparameter inGetRelatedProductListParamsand provides a clean API for the sorting functionality implemented in the mapper.packages/integrations/mocked/src/modules/products/products.mapper.ts (1)
344-387: LGTM on mapProducts implementation.The sorting logic correctly handles
name,price, and generic string/number fields. The shallow copy on line 356 ([...filteredProducts]) properly prevents mutation of the original array..changeset/cute-eagles-cheer.md (2)
18-22: LGTM: Sorting feature documentation is clear.The sorting feature addition is well-documented with clear format specification (name_ASC, name_DESC, price_ASC, price_DESC) and notes that sorting is applied post-filtering. No concerns with this section.
1-7: All package names are valid and version bump severity is correct.All five packages (
@o2s/configs.integrations,@o2s/blocks.product-list,@o2s/integrations.mocked,@o2s/framework,@o2s/ui) exist in the monorepo and match their actual package configurations. Theminorversion bump is appropriate since these are feature additions to established packages, not new package releases.packages/configs/integrations/package.json (1)
33-33: No functional change in package.jsonOnly formatting/newline on the closing brace; no functional or configuration impact.
packages/blocks/product-list/src/api-harmonization/product-list.request.ts (1)
3-11: GetProductListBlockQuery interface extension looks consistentImplementing
Products.Request.GetProductListQuerytogether with the existing CMS params and exposingoffset,limit,type,category, andsortkeeps this query structurally aligned with the framework request types; nothing stands out as problematic.packages/ui/src/components/RichText/index.ts (1)
1-1: Publicly re-exportingdetectContentFormatis appropriateExposing
detectContentFormatalongsideRichTextfrom the index keeps the API cohesive and allows other components to reuse the same format detection logic.
tomaszpacior
changed the title
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ui/src/components/RichText/RichText.tsx (1)
78-86: XSS concern from past review has been addressed.The previous review flagged the use of
dangerouslySetInnerHTMLwithout sanitization. This has been correctly fixed by wrapping content withDOMPurify.sanitize(). The static analysis warnings (Biome, ast-grep) are false positives—they detectdangerouslySetInnerHTMLbut cannot recognize that sanitization is applied.Note: This approval is contingent on resolving the SSR compatibility issue with DOMPurify mentioned above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/components/RichText/RichText.tsx(4 hunks)
🧰 Additional context used
🪛 ast-grep (0.40.0)
packages/ui/src/components/RichText/RichText.tsx
[warning] 83-83: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
packages/ui/src/components/RichText/RichText.tsx
[error] 84-84: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ 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 (2)
packages/ui/src/components/RichText/RichText.tsx (2)
17-29: LGTM!The
detectContentFormatfunction correctly identifies HTML content using a reasonable tag pattern. The fallback to'markdown'for empty/falsy content is appropriate.
280-284: LGTM!The wrapper div ensures consistent structure between HTML and Markdown rendering paths, with
classNameand spread props correctly applied.
| @@ -1,3 +1,4 @@ | |||
| import DOMPurify from 'dompurify'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check dependencies in both package.json files
echo "=== Checking packages/ui/package.json ==="
if [ -f "packages/ui/package.json" ]; then
cat packages/ui/package.json | grep -A 50 '"dependencies"' | head -60
else
echo "File not found"
fi
echo ""
echo "=== Checking root package.json ==="
if [ -f "package.json" ]; then
cat package.json | grep -A 50 '"dependencies"' | head -60
else
echo "File not found"
fiRepository: o2sdev/openselfservice
Length of output: 1993
🏁 Script executed:
#!/bin/bash
# Check the RichText component for client-side markers and SSR context
echo "=== Checking RichText.tsx for 'use client' directive and component context ==="
if [ -f "packages/ui/src/components/RichText/RichText.tsx" ]; then
head -20 packages/ui/src/components/RichText/RichText.tsx
echo ""
echo "=== Checking for DOMPurify usage pattern ==="
grep -n "DOMPurify\|window\|document\|useEffect\|useClient" packages/ui/src/components/RichText/RichText.tsx | head -20
else
echo "File not found"
fiRepository: o2sdev/openselfservice
Length of output: 1066
🏁 Script executed:
#!/bin/bash
# Check if isomorphic-dompurify is actually a real package
echo "=== Verifying if isomorphic-dompurify exists on npm ==="
npm view isomorphic-dompurify version 2>/dev/null || echo "Package info unavailable in sandbox"Repository: o2sdev/openselfservice
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Check if dompurify is listed in @o2s/framework or other transitive dependencies
echo "=== Checking if @o2s/framework might provide dompurify ==="
if [ -f "packages/ui/package-lock.json" ]; then
grep -A 2 '"dompurify"' packages/ui/package-lock.json | head -10
elif [ -f "packages/ui/yarn.lock" ]; then
grep -A 2 'dompurify' packages/ui/yarn.lock | head -10
else
echo "Lock file not found in packages/ui"
fi
echo ""
echo "=== Checking root lock file ==="
if [ -f "package-lock.json" ]; then
grep -A 2 '"dompurify"' package-lock.json | head -10
elif [ -f "yarn.lock" ]; then
grep -A 2 'dompurify' yarn.lock | head -10
else
echo "Lock file not found"
fiRepository: o2sdev/openselfservice
Length of output: 304
🏁 Script executed:
#!/bin/bash
# Get full context of DOMPurify usage and check for 'use client' directive
echo "=== Checking full RichText.tsx file for 'use client' and complete context ==="
wc -l packages/ui/src/components/RichText/RichText.tsx
echo ""
head -5 packages/ui/src/components/RichText/RichText.tsx
echo ""
echo "=== Line 80-90 context (DOMPurify usage) ==="
sed -n '80,90p' packages/ui/src/components/RichText/RichText.tsxRepository: o2sdev/openselfservice
Length of output: 828
🏁 Script executed:
#!/bin/bash
# Check full first 10 lines including any 'use client' directive
echo "=== First 10 lines of RichText.tsx ==="
head -10 packages/ui/src/components/RichText/RichText.tsx
echo ""
echo "=== Search for 'use client' or 'use server' in entire file ==="
grep -n "use client\|use server" packages/ui/src/components/RichText/RichText.tsx || echo "No directive found"
echo ""
echo "=== Check @o2s/framework in root package.json ==="
cat package.json | grep -A 10 '"@o2s/framework"'Repository: o2sdev/openselfservice
Length of output: 591
DOMPurify will fail during Server-Side Rendering (SSR).
This component lacks a 'use client' directive and will be server-side rendered by Next.js. dompurify requires a DOM environment (window, document) which is unavailable on the server, causing runtime errors.
Use isomorphic-dompurify instead, which handles both server and browser environments:
-import DOMPurify from 'dompurify';
+import DOMPurify from 'isomorphic-dompurify';Add isomorphic-dompurify to packages/ui/package.json dependencies.
Committable suggestion skipped: line range outside the PR's diff.
What does this PR do?
RichText component has been enhanced to automatically detect content format (HTML or Markdown) and handle each format appropriately:
detectContentFormatutility function that is exported for use in other componentsAdded sorting support for products in mocked integration
mapProductsandmapRelatedProductsin mocked integrationname_ASC,name_DESC,price_ASC,price_DESC)Summary by CodeRabbit
New Features
Improvements
Removed
✏️ Tip: You can customize this high-level summary in your review settings.