fix: support nested markdown lists#40297
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughParser now captures list indentation and builds nested ListItem trees; ListItem type gains Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Utils
participant Renderer
Parser->>Utils: produce ListItem[] with `indent` (from grammar)
Note over Parser,Utils: buildNestedList transforms flat items into nested tree (children[])
Utils->>Parser: return nested ListItem tree
Parser->>Renderer: emit AST containing ListItem.children
Renderer->>Renderer: recursive renderChildren traverses children -> emit nested <ul>/<ol> and <li>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
added
the
type: feature
gk-j
force-pushed
the
fix/nested-markdown-lists
branch
from
f246583 to
1ebecfe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/message-parser/tests/unorderedList.test.ts (1)
56-77:⚠️ Potential issue | 🟠 MajorNested list behavior is unverified — please enable these tests.
The PR adds nested list parsing (
buildNestedList, capturedindent) but the only nested-list tests in this file are still commented out withparagraph([])placeholders. The newchildren/indentAST shape, the hyphen/asterisk mixing at different levels, the* *and* Hello*guards, and the tab-vs-space indent handling are all untested. Please uncomment and update these to exerciselistItem([...], undefined).children/indentand add matching cases toorderedList.test.ts. Without them, the feature can silently regress.For the expected shape you can extend the
listItemhelper to accept nested children (see comment ontests/helpers.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/tests/unorderedList.test.ts` around lines 56 - 77, Uncomment and re-enable the commented nested-list tests and update their expected ASTs to assert the new children/indent shape (use listItem(...).children and an indent value) so the buildNestedList behavior is covered; extend the listItem test helper (per the comment in tests/helpers.ts) to accept nested children arrays, add equivalent nested cases into orderedList.test.ts, and include permutations for mixed markers (hyphen/asterisk), the "* *" and "* Hello*" guard cases, and tab-vs-space indent variations to ensure indent parsing and guards are exercised.
🧹 Nitpick comments (1)
packages/message-parser/tests/helpers.ts (1)
60-66: Extend helper to express nested fixtures.With
children/indentnow part of everyLIST_ITEMnode, test fixtures for the new nesting feature need to set non-emptychildrenand non-zeroindent. Consider:Proposed signature change
-export const listItem = (value: unknown[], number?: number) => ({ +export const listItem = ( + value: unknown[], + number?: number, + children: unknown[] = [], + indent = 0, +) => ({ type: 'LIST_ITEM' as const, value, - children: [], - indent: 0, + children, + indent, ...(number !== undefined ? { number } : {}), });Defaults preserve the existing call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/tests/helpers.ts` around lines 60 - 66, The listItem helper currently hardcodes children: [] and indent: 0; update its signature (function listItem) to accept optional parameters for children (e.g., children?: unknown[]) and indent (e.g., indent?: number) so callers can create nested fixtures, and use those params when building the returned object while keeping defaults children = [] and indent = 0 to preserve existing call sites; ensure the returned object still conditionally includes number when provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gazzodown/src/blocks/UnorderedListBlock.tsx`:
- Around line 22-27: In UnorderedListBlock.tsx, remove the unnecessary
value={item.number} prop from the <li> rendered inside items.map so unordered
list items don't carry an undefined/misleading attribute; update the JSX in the
items.map callback (the <li> element that uses InlineElements and
renderChildren) to omit value and keep markup symmetrical with renderChildren
for nested lists.
In `@packages/message-parser/src/grammar.pegjs`:
- Around line 46-64: The list nesting currently uses raw indent.length which
treats tabs as one character; add a tab-aware function named indentWidth(s) that
computes columns with tab stops of 4 (tabs expand to next 4-column boundary) and
update the three list-item rules that currently set item.indent using
indent.length to instead set item.indent = indentWidth(indent); leave
buildNestedList as-is (it reads numeric item.indent) so nesting uses the
corrected column widths; ensure the new indentWidth is referenced by name and
available to the grammar so those rules can call it.
- Around line 220-234: The UnorderedListAsteriskContentItem rule's guard !("*" [
\t]) prevents consuming mid-line asterisks and causes inputs like "* A * B" to
be split into two items; remove the redundant guard from both alternatives in
UnorderedListAsteriskContentItem so the rule can use the same Inline/Any
behavior as UnorderedListHyphenItem, and update UnorderedListAsteriskItemContent
accordingly; then add a unit test in unorderedList.test.ts asserting that input
"* A * B" parses as a single UnorderedListAsteriskItem whose text/flattened
content equals "A * B".
---
Outside diff comments:
In `@packages/message-parser/tests/unorderedList.test.ts`:
- Around line 56-77: Uncomment and re-enable the commented nested-list tests and
update their expected ASTs to assert the new children/indent shape (use
listItem(...).children and an indent value) so the buildNestedList behavior is
covered; extend the listItem test helper (per the comment in tests/helpers.ts)
to accept nested children arrays, add equivalent nested cases into
orderedList.test.ts, and include permutations for mixed markers
(hyphen/asterisk), the "* *" and "* Hello*" guard cases, and tab-vs-space indent
variations to ensure indent parsing and guards are exercised.
---
Nitpick comments:
In `@packages/message-parser/tests/helpers.ts`:
- Around line 60-66: The listItem helper currently hardcodes children: [] and
indent: 0; update its signature (function listItem) to accept optional
parameters for children (e.g., children?: unknown[]) and indent (e.g., indent?:
number) so callers can create nested fixtures, and use those params when
building the returned object while keeping defaults children = [] and indent = 0
to preserve existing call sites; ensure the returned object still conditionally
includes number when provided.
🪄 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: 16de5a53-1397-459a-9b6b-742452e02409
📒 Files selected for processing (7)
packages/gazzodown/src/blocks/OrderedListBlock.tsxpackages/gazzodown/src/blocks/UnorderedListBlock.tsxpackages/message-parser/src/definitions.tspackages/message-parser/src/grammar.pegjspackages/message-parser/src/utils.tspackages/message-parser/tests/helpers.tspackages/message-parser/tests/unorderedList.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/message-parser/src/definitions.tspackages/message-parser/tests/helpers.tspackages/message-parser/tests/unorderedList.test.tspackages/gazzodown/src/blocks/OrderedListBlock.tsxpackages/message-parser/src/utils.tspackages/gazzodown/src/blocks/UnorderedListBlock.tsx
🧠 Learnings (4)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/message-parser/src/definitions.tspackages/message-parser/tests/helpers.tspackages/message-parser/tests/unorderedList.test.tspackages/message-parser/src/utils.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/message-parser/src/definitions.tspackages/message-parser/tests/helpers.tspackages/message-parser/tests/unorderedList.test.tspackages/message-parser/src/utils.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/message-parser/tests/unorderedList.test.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
packages/gazzodown/src/blocks/OrderedListBlock.tsxpackages/gazzodown/src/blocks/UnorderedListBlock.tsx
| function buildNestedList(flatItems) { | ||
| const root = []; | ||
| const stack = [{ level: -1, children: root }]; | ||
|
|
||
| for (const item of flatItems) { | ||
| const level = item.indent ?? 0; | ||
|
|
||
| // Pop stack until we find a parent whose level < current | ||
| while (stack.length > 1 && stack[stack.length - 1].level >= level) { | ||
| stack.pop(); | ||
| } | ||
|
|
||
| const node = { ...item, children: [] }; | ||
| stack[stack.length - 1].children.push(node); | ||
| stack.push({ level, children: node.children }); | ||
| } | ||
|
|
||
| return root; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CommonMark specification how tabs are interpreted for list indentation
💡 Result:
In CommonMark, tabs are treated specially when they affect block structure (including list indentation): 1) General rule for tabs Tabs are not automatically expanded to spaces. However, in contexts where whitespace defines block structure, a tab behaves as if it were replaced by spaces using a tab stop of 4 characters. 2) How this impacts list indentation When deciding whether a line is part of a list item (including sublist nesting / continuation blocks), CommonMark’s list-item parsing uses indentation based on the “width” of the list marker plus the following required spaces (and the general tab rule applies in the whitespace that determines indentation). A tab used to indent a continuation paragraph within a list item is equivalent to indenting with four spaces (because that tab stop is 4 characters). The spec includes an explicit example showing that effect. Practical implication: to nest/align content under list items, you can rely on tabs contributing indentation in 4-character steps; e.g., a single tab provides the same indentation “width” as four spaces for these list-structure decisions. Sources: CommonMark spec section on tabs (2.2) and the list-item indentation example demonstrating a tab as equivalent to four spaces.
Citations:
- 1: https://spec.commonmark.org/0.31.2
- 2: https://spec.commonmark.org/0.31.2/
- 3: https://spec.commonmark.org/0.31.2/changes.html
- 4: https://raw.githubusercontent.com/commonmark/commonmark-spec/0.30/spec.txt
- 5: https://github.com/jgm/CommonMark/blob/master/spec.txt
🏁 Script executed:
# First, let's locate and examine the grammar.pegjs file
cd packages/message-parser && ls -la src/
# Read the full grammar.pegjs file to see the context around lines 46-64
wc -l src/grammar.pegjsRepository: RocketChat/Rocket.Chat
Length of output: 634
🏁 Script executed:
# Read the grammar.pegjs file, focusing on the buildNestedList function and surrounding context
head -100 packages/message-parser/src/grammar.pegjs | tail -60Repository: RocketChat/Rocket.Chat
Length of output: 1167
🏁 Script executed:
# Search for where indent is captured and used in list-item rules
rg -n "indent" packages/message-parser/src/grammar.pegjs -A 2 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 940
🏁 Script executed:
# Look for OrderedListItem, UnorderedListHyphenItem, UnorderedListAsteriskItem rules
rg -n "OrderedListItem|UnorderedListHyphenItem|UnorderedListAsteriskItem" packages/message-parser/src/grammar.pegjs -A 5 -B 1Repository: RocketChat/Rocket.Chat
Length of output: 1268
Indentation must use tab-stop column width, not character count, to match CommonMark behavior.
buildNestedList receives item.indent from the three list-item rules (lines 207, 217, 221), all of which use indent.length where indent is captured as $([ \t]*). This treats a tab as 1 character, but CommonMark specifies that tabs expand to the next 4-column tab stop. Mixed input like \t- a (indent.length = 1) and - b (indent.length = 4) will produce inconsistent nesting hierarchies, even though both represent 4-column indentation in CommonMark.
Use a tab-aware function to compute column width:
Example implementation
const indentWidth = (s) => {
let c = 0;
for (const ch of s) c += ch === '\t' ? 4 - (c % 4) : 1;
return c;
};Then replace indent.length with indentWidth(indent) in all three list-item rules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/message-parser/src/grammar.pegjs` around lines 46 - 64, The list
nesting currently uses raw indent.length which treats tabs as one character; add
a tab-aware function named indentWidth(s) that computes columns with tab stops
of 4 (tabs expand to next 4-column boundary) and update the three list-item
rules that currently set item.indent using indent.length to instead set
item.indent = indentWidth(indent); leave buildNestedList as-is (it reads numeric
item.indent) so nesting uses the corrected column widths; ensure the new
indentWidth is referenced by name and available to the grammar so those rules
can call it.
1ebecfe to
d08e359
Compare
coderabbitai
Bot
removed
the
type: feature
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/gazzodown/src/blocks/OrderedListBlock.tsx (1)
10-30: Deduplicate by reusingrenderChildrenin the main component.The JSX inside
OrderedListBlock(lines 22-29) is identical torenderChildren(lines 11-18). Consider having the component delegate directly to the helper to avoid drift between the two branches.♻️ Proposed refactor
-const OrderedListBlock = ({ items }: OrderedListBlockProps): ReactElement => ( - <ol style={{ paddingInlineStart: '1.5rem' }}> - {items.map((item, index) => ( - <li key={index} value={item.number}> - <InlineElements>{item.value}</InlineElements> - {item.children?.length ? renderChildren(item.children) : null} - </li> - ))} - </ol> -); +const OrderedListBlock = ({ items }: OrderedListBlockProps): ReactElement => renderChildren(items);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gazzodown/src/blocks/OrderedListBlock.tsx` around lines 10 - 30, The OrderedListBlock duplicates the JSX from renderChildren; change OrderedListBlock to delegate to renderChildren instead of repeating the ol/li markup by calling renderChildren(items). Ensure the types align (renderChildren currently accepts MessageParser.ListItem[]) and that InlineElements and any optional children handling remain unchanged by using renderChildren(items) inside OrderedListBlock; keep renderChildren as the single source of list rendering to avoid drift between implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/gazzodown/src/blocks/OrderedListBlock.tsx`:
- Around line 10-30: The OrderedListBlock duplicates the JSX from
renderChildren; change OrderedListBlock to delegate to renderChildren instead of
repeating the ol/li markup by calling renderChildren(items). Ensure the types
align (renderChildren currently accepts MessageParser.ListItem[]) and that
InlineElements and any optional children handling remain unchanged by using
renderChildren(items) inside OrderedListBlock; keep renderChildren as the single
source of list rendering to avoid drift between implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0af92d3d-c124-4292-ad15-d3f01b537104
📒 Files selected for processing (7)
packages/gazzodown/src/blocks/OrderedListBlock.tsxpackages/gazzodown/src/blocks/UnorderedListBlock.tsxpackages/message-parser/src/definitions.tspackages/message-parser/src/grammar.pegjspackages/message-parser/src/utils.tspackages/message-parser/tests/helpers.tspackages/message-parser/tests/unorderedList.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/message-parser/src/definitions.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/gazzodown/src/blocks/UnorderedListBlock.tsx
- packages/message-parser/tests/unorderedList.test.ts
- packages/message-parser/src/utils.ts
- packages/message-parser/src/grammar.pegjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/message-parser/tests/helpers.tspackages/gazzodown/src/blocks/OrderedListBlock.tsx
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/message-parser/tests/helpers.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/message-parser/tests/helpers.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
packages/gazzodown/src/blocks/OrderedListBlock.tsx
🔇 Additional comments (1)
packages/message-parser/tests/helpers.ts (1)
60-66: LGTM!Defaulting
children: []andindent: 0here matches whatbuildNestedListingrammar.pegjsproduces for everyLIST_ITEM, so assertions built from this helper will line up with the parser output. Tests that need nested children or a non-zero indent can still override these via object spread at call sites.
coderabbitai
Bot
added
the
type: feature
2 participants
Proposed changes
This PR fixes how the message parser handles unordered markdown lists.
It adds support for nested unordered lists and also prevents cases like
* *and* Hello*from being incorrectly parsed as list items.Issue(s)
Fixes #40251
Steps to test or reproduce
I tested this locally in
packages/message-parser.Test command:
yarn testResult: all tests passed — 634/634.
Lint command:
yarn lintResult: lint completed with 0 errors.
Further comments
Screenshot added to show the local test result. The change itself is parser logic and is covered through updated tests.
Summary by CodeRabbit