feat: render an inline Markdown subset in list-note item text - #825
Merged
Conversation
List-item text was plain, with bare-URL autolinking as its only formatting. It now renders an inline-only subset of what text notes support: bold, italic, strikethrough, inline code, and links under the same scheme policy. Block syntax stays literal. An item is already a list item — it carries its own checkbox, one-level nesting and position — so `# x`, `- [ ] x`, `---` and table pipes have nothing left to describe. That falls out of lexing item text as inline content rather than parsing it as a document, so there is no suppression code to keep in step with the block rules. Rendering is on display surfaces only: note cards, mobile's read-only editor row, and the collapsed-completed parent label. The editable row is an always-live input with no preview mode and still shows its source; giving it a view/edit swap is #824. Both clients lex with marked and normalize through a new shared module, so the policy decisions — link schemes, image and raw-HTML degradation, which constructs survive — are made once and only leaf rendering differs. marked is a type-only import in shared/, which keeps @jot/shared free of runtime dependencies. This puts marked in mobile's tree alongside react-native-markdown-display until #822 removes the latter. The inline renderer is also the cheap proof that marked resolves under Metro on RN 0.86, which is the assumption #822 rests on: verified with a full `expo export` bundle. The webapp's LinkText had no callers left and is removed. Mobile keeps its copy for the server-setup screen, and its URL-opening helper moves to src/utils/openUrl.ts so the log-no-note-content rule is stated once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuHXcppq3BV4CfcT17cjj5
Contributor
|
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 PR defines an inline Markdown subset for list-item display surfaces while editable rows preserve source text. Shared token normalization supports formatting, links, line breaks, and literal fallback handling. Webapp and React Native renderers consume the normalized representation. Link validation and URL opening are centralized. Shared, client, browser, and component tests cover conformance and edge cases. Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
hanzei
marked this pull request as ready for review
August 3, 2026 17:36
Both consumers compile shared/src with their own tsc, and module resolution runs from shared/ — mobile's @jot/shared is a symlink and resolution follows the realpath, so a lookup never reaches the consumer's node_modules. CI installs dependencies in webapp/ and mobile/ only, so shared/node_modules does not exist during either typecheck and even a type-only import of marked fails to resolve. It passed locally only because shared/ had been installed. Same trap as the @babel/runtime note in CLAUDE.md, and the same fix mobile's own markdown.tsx uses for markdown-it: declare the fields we read as an interface. marked's Token union is structurally assignable to it, so callers still pass Lexer.lexInline output with no cast. marked stays a devDependency of shared/ for its own test suite, which shared-ci.yml does install. Verified by typechecking both consumers with shared/node_modules removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuHXcppq3BV4CfcT17cjj5
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@shared/src/markdownCases.ts`:
- Around line 1-15: Update the top-level documentation comment in
markdownCases.ts to replace the stale markdown.test.ts references with the
actual inlineMarkdown.test.ts and inlineMarkdown.test.tsx files that exercise
MARKDOWN_ITEM_CASES, while preserving the existing explanation of the two
corpora.
In `@webapp/src/components/NoteModal.tsx`:
- Around line 1746-1749: Update the ghost-parent accessibility label around
InlineMarkdown to use the same Markdown-normalized plain text displayed to users
instead of raw parent.text. Strip formatting markers and flatten links, code,
and other rendered nodes before passing the title to
t('note.completedItemGroup', ...), while preserving the existing visible
InlineMarkdown rendering.
🪄 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 Plus
Run ID: 42fe8316-7589-4308-aaad-dd8a7413db65
⛔ Files ignored due to path filters (2)
mobile/package-lock.jsonis excluded by!**/package-lock.jsonshared/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
docs/specs/markdown-rendering.mdmobile/__tests__/ListItem.test.tsxmobile/__tests__/inlineMarkdown.test.tsxmobile/jest.config.jsmobile/package.jsonmobile/src/components/InlineMarkdown.tsxmobile/src/components/LinkText.tsxmobile/src/components/ListItem.tsxmobile/src/components/NoteCard.tsxmobile/src/utils/openUrl.tsshared/package.jsonshared/src/__tests__/inlineMarkdown.test.tsshared/src/index.tsshared/src/inlineMarkdown.tsshared/src/markdownCases.tswebapp/e2e/tests/markdown.spec.tswebapp/src/components/InlineMarkdown.tsxwebapp/src/components/LinkText.tsxwebapp/src/components/NoteCard.tsxwebapp/src/components/NoteModal.tsxwebapp/src/index.csswebapp/src/utils/__tests__/inlineMarkdown.test.tswebapp/src/utils/markdown.ts
💤 Files with no reviewable changes (1)
- webapp/src/components/LinkText.tsx
…abel Two review findings. The collapsed-completed group's aria-label was built from raw parent.text while the row below it now renders markdown, so a screen reader heard "star star Milk star star" for text the eye reads as bold Milk. aria-label replaces the element's content for assistive tech, so the markers were the only thing announced. It matched before this branch, when the visible text was raw too — the divergence is a regression this feature introduced. Adds flattenInlineNodes to shared (beside the node type it walks) and a thin inlineMarkdownToText wrapper in the webapp. A corpus-wide test pins the invariant that matters: the flattened label equals the textContent of the rendered HTML, for every item case. Also updates the markdownCases.ts header, which named only the two text-note suites and so implied they covered MARKDOWN_ITEM_CASES. It now lists the suites per corpus, and fixes the mobile filename (.tsx, not .ts). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuHXcppq3BV4CfcT17cjj5
The same defect just fixed in the webapp's ghost-parent label. A read-only row renders its text, so the checkbox's accessibilityLabel built from raw source announced markers the user never sees. Flattened unconditionally rather than only when read-only: a control's accessible name should identify the item by its words in either mode, and this stays correct when #824 renders the editable row too. Adds mobile/src/utils/inlineMarkdown.ts, which also takes over the lexing the renderer component was doing inline — one place on this client decides how item text is parsed, matching webapp/src/utils/markdown.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuHXcppq3BV4CfcT17cjj5
hanzei
added a commit
that referenced
this pull request
Aug 4, 2026
…ormatting and nesting (#827) * feat: convert notes by the rendered Markdown subset, keeping inline formatting and nesting Text ↔ list conversion had its own opinion of what formatting to strip, pinned to the webapp's renderer ("It only needs to handle the syntax subset webapp/src/utils/markdown.ts renders"). #823 widened that subset and #825 gave list items an inline subset of their own, so the premise no longer held and the stripper was deleting formatting the destination now displays. Conversion now follows from the render subsets instead of restating them. Because the item subset (spec §2.1) is a strict subset of what text notes render, the rule is mechanical: remove the block markdown an item structurally replaces — its bullet, its checkbox, a heading prefix, blockquote markers — and keep everything else exactly as typed. Inline syntax survives because the item renders it. Block syntax that is not a line prefix (a fence, `---`, a table row, an image) survives too, because §2.1 shows it as literal source. That deletes stripInlineFormatting outright rather than extending it to the newly rendered syntax, and with it the `` → `!alt` mangling the link regex produced. The ReDoS reasoning behind its character classes is kept as a comment, since the hazard returns with any future regex scan of note content. Nesting now survives text → list. listToText has always emitted two-space indentation for children, but the reverse threw it away, so a nested list round-tripped through a text note came back flat. An indented line that also carries a list marker becomes a child of the nearest preceding top-level item. Indentation alone does not qualify: an indented line without a marker is usually a wrapped paragraph, and re-parenting it silently is worse than dropping nesting nobody asked for. The clients cannot send parent_id (the item ids do not exist yet), so they send indent_level and the server's existing buildCreateNoteItems rebuilds parent_id — no server change was needed. Mobile mirrors that same walk locally so an offline conversion matches its eventual replay. listToText still emits item text unescaped, now deliberately: both sides lex the same source with the same inline rules, so escaping would introduce a rendering change rather than prevent one, and every item is emitted behind a `- [ ] ` marker that keeps block syntax literal. Documented as spec §2.2, next to the subset it derives from. Known gap, unchanged by this commit and worth its own issue: conversion does not check ITEM_MAX_COUNT or ITEM_TEXT_MAX_LENGTH before sending, so a note over either cap fails with a generic "failed to convert" toast. The webapp's paste path does guard this; the convert path does not. Keeping inline markers makes the text-length cap marginally easier to hit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013qcu6eapDXfh5xsW42LL4i * fix: strip blockquote markers on both sides of a list marker, correct round-trip claim Review feedback on #827. `> - [x] Child` is a quoted checklist, but the blockquote strip ran only after the list-marker match, so the marker never matched: the item kept "- [x]" in its text and lost the completed state. Per spec §2.1 that text then renders as a literal checkbox beside the item's own real one, which is the exact outcome the subset exists to prevent. Stripping blockquotes on both sides of the marker rather than moving the strip, because the two nest in either order: moving it alone would fix `> - [x] a` and break `- > a`, leaving the `>` in the item text. The spec also overclaimed the round trip. list → text → list does return the same items, but text → list → text *normalizes* — an item has one representation, so `# Groceries`, `* Eggs`, `1. Eggs` and `> Eggs` all come back as task lines. And an item whose text begins with `#` or `>` loses that prefix on the way back, since the converter cannot tell it apart from the block markup it strips. Both are now stated in §2.2 and asserted in tests, and the e2e comment making the same overclaim is corrected. Verified separately that the no-escaping argument is unaffected: `# foo`, `> quoted`, `---` and table pipes all stay literal inside a task item under marked, while `**bold**` still renders. That claim was about rendering and holds; only the converter's own round trip is lossy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013qcu6eapDXfh5xsW42LL4i --------- Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Aug 9, 2026
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
2 participants
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.
List-note item text was plain, with bare-URL autolinking (
LinkText) as its only formatting. It now renders an inline-only subset of what text notes support.Follows the exploration in this session; the editable-row half is filed separately as #824.
What renders
**bold**,*italic*,~~strike~~`inline code`[text](url), barehttps://…http/https/mailtopolicy[alt](url), raw HTML# x,- x,1. x,- [ ] x,---,> xand table pipes all stay exactly as typed. An item is a list item — it has its own checkbox, its own one-level nesting and its own position — so block syntax inside one has nothing left to describe, and- [ ]in particular would be a second checkbox beside the real one.That falls out of lexing item text as inline content rather than parsing it as a document. There is no suppression code, and the block rules and item rules cannot drift on block syntax even in principle — which is why this renderer is a fraction of the size of the full one.
Where it renders — and where it deliberately doesn't
Display surfaces only: note cards (both clients), mobile's read-only editor row, and the webapp's collapsed-completed parent label.
The editable row still shows raw source. It is an always-live input with no preview mode, and giving it a view/edit swap raises a set of real questions — caret placement on click-to-edit, link-click vs caret-click, row height at mount, keyboard handling in render mode. Those are #824, with the option of a whole-list preview toggle instead of a per-row swap listed first.
So a user who types
**milk**sees milk on the card and**milk**in the editor. That gap is deliberate and tracked, not an oversight.Architecture
Both clients lex with
markedand normalize through a newshared/src/inlineMarkdown.ts, so the policy decisions — link schemes, image and raw-HTML degradation, which constructs survive — are made once. Only leaf rendering differs: an HTML string plus a DOMPurify allowlist in the webapp, a<Text>tree on mobile. Everything on mobile is aTextand never aView, so the RN text-nesting problem that dominates a block renderer never arises.markedis a type-only import inshared/and sits in devDependencies —@jot/sharedstill has no runtime dependencies.Two notes for reviewers:
INLINE_LEXER_OPTIONS) rather than inherited from each client's globalmarked.use(). The webapp registers a full block renderer at import time, and item rendering must not depend on configuration set for something else.bris its own node type. A literal\ncollapses to a space in HTML, so the webapp needs a real<br>while mobile wants the newline; neither is derivable from the other in the renderer.Relationship to #822
This puts
markedin mobile's tree alongsidereact-native-markdown-display, so mobile carries two Markdown libraries until #822 removes the second. That is the main cost of this PR and worth weighing explicitly.In exchange it de-risks #822's central assumption. That ticket rests on
markedresolving under Metro on RN 0.86, which was research rather than a verified fact. Verified here with a fullexpo export: Metro resolves it, it compiles into the Hermes bundle (7.1MB → 7.2MB), no config changes needed. If it had failed, it failed on a small inline renderer rather than on the full block rewrite.markdown-itwas the alternative. It is not actually a dependency of mobile — it appears only inoverrides, andreact-native-markdown-displaydeclares^10.0.0while running against the hoisted14.2.0. Building on it meant either importing an undeclared package or promoting a dependency #822 then removes. It is also more code for this subset:marked'sgfm: trueis the autolinking behaviour the spec defines, where markdown-it needs linkify plus the existing 33-linegfmAutolinksOnlyunwrapping rule.Also in this PR
LinkTexthad no callers left and is removed. Mobile keeps its copy for the server-setup screen; its URL-opening helper moves tomobile/src/utils/openUrl.tsso the "log the scheme, never note content" rule is stated once.markedadded to mobile's JesttransformIgnorePatterns— it ships ESM only. This is a Jest concern; Metro consumes it as-is.docs/specs/markdown-rendering.md§1, new §2.1, §5 and §6.Testing
MARKDOWN_ITEM_CASES(30 cases), asserted three times: at the normalizer (shared/), and once per client, each failing if any case id has no expectation — the same drift guard the existing corpus uses.webapp/e2e/tests/markdown.spec.ts: the subset rendering on a card, block syntax staying literal, and the editable row still showing source (that last assertion is what Render Markdown in list-item editor rows (view/edit swap) #824 will need to update).task lintandtask testpass.task test-e2eformarkdown.spec.ts+accessibility.spec.tspasses (26 tests), including the axe scans in both themes, which cover the new rendered links and code spans.Two environment notes, in the interest of not overclaiming: the pinned Playwright Chromium could not be downloaded in this sandbox, so e2e ran against the pre-installed build via a throwaway config override (not committed) — CI runs the pinned one. And
mobile/__tests__/SettingsScreen-offline.test.tsxtimed out on one run; it does the same on a clean tree and passed on the full re-run, so it is a pre-existing flake unrelated to this change.API and compatibility
No API, schema or data changes. Item text is stored exactly as before — this is purely how existing strings are displayed, so nothing migrates and older clients keep showing the source.
Screenshots
Not included: rendering was verified through the e2e assertions above and the axe scans rather than by driving a browser for capture in this environment.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EuHXcppq3BV4CfcT17cjj5
Generated by Claude Code