fix: align numbered markdown items with bold leading content#424
fix: align numbered markdown items with bold leading content#424jeffscottward wants to merge 7 commits intoRunMaestro:mainfrom
Conversation
|
Visual before/after for markdown numbered-list alignment fix (issue #53):
Left: previous behavior (marker can break onto its own line). |
Greptile SummaryFixed ordered list marker alignment when list items start with bold or other styled inline content (resolves issue #53). The fix ensures that markers (1., 2., etc.) remain on the same baseline as bold list titles. Key changes:
Technical approach: Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Markdown: 1. **Bold Text**] --> B{Renderer Output}
B --> C[<li><p><strong>Bold Text</strong></p></li>]
C --> D{CSS Applied}
D --> E[.prose li > p]
E --> F[display: inline<br/>vertical-align: baseline<br/>line-height: inherit]
D --> G[.prose li > p > strong:first-child]
G --> H[vertical-align: baseline<br/>line-height: inherit]
D --> I[.wizard-markdown rules]
I --> J[Same alignment fixes<br/>for wizard contexts]
F --> K[✓ Marker aligned with text]
H --> K
J --> K
K --> L[Visual Result:<br/>Numbered markers stay on<br/>same line as bold content]
Last reviewed commit: c81aefe |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughExports a shared GFM plugin list and centralized Markdown component/style factories (REMARK_GFM_PLUGINS, createMarkdownComponents variants, generateInlineWizardPreviewProseStyles), replaces many inline ReactMarkdown plugin/component maps with these factories, and adds CSS rules to normalize list-item paragraph rendering and inline-element alignment. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/renderer/components/InlineWizard/WizardMessageBubble.tsx (1)
90-90: Consider memoizingwizardMarkdownComponentsfor consistency.Inside this
React.memocomponent,createWizardBubbleMarkdownComponents(theme)is called on every render. WhileReact.memoreduces render frequency, when the component does render, the components object is recreated. For consistency withUpdateCheckModal.tsx(which usesuseMemo), consider:♻️ Suggested improvement
+import React, { useMemo } from 'react'; -import React from 'react'; ... export const WizardMessageBubble = React.memo(function WizardMessageBubble({ message, theme, agentName = 'Agent', providerName, }: WizardMessageBubbleProps): JSX.Element { const isUser = message.role === 'user'; const isSystem = message.role === 'system'; - const wizardMarkdownComponents = createWizardBubbleMarkdownComponents(theme); + const wizardMarkdownComponents = useMemo( + () => createWizardBubbleMarkdownComponents(theme), + [theme] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/InlineWizard/WizardMessageBubble.tsx` at line 90, The wizardMarkdownComponents object is recreated on every render because createWizardBubbleMarkdownComponents(theme) is called directly inside the React.memo component; wrap that call in useMemo (e.g., const wizardMarkdownComponents = useMemo(() => createWizardBubbleMarkdownComponents(theme), [theme])) so the components object is memoized and only recalculated when theme changes, matching the pattern used in UpdateCheckModal.tsx and improving consistency/stability.src/renderer/components/Wizard/screens/ConversationScreen.tsx (1)
152-152: Consider hoistingwizardMarkdownComponentsto parent scope for efficiency.
MessageBubbleis called for each message in the conversation history. CreatingwizardMarkdownComponentsinside it means the components object is recreated for every message bubble on every render ofConversationScreen.Consider hoisting to the parent
ConversationScreencomponent:♻️ Suggested improvement
export function ConversationScreen({ theme, showThinking, setShowThinking, }: ConversationScreenProps): JSX.Element { + const wizardMarkdownComponents = useMemo( + () => createWizardBubbleMarkdownComponents(theme), + [theme] + ); const { state, ... function MessageBubble({ message, theme, agentName, providerName, + wizardMarkdownComponents, }: { message: WizardMessage; theme: Theme; agentName: string; providerName?: string; + wizardMarkdownComponents: Partial<Components>; }): JSX.Element { const isUser = message.role === 'user'; const isSystem = message.role === 'system'; - const wizardMarkdownComponents = createWizardBubbleMarkdownComponents(theme);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Wizard/screens/ConversationScreen.tsx` at line 152, The creation of wizardMarkdownComponents inside each MessageBubble is inefficient; move its creation up into the parent ConversationScreen (or memoize it with React.useMemo in ConversationScreen) so the object is created once per theme change instead of per message render; reference createWizardBubbleMarkdownComponents and wizardMarkdownComponents and then pass the resulting wizardMarkdownComponents down into MessageBubble as a prop (or import/useMemo in ConversationScreen) to avoid recreating it for every message.src/renderer/utils/markdownConfig.ts (1)
465-525: Consider memoizing or documenting the intended usage pattern.This factory function creates a new components object on each call. While
UpdateCheckModal.tsxcorrectly wraps this inuseMemo, bothWizardMessageBubble.tsx(line 90) andConversationScreen.tsx(line 152) call it directly inside the render function without memoization.For memoized components like
WizardMessageBubble, the component object is recreated on every render, which can cause unnecessary reconciliation in ReactMarkdown. Consider either:
- Memoizing the result in consuming components (as
UpdateCheckModaldoes)- Documenting that callers should memoize the result
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/utils/markdownConfig.ts` around lines 465 - 525, The createWizardBubbleMarkdownComponents factory (used by WizardMessageBubble and ConversationScreen) returns a fresh components object on each call which forces ReactMarkdown to re-reconcile; wrap calls to createWizardBubbleMarkdownComponents(theme) in useMemo in WizardMessageBubble and ConversationScreen (like UpdateCheckModal does) so the same object identity is returned across renders, or alternatively add a clear JSDoc comment above createWizardBubbleMarkdownComponents advising all callers to memoize the result (e.g., via React.useMemo with theme as dependency).
🤖 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/renderer/components/InlineWizard/WizardMessageBubble.tsx`:
- Line 90: The wizardMarkdownComponents object is recreated on every render
because createWizardBubbleMarkdownComponents(theme) is called directly inside
the React.memo component; wrap that call in useMemo (e.g., const
wizardMarkdownComponents = useMemo(() =>
createWizardBubbleMarkdownComponents(theme), [theme])) so the components object
is memoized and only recalculated when theme changes, matching the pattern used
in UpdateCheckModal.tsx and improving consistency/stability.
In `@src/renderer/components/Wizard/screens/ConversationScreen.tsx`:
- Line 152: The creation of wizardMarkdownComponents inside each MessageBubble
is inefficient; move its creation up into the parent ConversationScreen (or
memoize it with React.useMemo in ConversationScreen) so the object is created
once per theme change instead of per message render; reference
createWizardBubbleMarkdownComponents and wizardMarkdownComponents and then pass
the resulting wizardMarkdownComponents down into MessageBubble as a prop (or
import/useMemo in ConversationScreen) to avoid recreating it for every message.
In `@src/renderer/utils/markdownConfig.ts`:
- Around line 465-525: The createWizardBubbleMarkdownComponents factory (used by
WizardMessageBubble and ConversationScreen) returns a fresh components object on
each call which forces ReactMarkdown to re-reconcile; wrap calls to
createWizardBubbleMarkdownComponents(theme) in useMemo in WizardMessageBubble
and ConversationScreen (like UpdateCheckModal does) so the same object identity
is returned across renders, or alternatively add a clear JSDoc comment above
createWizardBubbleMarkdownComponents advising all callers to memoize the result
(e.g., via React.useMemo with theme as dependency).
|
Phase 2 update (bundled into this same PR): Chaining summary:
What phase 2 changed:
Validation:
RUN v4.0.15 /Users/jeffscottward/Documents/GitHub/tools/maestro-bug-full-auto/Maestro ✓ src/tests/renderer/utils/markdownConfig.test.ts (83 tests) 8ms Test Files 4 passed (4) Visual artifact (requested): |
|
Correction to previous phase-2 note (shell escaping mangled component names). Phase 2 update (bundled into this same PR): Chaining summary:
What phase 2 changed:
Validation:
Visual artifact (requested): |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/InlineWizard/DocumentGenerationView.tsx`:
- Around line 548-549: The scoped CSS generated by
generateInlineWizardPreviewProseStyles expects selectors like `${scopeSelector}
.prose` but the preview container currently applies both 'doc-gen-view' and
'prose' on the same element, so the rules don't match; fix by either wrapping
the preview content in a child element with class "prose" (so the selector
`${scopeSelector} .prose` matches) or modify the call/site that uses
scopeSelector '.doc-gen-view' to also generate selectors that match the
same-element case (e.g., include `${scopeSelector}.prose` in the generated
selector); update the preview markup or the selector generation in
generateInlineWizardPreviewProseStyles accordingly (refer to
generateInlineWizardPreviewProseStyles and the preview container that uses
classes 'doc-gen-view' and 'prose').
In `@src/renderer/utils/markdownConfig.ts`:
- Around line 144-146: The current CSS makes every paragraph inside list items
display:inline which collapses multi-paragraph breaks; update the selectors in
both generateProseStyles and generateTerminalProseStyles to target only the
first paragraph (use li > p:first-child) for inline styling and explicitly
restore block layout for subsequent paragraphs (e.g., li > p:not(:first-child) {
display: block; margin-top: ... }). Also adjust the related adjacent-list
selectors (li > p:first-child + ul / + ol) so they still remove top margin for
lists immediately following the first paragraph.
| () => generateInlineWizardPreviewProseStyles(theme, '.doc-gen-view', 'document'), | ||
| [theme] |
There was a problem hiding this comment.
Generated prose styles won’t match the current DOM structure.
generateInlineWizardPreviewProseStyles scopes rules to ${scopeSelector} .prose, but the preview container on Line 669 applies doc-gen-view and prose on the same element, so the new rules won’t match. Consider nesting a .prose wrapper (or adjusting the selector) to avoid a styling regression.
🛠️ Suggested markup adjustment so scoped prose styles apply
- <div
- ref={previewRef}
- className="doc-gen-view h-full overflow-y-auto border rounded p-4 prose prose-sm max-w-none outline-none"
- tabIndex={0}
- onKeyDown={(e) => {
- if ((e.metaKey || e.ctrlKey) && e.key === 'e') {
- e.preventDefault();
- e.stopPropagation();
- onModeChange('edit');
- }
- }}
- style={{
- borderColor: theme.colors.border,
- color: theme.colors.textMain,
- fontSize: '13px',
- }}
- >
- <style>{proseStyles}</style>
- <ReactMarkdown remarkPlugins={REMARK_GFM_PLUGINS} components={markdownComponents}>
- {content || '*No content yet.*'}
- </ReactMarkdown>
- </div>
+ <div
+ ref={previewRef}
+ className="doc-gen-view h-full overflow-y-auto border rounded p-4 outline-none"
+ tabIndex={0}
+ onKeyDown={(e) => {
+ if ((e.metaKey || e.ctrlKey) && e.key === 'e') {
+ e.preventDefault();
+ e.stopPropagation();
+ onModeChange('edit');
+ }
+ }}
+ style={{
+ borderColor: theme.colors.border,
+ color: theme.colors.textMain,
+ fontSize: '13px',
+ }}
+ >
+ <style>{proseStyles}</style>
+ <div className="prose prose-sm max-w-none">
+ <ReactMarkdown remarkPlugins={REMARK_GFM_PLUGINS} components={markdownComponents}>
+ {content || '*No content yet.*'}
+ </ReactMarkdown>
+ </div>
+ </div>Also applies to: 667-686
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/InlineWizard/DocumentGenerationView.tsx` around lines
548 - 549, The scoped CSS generated by generateInlineWizardPreviewProseStyles
expects selectors like `${scopeSelector} .prose` but the preview container
currently applies both 'doc-gen-view' and 'prose' on the same element, so the
rules don't match; fix by either wrapping the preview content in a child element
with class "prose" (so the selector `${scopeSelector} .prose` matches) or modify
the call/site that uses scopeSelector '.doc-gen-view' to also generate selectors
that match the same-element case (e.g., include `${scopeSelector}.prose` in the
generated selector); update the preview markup or the selector generation in
generateInlineWizardPreviewProseStyles accordingly (refer to
generateInlineWizardPreviewProseStyles and the preview container that uses
classes 'doc-gen-view' and 'prose').
|
Final phase complete for #315 unification bundle in this PR. Chaining (final):
Final-phase changes in this commit:
Validation:
Final screenshot artifact (requested): |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/web/mobile/MobileMarkdownRenderer.tsx`:
- Around line 204-218: The CSS currently collapses all paragraphs inside list
items by using ".mobile-markdown-content li > p { display: inline; }"; update
the selectors to only target the first paragraph in a list item (e.g.
".mobile-markdown-content li > p:first-of-type { display: inline; ... }") and
restore block layout for subsequent paragraphs (e.g. ".mobile-markdown-content
li > p:not(:first-of-type) { display: block; margin-top: <reasonable spacing>;
}"); also adjust the nested inline rules (the selectors like
".mobile-markdown-content li > p > strong:first-child, ...") to apply only to
the first paragraph (use :first-of-type or equivalent) so only the first
paragraph inherits inline alignment while later paragraphs keep normal block
spacing.
---
Duplicate comments:
In `@src/renderer/utils/markdownConfig.ts`:
- Around line 144-146: The rule making all list-item paragraphs inline is
collapsing multi-paragraph list items; change the selectors so only the first
paragraph inside an li is rendered inline (e.g., target li > p:first-child) and
keep subsequent paragraphs block-level, and update the related selectors (the
ones matching li > p + ul / li > p + ol and the first-child inline rules for
strong/b/em/code/a) to use the same first-child-scoped selector so
multi-paragraph list items are not flattened.
|
Addressed the requested changes from the latest review cycle. What changed
Validation
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/renderer/utils/markdownConfig.test.ts (1)
16-19: Consider loosening the fixed plugin-count assertion.
AssertingREMARK_GFM_PLUGINS.length === 1can create churn if more shared plugins are added. A looser assertion (array existence + non‑empty) keeps the test focused on contract, not implementation detail.🧪 Suggested adjustment
it('should export a shared remark-gfm plugin array', () => { expect(Array.isArray(REMARK_GFM_PLUGINS)).toBe(true); - expect(REMARK_GFM_PLUGINS.length).toBe(1); + expect(REMARK_GFM_PLUGINS.length).toBeGreaterThan(0); });Also applies to: 656-688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/utils/markdownConfig.test.ts` around lines 16 - 19, The test currently asserts a strict plugin count (REMARK_GFM_PLUGINS.length === 1); change this to a looser contract check that ensures REMARK_GFM_PLUGINS is an array and not empty (e.g., Array.isArray(REMARK_GFM_PLUGINS) && REMARK_GFM_PLUGINS.length > 0) so the test verifies presence rather than exact count; apply the same relaxation to the other similar assertions noted (around the other REMARK_* plugin assertions referenced in the comment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/utils/markdownConfig.ts`:
- Around line 483-559: generateInlineWizardPreviewProseStyles is missing the
list-item paragraph normalization rules present in
generateProseStyles/generateTerminalProseStyles, which causes list markers to
misalign when a list item starts with formatted inline content; fix it by adding
the same selectors and rules used there (the li p:first-child, li > :first-child
selectors that normalize margin/padding for paragraphs/strong/em/code at the
start of a list item) into the returned template string in
generateInlineWizardPreviewProseStyles so list markers align correctly for
formatted-first children.
---
Nitpick comments:
In `@src/__tests__/renderer/utils/markdownConfig.test.ts`:
- Around line 16-19: The test currently asserts a strict plugin count
(REMARK_GFM_PLUGINS.length === 1); change this to a looser contract check that
ensures REMARK_GFM_PLUGINS is an array and not empty (e.g.,
Array.isArray(REMARK_GFM_PLUGINS) && REMARK_GFM_PLUGINS.length > 0) so the test
verifies presence rather than exact count; apply the same relaxation to the
other similar assertions noted (around the other REMARK_* plugin assertions
referenced in the comment).
|
Addressed the latest CodeRabbit follow-up:
Validation run in this branch:
RUN v4.0.15 /Users/jeffscottward/Documents/GitHub/tools/maestro-bug-full-auto/Maestro.jeffscottward-fix-issue-53-markdown-list-alignment ✓ src/tests/renderer/utils/markdownConfig.test.ts (86 tests) 7ms Test Files 1 passed (1) (Full
src/prompts/index.ts(51,8): error TS2307: Cannot find module '../generated/prompts' or its corresponding type declarations. still reports the existing generated-module issue: cannot resolve in this worktree context.) |
|
Correction to previous comment (shell escaped markdown got mangled). Latest follow-up is now pushed in commit b3d9d4b:
Validation run:
|
This may not be solvable by AI; needs review. @pedramamini |
pedramamini
left a comment
pedramamini
left a comment
There was a problem hiding this comment.
@jeffscottward — nice work on the markdown unification. The bug fix portion already landed via #425, so what's left here is the Phase 1 refactor which looks solid. A few things to address before we can merge:
1. Rebase onto main (required)
PR #425 was merged into main and touches the same 4 files, so you've got conflicts in:
src/__tests__/renderer/utils/markdownConfig.test.tssrc/renderer/index.csssrc/renderer/utils/markdownConfig.tssrc/web/index.css
Since the bug fix rules are already on main, take main's version of the alignment CSS and layer your unification additions on top.
2. Remove PNG files from repo root
markdown-list-alignment-before-after.png, phase2-markdown-before-after-final.png, phase3-markdown-unification-final.png — these shouldn't be committed. Remove them or move to the PR description as uploaded images.
3. CSS selector bug in generateInlineWizardPreviewProseStyles
The scope selector pattern "${scopeSelector}.prose, ${scopeSelector} .prose" used as a prefix for descendant rules is structurally wrong — the comma splits into two independent selectors, so rules like ${prefix} li > p expand to both ${scopeSelector}.prose li > p AND ${scopeSelector} .prose li > p. The first form targets descendants of the container itself (correct if .prose is on the container), but the second targets a .prose descendant. Make sure the generated selectors match the actual DOM structure in DocumentGenerationView.tsx.
4. Mobile :first-of-type vs :first-child inconsistency
MobileMarkdownRenderer.tsx uses :first-of-type while every other CSS path uses :first-child. Align to :first-child for consistency.
Once rebased and these are addressed, we're good to merge.
2 participants
Chained Scope (what this PR bundles)
This PR intentionally chains two related tracks so we can ship the bug fix safely while starting the renderer unification work from issue #315.
Bug fix for Seeking Assistance: Pesky Markdown Formatting Issue #53
li > prendering so markers stay on the same line as bold list titles.prosepaths and wizard markdown pathPhase 1 of Unified Markdown Renderer #315 (unification kickoff)
markdownConfig.tsConversationScreenWizardMessageBubbleUpdateCheckModalWhy these are bundled
Files touched (phase 1 refactor)
src/renderer/utils/markdownConfig.tssrc/renderer/components/Wizard/screens/ConversationScreen.tsxsrc/renderer/components/InlineWizard/WizardMessageBubble.tsxsrc/renderer/components/UpdateCheckModal.tsxsrc/__tests__/renderer/utils/markdownConfig.test.tsValidation
npm run test -- src/__tests__/renderer/utils/markdownConfig.test.ts src/__tests__/renderer/components/InlineWizard/WizardMessageBubble.test.tsx src/__tests__/renderer/components/UpdateCheckModal.test.tsxVisual proof
Follow-up for #315
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests