feat: align Markdown support between webapp and mobile - #823
Merged
Conversation
Text notes rendered differently in the two clients: the webapp ran marked through an 11-tag DOMPurify allowlist while mobile ran markdown-it with no allowlist at all, so strikethrough typed on mobile lost its formatting in the browser, `- [x]` (the syntax list-to-text conversion emits) landed on the webapp's weakest case, and mobile fetched arbitrary third-party URLs out of note content. Both clients now render one feature set, specified in docs/specs/markdown-rendering.md: - Supported: h1-h6 (h4-h6 as bold body text), bold/italic/strike, inline and block code, lists, non-interactive checkboxes, blockquotes, links, rules. - Shown as literal source: images, tables, raw HTML. - No smart typography, on either client. - Link schemes restricted to http/https/mailto, so a shared note cannot drive navigation into `tel:`, `sms:` or an app deep link. shared/ owns the two things that must be byte-identical (the scheme policy and the literal-image format) plus the conformance corpus both unit suites run, which fails if a case is covered on one client and forgotten on the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UahhmDPDzUjrngqeWeuDTw
hanzei
marked this pull request as ready for review
August 3, 2026 15:23
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 change defines a canonical Markdown specification and shared conformance corpus. Shared utilities enforce safe link schemes and literal image formatting. Web and mobile renderers now align on supported syntax, unsupported content, typography, links, images, tables, and raw HTML. Both clients use synchronized tests, with added web end-to-end coverage and mobile parser and interaction checks. Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 3
🤖 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 `@mobile/src/utils/markdown.tsx`:
- Around line 157-160: Update the press handler around Linking.openURL in the
press function to attach a rejection handler to the returned promise, preventing
unhandled rejections when the URL cannot be opened while preserving the existing
onLinkPress behavior.
- Around line 91-116: Ensure the MarkdownIt configuration around html, linkify,
and md.disable('table') prevents URL autolinking inside raw HTML text and
disabled table content, preserving those regions as inert literal text. Add
mobile coverage for URLs within displayed tags and pipe-table rows, or revert
linkifications spanning those protected regions without changing normal bare-URL
linking elsewhere.
In `@shared/src/markdown.ts`:
- Around line 24-28: Update the link parsing flow around isAllowedLinkHref so
scheme-less autolinks such as www.example.com are rejected before parser
normalization to http/https. Apply the restriction at parser time, or add
coverage and configuration ensuring marked with gfm and markdown-it with linkify
do not emit normalized links for scheme-less input; preserve acceptance of
explicitly allowed schemes.
🪄 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: 76fcb1d1-dea8-455b-85f5-86da8cf173b4
📒 Files selected for processing (14)
CLAUDE.mddocs/specs/markdown-rendering.mdmobile/__tests__/markdown.test.tsxmobile/src/screens/NoteEditorScreen.tsxmobile/src/utils/markdown.tsxmobile/src/utils/markdownStyles.tsshared/src/__tests__/markdown.test.tsshared/src/index.tsshared/src/markdown.tsshared/src/markdownCases.tswebapp/e2e/tests/markdown.spec.tswebapp/src/index.csswebapp/src/utils/__tests__/markdown.test.tswebapp/src/utils/markdown.ts
Three review findings, all real: - Linking.openURL rejects when nothing can handle a URL, and the href comes straight out of note content, so that is a normal outcome rather than a crash. Attach a rejection handler (and do not log it — note content never goes to the log). - linkify was reaching into regions the webapp shows as literal source: a URL in a table cell, or inside an href attribute, became a live link on mobile and stayed text in the browser. Parse tables and raw HTML instead of disabling them, then collapse the tokens into literal text afterwards, which discards the parsed contents along with the links. This also fixes an HTML block swallowing its Markdown on the webapp but not on mobile. - linkify-it autolinks a bare example.com; marked's GFM autolinker needs a scheme or a www. prefix. Unwrap the extra links rather than turning fuzzyLink off, which would also drop www.example.com — one marked does link. Corpus gains cases for each, so both clients stay pinned to the same answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UahhmDPDzUjrngqeWeuDTw
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 4, 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.
Closes #816.
The two clients rendered text notes with different libraries and had drifted: the webapp ran
markedthrough an 11-tag DOMPurify allowlist, mobile ranmarkdown-itwith no allowlist at all. Both now render one feature set, written down indocs/specs/markdown-rendering.md.Behaviour
####–######~~strike~~- [ ]/- [x]<pre>stripped, no block layout / block layouthttps://…---/***hr[alt](url)[alt](url "title")--,"…"Linking.openURLunfilteredhttp,https,mailtoonlyUser-visible changes worth calling out
docs/specs/file-attachments.md("no markdown/inline embedding" — images are a separate gallery) and meant a collaborator's note could drive a third-party request.http/https/mailtoare no longer followed on either client; they render as their label with nothing to tap. Mobile'slinkrule previously calledLinking.openURL(href)with no filtering at all, sotel:,sms:and arbitrary app deep links were followed.No API or schema change. No migration.
Two decisions that differ from the issue text
####–######render as bold body text, not as literal source and not silently stripped. Both clients emit real heading elements (so the document outline survives for assistive tech) and style them down —.markdown-content :is(h4, h5, h6)in the webapp,markdownStyles.tson mobile.<…>out of mobile's text tokens, which is lossy; escaping in the webapp renderer instead makes both clients agree and puts raw HTML in the same bucket as images and tables.#### foowas the only other member of that category and it moved (see 1), so "formatting dropped, text kept" is now empty — every unsupported construct shows its source. The spec says so explicitly rather than leaving the category undocumented.Also deviating on the pinned image format: the issue specified
[alt](src "title"), but the leading!is kept —[alt](src "title")— so what the reader sees is really what they typed and is not ambiguous with a link. Pinned informatLiteralImageand used by both clients.Structure
shared/owns the parts that must be byte-identical, because the two implementations have no other shared code path:shared/src/markdown.ts— link-scheme policy + the literal-image format.shared/src/markdownCases.ts— the conformance corpus. Both unit suites assert one expectation per case id and fail if an id has none, so a case cannot be covered on one client and forgotten on the other.The implementation traps (marked has no
imagetokenizer to disable anduse()treats afalse-returning tokenizer as "fall through to the default"; markdown-it's.disable('image')yields a live link, while.disable('table')works correctly; RNMD marks image tokensblock: true) are documented in §5 of the spec and in comments at each site.Testing
task check— full gate, green (1269 mobile tests, all webapp/shared/server suites).task test-e2e— 299 passed, 3 skipped.webapp/e2e/tests/markdown.spec.tsgains two specs covering the newly supported syntax and the literal-source/link-scheme cases.mobile/__tests__/markdown.test.tsx,shared/src/__tests__/markdown.test.ts; extendedwebapp/src/utils/__tests__/markdown.test.ts.Screenshots
Webapp, one kitchen-sink note — top and scrolled:
Mobile screenshots were not feasible: this environment has no simulator or device to render the app on. The mobile behaviour is covered by
mobile/__tests__/markdown.test.tsx, which asserts against the real parser and the real rendered component tree.Generated by Claude Code