Skip to content

refactor(frontend): type the SSE event layer with a discriminated union - #157

Closed
lucastononro wants to merge 5 commits into
fix/99-autoscrollfrom
fix/101-typed-sse
Closed

refactor(frontend): type the SSE event layer with a discriminated union#157
lucastononro wants to merge 5 commits into
fix/99-autoscrollfrom
fix/101-typed-sse

Conversation

@lucastononro

@lucastononro lucastononro commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Summary

  • ChatItem.meta was any, generatedFiles/restoredFiles were any[], the SSE handler cast event.data as any then read dozens of untyped fields, and MetricsTab's ChartTooltip props were any — all in exactly the place most likely to see a malformed/unvalidated backend payload.

Changes

  • src/lib/types.ts — defined SSEEvent as a discriminated union covering every event.type the backend publishes on /api/sessions/{id}/stream (state_change, agent_token/agent_message, tool_start/tool_end, code_output, agent_error, usage_event, report_ready, files_ready, file_created, agent_aborted, metrics_batch, metric, chart_config, log_event, canvas_html, subagent_start/subagent_end, clarification_request/clarification_resolved, agent_tool_call, clarification_exchange, notebook.created, the lineage events, and the task events). This replaces a set of unused, inaccurate *Data interfaces that predated this refactor (never imported anywhere, and didn't match the fields the handler actually reads).
  • src/app/page.tsx connectSSE — narrows on event.type per case (each case now has its own { } block so it can bind a correctly-typed local data), instead of one blanket const data = event.data as any shared across the whole switch. No behavior change — every field read is identical, just typed instead of implicit any.
  • ChatItem.meta is now ChatItemMeta — a flat, all-optional interface with real field names instead of any. (A precise per-ChatItem.type discriminated union would be more rigorous, but meta flows through ~8 render components — CollapsibleToolCard, SubAgentCard, ClarificationCard, AgentToolCard, ToolGroupCard — that read their own subset without first narrowing on item.type; threading a strict per-variant type through all of them is a much larger, riskier change than this issue calls for. Noted as deferred below.)
  • generatedFiles/restoredFiles are now GeneratedFile[] (reusing the existing GeneratedFile type) instead of any[].
  • Added metaStr/metaNum runtime-checked readers for Message.metadata (Record<string, unknown>, unvalidated persisted history) used when reconstructing ChatItem.meta on session-reload restore, replacing blind as casts with an actual runtime guard.
  • src/components/MetricsTab.tsxChartTooltip now has a real ChartTooltipProps interface instead of any.

Deferred

  • A full discriminated union for ChatItem (narrowing meta's shape per item.type) was scoped out — it would require changing the prop types of ~8 render components to accept only the narrowed member of the union, which is a much larger surface than this issue's ask and carries real regression risk for comparatively little benefit over the flat ChatItemMeta shape landed here.

Test plan

Existing (must still work) — this is a pure typing refactor, no runtime logic changed, but the SSE handler is the most-exercised code path in the app:

  • Streaming chat: full run — user message, assistant streaming, tool cards, subagent cards, clarification request/reply, agent-tool one-liners, error/status/stage-complete rows.
  • Live metrics tab: scalar metrics, metrics_batch, rich log_event panels (image grid/table/confusion matrix), chart_config.
  • File/image/PDF viewing: file_created/files_ready auto-opening the canvas, file tree updates.
  • Notebook updates (via notebook.created + the useNotebookSSE shared-stream path from PR Stop opening two EventSource connections to the same stream #100).
  • Reload a session mid-run: history restore (tool cards, subagent cards, agent-tool one-liners, clarification exchanges) renders identically from persisted Message.metadata.
  • Lineage events (experiment_created, dataset_registered, etc.) still refresh the lineage tab.
  • Task list CRUD (task_created/task_updated/task_deleted).

New

  • None — this PR has no new user-facing behavior, only stricter compile-time typing. npx tsc --noEmit and npm run lint both pass clean.

Closes #101

🤖 Generated with Claude Code

Greptile Summary

This PR replaces the untyped SSE event layer (event.data as any + type: string) with a discriminated union covering every event type the backend publishes, and strengthens adjacent any usages in page.tsx and MetricsTab.tsx. The changes are a pure compile-time refactor — no runtime logic is altered.

  • SSEEvent discriminated union (types.ts): each event.type literal now maps to a dedicated payload interface; the connectSSE switch narrows per-case with a locally-scoped const data = event.data instead of a blanket cast.
  • ChatItemMeta + metaStr/metaNum (page.tsx): meta: any replaced with a flat all-optional interface; session-restore path uses lightweight runtime guards instead of blind as casts on unvalidated persisted metadata.
  • ChartTooltip typed props (MetricsTab.tsx): any props replaced with ChartTooltipProps; entry.value now guarded before calling smartFormat.

Confidence Score: 5/5

Pure compile-time refactor with no runtime logic changes; all existing SSE handler behavior is preserved exactly.

Every field read in the SSE handler was already present before; the PR only adds types around them. The compile-time test file verifies discriminated-union narrowing, and the PR description confirms tsc --noEmit passes clean. No new execution paths are introduced.

Files Needing Attention: frontend/src/lib/types.ts — two small consistency gaps: MetricsBatchData.items typed as required while the handler still defends with || [], and NotebookCreatedData replicates the existing NotebookCreatedEvent shape instead of reusing it.

Important Files Changed

Filename Overview
frontend/src/lib/types.ts Replaces the flat SSEEvent with a full discriminated union; old inaccurate *Data interfaces removed; new per-event payload types added. Two minor inconsistencies: MetricsBatchData.items is required while the handler guards with
frontend/src/app/page.tsx SSE handler refactored to per-case const data = event.data narrowing; generatedFiles/restoredFiles typed as GeneratedFile[]; ChatItemMeta replaces any; metaStr/metaNum runtime guards added for session-restore path. No behavior changes.
frontend/src/components/MetricsTab.tsx ChartTooltip props typed via new ChartTooltipProps interface; entry.value now guarded with typeof === 'number' before calling smartFormat.
frontend/src/lib/notebook/useNotebookSSE.ts Inline StructureChangedEvent definition removed; type is now imported from ./types and re-exported, eliminating the duplicate.
frontend/src/lib/types.sse.test-d.ts New compile-time type assertion file verifying discriminated-union narrowing, ChartConfigSSEData.charts optionality, and rejection of unknown event types.
frontend/src/lib/notebook/types.ts StructureChangedEvent moved here from useNotebookSSE.ts; canonical home for all notebook SSE payload types.
frontend/src/lib/notebook/useNotebookSSE.test.tsx Test fixture updated to include the required exec_count: null field on notebook.cell.completed payloads, matching the newly-typed CellCompletedEvent.

Reviews (3): Last reviewed commit: "merge: staging-v0.0.5 into fix/101-typed..." | Re-trigger Greptile

… union

ChatItem.meta, generatedFiles/restoredFiles, the SSE handler's event.data,
and MetricsTab's ChartTooltip were all any-typed, defeating the type
system exactly where malformed backend payloads are most likely.

- lib/types.ts: define SSEEvent as a discriminated union over every
  event.type the backend publishes, replacing the old unused/inaccurate
  per-event Data interfaces with ones matching what the handler actually
  reads (and defends against).
- page.tsx connectSSE: narrow on event.type per case (each case gets its
  own block scope) instead of one blanket `event.data as any`; type
  ChatItem.meta as a flat, all-optional ChatItemMeta (a full discriminated
  union would require threading narrowed prop types through ~8 render
  components that read meta without first narrowing on item.type — out of
  scope for this change); type generatedFiles/restoredFiles as
  GeneratedFile[]; add metaStr/metaNum runtime-checked readers for
  Message.metadata on session-reload restore.
- MetricsTab.tsx: give ChartTooltip a real props interface instead of any.

Runtime behavior is unchanged — this is purely a typing refactor.

Closes #101

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wp66uSq9saSpRq9Bbmjxj4
Comment thread frontend/src/lib/types.ts Outdated
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wp66uSq9saSpRq9Bbmjxj4
@lucastononro

Copy link
Copy Markdown
Owner Author

Greptile review follow-up (c8a1bba)

Fixed — 1/1 findings:

  • [P2] frontend/src/lib/types.ts:383chart_config mapped to ChartConfig (required charts): Valid. The handler in page.tsx defensively guards data.charts && Array.isArray(data.charts), but with charts typed as required the guard read as dead code and was one "cleanup" away from a runtime break on a malformed payload. Added a dedicated wire type ChartConfigSSEData { charts?: ChartConfigEntry[] } (matching the existing LogEventSSEData wire-type convention) and pointed the union member at it. The guard now narrows ChartConfigEntry[] | undefinedChartConfigEntry[] before promoting to the app-level ChartConfig.

Dismissed: none.

Tests: the frontend has no test runner (no vitest/jest), and this PR is a compile-time typing change, so instead of bolting on a runner I added frontend/src/lib/types.sse.test-d.ts — compile-time assertions checked by both npx tsc --noEmit and next build (zero new deps, never bundled). It asserts:

  • narrowing on event.type yields the mapped payload type for chart_config / metric / tool_start / state_change;
  • ChartConfigSSEData.charts is optional (an empty chart_config payload stays assignable), so the defensive guard is load-bearing;
  • unknown event type strings are rejected by the union.

Verified the assertions bite: re-mapping chart_config back to the required-charts ChartConfig fails tsc with TS2344.

Manual smoke test (runtime path): start a session that emits charts (any run logging metrics with a chart config), open the Metrics tab, and confirm pinned panels appear when the chart_config SSE event arrives; the browser console should show no errors if the backend ever omits charts.

Validation: npm ci ✓ · npx tsc --noEmit ✓ · npm run lint ✓ · npm run build ✓ · prettier --check on touched files ✓

Conflicts/semantic reconciliation:
- MetricsTab.tsx: #157 typed ChartTooltip props (dropped `any`); staging's
  #161 compare page imports it — kept the typed props AND the export.
- SSE union extended with events other staging PRs added/consume:
  `budget_exceeded` (#165, new BudgetExceededSSEData) and the
  `notebook.kernel.state`/`notebook.structure.changed`/`notebook.cell.*`
  events dispatched by useNotebookSSE (#148's typed bus requires them).
  StructureChangedEvent moved to lib/notebook/types.ts (re-exported from
  useNotebookSSE) so lib/types.ts can reference it.
- page.tsx budget_exceeded case adapted to #157's per-case
  `const data = event.data` block pattern.
- useNotebookSSE.test.tsx: cell.completed payload gained exec_count: null
  to satisfy the now-typed CellCompletedEvent.
@lucastononro

Copy link
Copy Markdown
Owner Author

Merged into integration branch staging-v0.0.5 (see the STAGING-v0.0.5.md ledger on that branch for merge order, Greptile follow-ups and test results). These changes will land on main via the staging merge — closing to clear the queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant