feat(chat): render agent-bubble LaTeX with KaTeX and safe math detection#2697
feat(chat): render agent-bubble LaTeX with KaTeX and safe math detection#2697YellowSnnowmann wants to merge 11 commits into
Conversation
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds KaTeX-based LaTeX math rendering to markdown content. It introduces utility functions to detect and normalize LaTeX delimiters, adds the required dependencies and stylesheet, and integrates conditional math plugin configuration into markdown rendering components with comprehensive test coverage. ChangesLaTeX Math Rendering Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
coderabbitai
Bot
added
the
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/pages/conversations/components/AgentMessageBubble.test.tsx (1)
81-100: ⚡ Quick winPrefer behavior-level assertions over
.katexinternals.At Line 83, Line 88, and Line 95, querying
.katexties tests to renderer internals. Assert rendered output behavior instead (e.g., raw delimiters are gone and math text remains visible) so tests survive non-functional markup changes.As per coding guidelines: "app/**/*.test.{ts,tsx}: Prefer testing behavior over implementation details."♻️ Suggested test assertion shape
- expect(container.querySelector('.katex')).not.toBeNull(); + expect(container.textContent).toContain('x2+y2=z2'); + expect(container.textContent).not.toContain('\\['); + expect(container.textContent).not.toContain('\\]'); - expect(container.querySelector('.katex')).not.toBeNull(); + expect(container.textContent).toContain('a+b'); + expect(container.textContent).not.toContain('\\('); + expect(container.textContent).not.toContain('\\)'); - expect(container.querySelector('.katex')).not.toBeNull(); + expect(container.textContent).toContain('-2'); + expect(container.textContent).not.toContain('\\begin{vmatrix}');🤖 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/src/pages/conversations/components/AgentMessageBubble.test.tsx` around lines 81 - 100, Replace assertions that query the renderer internals (container.querySelector('.katex')) in the tests for BubbleMarkdown with behavior-level checks: for the tests "renders [ ... ] block math via KaTeX", "renders inline \\( ... \\) math via KaTeX", and "renders bare bracket vmatrix block via KaTeX" assert that the raw TeX delimiters (e.g., "\[", "\]", "\(", "\)", "["/"]" wrappers) are not present in container.textContent and that the rendered math text (e.g., "x^2 + y^2 = z^2", "a+b", "1 2 3 4" or a recognizable substring of the matrix) is visible; for "does NOT treat currency mentions as math" assert that the dollar signs and numeric amounts ("$10", "$20") remain in container.textContent and are rendered as plain text. Use the BubbleMarkdown component tests to locate and update the assertions accordingly.
🤖 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 `@app/src/pages/conversations/components/AgentMessageBubble.test.tsx`:
- Around line 81-100: Replace assertions that query the renderer internals
(container.querySelector('.katex')) in the tests for BubbleMarkdown with
behavior-level checks: for the tests "renders [ ... ] block math via KaTeX",
"renders inline \\( ... \\) math via KaTeX", and "renders bare bracket vmatrix
block via KaTeX" assert that the raw TeX delimiters (e.g., "\[", "\]", "\(",
"\)", "["/"]" wrappers) are not present in container.textContent and that the
rendered math text (e.g., "x^2 + y^2 = z^2", "a+b", "1 2 3 4" or a recognizable
substring of the matrix) is visible; for "does NOT treat currency mentions as
math" assert that the dollar signs and numeric amounts ("$10", "$20") remain in
container.textContent and are rendered as plain text. Use the BubbleMarkdown
component tests to locate and update the assertions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7f67dc5-bec6-41e4-9ae9-d069a0fd8204
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
app/package.jsonapp/src/main.tsxapp/src/pages/conversations/components/AgentMessageBubble.test.tsxapp/src/pages/conversations/components/AgentMessageBubble.tsxapp/src/utils/__tests__/latex.test.tsapp/src/utils/__tests__/loopbackOauthListener.test.tsapp/src/utils/latex.tssrc/openhuman/config/ops_tests.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Good feature — LaTeX rendering in agent bubbles is a real UX gap and the conservative heuristic approach is the right call. The hasLatexContent gate, constant plugin arrays, and delimiter normalization are all well thought out. Tests cover the key cases.
One real bug to fix before this ships, plus a couple of nits.
| Area | Files | Verdict |
|---|---|---|
| Utility | latex.ts |
Normalization mangles code blocks (see below) |
| Component | AgentMessageBubble.tsx |
Clean integration, good plugin gating |
| Tests | latex.test.ts, AgentMessageBubble.test.tsx |
Solid coverage, missing code-block case |
| Deps | package.json, pnpm-lock.yaml |
katex 0.16.47, remark-math 6, rehype-katex 7 — all current |
| Unrelated | loopbackOauthListener.test.ts, ops_tests.rs |
Should be separate commits |
…add cases for preserving LaTeX in code spans
… Unicode characters for better compatibility
…erved in LaTeX normalization
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Confirmed fixed — all prior feedback is now addressed.
Code-block mangling [major] ✅
- Implemented placeholder extraction for fenced code blocks (
...) and inline code (...) - Tests added for the exact scenario flagged: LaTeX delimiters in backtick-wrapped code spans
- Verified math bodies containing digits are preserved
Unrelated bundled changes [minor] ✅
- Removed timeout change from loopbackOauthListener.test.ts (300→60)
- Removed ops_tests.rs unrelated modification
CI & coverage ✅
- All 22 CI checks green
- Diff coverage ≥ 80% on changed lines
- Component + utility tests comprehensive
The PR is ready to approve and merge. Excellent work addressing the feedback quickly and thoroughly.
2 participants
Summary
Problem
Solution
Added app/src/utils/latex.ts with:normalizeLatexDelimiters() to convert unsupported delimiters into remark-math-friendly forms.
hasLatexContent() to gate math parsing only when content strongly signals LaTeX.
Updated AgentMessageBubble.tsx and table-cell markdown rendering to:Normalize content only when needed.
Conditionally apply remarkMath/rehypeKatex.
Imported KaTeX stylesheet in main.tsx for proper rendered output styling.
Added tests in:app/src/utils/tests/latex.test.ts
app/src/pages/conversations/components/AgentMessageBubble.test.tsx
Tradeoff: heuristic detection is intentionally conservative to avoid non-math regressions, at the cost of potentially not auto-rendering some unusual math-like text.
Submission Checklist
.github/workflows/coverage.yml. Run pnpm test:coverage and pnpm test:rust locally; PRs below 80% on changed lines will not merge.docs/RELEASE-MANUAL-SMOKE.md) — N/A: does not touch release-cut/manual-only surfaces.Impact
Related
Summary by CodeRabbit
Release Notes