docs: retire the last references to the pre-marked renderer - #888
Conversation
The Markdown work replaced mobile's markdown-it + react-native-markdown-display renderer with a marked lexer and the shared normalizer, but a few comments still describe the architecture it removed: - shared/src/markdown.ts opened by explaining that the two clients "reach the same feature set by completely different routes", which is the opposite of what the shared normalizer now guarantees. - Two comments in shared/src/inlineMarkdown.ts cite markdown-it, one of them pointing at a mobile/src/utils/markdown.tsx that does not exist under that extension. Both rationales stand on their own without the dead analogy. - The spec explained the absence of smart typography as markdown-it's typographer being switched off, and repeated the markdown.tsx pointer. Also drops the markdown-it override from mobile/package.json. Nothing depends on markdown-it since the renderer swap, and regenerating the lockfile with the override removed produces no diff, which is what confirms it was inert. Adds a by-name pointer to docs/specs/markdown-rendering.md from both stack entries in CLAUDE.md; the directory-tree comment already named the spec, but nothing linked it from the libraries it governs. No behaviour change: comments, docs, and a dead dependency override. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj3Rv8vAT86Yk6tAgsWKWm
|
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 pull request updates Markdown rendering documentation to describe shared Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CLAUDE.md`:
- Around line 557-559: Update both stack entries in CLAUDE.md to use Markdown
link syntax, including descriptive link text and the destination
docs/specs/markdown-rendering.md, instead of wrapping the paths in backticks.
Preserve the existing descriptions and ensure both references render as
clickable links.
🪄 Autofix
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: 87996a61-3a54-4c44-b727-686315bef710
📒 Files selected for processing (5)
CLAUDE.mddocs/specs/markdown-rendering.mdmobile/package.jsonshared/src/inlineMarkdown.tsshared/src/markdown.ts
💤 Files with no reviewable changes (1)
- mobile/package.json
Both stack entries wrapped docs/specs/markdown-rendering.md in backticks, so it rendered as a code span rather than something you can click through to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj3Rv8vAT86Yk6tAgsWKWm
2 participants
What
Cleanup left over from the Markdown program (#816, #817, #818, #819, #822 and the list-item work that followed). Mobile's renderer moved from
markdown-it+react-native-markdown-displayto amarkedlexer feeding the shared normalizer, but a handful of comments still describe the architecture that replaced.Stale comments
shared/src/markdown.tsopened by explaining that the two clients "reach the same feature set by completely different routes" — the opposite of whatblockMarkdown.ts/inlineMarkdown.tsnow guarantee. Rewritten to say what is actually true: both lex with marked and normalize through the same two modules, and only the renderers differ.shared/src/inlineMarkdown.tscite markdown-it, one pointing at amobile/src/utils/markdown.tsxthat does not exist under that extension (it is.ts). Both rationales — why this module imports nothing frommarked, and why an empty link label falls back to its href — stand on their own, so the dead analogies are dropped rather than re-pointed.docs/specs/markdown-rendering.mdexplained the absence of smart typography as markdown-it'stypographerbeing explicitly switched off, and repeated themarkdown.tsxpointer. The behaviour documented was correct; the mechanism no longer exists. Reworded to keep the history (mobile did have it) without implying a switch someone could go looking for.Dead dependency override
mobile/package.jsonstill pinnedmarkdown-it: ^14.2.0inoverrides. Nothing has depended on markdown-it since the renderer swap — regenerating the lockfile with the override removed produces no diff, which is what confirms it was inert rather than load-bearing.Spec discoverability
docs/specs/markdown-rendering.mdwas named in CLAUDE.md's directory-tree comment but not linked from either place the libraries it governs are listed. Added a by-name pointer to the webapp and mobile stack entries, matching howfile-attachments.mdis referenced from the blobstore section.Not in scope
The spec documents one known gap that this PR does not touch — text ↔ list conversion does not pre-check
ITEM_MAX_COUNTorITEM_TEXT_MAX_LENGTH, so an oversized note fails with a generic "failed to convert". Filed separately as #889.Testing
task checkpasses (lint + shared, server, webapp and mobile suites).No visual artifact: this is comments, docs, and a dependency override — no behaviour change, and nothing user-facing to screenshot.