refactor(web): extract lib/streaming/ transport layer from domains/chat/#32598
Conversation
…s/chat/ - Create domains/streaming/ with stream-transport.ts, event-parser.ts, and stream-debug.ts (+ colocated tests) - Eliminate domains/chat/api/client.ts barrel file — all 14 consumers now import directly from @/generated/api/client.gen and @/lib/api-errors - Lift DisplayAttachment to @/types/attachment-types.ts (cross-domain type) - Extract generic diagnostic recording to utils/diagnostics.ts; chat-domain diagnostics.ts retains only message-specific summary functions - Update cross-domain allowlist for chat → streaming dependency direction - Zero logic changes — purely structural Closes LUM-2033 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc22985a70
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…tion - Update mock.module path in use-event-bus-init.test.tsx to match new @/domains/streaming/stream-transport location - Add event-parser.test.ts to cross-domain allowlist (inline type imports reference chat domain types) Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
There was a problem hiding this comment.
✦ APPROVE — reviewed at 4b277e79
Value: Engineers and agents contributing to streaming behavior now have a clear, findable home (domains/streaming/) that doesn't require them to understand or navigate chat domain internals — this sharpens the codebase map for everyone.
What this does: Phase 1 of 2 — moves the SSE transport layer (stream.ts → stream-transport.ts, event-parser.ts, stream-debug.ts) out of domains/chat/api/ and into the new domains/streaming/ domain. Pulls generic diagnostics infrastructure to utils/diagnostics.ts, lifts DisplayAttachment to shared types, and deletes the domains/chat/api/client.ts barrel (19 lines of re-exports → 14 direct import sites). Stream handlers stay in chat for Phase 2.
Full analysis
Architecture
The boundary is correct. domains/chat/ had two distinct reasons to change: (1) chat UX/business logic — messages, composer, transcript rendering; (2) SSE infrastructure — reconnect logic, event schema parsing, stream lifecycle. This PR cleanly severs the second into its own domain. The rename stream.ts → stream-transport.ts per naming conventions is a nice touch.
File-by-file
domains/streaming/stream-transport.ts — Import rewire from the domains/chat/api/client barrel to direct client.gen + lib/api-errors imports is correct. The recordChatDiagnostic → recordDiagnostic rename aligns with the generalization. No logic changes visible.
domains/streaming/event-parser.ts — Single import path update (DisplayAttachment from shared types). Clean.
domains/streaming/stream-debug.ts — Doc comment update reflects the new module path. Minimal.
utils/diagnostics.ts (new) — The split between generic infrastructure (recordDiagnostic, resolvePlatformTag, getDiagnosticsEvents, buildDiagnosticsSnapshot) landing here vs. chat-specific summary functions (summarizeDisplayMessage, summarizeRuntimeMessage) staying in domains/chat/utils/diagnostics.ts is the right call. The share-feedback-modal.tsx import update confirms cross-domain usage made utils/ the correct home.
One small flag: bucketMessagesAdded and roleCounts are now in the generic utils file but feel chat-domain adjacent. They're typed generically enough that it's not a problem — just noting they may be better long-term candidates for domains/chat/utils/diagnostics.ts. Not blocking.
summarizeAssistantEvent operating on AssistantEvent (from shared @/types/event-types) is genuinely generic and belongs here.
types/attachment-types.ts (new) — Clean lift. DisplayAttachment was stuck in domains/chat/types/types.ts despite being consumed by the event parser. Now that event-parser lives in streaming, it can't import from chat — this resolves the backward dependency correctly. JSDoc is clear on the two population paths (SSE live vs. history reload).
domains/chat/utils/diagnostics.ts — 271 → ~20 lines. Correctly retains only chat-type summarizers.
.cross-domain-allowlist.json — 15+ new streaming entries. Devin flagged this against the "shrinks toward zero" guidance in CONVENTIONS.md (288-289). The PR description explains it well: these are documenting the legitimate chat → streaming consumer direction that exists today; Phase 2 removes them as stream handlers decouple via event bus. This is the right tradeoff for a 2-PR plan. The entries are principled, not convenient.
Bot findings at HEAD (4b277e79)
- Codex P2 (mock path stale in
use-event-bus-init.test.tsx) — ✅ fixed in this commit.@/domains/chat/api/stream→@/domains/streaming/stream-transport. Verified. - Devin cross-domain allowlist flag — addressed in PR description with Phase 2 plan. Acceptable.
- Devin empty review at HEAD — not a formal "No Issues Found," so second approval is still pending.
Merge gate
- ✅ Architecture sound, conventions followed
⚠️ Branch isdirty(merge conflict) — needs rebase against main before merge⚠️ CI incomplete — only Socket Security checks visible at HEAD; TypeScript/lint/tests need a clean run post-rebase- ⏳ Second approval still needed (Devin HEAD review was empty; needs
@devin review this PRafter rebase lands)
Vellum Constitution — Yours: streaming transport now has its own address in the codebase, so the assistant's infrastructure can evolve without the chat domain knowing it changed.
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
8087e62
Test Results — LUM-2033Ran Vite dev server locally against the PR branch and verified all moved files resolve correctly. All 5 tests passed.
EvidenceT1 + T2: Unit tests T3 + T4: Dev server + browser
T5: Static analysis |
…2033-extract-streaming-domain
|
@codex review |
… update docs - Move utils/diagnostics.ts → lib/diagnostics.ts (has side effects: sessionStorage I/O, Capacitor SDK) - Remove barrel re-exports from domains/chat/utils/diagnostics.ts (7 aliased symbols) - Update all 11 consumers to import directly from @/lib/diagnostics with canonical names - Move diagnostics tests to colocate with lib/diagnostics.ts - Update STYLE_GUIDE.md: streaming/ now shown under lib/ not domains/ - Update CONVENTIONS.md: replace stale streaming domain refs, clarify lib/streaming/ path - Remove internal ticket references (LUM-1431, LUM-1538) from public repo code Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8087e62ada
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
These were added during Playwright-based testing and should not be committed — Playwright belongs in apps/web/package.json if needed. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Closes LUM-2033
Prompt / plan
Phase 1 of a 2-phase plan to extract SSE transport infrastructure from
domains/chat/. This PR createslib/streaming/as the home for transport-only code (SSE connection, event parsing, debug tracking) — zero domain knowledge.Why
lib/streaming/notdomains/streaming/? Streaming transport is shared infrastructure with side effects (SSE connections), not a business domain. Placing it indomains/would require 15+ cross-domain allowlist entries for every chat file that imports from it.lib/is defined in CONVENTIONS.md for "infrastructure with side effects" andlib/sync/(EventSource state sync) is a direct precedent.Changes
domains/chat/api/client.tsbarrel file — 19-line re-export file deleted, 14 consumers updated to import directly from@/generated/api/client.genand@/utils/api-errorsDisplayAttachmentto@/types/attachment-types.ts— cross-domain type that was stuck indomains/chat/types/types.tsutils/diagnostics.ts→lib/diagnostics.ts— has side effects (sessionStorage I/O, Capacitor SDK import), belongs inlib/per CONVENTIONS.mdlib/ vs utils/tabledomains/chat/utils/diagnostics.ts— removed 7 aliased re-exports (recordDiagnostic as recordChatDiagnostic, etc.), updated all 11 consumers to import directly from@/lib/diagnosticswith canonical nameslib/streaming/:domains/chat/api/stream.ts→lib/streaming/stream-transport.ts(renamed per CONVENTIONS.md)domains/chat/api/event-parser.ts→lib/streaming/event-parser.tsdomains/chat/api/stream-debug.ts→lib/streaming/stream-debug.tslib/imports don't require allowlistinglib/streaming/andlib/diagnostics.ts, stalestreamingdomain references replacedLUM-1431andLUM-1538references with context-appropriate descriptions (iOS WKWebView constraint, turn-sending wiring)What stays in
domains/chat/Stream handlers (
utils/stream-handlers/) remain in chat — they have heavy coupling to chat stores, utilities, and types. Phase 2 (LUM-2053) will decouple these via event bus subscribers.Follow-up tickets created
Alternatives not taken
domains/streaming/— Would require 15+ new cross-domain allowlist entries since every chat consumer would cross a domain boundary.lib/eliminates this entirely.StreamHandlerContextto decouple first.Safety
References
lib/definition: "Third-party integrations and infrastructure wrappers (have side effects, configure SDK instances, manage lifecycle)"lib/ vs utils/table: utils = pure functions, lib = side effects + SDK integrationslib/directoryTest plan
bun run lint— 0 errorsbunx tsc --noEmit— 0 type errorsLink to Devin session: https://app.devin.ai/sessions/4370103ab5e547ae8696d39b423f7103
Requested by: @ashleeradka