WH-097: Review and improve frontend liveness with real-time updates#99
WH-097: Review and improve frontend liveness with real-time updates#99
Conversation
|
🦸 Review Hero (could not post inline comments — showing here instead)
[Bugs & Correctness] card_created events are appended to ALL cached board queries, not just the board for the card's project.
[Bugs & Correctness] The null guard
[Bugs & Correctness] The
[Design & Architecture] This hook has grown from a focused sidebar-events handler (~60 lines) into a 280-line god-hook that manages cache updates for 5 event types across 4+ query caches (sidebar-data, project-board, card-detail, branch-status, jockey). The name
[Design & Architecture] The refactor replaced per-userId listener sets with a single global broadcast set. Previously, a session notification for user A would only invoke user A's listener. Now every notification invokes every connected client's callback, and each callback fires its own DB query (findUnique for the changed record). With N connected SSE clients, a single DB write now triggers N database queries instead of 1. For sessions this is mitigated by the userId check, but for cards/comments/activities there's no early filtering — every client queries and processes every change. Consider restoring a lightweight routing layer (e.g. keying listeners by table name, or by project ID) so notifications only reach interested subscribers.
[Design & Architecture] The 150ms debounce is applied uniformly to every table. This makes sense for ConversationSession — a single agent turn fires many field-level UPDATE triggers in quick succession. But CardComment and CardActivity are insert-only events that fire exactly once per record, so debouncing them just adds 150ms of latency for no coalescing benefit. Consider skipping the debounce for insert operations or for tables that don't produce bursts (route directly to
[Frontend Snappiness] Broadcasting every notification to every listener causes O(N) DB queries per change event instead of O(1). Previously
[Frontend Snappiness]
[Security] Authorization bypass: card, comment, and activity events are broadcast to all authenticated users without checking GitHub repo access. The existing
[Security] The global listener fan-out means every Postgres notification triggers callbacks for ALL connected SSE clients. Each callback performs a full Prisma query (card, comment, or activity lookup) before checking access. With N connected users and M changes/sec, this is N×M database queries — most of which will be discarded. Previously, |
|
🦸 Review Hero Summary Below consensus threshold (5 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
|
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
|
🦸 Review Hero Summary Below consensus threshold (6 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
|
|
🦸 Review Hero Summary Below consensus threshold (26 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
|
…sion broadcast, fail-closed auth refresh
| } | ||
| } | ||
|
|
||
| export interface CardEvent { |
There was a problem hiding this comment.
[Design & Architecture] suggestion
The card payload shape is independently defined in three places: CardEvent here (line 44), CardSSE in realtimeCacheUpdaters.ts (line 11), and the Prisma select + manual mapping in hydrateCard (line 249). The comment/activity shapes have the same problem. Adding a field to the card model requires coordinated changes across all three files plus toSSE() in the route handler. Extract the wire-format payload types into a shared src/lib/realtimeTypes.ts (no server imports needed — they're plain interfaces) so the server hydration, the route toSSE mapper, and the client updaters all reference a single source of truth.
| const encoder = new TextEncoder() | ||
| let cleanup: (() => void) | null = null | ||
|
|
||
| // Build project access set filtered by GitHub repo access. |
There was a problem hiding this comment.
[Design & Architecture] suggestion
The access-refresh state machine (refreshAccessibleTeams, isRefreshing, consecutiveRefreshFailures, MAX_REFRESH_FAILURES, accessibleTeamIds) is a non-trivial piece of logic embedded inside the SSE route handler closure. It mixes authorisation concerns (which teams can this user see?) with SSE wiring (stream events to the client). This makes it untestable in isolation and adds ~40 lines of complexity to an already long route. Consider extracting it into a self-contained class or module (e.g. UserAccessScope) that encapsulates the set, refresh interval, failure counting, and fail-closed behaviour. The route handler would then just call scope.teamIds in the filter and scope.maybeRefresh() in the keepalive.
|
|
||
| if (event.type === 'session_updated') { | ||
| qc.setQueryData<SidebarData>(['sidebar-data'], (old) => { | ||
| if (!old) return old |
There was a problem hiding this comment.
[Design & Architecture] suggestion
updateCardCache manually patches three independent query caches (sidebar, board, card-detail) by copying every field from the SSE event. This is 90+ lines of hand-rolled cache synchronisation that must be kept in sync with each cache's shape. For the board and card-detail caches — which are page-scoped and benefit less from sub-second latency — consider using invalidateQueries (triggering a background refetch) instead of manual patching. This would halve the function's size and eliminate a class of bugs where the patched shape drifts from the query's actual return type. Reserve manual setQueryData for the sidebar, where the instant update is visually important.
| let cachedProjects: Awaited<ReturnType<typeof getProjects>> | null = null | ||
| let projectCacheTime = 0 | ||
|
|
||
| async function getCachedProjects() { |
There was a problem hiding this comment.
[Frontend Snappiness] suggestion
Classic cache stampede: when the 60s TTL expires, every concurrent caller to getCachedProjects() (e.g. multiple SSE connections starting up or hitting their keepalive refresh simultaneously) will all see the stale cache and each independently call getProjects(). Fix by storing the in-flight promise and letting concurrent callers await it:
let inflightPromise: Promise<...> | null = null
async function getCachedProjects() {
if (cachedProjects && Date.now() - projectCacheTime < PROJECT_CACHE_TTL_MS) return cachedProjects
if (inflightPromise) return inflightPromise
inflightPromise = getProjects().then(result => {
cachedProjects = result
projectCacheTime = Date.now()
return result
}).finally(() => { inflightPromise = null })
return inflightPromise
}
|
🦸 Review Hero Summary Below consensus threshold (16 unique issues not confirmed by majority)
Nitpicks
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
|
1 participant
Summary
Liveness of interface improvements:
These are just a few that I've noticed. In general I'd really like the front end to be as live as possible, using sockets or whatever and db trigger listen/notify tooling to ensure that happens. Do a full review and propose changes
Journey
Specs
.workhorse/specs/agent-streaming-status.md.workhorse/specs/conversation-sessions.md.workhorse/specs/frontend-liveness.mdChanged files
prisma/migrations/0002_generic_change_notify_trigger/migration.sqlsrc/app/api/sidebar-events/route.tssrc/components/card/CardWorkspace.tsxsrc/lib/dbNotifications.tssrc/lib/hooks/useSidebarEvents.tsTest plan
Review Hero