Conversation
A streamed answer arrives as a series of cumulative snapshots and every snapshot re-rendered the whole answer, so a long reply re-parsed from the top on each chunk. Coalesce the chunks into one render per animation frame (`answer-buffer.mjs`) and always flush the newest text before the answer is finalized. Buffered frames are dropped when the card unmounts, is cleared, or starts a new question, so a stale frame cannot resurrect an old answer. The buffer falls back to a timer where the host has no animation frames (jsdom and other headless renderers), which keeps the lifecycle tests working. Syntax auto-detection is limited to a common-language subset in `highlight-options.mjs`. `detect: true` compiles every registered grammar on first use; narrowing the scan keeps auto-detection and drops that cost. Explicitly labelled blocks are unaffected. Measured on a 394-character answer delivered as 19 chunks of 20 characters: - First code block on a cold page: ~132 ms before, ~4 ms after. - A burst: 48.6 ms of CPU over 19 renders before, 11.0 ms over 6 after. - A slow stream (60 ms per chunk): 48.6 ms before, 29.6 ms after.
Replace react-markdown with @aeven-ai/hypermarkdown, which parses only the block that is still changing and caches the blocks already settled, so a long answer is no longer re-parsed from the top on every update. Answers are handed over as deltas (`stream-delta.mjs`) and finalized once the stream ends. The react/react-dom alias moves from @preact/compat 17.1.2 to 18.3.2 so packages that declare a React 18 peer range can be installed. The alias is a thin re-export of preact/compat, so the runtime implementation is unchanged. Removed as redundant: - `markdown-without-katex.jsx`, a 200-line near-duplicate of the renderer. The KaTeX-free build now swaps `math-plugin.mjs` for `math-plugin-without-katex.mjs` and `mykatex.min.css` for `mykatex-without-katex.css` through the build hook that already existed for this purpose. The minimal build ships 0 KaTeX font faces; the full build is unchanged. - `Pre.jsx`, now that the renderer has its own code-block toolbar, and the orphaned `change-children-font-size.mjs` helper. - `react-markdown`, `remark-breaks`, `rehype-raw`, `github-markdown-css`, `parse5` and the `parse5` webpack alias (nothing in `src` imported it, and the alias broke `hast-util-raw`). Rendering fixes that came with the swap: the renderer stylesheets are imported by the content-script entry so every variant ships them (they used to land in a chunk whose CSS is never packaged), list markers are drawn by the browser (`list-markers.mjs`) so bullets nested in a numbered list stay bullets and a list that resumes at another number keeps its `start`, and a thrown error is stored as its message, since the renderer takes text and would otherwise break on an `Error` object.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (26)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
PR Summary by QodoRender streamed replies incrementally with HyperMarkdown
AI Description
Diagram
High-Level Assessment
Files changed (24)
|
Contributor
Code Review by Qodo
1. One renderer attribute uses double quotes
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| {props.children.replace('</think>', '\n\n</think>\n\n')} | ||
| </ReactMarkdown> | ||
| return ( | ||
| <div dir="auto" ref={containerRef}> |
Contributor
There was a problem hiding this comment.
1. One renderer attribute uses double quotes 📘 Rule violation ⚙ Maintainability
MarkdownRender adds dir="auto" as a double-quoted JSX attribute value. The attribute is on the new renderer wrapper, so this changed line breaks the single-quote convention.
Agent Prompt
## Issue description
The new renderer wrapper uses a double-quoted JSX attribute value.
## Fix Focus Areas
- src/components/MarkdownRender/markdown.jsx[74-74]
## Recommended Fix
Change the `dir` attribute value to single quotes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| 'cite', | ||
| // `{seconds}` is filled in by the renderer, and is not i18next interpolation syntax. | ||
| const translations = useMemo( | ||
| () => ({ thinking: t('Thinking Content'), thoughtFor: t('Thought for {seconds}s') }), |
Contributor
There was a problem hiding this comment.
2. The new thought label stays english 📘 Rule violation ⚙ Maintainability
MarkdownRender requests Thought for {seconds}s, but the new key is present only in the English
and two Chinese locale files. For the other registered locales, the configured English fallback
supplies the label instead of a locale-specific value or placeholder.
Agent Prompt
## Issue description
The new thought-duration label has no entry in most registered locales and falls back to English.
## Fix Focus Areas
- src/components/MarkdownRender/markdown.jsx[69-69]
- src/_locales/en/main.json[35-35]
- src/_locales/resources.mjs[1-52]
## Recommended Fix
Add `Thought for {seconds}s` to each remaining registered locale file with a translation or a clearly marked placeholder.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+9
to
+11
| const lists = container?.querySelectorAll?.('ol[start="0"]') | ||
| if (!lists) return 0 | ||
| for (const list of lists) list.removeAttribute('start') |
Contributor
There was a problem hiding this comment.
3. Zero-based lists display from one 🐞 Bug ≡ Correctness
normalizeListStarts removes start="0" from every ordered list, without distinguishing a temporary streaming value from a list explicitly numbered from 0.. When a reply contains a zero-based Markdown list, the browser numbers it from one, and the MutationObserver repeats the cleanup after subsequent renders, including after the answer is finalized.
Agent Prompt
## Issue description
`normalizeListStarts` treats an explicitly zero-based Markdown list like a list with a temporary renderer-generated `start="0"`, so the browser displays the explicit list starting at one.
## Fix Focus Areas
- src/components/MarkdownRender/list-markers.mjs[8-13]
- src/components/MarkdownRender/markdown.jsx[41-54]
- tests/unit/components/list-markers.test.mjs[11-22]
## Recommended Fix
Limit attribute removal to lists known to have a temporary streaming start value, rather than every `ol[start="0"]`. For example, distinguish lists by whether the first list item's source number is zero, and ensure cleanup after finalization preserves explicit zero-based lists. Add a rendering test for Markdown beginning with `0.` and confirm it remains numbered from zero.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+161
to
+163
| .markdown-body .hypermarkdown ol { | ||
| list-style-type: decimal !important; | ||
| } |
Contributor
There was a problem hiding this comment.
4. Lettered lists display as numbered lists 🐞 Bug ≡ Correctness
The new .hypermarkdown ol rule forces list-style-type: decimal !important regardless of an ordered list's type attribute. When rendered HTML contains an alphabetic or Roman ordered list, this rule overrides the existing type-specific styles while the new li::before rule suppresses the renderer's markers.
Agent Prompt
## Issue description
The native-marker override forces decimal markers even for ordered lists with alphabetic or Roman types.
## Fix Focus Areas
- src/content-script/styles.scss[155-175]
- src/content-script/styles.scss[697-714]
## Recommended Fix
Apply the decimal fallback only to ordered lists without a non-decimal `type`, or add matching higher-priority rules for supported types. Verify lettered and Roman lists retain their markers.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
266
to
268
| if (msg.error) { | ||
| answerBufferRef.current.flush() | ||
| const retryRecord = retryRecordRef.current |
Contributor
There was a problem hiding this comment.
5. A reply cut off by an error never finishes rendering 🐞 Bug ≡ Correctness
createStreamDelta.next only asks the renderer to finalize once done is true, and done only becomes true in the msg.done branch of portMessageListener. When a provider error arrives after part of the answer has streamed, the msg.error branch calls flush() (which renders the partial text with done=false) and the default case then appends a separate error item, so the partial answer keeps done=false and HyperMarkdown leaves its last block in the streaming state for good.
Agent Prompt
## Issue description
When `msg.error` arrives after a partial answer has streamed, the answer item keeps `done=false`, so the HyperMarkdown renderer never finalizes its last block.
## Fix Focus Areas
- src/components/ConversationCard/index.jsx[266-327]
## Recommended Fix
In the `msg.error` branch, after `answerBufferRef.current.flush()`, mark the existing answer item as done before adding the error, for example with `updateAnswer('', true, 'answer', true)` when the last answer item is a real partial answer and not the loading placeholder. Alternatively, set `done: true` on that item inside the `setConversationItemData` updater in the default case.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The committed package-lock.json now resolves 135 packages from registry.npmmirror.com; master had zero. Please regenerate the lockfile against the official registry before merging.
Reviewed changes
- Renderer migration —
react-markdownis replaced by@aeven-ai/hypermarkdown@0.4.4.MarkdownRendermounts<HyperMarkdown streaming>and drives its imperative handle;stream-delta.mjsconverts cumulative snapshots into deltas. - Streaming coalescing —
answer-buffer.mjsbatches chunk bursts to one render per animation frame;ConversationCardpushes, flushes on done/error, and discards on retry/clear. - Build variants — the minimal build swaps
math-plugin.mjs→math-plugin-without-katex.mjsandmykatex.min.css→mykatex-without-katex.css; the oldmarkdown.jsx→markdown-without-katex.jsxswap is removed. Renderer stylesheets moved to the content-script entry, whichIndependentPanel.htmllinks. - Cleanup —
Pre.jsx,markdown-without-katex.jsx,change-children-font-size.mjs, theparse5alias, and thegithub-markdown-css/parse5/react-markdown/rehype-raw/remark-breaksdeps are gone; thereact/react-domalias is bumped to@preact/compat@^18.3.2. - New behavior — browser-drawn list markers (
list-markers.mjs), a detection subset for highlighting (highlight-options.mjs), andThinking Content/Thought for {seconds}stranslations.
I verified against fc80bc3: npm test (1111 pass), npm run lint clean, npm run build produces all four variants with the expected artifacts. The minimal CSS has 0 @font-face and its JS has 0 KaTeX references; the full build keeps its 26 font faces. CSS packaging is correct because IndependentPanel.html links content-script.css.
⚠️ Lockfile now resolves 135 packages from a third-party mirror
package-lock.json mixes registries: 962 entries point at registry.npmjs.org and 135 at registry.npmmirror.com (Alibaba's mirror). The base lockfile had none — every entry was npmjs. The mirror-hosted set is the new @aeven-ai/hypermarkdown transitive tree plus peers (hast-util-*, rehype-*, remark-*, katex, tippy.js, @popperjs/core, @types/*). Integrity hashes still guard tarball contents, but npm ci in CI and on other machines will now fetch from a third-party host, which is both a provenance inconsistency and an availability dependency (some environments block non-npmjs hosts).
Technical details
# Lockfile registry provenance
## Affected sites
- `package-lock.json` — 135 `"resolved"` URLs under `https://registry.npmmirror.com`,
introduced by this PR. `master`'s lockfile (`git show master:package-lock.json`) has
1072 resolved entries, all `https://registry.npmjs.org`.
## Required outcome
- The committed lockfile should resolve from `https://registry.npmjs.org` (or another
single, intentionally chosen registry), not carry entries the author's local npm
config injected.
## Suggested approach (optional)
- Regenerate without the mirror configured, e.g.
`npm install --package-lock-only --registry https://registry.npmjs.org`,
then confirm `grep -c registry.npmmirror.com package-lock.json` is `0`.
- Worth checking whether the PR author's environment has `registry=https://registry.npmmirror.com`
in `~/.npmrc`, since that is the usual source of a mixed lockfile.ℹ️ remark-breaks removal changes soft line breaks outside paragraphs
The old renderer passed remarkPlugins={[remarkMath, remarkGfm, remarkBreaks]}. HyperMarkdown's pipeline (remark-parse → remark-gfm → remark-rehype, per its lib/processors.ts) has no break plugin, so a single newline is now a soft break everywhere. HyperMarkdown's stylesheet sets white-space: pre-wrap on .hypermarkdown p, so soft breaks inside paragraphs still render as line breaks — but <li>, headings, <blockquote>, and table cells have no such rule and will collapse them to spaces. The PR body lists remark-breaks as "removed as redundant"; it is redundant for paragraphs only.
Technical details
# Soft line breaks outside paragraphs
## Affected sites
- `src/components/MarkdownRender/markdown.jsx` — old `remarkPlugins={[remarkMath, remarkGfm, remarkBreaks]}` is replaced by `@aeven-ai/hypermarkdown`'s built-in pipeline, which has no equivalent.
- `node_modules/@aeven-ai/hypermarkdown/dist/hypermarkdown.css` — only `.hypermarkdown p` carries `white-space: pre-wrap`; `li`/`h1-6`/`blockquote`/`td` do not.
## Evidence
- Pipeline parity probe (`unified().use(remarkParse).use(remarkGfm).use(remarkRehype)` on
`- First point\n continued on next line`) yields `<li>First point\ncontinued on next line</li>`
with no `<br>` node. With `remark-breaks` this was a `<br>`.
- Paragraph content is unaffected visually because of the `pre-wrap` rule.
## Open questions for the human (optional)
- HyperMarkdown does not accept arbitrary `remarkPlugins` (its README says to use its typed
plugins), so restoring the behavior needs an upstream break/`pre-wrap` option or a host-side
transform. Is losing line breaks in tight list items / headings / blockquotes acceptable?deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Author
|
Closing this one to fold the HyperMarkdown migration and the reasoning work into a single PR — HyperMarkdown already renders reasoning blocks natively, so the two land more naturally together. A combined PR replaces it. |
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
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.
Splits part of #1084 into a focused PR, as requested there.
Depends on #1101 (streaming render performance). The branch is based on that branch, so this diff currently includes its commit as well; it shrinks to just the renderer migration once #1101 merges. The renderer reuses the shared syntax-highlight options introduced there.
What this does
Replace
react-markdownwith@aeven-ai/hypermarkdown, which parses only the block that is still changing and caches the blocks already settled, so a long answer is no longer re-parsed from the top on every update. Answers are handed over as deltas (stream-delta.mjs) and finalized once the stream ends.react/react-dommove from@preact/compat@^17.1.2to^18.3.2so packages that declare a React 18 peer range can be installed. The alias is a thin re-export ofpreact/compat, so the runtime implementation is stillpreact@10.22.1— this changes the reported version, not the rendering. Every API the extension uses is still exported:unmountComponentAtNode,render,createPortal,findDOMNode,flushSync,unstable_batchedUpdates, and the hooks.Removed as redundant:
markdown-without-katex.jsx, a 200-line near-duplicate of the renderer. The KaTeX-free build now swapsmath-plugin.mjsformath-plugin-without-katex.mjsandmykatex.min.cssformykatex-without-katex.cssthrough theNormalModuleReplacementPluginhook that already existed for this purpose. The minimal build ships 0 KaTeX font faces; the full build is unchanged.Pre.jsx, the custom code-block wrapper, now that the renderer has its own toolbar. This drops the per-block font-size selector — the renderer has no equivalent.change-children-font-size.mjsis orphaned by that removal.react-markdown,remark-breaks,rehype-raw,github-markdown-css(never imported; its CSS was vendored intostyles.scss),parse5, and theparse5webpack alias, which forced everyparse5import to the root v6 and brokehast-util-rawv9.Other behavior details:
.gpt-loading, soallowedTags: { p: ['className'] }keeps that class.list-markers.mjs): bullets nested in a numbered list stay bullets, and a list that resumes at another number keeps itsstart.Errorobject.Addresses #892.
Tests
tests/unit/components/stream-delta.test.mjs— the delta contract (first write, growth, no-op, finalize).tests/unit/components/list-markers.test.mjs—start="0"cleanup, real starts preserved.conversation-card-lifecycle.test.mjsand the selection-toolbar loader hooks are updated/stubbed for the new stylesheet imports.Validation
npm test— 1111 passing.npm run lint— clean.npm run build— all four variants;build/chromium/has the expected artifacts, and the minimalcontent-script.csscarries 313.hypermarkdownrules with 0@font-face(the full build keeps its 26 KaTeX faces).npm ciinstalls cleanly. Note: the lockfile the original branch carried was missing the optional dependencies ofless(errno,needle,probe-image-size, …), which npm 11 rejects as out of sync; the lockfile here is reconciled withnpm install.Validation skipped: manual browser testing — no browser automation is available here. A smoke test would check a streamed answer with a code block and a table, the loading placeholder, a Bing answer with
<sup>citations, and the minimal build's styling.Summary by CodeRabbit