Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 (1)
WalkthroughAdds range-aware markdown table parsing and rich inline serialization/rendering so ParagraphBlock can detect tables from React inlines, compute character ranges for cells, and render formatted cell content (bold, links) by mapping ranges to positioned inline segments. Changes
Sequence DiagramsequenceDiagram
participant PB as ParagraphBlock
participant S as Serializer
participant P as TableParser
participant TR as TableRenderer
participant CR as CellRenderer
PB->>S: serializeParagraphInlinesFromReact(children)
S-->>PB: { plain, positioned[] }
PB->>P: parseMarkdownTableWithRanges(plain)
P-->>PB: { headers, rows, headerRanges, dataRanges }
PB->>TR: render table with headers/rows
TR->>CR: renderTableCellFromSegments(plain, positioned, range)
CR->>CR: filter/clamp positioned parts → nodes
CR->>CR: use renderTextWithOptionalBoldMarkdown / LinkRenderer
CR-->>TR: ReactNode cell
TR-->>PB: rendered table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ All snapshot tests passed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/components/RichBlocks/blocks/ParagraphBlock.tsx (1)
80-87: Minor optimization: Avoid double serialization.
serializeParagraphInlinesFromReactis called on line 81, and thenbuildPositionedInlineParts(which internally calls the same function) is called on line 84. This results in the inline nodes being walked twice.♻️ Suggested refactor to avoid redundant work
const inlineNodes = children as ReactNode[]; - const tablePlain = serializeParagraphInlinesFromReact(inlineNodes); - const tableData = parseMarkdownTable(tablePlain); + const { plain, positioned } = buildPositionedInlineParts(inlineNodes); + const tableData = parseMarkdownTable(plain); if (tableData) { - const { plain, positioned } = buildPositionedInlineParts(inlineNodes); const work = plain.trim(); const lead = plain.length - plain.trimStart().length; const withRanges = parseMarkdownTableWithRanges(work);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RichBlocks/blocks/ParagraphBlock.tsx` around lines 80 - 87, The code double-serializes inline nodes: serializeParagraphInlinesFromReact(inlineNodes) is called to produce tablePlain, and buildPositionedInlineParts(inlineNodes) is called shortly after but it internally re-runs the same serialization; remove the first redundant call and instead call buildPositionedInlineParts(inlineNodes) once, then use its returned plain (and positioned) to call parseMarkdownTable and parseMarkdownTableWithRanges (or alternatively change buildPositionedInlineParts to accept a precomputed plain string and reuse it) so the inline nodes are only walked once.src/components/RichBlocks/utils/tableRowUtils.ts (1)
34-37: Minor: Redundant variable assignment.
const u = tcreates an alias that's immediately used. Sincetis already available and not mutated, this assignment is unnecessary.♻️ Suggested cleanup
export const getCellCharRangesInLine = ( t: string, lineStartInWork: number, ): CellCharRange[] => { const cells = splitRow(t); if (cells.length === 0) { return []; } - const u = t; - if (!u.includes('|')) { + if (!t.includes('|')) { return []; } - const hasLead = u[0] === '|'; - const hasTrail = u[u.length - 1] === '|'; + const hasLead = t[0] === '|'; + const hasTrail = t[t.length - 1] === '|'; const i0 = hasLead ? 1 : 0; - const innerT = u.slice(i0, u.length - (hasTrail ? 1 : 0)); + const innerT = t.slice(i0, t.length - (hasTrail ? 1 : 0));(And similarly update remaining
ureferences tot)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RichBlocks/utils/tableRowUtils.ts` around lines 34 - 37, Remove the redundant alias "const u = t" in tableRowUtils.ts and replace all subsequent uses of "u" with "t" (e.g., within the early guard `if (!u.includes('|')) { return []; }`), ensuring no logic/mutation relies on "u" before deleting the declaration; update any other references to the variable "u" in the same function to use "t" instead.src/components/RichBlocks/utils/serializeParagraphInlinesFromReact.spec.ts (1)
71-78: Consider expanding range alignment test coverage.The test validates only the first header cell range. Consider adding assertions for other cells and data rows to ensure the range calculation works correctly across the entire table structure.
💡 Example: Additional assertions
it('cell ranges in trimmed work string slice the same substrings as splitRow', () => { const work = '| A | B |\n| --- | --- |\n| 1 | 2 |'; const p = parseMarkdownTableWithRanges(work); expect(p).not.toBeNull(); expect( work.slice(p!.headerCellRanges[0]!.start, p!.headerCellRanges[0]!.end), ).toBe('A'); + expect( + work.slice(p!.headerCellRanges[1]!.start, p!.headerCellRanges[1]!.end), + ).toBe('B'); + expect( + work.slice(p!.dataRowCellRanges[0]![0]!.start, p!.dataRowCellRanges[0]![0]!.end), + ).toBe('1'); + expect( + work.slice(p!.dataRowCellRanges[0]![1]!.start, p!.dataRowCellRanges[0]![1]!.end), + ).toBe('2'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RichBlocks/utils/serializeParagraphInlinesFromReact.spec.ts` around lines 71 - 78, The test only asserts the first header cell range; extend coverage by adding assertions that verify the remaining header cells (e.g., p!.headerCellRanges[1] etc.) and the data row cell ranges (e.g., p!.rows or p!.dataCellRanges for row 0/1) slice out the expected substrings from work using their start/end, and include checks for the separator row if applicable; use the same slice/expect pattern as the existing assertion with the specific symbols parseMarkdownTableWithRanges, headerCellRanges and the parsed row/cell range properties to locate where to add these extra expects.src/components/RichBlocks/utils/parseMarkdownTable.ts (2)
23-47: Consider extracting shared parse/validation steps to avoid drift.Both parsing functions repeat header/delimiter validation and row-shape normalization. A shared internal parser would reduce future inconsistency risk between plain parsing and range parsing.
Also applies to: 70-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RichBlocks/utils/parseMarkdownTable.ts` around lines 23 - 47, The header/delimiter validation and row-shape normalization logic duplicated in parseMarkdownTable (and the other parsing function around lines 70-96, likely parseMarkdownTableRange) should be extracted into shared helpers to prevent drift: create small internal functions (e.g., getTrimmedLines(text: string): string[], validateHeadersAndDelimiters(headers: string[], delimiterCells: string[]): boolean, and normalizeRowShape(row: string[], targetLen: number): string[]) and reuse splitRow, isDelimiterRow and compact there; update parseMarkdownTable and parseMarkdownTableRange to call these helpers (keeping return types like ParsedTable) so both parsing flows share the same trimming, header/delimiter checks, and row-length normalization logic.
48-56: Avoid cloning accumulator on every row append.Line 55 (
return [...acc, padded]) reallocates the rows array for each line. A simplepushloop avoids repeated copies.♻️ Proposed allocation-friendly rewrite
- const rows = lines.slice(2).reduce<string[][]>((acc, line) => { - const cells = splitRow(line); - if (cells.length === 0) { - return acc; - } - - const padded = times(headers.length, (i) => cells[i] ?? ''); - return [...acc, padded]; - }, []); + const rows: string[][] = []; + for (const line of lines.slice(2)) { + const cells = splitRow(line); + if (cells.length === 0) continue; + rows.push(times(headers.length, (i) => cells[i] ?? '')); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RichBlocks/utils/parseMarkdownTable.ts` around lines 48 - 56, The rows reducer currently allocates a new array for each appended row by returning [...acc, padded]; change this to mutate the accumulator instead—e.g., use a forEach or keep reduce but call acc.push(padded) and return acc—so in parseMarkdownTable.ts (the rows calculation that calls splitRow and builds padded from headers.length) you should avoid creating new arrays on each iteration and simply push padded into the existing acc before returning it.src/components/RichBlocks/utils/serializeParagraphInlinesFromReact.ts (1)
16-18: Broadenchildrentype to match actual runtime behavior.Both functions explicitly handle non-array input via
Array.isArray(children) ? children : [children](lines 19 and 30), but their type signatures restrictchildrento arrays only. This mismatch forces unnecessary array wrapping at call sites or casts to avoid type errors.Change the type from
Array<ReactNode> | ReadonlyArray<ReactNode>toReactNode | ReadonlyArray<ReactNode>for bothserializeParagraphInlinesFromReact(line 17) andbuildPositionedInlineParts(line 28) to reflect the actual implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RichBlocks/utils/serializeParagraphInlinesFromReact.ts` around lines 16 - 18, The function parameter types are too narrow: update serializeParagraphInlinesFromReact and buildPositionedInlineParts to accept ReactNode | ReadonlyArray<ReactNode> (instead of Array<ReactNode> | ReadonlyArray<ReactNode>) so the signatures match the runtime handling that wraps non-array inputs via Array.isArray(children) ? children : [children]; adjust both function declarations' children parameter types to ReactNode | ReadonlyArray<ReactNode> to remove the need for callers to pre-wrap or cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/RichBlocks/blocks/ParagraphBlock.tsx`:
- Around line 80-87: The code double-serializes inline nodes:
serializeParagraphInlinesFromReact(inlineNodes) is called to produce tablePlain,
and buildPositionedInlineParts(inlineNodes) is called shortly after but it
internally re-runs the same serialization; remove the first redundant call and
instead call buildPositionedInlineParts(inlineNodes) once, then use its returned
plain (and positioned) to call parseMarkdownTable and
parseMarkdownTableWithRanges (or alternatively change buildPositionedInlineParts
to accept a precomputed plain string and reuse it) so the inline nodes are only
walked once.
In `@src/components/RichBlocks/utils/parseMarkdownTable.ts`:
- Around line 23-47: The header/delimiter validation and row-shape normalization
logic duplicated in parseMarkdownTable (and the other parsing function around
lines 70-96, likely parseMarkdownTableRange) should be extracted into shared
helpers to prevent drift: create small internal functions (e.g.,
getTrimmedLines(text: string): string[], validateHeadersAndDelimiters(headers:
string[], delimiterCells: string[]): boolean, and normalizeRowShape(row:
string[], targetLen: number): string[]) and reuse splitRow, isDelimiterRow and
compact there; update parseMarkdownTable and parseMarkdownTableRange to call
these helpers (keeping return types like ParsedTable) so both parsing flows
share the same trimming, header/delimiter checks, and row-length normalization
logic.
- Around line 48-56: The rows reducer currently allocates a new array for each
appended row by returning [...acc, padded]; change this to mutate the
accumulator instead—e.g., use a forEach or keep reduce but call acc.push(padded)
and return acc—so in parseMarkdownTable.ts (the rows calculation that calls
splitRow and builds padded from headers.length) you should avoid creating new
arrays on each iteration and simply push padded into the existing acc before
returning it.
In `@src/components/RichBlocks/utils/serializeParagraphInlinesFromReact.spec.ts`:
- Around line 71-78: The test only asserts the first header cell range; extend
coverage by adding assertions that verify the remaining header cells (e.g.,
p!.headerCellRanges[1] etc.) and the data row cell ranges (e.g., p!.rows or
p!.dataCellRanges for row 0/1) slice out the expected substrings from work using
their start/end, and include checks for the separator row if applicable; use the
same slice/expect pattern as the existing assertion with the specific symbols
parseMarkdownTableWithRanges, headerCellRanges and the parsed row/cell range
properties to locate where to add these extra expects.
In `@src/components/RichBlocks/utils/serializeParagraphInlinesFromReact.ts`:
- Around line 16-18: The function parameter types are too narrow: update
serializeParagraphInlinesFromReact and buildPositionedInlineParts to accept
ReactNode | ReadonlyArray<ReactNode> (instead of Array<ReactNode> |
ReadonlyArray<ReactNode>) so the signatures match the runtime handling that
wraps non-array inputs via Array.isArray(children) ? children : [children];
adjust both function declarations' children parameter types to ReactNode |
ReadonlyArray<ReactNode> to remove the need for callers to pre-wrap or cast.
In `@src/components/RichBlocks/utils/tableRowUtils.ts`:
- Around line 34-37: Remove the redundant alias "const u = t" in
tableRowUtils.ts and replace all subsequent uses of "u" with "t" (e.g., within
the early guard `if (!u.includes('|')) { return []; }`), ensuring no
logic/mutation relies on "u" before deleting the declaration; update any other
references to the variable "u" in the same function to use "t" instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f640639-3707-4e0f-a0b7-d4fa82047241
📒 Files selected for processing (10)
src/components/RichBlocks/blocks/ParagraphBlock.tsxsrc/components/RichBlocks/renderers/TableRenderer.tsxsrc/components/RichBlocks/renderers/renderTableCellFromSegments.tsxsrc/components/RichBlocks/renderers/textWithOptionalBoldMarkdown.tsxsrc/components/RichBlocks/utils/inlineNodeWalker.tssrc/components/RichBlocks/utils/inlinePartTypes.tssrc/components/RichBlocks/utils/parseMarkdownTable.tssrc/components/RichBlocks/utils/serializeParagraphInlinesFromReact.spec.tssrc/components/RichBlocks/utils/serializeParagraphInlinesFromReact.tssrc/components/RichBlocks/utils/tableRowUtils.ts
Playwright test results
Details
Flaky testschromium › mainMenu.spec.ts › Main Menu flows › Should be able to navigate to LI.FI Scan (Qase ID: 21) Skipped testschromium › themeManipulation.spec.ts › Switch between dark and light theme and check the background color › Partner theme should appear in theme menu and apply background color (Qase ID: 49) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which Jira task belongs to this PR?
Testing steps
Note
Might need to run locally having STRAPI env vars pointed to prod environment
/learn/dex-aggregator-best-swap-rateson production compared to this branchWhy did I implement it this way?
Checklist before requesting a review
Summary by CodeRabbit
New Features
Bug Fixes
Tests