fix(ui): improve list rendering in event descriptions#29760
Conversation
Use outside list markers, merge orphaned nested lists, and add regression tests for numbered lists followed by trailing bullet points. Fixes calcom#24366
|
Welcome to Cal.diy, @akshitj11! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
github-actions
Bot
added
Low priority
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated both markdown-to-safe-HTML implementations to repeatedly nest sibling unordered lists under the preceding list item and to use outside list positioning. Added Vitest coverage for ordered and unordered lists, nested bullets, mixed list and paragraph content, styling, and item ordering. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (2)
packages/lib/markdownToSafeHTML.test.ts (1)
1-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tests for
markdownToSafeHTMLClientas well.Both implementations share identical formatting logic, but only the server-side
markdownToSafeHTMLis tested. If the implementations diverge (e.g., one gets a fix the other doesn't), the lack of client-side tests could allow regressions to go unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/markdownToSafeHTML.test.ts` around lines 1 - 53, Add equivalent coverage for markdownToSafeHTMLClient alongside the existing markdownToSafeHTML tests, covering ordered/unordered lists, nested trailing sub-bullets, and mixed paragraphs with bullets. Reuse the same expected formatting and safety assertions so client-side regressions are detected if the implementations diverge.packages/lib/markdownToSafeHTML.ts (1)
17-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared list formatting logic to avoid duplication with
markdownToSafeHTMLClient.ts.The nested list transformation loop and the entire styling
.replace()chain (lines 17-43) are identical in bothmarkdownToSafeHTML.tsandmarkdownToSafeHTMLClient.ts. Only the sanitization step differs (sanitizeHtmlvsDOMPurify.sanitize). Extracting the post-sanitization formatting into a shared utility (e.g.,formatListHTML(safeHTML: string): string) would prevent the two implementations from diverging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/markdownToSafeHTML.ts` around lines 17 - 43, The post-sanitization formatting in markdownToSafeHTML and markdownToSafeHTMLClient is duplicated. Extract the nested-list transformation loop and styling replacement chain into a shared formatListHTML(safeHTML: string) utility, then have both functions call it after their respective sanitization steps while preserving the existing output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/lib/markdownToSafeHTML.test.ts`:
- Line 34: Update the nested-list assertion in the markdown-to-safe-HTML test to
match li elements with attributes by using the broader li opening-tag pattern,
while preserving the existing whitespace and nested ul structure so the
assertion fails for untransformed output and passes only for the corrected text
placement.
In `@packages/lib/markdownToSafeHTML.ts`:
- Around line 17-29: Broaden nestedListInSiblingLiPattern in
markdownToSafeHTML.ts to match arbitrary safe inline content before the sibling
nested <ul>, preserving captured attributes and inner list content during
merging. Apply the same matcher change in markdownToSafeHTMLClient.ts, and add a
regression test covering inline content such as a link, emphasis, code, span, or
a mixed combination.
---
Nitpick comments:
In `@packages/lib/markdownToSafeHTML.test.ts`:
- Around line 1-53: Add equivalent coverage for markdownToSafeHTMLClient
alongside the existing markdownToSafeHTML tests, covering ordered/unordered
lists, nested trailing sub-bullets, and mixed paragraphs with bullets. Reuse the
same expected formatting and safety assertions so client-side regressions are
detected if the implementations diverge.
In `@packages/lib/markdownToSafeHTML.ts`:
- Around line 17-43: The post-sanitization formatting in markdownToSafeHTML and
markdownToSafeHTMLClient is duplicated. Extract the nested-list transformation
loop and styling replacement chain into a shared formatListHTML(safeHTML:
string) utility, then have both functions call it after their respective
sanitization steps while preserving the existing output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 755289c0-1214-4533-a6bb-17dbfa851f05
📒 Files selected for processing (3)
packages/lib/markdownToSafeHTML.test.tspackages/lib/markdownToSafeHTML.tspackages/lib/markdownToSafeHTMLClient.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/markdownToSafeHTML.ts (1)
18-28: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAvoid regex-based matching for nested
<ul>structures.
([\s\S]*?)<\/ul>stops at the first closing</ul>. For lists nested more than one level, the replacement can consume the wrong closing</li>and emit malformed HTML. Add a multi-level regression test and use DOM/stack-based normalization instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/markdownToSafeHTML.ts` around lines 18 - 28, The nested-list normalization around nestedListInSiblingLiPattern must not use regex matching, because it cannot correctly pair multi-level ul/li elements. Replace the regex loop with DOM- or stack-based traversal that identifies a text-only li followed by a sibling li containing its nested ul, moves that ul into the preceding li, and preserves valid structure at every depth. Add a regression test covering lists nested more than one level.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/lib/markdownToSafeHTML.ts`:
- Around line 18-28: The nested-list normalization around
nestedListInSiblingLiPattern must not use regex matching, because it cannot
correctly pair multi-level ul/li elements. Replace the regex loop with DOM- or
stack-based traversal that identifies a text-only li followed by a sibling li
containing its nested ul, moves that ul into the preceding li, and preserves
valid structure at every depth. Add a regression test covering lists nested more
than one level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5b45a38-4cd1-44b5-8cc5-2ca954527500
📒 Files selected for processing (2)
packages/lib/markdownToSafeHTML.tspackages/lib/markdownToSafeHTMLClient.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/lib/markdownToSafeHTMLClient.ts
Fix vacuous nested-list test assertion and broaden li content matching for inline HTML in markdownToSafeHTML helpers.
|
CodeRabbit fixes pushed. Please add the |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/markdownToSafeHTML.test.ts (1)
15-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBind assertions to the expected list structure.
The current matches can start at any
<li>and cross</li>boundaries, so the tests may pass even if bullet text is rendered outside the<ul>. They also do not verify that the ordered list precedes the unordered list or that both list items have the expected styling.🐛 Proposed fix
- expect(html).toContain("<ol"); - expect(html).toContain("<ul"); - expect(html.match(/<li[^>]*>[\s\S]*?Bullet one/g)?.length).toBeGreaterThanOrEqual(1); - expect(html.match(/<li[^>]*>[\s\S]*?Bullet two/g)?.length).toBeGreaterThanOrEqual(1); + expect(html).toMatch(/<ol[^>]*>[\s\S]*?<\/ol>[\s\S]*?<ul[^>]*>/); + expect(html).toMatch( + /<ul[^>]*>[\s\S]*?<li[^>]*>\s*Bullet one[\s\S]*?<\/li>[\s\S]*?<li[^>]*>\s*Bullet two[\s\S]*?<\/li>[\s\S]*?<\/ul>/ + ); - expect(html).toContain("Item A"); - expect(html).toContain("Item B"); - expect(html).toContain("list-style-position: outside"); + expect(html).toMatch( + /<ul[^>]*>[\s\S]*?<li[^>]*>\s*Item A[\s\S]*?<\/li>[\s\S]*?<li[^>]*>\s*Item B[\s\S]*?<\/li>[\s\S]*?<\/ul>/ + );Also applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/markdownToSafeHTML.test.ts` around lines 15 - 20, Strengthen the list assertions in the markdown-to-safe-HTML test by matching each expected item within its correct list structure, preventing matches from crossing </li> boundaries. Verify the ordered list appears before the unordered list, and assert both list items include the expected styling; apply the same structure-aware checks to the corresponding assertions around the second list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/lib/markdownToSafeHTML.test.ts`:
- Around line 15-20: Strengthen the list assertions in the markdown-to-safe-HTML
test by matching each expected item within its correct list structure,
preventing matches from crossing </li> boundaries. Verify the ordered list
appears before the unordered list, and assert both list items include the
expected styling; apply the same structure-aware checks to the corresponding
assertions around the second list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3efbf3eb-94c6-4a8f-a167-63c9598810cd
📒 Files selected for processing (3)
packages/lib/markdownToSafeHTML.test.tspackages/lib/markdownToSafeHTML.tspackages/lib/markdownToSafeHTMLClient.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/lib/markdownToSafeHTMLClient.ts
- packages/lib/markdownToSafeHTML.ts
1 participant
What does this PR do?
Event descriptions with a numbered list followed by trailing bullet points rendered incorrectly on the booking page — nested
<ul>elements appeared as orphan list items and bullets were misaligned due tolist-style-position: inside.This PR improves markdown list rendering by using outside list markers, merging orphaned nested
<ul>siblings into their parent<li>, and adding regression tests.packages/lib/markdownToSafeHTML.tsandmarkdownToSafeHTMLClient.tspackages/lib/markdownToSafeHTML.test.ts(3 test cases)Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
1. stepfollowed by- bullet oneand- bullet two. Before: trailing bullets missing or misaligned. After: both bullets render with outside markers.Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
yarn vitest run packages/lib/markdownToSafeHTML.test.ts(3 tests pass).