fix(telegram): buffered streaming for Pi token-level chunks#1716
fix(telegram): buffered streaming for Pi token-level chunks#1716fezzzza wants to merge 4 commits into
Conversation
Adds a theme switcher to the Web UI with light and dark mode support. Defaults to dark (preserving existing appearance). User preference is persisted in localStorage and applied before React hydrates to prevent flash of wrong theme. Changes: - index.css: Add :root:not(.dark) light theme variables, rename :root to :root.dark for dark theme - index.html: Add inline script to read localStorage and set .dark class before first paint (flash prevention) - useTheme.ts: New hook that toggles .dark class on <html> and persists to localStorage - TopNav.tsx: Add Sun/Moon toggle button next to version number
Addresses CodeRabbit review comments on coleam00#1713. - index.html: Wrap pre-mount IIFE in try-catch, fallback to dark mode - useTheme.ts: Wrap both getItem and setItem in try-catch with comments matching the established pattern in ProjectContext/Sidebar/WorkflowBuilder
Pi's text_delta events can drop newlines and spaces from the model's output. The assembled transcript from agent_end.messages preserves the correct formatting. This change suppresses text_delta streaming and emits the full assembled text instead. For Telegram specifically, adds a 'buffered' streaming mode that coalesces chunks via a 3-second debounce timer. This prevents each token from becoming a separate Telegram message. - Pi event bridge: suppress text_delta, emit assembled text at agent_end - Telegram adapter: buffered mode with debounce, short-buffer skip - Whitespace-only message guard for Telegram - Shutdown flush to avoid losing in-flight buffered text - Tests for buffered mode and updated streaming tail tests Note: Pi's assembled transcript escapes nested markdown (e.g. **bold and *italic*** becomes **\*\*bold and \*italic\*\*\*), which is a Pi SDK bug that cannot be fixed in Archon.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Telegram buffered streaming mode with per-chat debounced flushing; updates Pi bridge to suppress streaming deltas and emit assembled assistant text at agent_end; implements persistent light/dark theme with pre-render script, hook, toggle UI, and CSS restructuring. ChangesTelegram Adapter Buffered Mode
Pi Event Bridge Assembled Text Emission
Web Theme System Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/adapters/src/chat/telegram/adapter.test.ts (1)
194-205: ⚡ Quick winAvoid real 3.1s waits in buffered-mode tests.
Using wall-clock delays here makes the suite slower and more timing-sensitive in CI. Prefer deterministic timer control (or a test-specific shorter debounce path) for these assertions.
As per coding guidelines, "Prefer reproducible commands and locked dependency behavior in CI-sensitive paths; keep tests deterministic with no flaky timing or network dependence without guardrails".
🤖 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 `@packages/adapters/src/chat/telegram/adapter.test.ts` around lines 194 - 205, The test uses real 3.1s setTimeouts which slows and flakes CI; update the buffered-mode tests (referencing adapter.sendMessage and mockSendMessage expectations) to use deterministic timers or a test-only shorter debounce instead of waiting 3100ms: replace the wall-clock wait with Jest fake timers (advanceTimersByTime) or inject a configurable/dependency-injected debounce delay and set it to a small value in the test so you can synchronously advance timers and immediately assert the mockSendMessage call count and arguments.packages/adapters/src/chat/telegram/adapter.ts (1)
127-139: ⚡ Quick winUse standard event-state suffixes for the new buffer log events.
telegram.buffer_skip_shortandtelegram.buffer_flushdo not follow the required{domain}.{action}_{state}pattern with standard states, which will make observability less consistent.As per coding guidelines, "Use structured logging with Pino; event naming format:
{domain}.{action}_{state}with standard states: _started, _completed, _failed, _validated, _rejected".🤖 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 `@packages/adapters/src/chat/telegram/adapter.ts` around lines 127 - 139, The log event names used in the getLog().debug calls don't follow the required {domain}.{action}_{state} pattern; update the two event strings where getLog().debug is called around this.buffers handling—replace 'telegram.buffer_skip_short' with 'telegram.buffer_skip_rejected' (skip => rejected) and replace 'telegram.buffer_flush' with 'telegram.buffer_flush_started' (flush beginning) so they conform to the standard states; ensure the calls in the code that reference the same events (the getLog().debug invocations around the existing variable this.buffers and the subsequent deliver/send logic) are updated 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.
Inline comments:
In `@packages/adapters/src/chat/telegram/adapter.ts`:
- Around line 124-135: Re-buffering short Telegram chunks currently stores {
text, timer: null } so without a subsequent chunk the message never flushes;
when updating this.buffers (both the existing branch and the new set branch)
start or reset a flush timer (use BUFFER_FLUSH_TIMEOUT) and store the timer
handle in the buffer object instead of null so the buffer will be auto-flushed;
reference this.buffers, BUFFER_MIN_FLUSH_LENGTH, BUFFER_FLUSH_TIMEOUT, chatId
and the existing variable when implementing the timer logic to match the
module's other buffering/flush behavior.
In `@packages/providers/src/community/pi/event-bridge.ts`:
- Around line 367-371: On handling the agent_end event, always clear/reset
assistantBuffer instead of only doing so when finalAssembledText is truthy:
inside the branch where you check if (event.type === 'agent_end') keep the
queue.push for the final chunk but ensure assistantBuffer is assigned (e.g. to
finalAssembledText or an explicit empty string) unconditionally when
wantsStructured is true so stale text_delta content cannot persist; update the
logic around assistantBuffer and finalAssembledText (and the call to
extractLastAssistantText if used) to guarantee assistantBuffer is reset on every
agent_end.
In `@packages/server/src/index.ts`:
- Around line 618-620: The env value for TELEGRAM_STREAMING_MODE is being
blindly cast and can silently accept typos; before creating TelegramAdapter,
validate process.env.TELEGRAM_STREAMING_MODE (or the default 'stream') against
the allowed TelegramStreamingMode values (imported from '`@archon/adapters`') and
throw a clear startup error if it’s not one of them; then pass the validated
value (not an unchecked cast) into new
TelegramAdapter(process.env.TELEGRAM_BOT_TOKEN, streamingMode).
In `@packages/web/index.html`:
- Around line 25-39: The theme bootstrap IIFE that reads
localStorage.getItem('archon-theme') and toggles
document.documentElement.classList (adding/removing 'dark') must be moved from
the body to the document <head> so it runs before first paint; relocate the
self-invoking function (the anonymous IIFE) into the head element before React
mounts and keep the same try/catch behavior and localStorage key
('archon-theme') to ensure no flash of light occurs on initial load.
---
Nitpick comments:
In `@packages/adapters/src/chat/telegram/adapter.test.ts`:
- Around line 194-205: The test uses real 3.1s setTimeouts which slows and
flakes CI; update the buffered-mode tests (referencing adapter.sendMessage and
mockSendMessage expectations) to use deterministic timers or a test-only shorter
debounce instead of waiting 3100ms: replace the wall-clock wait with Jest fake
timers (advanceTimersByTime) or inject a configurable/dependency-injected
debounce delay and set it to a small value in the test so you can synchronously
advance timers and immediately assert the mockSendMessage call count and
arguments.
In `@packages/adapters/src/chat/telegram/adapter.ts`:
- Around line 127-139: The log event names used in the getLog().debug calls
don't follow the required {domain}.{action}_{state} pattern; update the two
event strings where getLog().debug is called around this.buffers
handling—replace 'telegram.buffer_skip_short' with
'telegram.buffer_skip_rejected' (skip => rejected) and replace
'telegram.buffer_flush' with 'telegram.buffer_flush_started' (flush beginning)
so they conform to the standard states; ensure the calls in the code that
reference the same events (the getLog().debug invocations around the existing
variable this.buffers and the subsequent deliver/send logic) are updated
accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e34be7a2-f472-4986-8e51-0c3b605bf7fb
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
packages/adapters/package.jsonpackages/adapters/src/chat/telegram/adapter.test.tspackages/adapters/src/chat/telegram/adapter.tspackages/adapters/src/chat/telegram/index.tspackages/adapters/src/index.tspackages/providers/src/community/pi/event-bridge.test.tspackages/providers/src/community/pi/event-bridge.tspackages/providers/src/community/pi/provider.test.tspackages/server/src/index.tspackages/web/index.htmlpackages/web/src/components/layout/TopNav.tsxpackages/web/src/hooks/useTheme.tspackages/web/src/index.css
| <script> | ||
| // Apply saved theme before React renders to prevent flash | ||
| (function () { | ||
| try { | ||
| var theme = localStorage.getItem('archon-theme'); | ||
| if (theme === 'light') { | ||
| document.documentElement.classList.remove('dark'); | ||
| } else { | ||
| document.documentElement.classList.add('dark'); | ||
| } | ||
| } catch (_) { | ||
| document.documentElement.classList.add('dark'); | ||
| } | ||
| })(); | ||
| </script> |
There was a problem hiding this comment.
Move theme bootstrap script into <head> to reliably avoid first-paint flash.
At Line 25, running the class-toggle script in <body> can still allow a light first paint before .dark is applied. Put it in <head> before rendering starts.
Suggested change
<head>
@@
+ <script>
+ (function () {
+ try {
+ var theme = localStorage.getItem('archon-theme');
+ if (theme === 'light') {
+ document.documentElement.classList.remove('dark');
+ } else {
+ document.documentElement.classList.add('dark');
+ }
+ } catch (_) {
+ document.documentElement.classList.add('dark');
+ }
+ })();
+ </script>
<style>
@@
<body>
- <script>
- // Apply saved theme before React renders to prevent flash
- (function () {
- try {
- var theme = localStorage.getItem('archon-theme');
- if (theme === 'light') {
- document.documentElement.classList.remove('dark');
- } else {
- document.documentElement.classList.add('dark');
- }
- } catch (_) {
- document.documentElement.classList.add('dark');
- }
- })();
- </script>
<div id="root"></div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <script> | |
| // Apply saved theme before React renders to prevent flash | |
| (function () { | |
| try { | |
| var theme = localStorage.getItem('archon-theme'); | |
| if (theme === 'light') { | |
| document.documentElement.classList.remove('dark'); | |
| } else { | |
| document.documentElement.classList.add('dark'); | |
| } | |
| } catch (_) { | |
| document.documentElement.classList.add('dark'); | |
| } | |
| })(); | |
| </script> | |
| <head> | |
| <script> | |
| (function () { | |
| try { | |
| var theme = localStorage.getItem('archon-theme'); | |
| if (theme === 'light') { | |
| document.documentElement.classList.remove('dark'); | |
| } else { | |
| document.documentElement.classList.add('dark'); | |
| } | |
| } catch (_) { | |
| document.documentElement.classList.add('dark'); | |
| } | |
| })(); | |
| </script> | |
| <style> | |
| /* ... existing styles ... */ | |
| </style> | |
| </head> | |
| <body> | |
| <div id="root"></div> | |
| </body> |
🤖 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 `@packages/web/index.html` around lines 25 - 39, The theme bootstrap IIFE that
reads localStorage.getItem('archon-theme') and toggles
document.documentElement.classList (adding/removing 'dark') must be moved from
the body to the document <head> so it runs before first paint; relocate the
self-invoking function (the anonymous IIFE) into the head element before React
mounts and keep the same try/catch behavior and localStorage key
('archon-theme') to ensure no flash of light occurs on initial load.
- adapter.ts: re-buffer short text with fallback timer so short final responses aren't held indefinitely in buffered mode - event-bridge.ts: reset assistantBuffer unconditionally on agent_end to prevent stale text_delta content; remove duplicate agent_end block - server/index.ts: add runtime validation of TELEGRAM_STREAMING_MODE env var against valid values ['stream', 'batch', 'buffered'] - provider.test.ts: update scriptedAgentEnd to include text in agent_end message content (matches real Pi SDK behavior), fixing outputFormat structured output tests that broke when text_delta suppression was added
Review SummaryVerdict: blocking-issues This PR adds a Blocking issues
Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality, docs-impact. |
2 participants
Summary
text_deltaevents that drop newlines and spaces. Each token becomes a separate Telegram message, and the concatenated result has mangled formatting (missing newlines between headings, lists, and body text).text_deltastreaming and emits the full assembled text fromagent_end.messagesinstead, which preserves correct formatting. (2) Telegram adapter gains a"buffered"streaming mode that coalesces chunks via a 3-second debounce timer.UX Journey
Before
After
Architecture Diagram
Before
After
Label Snapshot
risk: mediumsize: Sadapters,providersadapters:telegram,providers:piChange Metadata
bugadaptersLinked Issue
Validation Evidence (required)
All checks pass: type-check, lint (max-warnings 0), format, tests.
Security Impact (required)
Compatibility / Migration
"buffered"is opt-in viaTELEGRAM_STREAMING_MODE=bufferedenv varTELEGRAM_STREAMING_MODE=bufferedfor Pi providers with token-level streamingHuman Verification (required)
Side Effects / Blast Radius (required)
bridgeSession), Telegram adapterRollback Plan (required)
TELEGRAM_STREAMING_MODE=bufferedto revert to stream modeRisks and Mitigations
extractLastAssistantTextreads from the full transcript which is the authoritative source**bold and *italic***becomes**\\*\\*bold and \\*italic\\*\\*\\***)Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests