perf(markdown): reduce room-open render cost with parse cache and compiler memoization#7348
perf(markdown): reduce room-open render cost with parse cache and compiler memoization#7348diegolmello wants to merge 1 commit into
Conversation
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
WalkthroughRefactors Markdown to parse messages via a bounded in-memory cache and resolveTokens helper, introduces MarkdownBlockView to render Root block types, and integrates these with a styled Markdown component that safely computes and renders tokens. ChangesMarkdown Container Rendering Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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
🧹 Nitpick comments (1)
app/containers/markdown/index.tsx (1)
71-102: ⚡ Quick winConsider adding exhaustive type checking to the switch statement.
The component correctly uses the React 19 Compiler
'use memo'directive and handles all current block types. However, the switch lacks compile-time exhaustiveness checking. If new block types are added toRootin the future, TypeScript won't alert you to update this component.Optional: Add exhaustive check
const MarkdownBlockView = ({ block }: { block: MarkdownBlock }) => { 'use memo'; switch (block.type) { case 'BIG_EMOJI': return <BigEmoji value={block.value} />; case 'UNORDERED_LIST': return <UnorderedList value={block.value} />; case 'ORDERED_LIST': return <OrderedList value={block.value} />; case 'TASKS': return <TaskList value={block.value} />; case 'QUOTE': return <Quote value={block.value} />; case 'PARAGRAPH': return <Paragraph value={block.value} />; case 'CODE': return <Code value={block.value} />; case 'HEADING': return <Heading value={block.value} level={block.level} />; case 'LINE_BREAK': return <LineBreak />; case 'KATEX': return <KaTeX value={block.value} />; default: + // Exhaustive check: if a new block type is added, TypeScript will error here + const _exhaustive: never = block; + void _exhaustive; return null; } };This ensures that if
Rootgains new block types, the compiler will flag this location.🤖 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 `@app/containers/markdown/index.tsx` around lines 71 - 102, The switch in MarkdownBlockView handling block.type should be made exhaustive so TypeScript will error if a new MarkdownBlock type is added; add an assertUnreachable helper (e.g., function assertUnreachable(x: never): never { throw new Error(`Unexpected block type: ${String(x)}`); }) and replace the default branch with a call like return assertUnreachable(block.type) (or assign block.type to a never-typed variable and return it) so the compiler enforces exhaustiveness for MarkdownBlockView and its block.type union.
🤖 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 `@app/containers/markdown/index.tsx`:
- Around line 63-69: The parameter type for resolveTokens is too narrow: change
the signature of resolveTokens (function name: resolveTokens) so msg accepts the
actual runtime types (e.g., msg: string | null | undefined) instead of just
string, and keep the existing defensive conversion inside (typeof msg ===
'string' ? msg : String(msg || '') ) so parseMessage receives a proper string;
ensure any callers that pass optional/null props still type-check against the
updated signature.
---
Nitpick comments:
In `@app/containers/markdown/index.tsx`:
- Around line 71-102: The switch in MarkdownBlockView handling block.type should
be made exhaustive so TypeScript will error if a new MarkdownBlock type is
added; add an assertUnreachable helper (e.g., function assertUnreachable(x:
never): never { throw new Error(`Unexpected block type: ${String(x)}`); }) and
replace the default branch with a call like return assertUnreachable(block.type)
(or assign block.type to a never-typed variable and return it) so the compiler
enforces exhaustiveness for MarkdownBlockView and its block.type union.
🪄 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
Run ID: de187e0b-fe57-4ebf-ae5f-688914c0fb44
📒 Files selected for processing (2)
app/containers/markdown/index.tsxapp/containers/markdown/styles.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/containers/markdown/styles.tsapp/containers/markdown/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode and baseUrl set to app/ for import resolution
Files:
app/containers/markdown/styles.tsapp/containers/markdown/index.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/containers/markdown/styles.tsapp/containers/markdown/index.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Reusable UI components should be placed in app/containers/ directory
Files:
app/containers/markdown/styles.tsapp/containers/markdown/index.tsx
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/containers/markdown/styles.tsapp/containers/markdown/index.tsx
🔇 Additional comments (6)
app/containers/markdown/index.tsx (5)
21-21: LGTM!Also applies to: 39-39
117-128: LGTM!
130-139: LGTM!
141-153: LGTM!
41-61: ⚡ Quick winConfirm FIFO eviction logic; document/guard Root immutability for cached shared instances.
parseCache.keys().next().valuerelies onMapinsertion order, so deletion is truly FIFO (and the 200-entry cap bounds memory).- Because
parseCacheis module-scoped, identical messages reuse the sameRootinstance; this is safe only if downstream code treatsRoot/its blocks as immutable (otherwise consider freezing/cloning or an explicit immutability guarantee).app/containers/markdown/styles.ts (1)
11-13: LGTM!
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.73.0.108979 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTtL_ar8AN4abqqBydXzyvRAM8XE_eeCrvpwA3_G7Aw-eBjB7dhHI9m9L_YxSvmoElvYirCBKVxw9Ls3TqT |
diegolmello
requested a deployment
to
android_build
GitHub Actions
Waiting
|
iOS Build Available Rocket.Chat 4.73.0.108980 |
…piler memoization Markdown was a top offender when opening a room. Cache parsed tokens, split block rendering, stabilize context, and rely on React Compiler instead of manual useMemo. Co-authored-by: Cursor <cursoragent@cursor.com>
diegolmello
force-pushed
the
cursor/5887f67d
branch
from
8e5f82d to
c14a1d9
Compare
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
|
Actionable comments posted: 0 |
diegolmello
requested a deployment
to
android_build
GitHub Actions
Waiting
2 participants
Proposed changes
Opening a room was spending significant time re-parsing markdown for every message. This PR reduces that cost by:
MarkdownBlockViewcomponent with React Compiler'use memo'styles.tsIssue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1184
How to test or reproduce
isTranslatedpath should still re-parse)Benchmark (iOS sim, Argent React Profiler)
Controlled A/B on the same app state — original
Markdownvs this PR (compiler-only, no manualuseMemo).open.rocket.chat, logged in(0.5, 0.297), swipe(0.5,0.7)→(0.5,0.3), back(0.05, 0.07)Markdown(git stash) — session20260527-160938Markdown(this PR) — session20260527-163815Per-commit Markdown self-time
Caveats: Diego Mello DM is mostly voice-call system messages (
InfoCard), not rich markdown — a text-heavy room would be a stronger benchmark. Dev-mode numbers; divide by ~3 for rough production estimates.Screenshots
Types of changes
Checklist
Further comments
The parse cache uses a simple FIFO eviction when it exceeds 200 entries — enough to cover typical room message reuse without unbounded memory growth. Translated messages bypass the cache and always re-parse, preserving existing behavior for that path.
Summary by CodeRabbit