Skip to content

Commit b3fc1f7

Browse files
authored
fix(ui): render numerals for ordered list items (#520)
* feat(parser): detect ordered list markers and compute display indices The block parser collapsed `*`, `-`, and `\d+.` markers into a single `list-item` block type, discarding ordered/unordered status. Add `ordered` + `orderedStart` to Block, capture the numeric marker in the list regex, and introduce `computeListIndices()` — a pure helper that walks a list group and assigns each ordered item a CommonMark-correct display number (sequential renumbering, streak break/restart on unordered items, deeper-level state truncation, top-level numbering preserved across nested children). 21 new unit tests cover both the parser changes and the indexing helper, including the tricky cases: `1./2./99.` renumbers as 1,2,3; sub-bullets between ordered items keep the top-level streak alive; nested ordered sublists number independently and reset between siblings; numeric checkboxes set both `ordered` and `checked`. For provenance purposes, this commit was AI assisted. * feat(ui): render numerals for ordered list items Branch the list-item marker span on `block.ordered`: render `${index}.` (with `tabular-nums` and a 1.5rem min-width to keep columns stable across single- and double-digit numerals) when the source marker was numeric, otherwise fall through to the existing `•`/`◦`/`▪` bullet symbols. Indices come from `computeListIndices()` called once per list group; `groupBlocks` is unchanged so mixed nested lists still share a single `data-pinpoint-group="list"` hover wrapper and annotation anchoring is unaffected. Checkbox items still take precedence over numerals. Adds a real-world manual fixture (06-ordered-list-plan.md) whose `## Verification` section exercises a 10-item ordered list, the case that originally surfaced the bug. For provenance purposes, this commit was AI assisted. * fix(parser): merge consecutive blockquote lines into one block Each `>` line was emitted as its own blockquote block, so the renderer's `my-4` margin produced visible gaps between every line of a multi-line quote (the parser had a literal TODO comment: "Check if previous was blockquote, if so, merge? No, separate for now"). Fix: append to the previous block when it's a blockquote and the prior line wasn't blank, mirroring the list-continuation pattern. A blank line still breaks the quote so two `>` runs separated by a blank line stay distinct. Adds 5 unit tests (merge, blank-line break, paragraph boundaries, single-line) and a manual fixture (07-blockquotes.md) covering the bug case, the blank-line-break case, sandwich-between-paragraphs, and inline markdown across merged lines. For provenance purposes, this commit was AI assisted. * fix(ui): address code review — diff view, task lists, blockquote paragraphs Three issues surfaced by PR review #520: 1. **Diff view flattened ordered lists to bullets.** PlanCleanDiffView's SimpleBlockRenderer duplicated Viewer's list-item JSX with hardcoded bullet symbols, so a denied+resubmitted plan with numbered steps showed numerals in the main view but `•` in the diff view — exactly the screen where "which step changed?" matters most. Fixed by threading computeListIndices through MarkdownChunk and sharing the marker rendering via a new ListMarker component used by both renderers, which also removes the root-cause duplication. 2. **Ordered task lists dropped their numbers.** `1. [ ] step` set both `ordered=true` and `checked=false` in the parser, but the renderer's checkbox branch took precedence and the numeral was never shown. GitHub renders `1. [ ]` as numeral + checkbox side by side; we now match that by rendering both glyphs in ListMarker when an ordered task list item appears. 3. **Multi-paragraph blockquotes collapsed.** After the blockquote-merge fix in the previous commit, `> a\n>\n> b` produced content `"a\n\nb"` but the renderer passed it straight to InlineMarkdown, which renders `\n\n` as whitespace — so two quoted paragraphs mashed into one line. Fixed by splitting blockquote content on `/\n\n+/` in both Viewer and PlanCleanDiffView and emitting one `<p>` child per paragraph. Adds one unit test for the multi-paragraph blockquote content shape and a manual fixture (08-ordered-edge-cases.md) covering ordered task lists, multi-paragraph quotes, nested-bullet counter preservation, double-digit alignment, and start-at-N numbering. The fourth review comment — loose ordered lists with intervening non-list blocks restarting numbering — is deferred. It requires parser-level loose-list detection (CommonMark's indented-continuation rule) and the bug only fires when users rely on lazy `1./1./1.` markers across a break. Tracked as a follow-up. For provenance purposes, this commit was AI assisted. * fix(parser): don't merge blockquote lines containing block-level markers Round-two review flagged a regression: `> 1. foo\n> 2. bar\n> 3. baz` was merging into one blockquote whose content was `"1. foo\n2. bar\n3. baz"`. The renderer split on `\n\n+` (paragraph breaks), found none, and emitted a single `<p>` — so `\n` collapsed to whitespace in HTML and the user saw `"1. foo 2. bar 3. baz"` as one run-on line. Worse than the pre-PR behavior (which at least kept each line in its own box). Pragmatic fix: don't merge a `>` line whose stripped content starts with a block-level marker (`*`, `-`, `\d+.`, `#`, `` ``` ``, `>`). Those stay as separate blockquote blocks so each marker line is visually distinct (legible, matching pre-PR behavior for quoted lists). Wrapped prose quotes — the original motivating case — still merge correctly because prose lines don't start with markers. Also check the PREVIOUS block's content for markers so a trailing prose line after a `> 1. item` doesn't glue onto the list-item block. 7 new unit tests cover: quoted ordered list stays separate, quoted unordered list stays separate, quoted heading stays separate, quoted code fence stays separate, nested blockquote stays separate, wrapped prose quote still merges (regression guard), and mixed prose+list where prose merges and list lines stay separate. Adds tests/test-fixtures/09-quoted-list-regression.md as a manual repro. Known follow-ups (tracked separately, not in this PR): - Consecutive separate blockquote blocks still get individual `my-4` margins, so a quoted list shows as stacked boxes with gaps between lines. The proper fix is recursive blockquote parsing (render the content as its own Block[] tree with an actual nested list inside the quote). Deferred — requires `children?: Block[]` on Block, parser rework, and exportAnnotations traversal changes. - Clean diff view renumbers ordered lists from the start of each diff chunk when users rely on CommonMark's lazy `1./1./1.` markers. Same power-user population as the earlier deferred loose-list case. - Pure code-hygiene items from the second review (non-list-block handling in computeListIndices, BULLET_BY_LEVEL modulo cycle, <ListGroup> extraction, CLAUDE.md Block interface drift, splitBlockquoteParagraphs helper) — batch into a follow-up cleanup. For provenance purposes, this commit was AI assisted.
1 parent c342ffb commit b3fc1f7

10 files changed

Lines changed: 868 additions & 88 deletions

File tree

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
import React from 'react';
2+
3+
/**
4+
* Shared list-item marker used by both the main Viewer and the plan-diff
5+
* clean view. Renders the appropriate glyph(s) for a list item given its
6+
* level, ordered flag, display index, and checkbox state.
7+
*
8+
* Rendering rules:
9+
* - Ordered items render `${orderedIndex}.` with tabular-nums so digit
10+
* widths stay stable across e.g. `9.` → `10.`.
11+
* - Checkbox items render the circle / checkmark SVG.
12+
* - Ordered + checkbox renders BOTH: numeral first, checkbox second
13+
* (matches GitHub's `1. [ ] task` rendering).
14+
* - Plain bullets fall back to `•` / `◦` / `▪` by level.
15+
*
16+
* Interactivity is opt-in: the Viewer passes `interactive` + `onToggle`
17+
* for click-to-toggle checkboxes; the diff view leaves both undefined.
18+
*/
19+
interface ListMarkerProps {
20+
level: number;
21+
ordered?: boolean;
22+
orderedIndex?: number | null;
23+
checked?: boolean; // undefined = not a checkbox
24+
interactive?: boolean;
25+
onToggle?: () => void;
26+
}
27+
28+
const BULLET_BY_LEVEL = ['\u2022', '\u25E6', '\u25AA'];
29+
30+
export const ListMarker: React.FC<ListMarkerProps> = ({
31+
level,
32+
ordered,
33+
orderedIndex,
34+
checked,
35+
interactive,
36+
onToggle,
37+
}) => {
38+
const isCheckbox = checked !== undefined;
39+
const showNumeral = !!ordered && orderedIndex != null;
40+
const bullet = BULLET_BY_LEVEL[Math.min(level, BULLET_BY_LEVEL.length - 1)];
41+
42+
const handleClick = interactive && onToggle
43+
? (e: React.MouseEvent) => { e.stopPropagation(); onToggle(); }
44+
: undefined;
45+
46+
return (
47+
<span
48+
className={`select-none shrink-0 flex items-center gap-1${interactive ? ' cursor-pointer' : ''}`}
49+
onClick={handleClick}
50+
role={interactive ? 'checkbox' : undefined}
51+
aria-checked={interactive ? checked : undefined}
52+
>
53+
{showNumeral && (
54+
<span className="text-primary/60 tabular-nums text-right" style={{ minWidth: '1.5rem' }}>
55+
{orderedIndex}.
56+
</span>
57+
)}
58+
{isCheckbox ? (
59+
checked ? (
60+
<svg className="w-4 h-4 text-success" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth={2.5}>
61+
<path strokeLinecap="round" strokeLinejoin="round" d="M9 12.75L11.25 15 15 9.75M21 12a9 9 0 11-18 0 9 9 0 0118 0z" />
62+
</svg>
63+
) : (
64+
<svg className={`w-4 h-4 text-muted-foreground/50${interactive ? ' hover:text-muted-foreground transition-colors' : ''}`} viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth={2}>
65+
<circle cx="12" cy="12" r="9" />
66+
</svg>
67+
)
68+
) : !showNumeral ? (
69+
<span className="text-primary/60">{bullet}</span>
70+
) : null}
71+
</span>
72+
);
73+
};

packages/ui/components/Viewer.tsx

Lines changed: 41 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@ import { createPortal } from 'react-dom';
33
import hljs from 'highlight.js';
44
import 'highlight.js/styles/github-dark.css';
55
import { Block, Annotation, AnnotationType, EditorMode, type InputMethod, type ImageAttachment, type ActionsLabelMode } from '../types';
6-
import { Frontmatter } from '../utils/parser';
6+
import { Frontmatter, computeListIndices } from '../utils/parser';
7+
import { ListMarker } from './ListMarker';
78
import { AnnotationToolbar } from './AnnotationToolbar';
89
import { FloatingQuickLabelPicker } from './FloatingQuickLabelPicker';
910

@@ -530,11 +531,25 @@ export const Viewer = forwardRef<ViewerHandle, ViewerProps>(({
530531
{!frontmatter && blocks.length > 0 && blocks[0].type !== 'heading' && <div className="mt-4" />}
531532
{groupBlocks(blocks).map(group =>
532533
group.type === 'list-group' ? (
533-
<div key={group.key} data-pinpoint-group="list" className="py-1 -mx-2 px-2">
534-
{group.blocks.map(block => (
535-
<BlockRenderer imageBaseDir={imageBaseDir} onImageClick={(src, alt) => setLightbox({ src, alt })} key={block.id} block={block} onOpenLinkedDoc={onOpenLinkedDoc} onToggleCheckbox={onToggleCheckbox} checkboxOverrides={checkboxOverrides} />
536-
))}
537-
</div>
534+
(() => {
535+
const indices = computeListIndices(group.blocks);
536+
return (
537+
<div key={group.key} data-pinpoint-group="list" className="py-1 -mx-2 px-2">
538+
{group.blocks.map((block, i) => (
539+
<BlockRenderer
540+
imageBaseDir={imageBaseDir}
541+
onImageClick={(src, alt) => setLightbox({ src, alt })}
542+
key={block.id}
543+
block={block}
544+
orderedIndex={indices[i]}
545+
onOpenLinkedDoc={onOpenLinkedDoc}
546+
onToggleCheckbox={onToggleCheckbox}
547+
checkboxOverrides={checkboxOverrides}
548+
/>
549+
))}
550+
</div>
551+
);
552+
})()
538553
) : group.block.type === 'code' && isMermaidLanguage(group.block.language) ? (
539554
<MermaidBlock key={group.block.id} block={group.block} />
540555
) : group.block.type === 'code' && isGraphvizLanguage(group.block.language) ? (
@@ -967,7 +982,8 @@ const BlockRenderer: React.FC<{
967982
onImageClick?: (src: string, alt: string) => void;
968983
onToggleCheckbox?: (blockId: string, checked: boolean) => void;
969984
checkboxOverrides?: Map<string, boolean>;
970-
}> = ({ block, onOpenLinkedDoc, imageBaseDir, onImageClick, onToggleCheckbox, checkboxOverrides }) => {
985+
orderedIndex?: number | null;
986+
}> = ({ block, onOpenLinkedDoc, imageBaseDir, onImageClick, onToggleCheckbox, checkboxOverrides, orderedIndex }) => {
971987
switch (block.type) {
972988
case 'heading':
973989
const Tag = `h${block.level || 1}` as React.ElementType;
@@ -979,15 +995,23 @@ const BlockRenderer: React.FC<{
979995

980996
return <Tag className={styles} data-block-id={block.id} data-block-type="heading"><InlineMarkdown imageBaseDir={imageBaseDir} onImageClick={onImageClick} text={block.content} onOpenLinkedDoc={onOpenLinkedDoc} /></Tag>;
981997

982-
case 'blockquote':
998+
case 'blockquote': {
999+
// Content may span multiple merged `>` lines. Split on blank-line
1000+
// paragraph breaks so `> a\n>\n> b` renders as two <p> children.
1001+
const paragraphs = block.content.split(/\n\n+/);
9831002
return (
9841003
<blockquote
9851004
className="border-l-2 border-primary/50 pl-4 my-4 text-muted-foreground italic"
9861005
data-block-id={block.id}
9871006
>
988-
<InlineMarkdown imageBaseDir={imageBaseDir} onImageClick={onImageClick} text={block.content} onOpenLinkedDoc={onOpenLinkedDoc} />
1007+
{paragraphs.map((para, i) => (
1008+
<p key={i} className={i > 0 ? 'mt-2' : ''}>
1009+
<InlineMarkdown imageBaseDir={imageBaseDir} onImageClick={onImageClick} text={para} onOpenLinkedDoc={onOpenLinkedDoc} />
1010+
</p>
1011+
))}
9891012
</blockquote>
9901013
);
1014+
}
9911015

9921016
case 'list-item': {
9931017
const indent = (block.level || 0) * 1.25; // 1.25rem per level
@@ -1002,28 +1026,14 @@ const BlockRenderer: React.FC<{
10021026
data-block-id={block.id}
10031027
style={{ marginLeft: `${indent}rem` }}
10041028
>
1005-
<span
1006-
className={`select-none shrink-0 flex items-center${isInteractive ? ' cursor-pointer' : ''}`}
1007-
onClick={isInteractive ? (e) => { e.stopPropagation(); onToggleCheckbox!(block.id, !isChecked); } : undefined}
1008-
role={isInteractive ? 'checkbox' : undefined}
1009-
aria-checked={isInteractive ? isChecked : undefined}
1010-
>
1011-
{isCheckbox ? (
1012-
isChecked ? (
1013-
<svg className="w-4 h-4 text-success" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth={2.5}>
1014-
<path strokeLinecap="round" strokeLinejoin="round" d="M9 12.75L11.25 15 15 9.75M21 12a9 9 0 11-18 0 9 9 0 0118 0z" />
1015-
</svg>
1016-
) : (
1017-
<svg className={`w-4 h-4 text-muted-foreground/50${isInteractive ? ' hover:text-muted-foreground transition-colors' : ''}`} viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth={2}>
1018-
<circle cx="12" cy="12" r="9" />
1019-
</svg>
1020-
)
1021-
) : (
1022-
<span className="text-primary/60">
1023-
{(block.level || 0) === 0 ? '•' : (block.level || 0) === 1 ? '◦' : '▪'}
1024-
</span>
1025-
)}
1026-
</span>
1029+
<ListMarker
1030+
level={block.level || 0}
1031+
ordered={block.ordered}
1032+
orderedIndex={orderedIndex}
1033+
checked={isChecked}
1034+
interactive={isInteractive}
1035+
onToggle={isInteractive ? () => onToggleCheckbox!(block.id, !isChecked) : undefined}
1036+
/>
10271037
<span className={`text-sm leading-relaxed ${isCheckbox && isChecked ? 'text-muted-foreground line-through' : 'text-foreground/90'}`}>
10281038
<InlineMarkdown imageBaseDir={imageBaseDir} onImageClick={onImageClick} text={block.content} onOpenLinkedDoc={onOpenLinkedDoc} />
10291039
</span>

packages/ui/components/plan-diff/PlanCleanDiffView.tsx

Lines changed: 33 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@
88

99
import React, { useEffect, useRef, useState, useCallback } from "react";
1010
import hljs from "highlight.js";
11-
import { parseMarkdownToBlocks } from "../../utils/parser";
11+
import { parseMarkdownToBlocks, computeListIndices } from "../../utils/parser";
12+
import { ListMarker } from "../ListMarker";
1213
import type { Block, Annotation, EditorMode, ImageAttachment } from "../../types";
1314
import { AnnotationType } from "../../types";
1415
import type { PlanDiffBlock } from "../../utils/planDiffEngine";
@@ -409,17 +410,29 @@ const MarkdownChunk: React.FC<{ content: string }> = ({ content }) => {
409410
() => parseMarkdownToBlocks(content),
410411
[content]
411412
);
413+
// Compute ordered-list display indices across the entire chunk so every
414+
// list-item gets the right numeral even though we don't group here.
415+
// Non-list blocks pass through as `null` and act as streak-breaks — same
416+
// behavior as the main Viewer's per-group counter.
417+
const orderedIndices = React.useMemo(
418+
() => computeListIndices(blocks),
419+
[blocks]
420+
);
412421

413422
return (
414423
<>
415-
{blocks.map((block) => (
416-
<SimpleBlockRenderer key={block.id} block={block} />
424+
{blocks.map((block, i) => (
425+
<SimpleBlockRenderer
426+
key={block.id}
427+
block={block}
428+
orderedIndex={orderedIndices[i]}
429+
/>
417430
))}
418431
</>
419432
);
420433
};
421434

422-
const SimpleBlockRenderer: React.FC<{ block: Block }> = ({ block }) => {
435+
const SimpleBlockRenderer: React.FC<{ block: Block; orderedIndex?: number | null }> = ({ block, orderedIndex }) => {
423436
switch (block.type) {
424437
case "heading": {
425438
const Tag = `h${block.level || 1}` as keyof React.JSX.IntrinsicElements;
@@ -437,12 +450,20 @@ const SimpleBlockRenderer: React.FC<{ block: Block }> = ({ block }) => {
437450
);
438451
}
439452

440-
case "blockquote":
453+
case "blockquote": {
454+
// Split on blank-line paragraph breaks so merged `> a\n>\n> b`
455+
// renders as two <p> children instead of collapsing to one line.
456+
const paragraphs = block.content.split(/\n\n+/);
441457
return (
442458
<blockquote className="border-l-2 border-primary/50 pl-4 my-4 text-muted-foreground italic">
443-
<InlineMarkdown text={block.content} />
459+
{paragraphs.map((para, i) => (
460+
<p key={i} className={i > 0 ? "mt-2" : ""}>
461+
<InlineMarkdown text={para} />
462+
</p>
463+
))}
444464
</blockquote>
445465
);
466+
}
446467

447468
case "list-item": {
448469
const indent = (block.level || 0) * 1.25;
@@ -452,43 +473,12 @@ const SimpleBlockRenderer: React.FC<{ block: Block }> = ({ block }) => {
452473
className="flex gap-3 my-1.5"
453474
style={{ marginLeft: `${indent}rem` }}
454475
>
455-
<span className="select-none shrink-0 flex items-center">
456-
{isCheckbox ? (
457-
block.checked ? (
458-
<svg
459-
className="w-4 h-4 text-success"
460-
viewBox="0 0 24 24"
461-
fill="none"
462-
stroke="currentColor"
463-
strokeWidth={2.5}
464-
>
465-
<path
466-
strokeLinecap="round"
467-
strokeLinejoin="round"
468-
d="M9 12.75L11.25 15 15 9.75M21 12a9 9 0 11-18 0 9 9 0 0118 0z"
469-
/>
470-
</svg>
471-
) : (
472-
<svg
473-
className="w-4 h-4 text-muted-foreground/50"
474-
viewBox="0 0 24 24"
475-
fill="none"
476-
stroke="currentColor"
477-
strokeWidth={2}
478-
>
479-
<circle cx="12" cy="12" r="9" />
480-
</svg>
481-
)
482-
) : (
483-
<span className="text-primary/60">
484-
{(block.level || 0) === 0
485-
? "\u2022"
486-
: (block.level || 0) === 1
487-
? "\u25E6"
488-
: "\u25AA"}
489-
</span>
490-
)}
491-
</span>
476+
<ListMarker
477+
level={block.level || 0}
478+
ordered={block.ordered}
479+
orderedIndex={orderedIndex}
480+
checked={block.checked}
481+
/>
492482
<span
493483
className={`text-sm leading-relaxed ${isCheckbox && block.checked ? "text-muted-foreground line-through" : "text-foreground/90"}`}
494484
>

packages/ui/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ export interface Block {
5858
level?: number; // For headings (1-6) or list indentation
5959
language?: string; // For code blocks (e.g., 'rust', 'typescript')
6060
checked?: boolean; // For checkbox list items (true = checked, false = unchecked, undefined = not a checkbox)
61+
ordered?: boolean; // For list items: true when source marker was \d+.
62+
orderedStart?: number; // For ordered list items: integer parsed from the marker (e.g. 5 for "5.")
6163
order: number; // Sorting order
6264
startLine: number; // 1-based line number in source
6365
}

0 commit comments

Comments
 (0)