Render Markdown in the webapp's editable list-item rows - #868
Conversation
A list-item row now shows its Markdown rendered until it holds the caret, and its source for exactly as long as it does. Closes the gap where a card read **Milk** and the editor read `**Milk**`, which looked like a rendering bug rather than a deliberate limit. Focused and editing are the same state. That is the decision the rest follows from: every keystroke handler stays on a real textarea, because a row with the caret *is* one — no render-mode duplicate of Tab, Enter or the suggestion arrows, and no focusable non-interactive element for axe to flag. Four things that fall out of it: - The textarea is never unmounted, only moved out of flow at opacity-0, so NoteModal's imperative focus paths keep working through itemInputRefs and the field comes back at a height measured while it was on screen. - A row only swaps when rendering changes something. `buy milk` renders to itself, so a list with no Markdown behaves exactly as it did before. - An editable row's links are inert and a read-only row's are live: a row that owns a caret does not own a link. - Completed rows render, accepting the ~~strike~~ / line-through collision that the spec already accepted for display surfaces. A click maps back to a source offset rather than landing at 0: nodes now carry source spans and inlineSourceOffset walks them, both in shared/ so mobile can reuse them. Both forms share one class list for layout, including the inline-block/overflow pairing that keeps a span's baseline where a textarea's is — without it every row silently lost 7px when unfocused. The grip prevents its default mousedown so a drag never collapses the row dnd-kit just measured. Mobile's editable row is unchanged; #867 tracks it and records these decisions so only the keyboard-churn question is left open there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QuVeVda1EiJrJrdzsybRAr
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change adds source-span tracking and rendered-to-source caret mapping for inline Markdown. The webapp now renders formatted list items when rows are unfocused and restores the persistent textarea during editing. It preserves row height, keyboard interactions, links, completed-item styling, and drag behavior. Tests cover shared mapping utilities, component behavior, and browser interactions. Documentation records the webapp behavior and the mobile limitation. Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
webapp/src/utils/inlineCaret.ts (1)
60-62: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe container branch ignores the child index.
When
node === container, the function returns the total text length and discardsoffset. The browser reports the container with a child index, and that index is not always the last child. A click in the leading padding of the row therefore maps to the end of the text instead of the start. The comment above the return describes the offset as a child index, so the value is available.Sum the text of only the first
offsetchildren to keep the two cases consistent.♻️ Proposed change to honour the child index
- // The position is inside `container` but the walk never reached it, which - // means `node` is the container itself with a child index for an offset. - return node === container ? consumed : null; + // The position is inside `container` but the walk never reached it, which + // means `node` is the container itself with a child index for an offset. + if (node !== container) return null; + let before = 0; + for (const child of Array.from(container.childNodes).slice(0, offset)) { + before += child.nodeType === Node.TEXT_NODE + ? (child as Text).length + : renderedOffsetOf(container, child, 0) === null + ? 0 + : (child.textContent?.length ?? 0); + } + return before;🤖 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 `@webapp/src/utils/inlineCaret.ts` around lines 60 - 62, Update the container branch in the caret-position function so that when node === container it returns the text length of only the first offset children, rather than the total consumed length. Preserve the null result for other nodes and use the existing text-measurement traversal to handle the child-index offset consistently.webapp/e2e/tests/markdown.spec.ts (1)
364-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the new list-row locators into the page object.
Lines 365, 416 and 445 build raw
page.locator('[data-testid="list-item-row"]')andlist-item-renderedselectors inside the spec. Line 186 does the same.DashboardPagealready owns list-item locators, because the same tests calldashboardPage.listItemInput(0)anddashboardPage.expectListItemValue(...). AddinglistItemRow(index)andlistItemRendered(index)toDashboardPagekeeps the selector in one place.As per path instructions: "Use Playwright Page Object Model classes under
e2e/pages/and tests undere2e/tests/".♻️ Proposed change in the spec once the page object exposes the locators
- /** The rendered form of the row at `index`, if it is showing one. */ - const rendered = (page: Page, index: number) => - page.locator('[data-testid="list-item-rendered"]').nth(index); + /** The rendered form of the row at `index`, if it is showing one. */ + const rendered = (dashboardPage: DashboardPage, index: number) => + dashboardPage.listItemRendered(index);Also applies to: 416-416, 445-446
🤖 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 `@webapp/e2e/tests/markdown.spec.ts` around lines 364 - 366, Move the list-row locator helpers out of the markdown spec and into the DashboardPage page object: add listItemRow(index) and listItemRendered(index) methods alongside its existing list-item APIs. Update the spec’s rendered helper and the raw selectors at the referenced locations to call these page-object methods, preserving the existing index-based locator behavior.Source: Path instructions
🤖 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.
Nitpick comments:
In `@webapp/e2e/tests/markdown.spec.ts`:
- Around line 364-366: Move the list-row locator helpers out of the markdown
spec and into the DashboardPage page object: add listItemRow(index) and
listItemRendered(index) methods alongside its existing list-item APIs. Update
the spec’s rendered helper and the raw selectors at the referenced locations to
call these page-object methods, preserving the existing index-based locator
behavior.
In `@webapp/src/utils/inlineCaret.ts`:
- Around line 60-62: Update the container branch in the caret-position function
so that when node === container it returns the text length of only the first
offset children, rather than the total consumed length. Preserve the null result
for other nodes and use the existing text-measurement traversal to handle the
child-index offset consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2152b7aa-88f8-4c44-98b1-25808375b22e
📒 Files selected for processing (13)
docs/specs/markdown-rendering.mdmobile/__tests__/ListItem.test.tsxmobile/src/components/ListItem.tsxmobile/src/utils/inlineMarkdown.tsshared/src/__tests__/inlineMarkdown.test.tsshared/src/inlineMarkdown.tswebapp/e2e/tests/keyboard-focus.spec.tswebapp/e2e/tests/markdown.spec.tswebapp/src/components/SortableItem.tsxwebapp/src/components/__tests__/SortableItem.test.tsxwebapp/src/utils/__tests__/inlineCaret.test.tswebapp/src/utils/inlineCaret.tswebapp/src/utils/markdown.ts
Two review findings, both valid. DashboardPage gains listItemRow and listItemRendered alongside its existing listItemInput, and the specs call those instead of raw data-testid selectors. The row-scoped form is kept where it matters: in the literal-syntax test only one of three rows renders, so a global nth() would point at the wrong row. renderedOffsetOf mishandled an element position. A TreeWalker never yields its own root, so `node === container` fell out of the loop and returned the total text length, discarding the offset entirely — (container, 1) on `buy <strong>milk</strong>` gave 8 where it should give 4. An element offset is a child index, so it is now resolved to the first unconsumed node before the walk, which handles a nested element's child index by the same path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QuVeVda1EiJrJrdzsybRAr
An inline box is as tall as its line-height and sits around the shared baseline, so a child in a different font is offset differently from the line's strut and can push the line box past it. `.markdown-inline code` sets `font-mono`, which made an item row containing `code` taller when rendered than when showing its source — the source being one font throughout. A visible jump on click, sized by whichever mono font the reader happens to have: 0.6px here, unbounded elsewhere. `leading-none` keeps the code's inline box under the strut, so it cannot grow the line box on any font. It does not change the chip, because an inline element's background paints its content area, which comes from the font rather than from line-height. Measured across four body/mono pairings: the row delta was -0.61px on the default stack and 0.00 after, 0.00 on the rest either way. The height assertion in markdown.spec.ts now covers a code row, which is the one that regressed; a bold row never moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QuVeVda1EiJrJrdzsybRAr
The rendered view and the textarea it stands in for were matched by reproducing a textarea's baseline on a span: a textarea is an inline-block that baselines on its bottom margin edge, so the line box reserves descender space underneath, and `overflow` moves a span's baseline to the same place (CSS 2.1 §10.8.1). That held only on the font it was measured against. How much space a line box reserves is a property of the platform's font and UA stylesheet: on Windows the textarea did not reserve it and the span did, so every row grew about 7px the moment it lost focus — a jump that was exactly 0.00px here and plainly visible there. Blocks have no baseline to disagree about. Each box is then `lines × line-height + padding` from the same inherited metrics, equal by construction on any platform, and the vertical-align/overflow/display overrides that used to move the row by ±7px in a probe now all measure 0.00. Rows lose the descender space they used to carry, so a list is that much tighter. It was accidental rather than designed, it never applied to a focused row, and it is now absent from both states rather than one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QuVeVda1EiJrJrdzsybRAr
Closes #824 for the webapp. Mobile is split out into #867, which records the decisions made here so only its own open question is left.
A list-item row now shows its Markdown rendered until it holds the caret, and its source for exactly as long as it does. Type
**Milk**, move away, the row reads Milk; click back in and it reads**Milk**, with the caret where you clicked.The decisions, and where each one came from
#824 listed eight open questions. Resolved as follows — the reasoning is in
docs/specs/markdown-rendering.md§1.2, not just here.linksflag already existed — and it closes the divergence §1.1 complained about, since read-only rows previously showed source and now match mobileinlineSourceOffsetwalks themprefersReducedMotion~~strike~~/line-throughcollision §2.1 already acceptedQ5 is the load-bearing one. If focus implies editing, a row that has the caret is a textarea, so every existing handler — Tab to indent, Enter to split, arrow navigation, the suggestion combobox contract — stays exactly where it was. There is no render-mode duplicate of any of it, and no tabbable non-interactive element inside the dnd-kit wrapper, so no new axe surface and no new
AcceptedViolation.How it works
The textarea is never unmounted, only moved out of flow at
opacity-0. That is what keeps NoteModal's imperative focus paths (itemInputRefs→ Enter-to-split, arrow navigation, "add item") working on a row that happens to be rendered, and what lets the field come back at a height measured while it was on screen rather than at the moment of focus.visibility: hiddenanddisplay: noneboth remove an element from the focus order, so neither works here.A row only swaps when rendering changes something.
buy milkrenders tobuy milk; so do# not a headingand[alt](url), which §2.1 shows as literal source.inlineRendersAsSourcedecides this, and it compares the text rather than looking for formatting nodes — an escape (\*) is all-text and still renders differently. The effect is that a list with no Markdown in it is byte-for-byte the behaviour it had before this PR, which is most lists.A click maps back to a source offset. The user points at character 4 of
buy milkand the field holdsbuy **milk**, where that character is at 6.normalizeInlineTokensnow optionally records a source span per node,inlineSourceOffsetmaps a rendered offset back through them, andwebapp/src/utils/inlineCaret.tsgets the rendered offset from the browser. Both halves of the map live inshared/, so #867 inherits them. Without this the caret lands at 0 on every click — which is what the text-note editor does today, and is tolerable there only because it happens once per note instead of once per row.Both forms are the same height. They share one class list, including two properties that are not obvious: a textarea is an
inline-blockthat baselines on its bottom margin edge, so the line box reserves 7px of descender space that is part of every list row today; a span only does the same with a non-visibleoverflow(CSS 2.1 §10.8.1). The first version of this usedblockand every row silently lost 7px when it lost focus — caught by the height assertion inmarkdown.spec.ts, which is why that test exists.Visual artifacts
Screenshots (light + dark) and a screen recording of the click-to-edit and row-to-row transitions were captured against a real server and reviewed. They still need attaching to this PR by hand — the images cannot be uploaded through the API, so they are not inlined above rather than left as broken links. What they show:
`sourdough` from the corner placeas source while every other row holds its exact position.**today**into a row and arrowing away to see it render, and ticking a checkbox without the row leaving its rendered form.Testing
task check— passes.task test-e2e— passes. New coverage: six cases inmarkdown.spec.ts(swap, caret placement, row height, no-Markdown rows, completed rows, inert links) and three inkeyboard-focus.spec.ts(split/navigate between rendered rows, Tab-to-indent a rendered row, keyboard reorder of a rendered row), perwebapp/CLAUDE.md.SortableItem.test.tsxgains nine tests for the state machine;inlineCaret.test.tscovers the DOM offset walk;sharedgains source-span,inlineSourceOffsetandinlineRendersAsSourcesuites, the last of which classifies every case inMARKDOWN_ITEM_CASES.AcceptedViolationentries.Two unrelated specs (
toast-timing,notes.specduplicate) each flaked once across two full e2e runs under 4-worker load, and passed in the other run and in isolation, with and without this change.Notes for review
renderInlineMarkdownis untouched, so note cards and the collapsed-completed label produce byte-identical HTML. The newrenderInlineItemis the only caller that lexes with source tracking on.[call](tel:+15550100)row now swaps, because a disallowed scheme drops the target and keeps the label (§3) — so the rendered form genuinely differs from the source. Thetel:URL is one click away in the field. My first version of the literal-syntax e2e assertion got this wrong and the test caught it.inlineSourceOffset.