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.
![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.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughConversation cards now buffer streamed answers and schedule updates through animation frames or timers. They flush or discard pending text at specified lifecycle events. Both Markdown renderers use shared highlighting options, with tests covering answer buffering and language highlighting. ChangesStreamed Answer Buffering
Markdown Highlighting
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ConversationCard
participant AnswerBuffer
participant FrameScheduler
ConversationCard->>AnswerBuffer: Push streamed answer
AnswerBuffer->>FrameScheduler: Schedule render callback
FrameScheduler-->>AnswerBuffer: Invoke callback
AnswerBuffer->>ConversationCard: Render latest pending answer
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change batches streamed answer rendering and shares the Markdown highlighting options. No concrete merge-blocking issue was identified, and the reported tests, lint, and build pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes remain within conversation rendering and card-owned transport cleanup. No new privileges or external entrypoints were identified. Remaining uncertainty concerns callback timing during unmount and provider-specific interruption behavior, rather than a demonstrated security regression. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
PR Summary by QodoReduce streaming answer render and syntax-detection costs
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — two low-stakes notes below.
Reviewed changes
- Streamed-chunk coalescing — new
src/components/ConversationCard/answer-buffer.mjsbuffers cumulative answer snapshots and renders at most once per animation frame, with asetTimeout(…, 16)fallback when the host lacksrequestAnimationFrame(jsdom/headless).ConversationCardflushes beforedone/error, discards on retry/clear/new-question/unmount, and stays stateless viaupdateAnswer's functional updater. - Narrowed syntax auto-detection — new
src/components/MarkdownRender/highlight-options.mjsadds a 19-languagesubsettorehype-highlight; bothmarkdown.jsxandmarkdown-without-katex.jsxshare it. Labelled blocks are unaffected. - Tests —
answer-buffer.test.mjspins push/flush/discard and both scheduler paths;highlight-options.test.mjscovers the detection subset plus labelled and unknown languages.
ℹ️ Auto-detection no longer covers several common unlabelled languages
The new subset deliberately narrows highlightAuto, but the excluded set includes widely used grammars: diff, shell, markdown, ini, makefile, scss, graphql, r, perl, lua, objectivec, vbnet, wasm, less, arduino, and php-template. Unlabelled code in those languages (e.g. an assistant reply with a bare diff block) will now render unhighlighted. Explicitly fenced blocks are unaffected — verified against rehype-highlight@6.0.0/lib/index.js, where labelled blocks dispatch to lowlight.highlight(lang, …) and only unlabelled blocks call lowlight.highlightAuto(…, { subset }). Likely intended, but flagging it as a user-visible change beyond the render-cost goal.
ℹ️ Nitpicks
src/components/ConversationCard/index.jsx:189resetspartialAnswerRef/retryRecordRefin theprops.questioneffect without theanswerBufferRef.current.discard()that the other three reset sites now call. Not reachable with the current callers (a card is mounted fresh per question), but the asymmetry is easy to align.- The new flush-on-
done/errorand discard-on-retry wiring has no integration coverage —conversation-card-lifecycle.test.mjsnever drives the port listener withanswer/done, so the wiring is exercised only by the isolated buffer unit test.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
The detection subset was meant to avoid compiling every grammar on first use, but it dropped languages that show up unlabelled in answers. A `diff` block rendered as `css`, `lua` as `sql`, `ini` as `csharp`, and `markdown` and `perl` as `bash`. Use lowlight's registered set minus the entries that only mislead detection: `shell` (a `bash` alias), `plaintext`/`python-repl`/ `php-template` (not useful for detection), and `arduino`/`objectivec`/ `vbnet`/`wasm` (rare in answers, and they win ambiguous `c`, `cpp`, `ini` and `sql` matches away). Keep lowlight's registration order, because `highlightAuto` settles equal-relevance candidates by subset order. On the sampled answers detection now matches or beats the full set while still compiling well under half the grammars.
The `props.question` effect reset the partial answer but left the answer buffer alone, so a frame queued by the previous answer could render after the new question started. `FloatingToolbar` and `DecisionCard` reuse the card for a new question, so the path is reachable. Also cover the wiring the unit tests missed: a burst stays buffered until completion flushes it, switching the question drops the previous answer, and the scheduler's timer fallback cancels a pending frame.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Followed up on the review findings, in two commits on top of 1. Auto-detection keeps the common languages — The subset was dropping more than it looked: an unlabelled
It stays in lowlight's registration order because 2. Buffered frames are dropped on a question switch — Good catch — 3. Coverage for the wiring — in
4. rAF on hidden pages — left as is. Validation: |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the two commits added since the prior pullfrog review at a3af35a. Both directly address the low-stakes notes from that review, and no new issues surfaced.
- Discard buffered frames when the question switches — the
props.questioneffect inConversationCard/index.jsxnow callsanswerBufferRef.current.discard(), aligning the last reset site with the retry, clear, new-question, and unmount paths. - Restore common languages to the auto-detection subset —
highlight-options.mjsscans 29 grammars again, adding backdiff,graphql,ini,less,lua,makefile,markdown,perl,r, andscss. Onlyshell,plaintext,python-repl,php-template,arduino,objectivec,vbnet, andwasmstay excluded, each with a documented rationale. - Integration coverage for the buffer wiring —
conversation-card-lifecycle.test.mjsnow drives the port listener, asserting a burst stays buffered untildoneflushes it and that switching questions drops a stale buffered answer.answer-buffer.test.mjsadds a timer-fallback cancellation case, and the highlight test adds exact auto-detection assertions fordiff,markdown, andlua.
I verified the subset ordering claim against lowlight@2.9.0's common registration order (the 29 names are a subsequence in the same order) and confirmed the three touched suites pass locally (node --test, 30 passing).
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
A stream can end without a `done`/`error` message: an extension reload, a background crash or a dropped port. The buffered chunk then waited for the next animation frame, which never runs on a hidden page, so the card kept a stale answer until the tab became visible. Flush before reconnecting so the newest chunk renders synchronously either way. A lifecycle test drops the port mid-stream and asserts the chunk is rendered before the replacement Port is used.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the single commit added since the prior pullfrog review at f2fae2f (one source file plus one test).
- Flush the answer buffer when the transport drops —
ConversationCard/index.jsx'sportListenernow callsanswerBufferRef.current.flush()before reconnecting, so the newest buffered chunk renders synchronously even on a hidden page where animation frames are paused. It runs after the existingreplacedPortRef/disposedRefguards, so clear-conversation, port supersession, and unmount still discard rather than resurrect a stale buffer. - Integration coverage for the dropped-transport path — the lifecycle suite's new test drives a chunk through the port, asserts it stays buffered, then triggers a remote disconnect and asserts the chunk rendered synchronously and a replacement port was created. Verified it fails (17/18) with the flush removed and passes (18/18) with it; the full
conversation-card-lifecycle.test.mjsrun is 18/18.
This directly addresses the earlier non-Pullfrog concern that a stream ending via onDisconnect (no done/error) left the newest chunk pending until the next animation frame.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
A runtime Port that drops mid-generation is replaced, but a foreground generation (Bing web) streams through its own transport and only uses this Port as a keepalive. Flushing the buffer and calling setIsReady(true) here finalized that answer early and unlocked sending while chunks were still arriving, so guard both behind "no foreground generation is in flight". The highlight subset note also described `shell` as a `bash` alias; it is the Shell Session console-prompt grammar, and `bash` only aliases `sh`/`zsh`.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
ℹ️ No critical issues — one minor comment-accuracy suggestion inline.
Reviewed changes
Re-reviewed the single commit added since the prior pullfrog review at 7d73b32.
- Keepalive Port drop no longer unlocks sending during a foreground generation —
portListenernow gatesanswerBufferRef.current.flush()andsetIsReady(true)onforegroundPortsRef.current.size === 0, so a keepalive Port drop while a Bing-web foreground stream is in flight neither renders an intermediate partial nor re-enables the input. The fakePort is added at generation start and removed infinally, so the guard tracks "foreground generation in flight" exactly; background mode keeps an empty set and behaves as before, and the foreground stream's owndone/errorstill flush and unlock. - Corrected the
shellauto-detection rationale — the comment now identifiesshellas the Shell Session console-prompt grammar rather than abashalias, addressing the earlier inaccuracy. - Integration coverage — new lifecycle test asserts sending stays locked when the keepalive Port drops mid-foreground-generation, which fails against the pre-
e3f81bfunconditionalsetIsReady(true).
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
Review follow-up complete.
CI note: the |
Splits part of #1084 into a focused PR, as requested there. Based on the latest
master.What this does
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. Two independent fixes to that:
1. Coalesce streamed chunks (
answer-buffer.mjs). A burst renders once per animation frame instead of once per chunk, and the newest text is always flushed 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 is what lets upstream's lifecycle tests keep working.2. Narrow syntax auto-detection (
highlight-options.mjs).detect: truecompiles every registered grammar the first time it runs; restricting the scan to a common-language subset keeps auto-detection and drops that first-use cost. Explicitly labelled blocks are unaffected.Measured on a 394-character answer delivered as 19 chunks of 20 characters:
Addresses #265.
Tests
tests/unit/components/answer-buffer.test.mjs— the buffer contract against a fake frame clock, plus the frame-scheduler fallback.tests/unit/components/highlight-options.test.mjs— the detection subset only narrows auto-detection, labelled blocks unaffected.tests/unit/components/conversation-card-lifecycle.test.mjs(existing) — still green, which is what exercises the timer fallback.Validation
npm test— 1101 passing.npm run lint— clean.npm run build— all four variants build; expected artifacts present inbuild/chromium/.Validation skipped: manual browser testing — no browser automation is available here. A smoke test would stream a long answer with a code block and check that it renders smoothly without a first-block stall.
Summary by CodeRabbit