refactor(dashboard): use Svelte attachments and share the SSE client - #650
Conversation
Replace element-bound `bind:this` + `$effect` pairs with `{@Attach ...}`:
the element arrives as an argument and the returned teardown runs with it,
so ChartCanvas drops its nullable ref and its three duplicated
`chart.destroy()` branches, Modal drops the `tick()` hop before autofocus
(the backdrop test now compares `event.currentTarget`), and DatePicker's
outside-click dismissal becomes a reusable attachment.
Extract the duplicated live-stream transport into $lib/api/eventStream.js.
The audit-log stream and the overview's usage-signal stream carried
byte-identical SSE framing plus the same reconnect backoff; both now share
one implementation and keep only their own event handling and reconnect
policy. live-logs-logic.js imports it by relative path because node --test
loads that file directly and cannot resolve the $lib alias.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # internal/admin/dashboard/static/dist/index.html
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds shared SSE parsing and reconnect helpers. It adds reusable Svelte attachments. Live streams and dashboard components now use these shared utilities. ChangesShared SSE event streaming
Svelte attachment utilities
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StreamReader
participant consumeEventStream
participant LiveStreamHandler
StreamReader->>consumeEventStream: Read streamed chunks
consumeEventStream->>consumeEventStream: Split SSE frames and parse JSON
consumeEventStream->>LiveStreamHandler: Forward valid events
LiveStreamHandler->>consumeEventStream: Request reconnect delay after failure
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 1
🤖 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 `@web/dashboard/src/lib/api/eventStream.js`:
- Around line 14-15: Correct the reconnect documentation to match the existing
capped behavior: in web/dashboard/src/lib/api/eventStream.js lines 14-15,
describe MAX_RECONNECT_ATTEMPTS as capping the attempt counter rather than
stopping retries; in lines 64-73, document the effective 16000ms delay cap
without changing nextReconnect(); and in
web/dashboard/src/pages/audit-logs/liveLogs.svelte.js lines 20-22, align the
stream documentation with this capped-attempt and 16000ms-delay behavior.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e068c950-52e2-46b6-a592-4b1834201739
⛔ Files ignored due to path filters (3)
internal/admin/dashboard/static/dist/assets/index-G3uz-AWc.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-uqP7q8EU.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (10)
web/dashboard/CONVENTIONS.mdweb/dashboard/src/lib/api/eventStream.jsweb/dashboard/src/lib/components/atoms/Modal.svelteweb/dashboard/src/lib/components/molecules/ChartCanvas.svelteweb/dashboard/src/lib/components/molecules/DatePicker.svelteweb/dashboard/src/lib/utils/attachments.jsweb/dashboard/src/pages/audit-logs/live-logs-logic.jsweb/dashboard/src/pages/audit-logs/liveLogs.svelte.jsweb/dashboard/src/pages/overview/liveTokensState.svelte.jsweb/dashboard/tests/event-stream.test.js
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Confidence Score: 4/5Not safe to merge until stale live-usage SSE readers are prevented from reconnecting after a replacement stream is active. A focused runtime harness reproduced an extra live-usage stream request after completing only a superseded reader. The interactive attachment behaviors were also exercised successfully in Chromium. Files Needing Attention: web/dashboard/src/pages/overview/liveTokensState.svelte.js needs an active-controller ownership check before scheduling reconnects.
 T-Rex LogsWhat T-Rex did
Comments Outside Diff (1)
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await consumeEventStream(res.body.getReader(), (event) => | ||
| this.#handleEvent(event), | ||
| ); | ||
| this.#scheduleReconnect(); |
There was a problem hiding this comment.
start() replaces and aborts the previous controller, but an older #readStream(controller) can continue to this completion path after its reader finishes. It then unconditionally schedules a reconnect against the current state, even though a replacement stream is already active. The executed harness restarted the state to create two streams, completed only the stale reader, and observed a third stream request. Guard reconnect scheduling in this method with controller === this.#sseController and ignore aborted controllers so only the active stream can reconnect.
Artifacts
Node harness source for the stale live-usage stream reader race
- The captured harness starts two replacement streams, then selectively completes the old reader; it exercises the actual source after only dependency substitution.
Baseline stream count before stale reader completion
- The first executed harness capture confirms two stream fetches after restart and before the stale reader is allowed to complete; this is the expected baseline.
Stale reader completion opens a third stream
- The second executed harness capture completes the stale reader and observes stream fetches increase from two to three; this reproduces the defect.
MAX_RECONNECT_ATTEMPTS caps the attempt counter, it does not stop retries: both consumers reconnect indefinitely once it pins. With the counter capped at 6 the delay tops out at 16s, so the "capped at 30s" wording described a ceiling the attempt cap keeps unreachable. Comments only — nextReconnect() and the retry behaviour are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(dashboard): stop a stale usage stream from reopening itself #readStream scheduled a reconnect whenever its reader finished, without checking whether its controller was still the active one. Leaving the Overview page and returning replaces the controller, but the old reader can still complete normally afterwards and schedule a reconnect against the current state, opening a second usage stream alongside the live one. Guard the completion path with the controller identity/abort check the audit-log stream already uses (liveLogs.svelte.js), so only the active stream can reconnect. Reported by Greptile on #650; the flaw predates that PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(dashboard): guard every usage-stream reconnect path The first pass only guarded the normal-completion path. The invalid-response branch and the catch block also schedule reconnects, and both are reachable by a reader whose stream was already replaced — the fetch can resolve before the abort lands. Route all three through one controller-identity check. Firefox also rejects a deliberately-aborted read with a plain TypeError instead of an AbortError, so the catch now trusts the controller state as well as the error name, matching liveLogs.svelte.js. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Internal dashboard refactor — no user-visible behaviour change, no API surface touched.
Svelte attachments
Replaces element-bound
bind:this+$effectpairs with{@attach ...}, which was unused in the codebase (there were nouse:actions either). The element arrives as the attachment's argument and the returned teardown runs with it:ChartCanvas(38 → 12 lines) — drops the nullable$stateref, the null guard, and the three duplicatedchart.destroy(); chart = nullbranches that existed because the effect had to handle "no config", "rebuild" and "unmount" separately. One returned cleanup covers all three.Modal— autofocus is an attachment, so thetick()hop is gone (attachments already run after the DOM exists). The backdrop-click test now comparesevent.target === event.currentTarget, which is what theshellElref was standing in for.DatePicker— outside-click/Escape dismissal moves into the reusabledismissOnOutsideattachment, live only while open via a falsy{@attach}. It stays on the picker root (trigger included) so a trigger click remains an inside click andtogglekeeps owning that case.New:
$lib/utils/attachments.js(dismissOnOutside,autofocusWithin), plus a short note inCONVENTIONS.md.Shared SSE client
The audit-log live stream and the overview's usage-signal stream each carried their own copy of the transport: byte-identical framing/
data:parsing (live-logs-logic.jsvsliveTokensState.svelte.js) and the same reconnect backoff. Both now use$lib/api/eventStream.js(consumeEventStream,nextReconnect) and keep only their own event handling and reconnect policy — roughly 75 duplicated lines down to one implementation.live-logs-logic.jsimports it by relative path, not$lib: that file is loaded directly bynode --test, which cannot resolve the alias. Noted in both file headers.Note, not fixed here
The "capped at 30s" backoff comment is unreachable — attempts cap at 6, so the largest delay is 16s. That was true of both copies before this change, so behaviour is preserved rather than silently retimed. Happy to raise the cap or correct the comment in a follow-up.
Verification
tests/event-stream.test.js; the existingconsumeLiveLogsBodycases still run through the shared code, which is the equivalence check),svelte-check0 errors, clean build.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes