fix(ui): throttle SSE part deltas and prevent stale delta text duplication#536
Conversation
Accumulate text deltas in a buffer and flush every 50ms, so rapid streaming chunks don't trigger a Solid store mutation + re-render on every single delta event. On mobile this cuts the main-thread blocking caused by KaTeX / highlight.js re-renders during streaming responses.
When message.part.updated arrives with a complete part, applyPartUpdate replaces the part entirely with the server's state. But if deltas were enqueued before the part update arrived and haven't flushed yet, those stale deltas would be applied AFTER the replacement, causing text duplication at the end of assistant messages. Fix: clear pending deltas for a part before applying the full part update, since the update already contains the complete state and any accumulated deltas are now stale. This prevents the race where: 1. Deltas accumulate in the 50ms throttle window 2. message.part.updated arrives and replaces the part with complete text 3. Delta timer expires and concatenates stale deltas → duplicate text
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27169076204 Artifacts expire in 7 days.
|
|
I want you to perform the gatekeeper checks for latest PR state check for regressions |
|
Gatekeeper review for latest PR state. I interpreted the request as: perform a ruthless PR-quality review of the current implementation in #536, focused on regressions, correctness, and better implementation options. I did not make code changes. Findings:
That is an ordering regression, not just a cosmetic delay. It can produce transient incomplete final content, stale scroll/revision behavior around completion, and terminal-status UI updates before the last text mutation. The implementation handles this ordering problem for Recommended fix: before applying
The new behavior in
This is exactly the sort of code that will regress silently without fake-timer coverage. I would not merge this without at least targeted unit coverage around the buffering helper behavior or the exported handlers.
Quality notes:
Verification performed:
File size note:
Gatekeeper verdict: request changes before merge. The approach is directionally right, but the buffering must preserve event ordering across -- |
I am not sure if this is valid. Can you first confirm in opencode source code if this is expected and matches with how TUI behaves? |
…t ordering When deltas are buffered for up to 50ms and message.updated arrives before the flush timer fires, the message could be marked complete/error before pending text mutations are applied. This causes the UI to observe a terminal-status message with stale content. Fix: flush any pending deltas for the message before applying the message.updated event. This preserves the server's event ordering: all delta content is applied first, then the message status/metadata update runs on the complete content. Addresses gatekeeper review blocking finding NeuralNomadsAI#1 on PR NeuralNomadsAI#536.
|
@shantur Regarding Yes, this matches how the TUI behaves. Here is the reasoning: In the TUI (terminal), each In our web UI, we buffer deltas for 50ms to batch Solid store mutations and avoid main-thread blocking during streaming (KaTeX/highlight.js re-renders are expensive on mobile). This buffering introduces a race condition that does NOT exist in the TUI:
|
|
I want you to perform the gatekeeper checks for latest PR state check for regressions |
|
@pascalandr - Please hold onto this one, I would like to play with it too. |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27200174326 Artifacts expire in 7 days.
|
|
@JDis03 |
@shantur I don’t see it as a blocker for this PR. This PR reduces delta-driven render frequency and also fixes stale buffered deltas causing text duplication, which is independent from markdown rendering. A markdown/highlighting refactor would reduce the cost per render; this PR reduces the number of renders and preserves event ordering. Unless you see a concrete conflict with the pipeline work, I’d prefer to keep this narrow and land it, then handle markdown/highlighting as a follow-up. |
Apply two P2 (style-level) improvements from Greptile gatekeeper review: 1. Consistent snapshot pattern in flushPendingDeltasForMessage - Collect keys first, then iterate to flush (matches clearPendingDeltasForPart) - Prevents mutation-during-iteration and ensures delete() runs before apply - If applyPartDeltaV2 throws, entry is already removed (no re-apply on timer) 2. Extract delta-buffer to dedicated module - session-events.ts: 854 → 808 lines (under 850, closer to 800 target) - delta-buffer.ts: 80 lines (focused, single-responsibility) - Improves maintainability per AGENTS.md file-length guidelines No functional changes. Build verified. Addresses: Pagecran#17 (comment) Pagecran#17 (comment)
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27309776036 Artifacts expire in 7 days.
|
|
@pascalandr - Go for it, once you are happy testing it |
|
I want you to perform the gatekeeper checks for latest PR state check for regressions |
|
Gatekeeper review for latest PR state. I interpreted the request as another strict PR-quality review of #536 after the follow-up commits, focused on regressions and implementation quality. I did not make code changes. Findings:
This is trivial to fix, but it is still a merge-gate issue. The latest extracted module introduced the whitespace failure.
The previous event-ordering issue was addressed with
Given the bug is specifically a timing/race bug, manual mobile testing and build artifacts are not enough. I would still require focused fake-timer tests for
Resolved from prior gatekeeper pass:
Quality notes:
Verification performed:
File size note:
Gatekeeper verdict: request changes before merge. The functional direction is now much better, but the PR should at least fix the whitespace failure and add targeted regression tests for the buffering/order guarantees before landing. -- |
Addresses whitespace hygiene failure found by CodeNomadBot gatekeeper review. git diff --check was failing on line 3 of delta-buffer.ts.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27465709734 Artifacts expire in 7 days.
|
Add targeted node:test coverage for SSE delta buffering, including concatenation, stale part clearing, message-level flushing before updates, and isolation across instances/messages/parts. Keep optimistic synthetic message reconciliation scoped to sending records by removing the unrelated sent-status match. This preserves the original pending-message behavior while the PR focuses on delta throttling. Validation: node --test --import tsx packages/ui/src/stores/delta-buffer.test.ts; npm run typecheck --workspace @codenomad/ui; npm run build:ui.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27469581204 Artifacts expire in 7 days.
|
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Works nicely in my tests and improvement confirmed especially for Electron.
|
@CodeNomadBot |
|
Post-merge gatekeeper pass for latest PR state. I interpreted Findings: No blocking findings remain in the current diff I reviewed. Resolved since the prior gatekeeper pass:
Verification performed:
Residual risk:
File size note:
Gatekeeper verdict: no remaining code-quality blocker found in the latest reviewed state. The implementation is now in a substantially better place than the earlier revisions; the only follow-up I would push for is making sure the new delta-buffer test is included in automated CI. -- |
|
Thanks @JDis03 ! |
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27469672020 Artifacts expire in 7 days.
|
3 participants
Problem
Streaming responses send many
message.part.deltaevents per second. Each event triggers a Solid store mutation + re-render, causing:message.part.updatedreplaces a part with the server's complete state, previously enqueued deltas would flush AFTER the replacement, concatenating stale text onto the fresh partSolution
1. Delta throttling (50ms buffer)
Accumulate deltas in a buffer and flush every 50ms instead of updating on every event. This batches Solid mutations and cuts main-thread blocking during streaming.
2. Stale delta cleanup
When a full
message.part.updatedarrives, clear any pending deltas for that part before applying the update. The update already contains the complete server state, so accumulated deltas are stale.Changes
packages/ui/src/stores/session-events.ts: AddedenqueueDelta,flushDeltas,clearPendingDeltasForPart; updatedhandleMessagePartDeltaandhandleMessageUpdateVerification
npm run build:ui