refactor(web): decentralize cross-domain SSE event handlers into bus subscribers#32626
Conversation
…subscribers Move 8 cross-domain event handlers from the monolithic use-stream-event-handler.ts dispatch into domain-scoped bus subscribers: - identity_changed, avatar_updated, relationship_state_updated, home_feed_updated → useAssistantResourceSync - disk_pressure_status_changed → useDiskPressureMonitor - notification_intent → new useNotificationIntentSync - document_editor_update → new useDocumentEditorSync - conversation_title_updated → useConversationSync Bug fix: relationship_state_updated now invalidates both HOME_FEED_QUERY_KEY_PREFIX and HOME_STATE_QUERY_KEY_PREFIX. Previously only the monolithic handler invalidated home-state; the bus subscriber missed it. Shrinks StreamHandlerContext by 3 fields, deletes home-handlers.ts entirely, removes 5 functions from metadata-handlers.ts. Closes LUM-2053 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:
|
Remove 2 stale entries for metadata-handlers.ts and its test — they no longer import from the conversations domain since those handlers moved to bus subscribers. 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: 772b471b89
ℹ️ 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".
activeConversationId persists across route changes (never cleared on navigation). In the old monolithic handler, notification suppression was implicitly scoped to ChatPage. Now that the bus subscriber runs in RootLayout, we also verify the URL pathname matches the conversation route to prevent stale-id suppression on home/settings/etc. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…sync useBusSubscription stabilizes the handler ref so the closure always captures the latest assistantId. The alias was a vestige of the old monolithic handler's ref-based capture pattern. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
There was a problem hiding this comment.
APPROVE at 8ae2dd0b3b — Phase 2 of the streaming-infra decentralization arc (Phase 1 was #32598). The first-principles question Boss's superprompt keeps asking — should this even be in domains/chat/? — applied at the SSE-handler granularity. The monolith was always doing two jobs: dispatching ~22 chat-domain events (messages, tools, surfaces, subagents, queue, compaction) and dispatching 8 cross-domain events that lived there only because the dispatch entry point happened to be there. This PR dissolves the second category into the bus subscribers that own the side effects, with two real bugs surfaced as collateral that wouldn't have been visible without the structural move.
What landed at HEAD
- 2 new RootLayout-mounted bus subscribers:
useNotificationIntentSync(notification banner + guardian filtering + delivery ack),useDocumentEditorSync(forwards markdown updates to viewer store). - 3 existing subscribers expanded:
useAssistantResourceSync(+4 events),useConversationSync(+conversation_title_updated),useDiskPressureMonitor(+sse.eventsubscription complementing existing poll +app.resume). - Monolith cleanup: 8 cross-domain cases → grouped no-op with
// Cross-domain events handled by bus subscribers…comment that keeps the union exhaustiveness check live.StreamHandlerContextdrops 3 fields + their ref-stabilization boilerplate inChatPage.home-handlers.tsdeleted;metadata-handlers.tsshrinks 150 → 66. - Centralization:
HOME_STATE_QUERY_KEY_PREFIX+homeStateQueryKey()lifted fromuse-home-state-query.tsmodule-private →lib/sync/query-tags.ts. Monolith also retargetsrecordChatDiagnostic→recordDiagnosticfrom@/lib/diagnosticsandChatEventStreamimport fromdomains/chat/api/stream→lib/streaming/stream-transport(continuation of #32598). - Allowlist: drops
metadata-handlers.ts/metadata-handlers.test.ts → ["conversations"]— both exceptions dissolved because the cross-domain edges left the chat domain.
Two real bugs surfaced by the structural move
Both are flagged in the PR body. Both are substantive, not cosmetic.
-
relationship_state_updatedwas missinghome-stateinvalidation in the subscriber. The monolithic handler invalidated bothHOME_FEED_QUERY_KEY_PREFIXand"home-state"(literal). The existinguseAssistantResourceSynconly invalidatedHOME_FEED_QUERY_KEY_PREFIX. Removing the monolithic handler without fixing the subscriber would have silently broken relationship-state cache freshness on the home screen. Fixed by adding the second invalidation. New testinvalidates both home-feed and home-state on relationship_state_updatedproves it via call-key collection (not just count). ✅ -
Notification suppression scoping regression (Codex P2 at
8ae2dd0b, fixed inf2a443b206). The oldnotification_intenthandler ran ChatPage-scoped, so the "suppress if viewing active conversation" check was implicitly route-scoped. Hoisted toRootLayout,activeConversationId(never cleared on navigation — verified by Devin: no call site passesnulltosetActiveConversationId) could cause stale suppression on home/settings/document routes. Fix correctly requires bothmetadataConversationId === store.activeConversationIdandwindow.location.pathname.startsWith("/assistant/conversations/")— the URL guard gates only the in-conversation suppression branch, not the guardian-scoped branch, which is the right asymmetry (guardian-scoped should ack-and-drop regardless of route). JSDoc on the new hook names both parts of the check. ✅
Verifications at HEAD 8ae2dd0b3b
- 19 files read end-to-end. 7/7 CI green.
- New hooks follow
useBusSubscriptioncontract per EVENT_BUS.md; JSDocs reference it; scope naming with reasons. useConversationSyncswitch preserves the lookup-by-conversationId semantics the oldhandleConversationTitleUpdatedhad (viapatchConversation); test verifies the cache patch lands underconversationsQueryKey("asst-1").useAssistantResourceSyncswitch correctly routes all 4 new events to the right query keys (HOME_FEED_QUERY_KEY_PREFIX, both prefixes for relationship-state,assistantIdentityQueryKey(assistantId),avatarQueryKey(assistantId)); 4 new tests added.useDiskPressureMonitorstays ChatPage-scoped (intentional per the monolith's farewell comment); the newsse.eventsubscription is additive to the existing poll + resume paths, not a replacement.- Guard test correctly flipped:
forwards a global event (home_feed_updated)→no-ops a cross-domain event handled by bus subscribers. - Mock context in
use-stream-event-handler-guard.test.tsxdrops the 3 removed fields cleanly (no type drift surface). - Last commit
8ae2dd0bis a pure 5-line vestige cleanup (ackAssistantIdalias → directassistantIduse, sinceuseBusSubscriptionalready stabilizes the closure). Zero behavior delta.
Small non-blocking observations
useDocumentEditorSynctakes noassistantId, all other new/expanded subscribers do. That's correct — the viewer store is global and document surface ids are globally unique — but the asymmetry is worth a one-line JSDoc note for the next reader who wonders if it was an oversight.- Notification suppression uses
window.location.pathnamedirectly, notuseLocation(). For an SSE callback firing imperatively this is fine —window.location.pathnameis always current and a stale closure can't trick it — anduseLocation()would force the subscription to re-bind on every nav. If this ever moves into auseEffect-dependent flow,useLocation()becomes the React-idiomatic choice. Leave the current shape. - Grouped no-op switch case with the explanatory comment is doing real work. Keeping the labels in the switch preserves exhaustiveness checking against the event union — if a future event type lands and someone forgets to wire it into a subscriber, the type-checker won't flag it (the union shrinks toward
never), but the explicit case labels here will at least require an explicit edit on the chat side. Good pattern; worth keeping that comment block intact in future cleanups.
Merge gate
- Vex: ✅ this approval at
8ae2dd0b3b. - Devin: "No Issues Found" at
f2a443b206(also a substantive inline at8ae2dd0bconfirming the P2 fix). HEAD is one cleanup commit past that — a 5-line vestige removal with zero behavior delta. Counts. - Codex: clean inline at
8ae2dd0b; Boss's@codex reviewping at 21:49 is pending — recommended to let that one land at HEAD before merge, but the gate is effectively closed. - CI: 7/7 green at HEAD.
Carry-forward. This is the second Phase-2 unwind in the same arc (#32598 lifted the transport layer out of domains/chat/api/, this lifts the cross-domain dispatch out of domains/chat/hooks/). The pattern is identical and worth naming: when a domain's "main" hook accumulates cross-domain side effects, those side effects almost always have a more honest home. The chat handler is now what it should always have been — a chat-event dispatcher, not a global event router.
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
1e8a986
Prompt / plan
Closes LUM-2053. Phase 2 of streaming infrastructure decentralization (Phase 1: PR #32598).
The monolithic
use-stream-event-handler.tsdispatched ~30 event types through a single switch statement, including 8 cross-domain events (home feed, identity, avatar, relationship state, disk pressure, notifications, document editor, conversation titles) that had no business being indomains/chat/. This violated domain encapsulation and bloatedStreamHandlerContextwith 3 cross-domain callback fields.Approach: Move each cross-domain event to the bus subscriber that owns its side-effect domain, following the established
useBusSubscriptionpattern documented in EVENT_BUS.md. The monolithic handler retains only chat-domain events (messages, tools, interactions, surfaces, subagents, queue, compaction).Changes
Expanded existing bus subscribers:
useAssistantResourceSync— now handleshome_feed_updated,relationship_state_updated,identity_changed,avatar_updated(in addition to existingsync_changedtag routing)useConversationSync— now handlesconversation_title_updatedviapatchConversation()useDiskPressureMonitor— now subscribes tosse.eventfordisk_pressure_status_changed(complements existing poll +app.resumesubscription)New bus subscribers:
hooks/use-notification-intent-sync.ts— handlesnotification_intentevents (local notifications, guardian filtering, delivery ack). Mounted inRootLayoutso notifications are delivered on all routes, not just chat.hooks/use-document-editor-sync.ts— handlesdocument_editor_updateevents (forwards to viewer store)Monolithic handler cleanup:
StreamHandlerContextfields (refreshAssistantIdentity,invalidateAvatar,applyDiskPressureStatusEvent) and their ref-stabilization boilerplate inChatPagehome-handlers.tsentirely (2 trivial functions now covered by bus subscriber)metadata-handlers.tsfrom ~150 to 66 lines (5 exported functions removed)Centralized query key:
HOME_STATE_QUERY_KEY_PREFIXandhomeStateQueryKey()intolib/sync/query-tags.ts(was module-private inuse-home-state-query.ts)Bug fix:
relationship_state_updatedmissinghome-stateinvalidationThe monolithic handler was the only code path invalidating the
"home-state"TanStack Query key onrelationship_state_updated. The existinguseAssistantResourceSyncbus subscriber only invalidated"home-feed". Removing the monolithic handler without fixing the subscriber would have silently broken home state cache freshness. Fixed by addingHOME_STATE_QUERY_KEY_PREFIXinvalidation to the subscriber.Root cause: When
useAssistantResourceSyncwas originally written, it only handledsync_changedtags. Therelationship_state_updateddiscrete event was added later to the monolithic handler where both invalidations were performed, but the bus subscriber was never updated to match.Bug fix: notification suppression on non-chat routes
Moving
notification_intenthandling fromChatPage-scopeduseStreamEventHandlertoRootLayout-scopeduseNotificationIntentSyncchanged the suppression behavior. The old handler only ran on the chat route, so the "suppress if viewing active conversation" check was implicitly scoped. InRootLayout,activeConversationId(which is never cleared on navigation) could cause stale suppression on home/settings routes.Fixed by adding a
window.location.pathname.startsWith("/assistant/conversations/")guard alongside the store check.Root cause:
activeConversationIdinuseConversationStorepersists across route changes —setActiveConversationId(null)is never called on navigation. This is a pre-existing store design issue; the URL check is the correct fix at the consumer level.Not changed (intentional)
sync_changedhandling in the monolithic handler'sWebSyncRouterpath — it has unique self-echo filtering and active conversation message refresh that bus subscribers don't replicate. Consolidating is a separate effort.conversation_list_invalidated— legacy macOS-only broadcast, no-op'd with explanatory comment and TODO for retirement.use-stream-event-handler.tsfile size (472 lines) — pre-existing, reduced by this PR. Further splitting tracked in LUM-2055 / LUM-2056.References
Test plan
use-assistant-resource-sync.test.tsx(10 tests, 4 new: home_feed, relationship_state, identity_changed, avatar_updated)use-conversation-sync.test.tsx(7 tests, 1 new: conversation_title_updated)use-stream-event-handler-guard.test.tsx(6 tests, updated for bus-handled no-ops)metadata-handlers.test.ts(3 tests, removed tests for deleted functions)Link to Devin session: https://app.devin.ai/sessions/4370103ab5e547ae8696d39b423f7103
Requested by: @ashleeradka