[codex] Fix live arena SSE and add commentary booth#384
Merged
Conversation
Checkpoint: completes reviewed steps 1-2 from testing/codex-live-view-commentary-sidebar.md.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR fixes a critical SSE field-name mismatch that caused live arena lanes to remain stuck during active runs, and adds an optional commentary sidebar as a new feature. Core bug fix — SSE normalization:
New commentary sidebar:
Test coverage:
Confidence Score: 4/5Safe to merge — the core bug fix is correct and well-tested; remaining concerns are all non-blocking P2 style items. The SSE normalization fix is the primary goal and is implemented correctly with solid unit and integration test coverage. The commentary feature is well-structured (bounded store, deduplication, correct reducer pattern, proper cleanup effect). All open issues are P2: a display-limit vs store-limit mismatch, unused
|
| Filename | Overview |
|---|---|
| web/src/hooks/use-run-events.ts | Adds normalizeRunEvent to map snake_case SSE wire format to the Pascal/camelCase RunEvent shape; hooks into the SSE listener correctly with a stable onEventRef pattern. |
| web/src/hooks/use-agent-commentary.ts | New hook with bounded, deduplicated commentary reducer; well-structured, but reset() is exported and never used, leading to stale entries accumulating while the sidebar is hidden. |
| web/src/components/arena/live-commentary-sidebar.tsx | New sidebar component; slice(-12) display limit mismatches MAX_COMMENTARY_ENTRIES = 24, and formatClock uses local timezone instead of UTC for backend ISO timestamps. |
| web/src/app/(workspace)/workspaces/[workspaceId]/runs/[runId]/run-detail-client.tsx | Wires commentary hook into the SSE event pipeline and adds the toggle + sidebar layout; adds correct cleanup useEffect for fetchTimerRef; reset() is not called on toggle-off. |
| web/src/hooks/use-run-events.test.ts | Good coverage of normalizeRunEvent for both snake_case and already-normalized inputs, plus an integration test for the SSE listener normalization path. |
| web/src/hooks/use-agent-commentary.test.ts | Tests deduplication, bounding, event suppression, and meaningful event commentary generation; well-structured. |
| web/src/app/(workspace)/workspaces/[workspaceId]/runs/[runId]/run-detail-client.test.tsx | Integration test for the toggle + live event → lane + sidebar update pipeline; good use of real module parts via vi.importActual. |
Sequence Diagram
sequenceDiagram
participant BE as Backend SSE
participant UE as useRunEvents
participant NE as normalizeRunEvent
participant HS as handleSSEEvent
participant UA as useAgentArena
participant UC as useAgentCommentary
participant UI as RunDetailClient
BE->>UE: run_event (snake_case JSON)
UE->>NE: normalizeRunEvent(raw)
NE-->>UE: RunEvent (PascalCase fields)
UE->>HS: onEvent(RunEvent)
HS->>UA: handleArenaEvent(event)
UA-->>UI: updated arenaLanes → LiveAgentLane re-renders
HS->>UC: handleCommentaryEvent(event)
UC-->>UI: updated entries → LiveCommentarySidebar (if showCommentary)
HS->>HS: debounce 300ms → fetchAll()
Comments Outside Diff (1)
-
web/src/app/(workspace)/workspaces/[workspaceId]/runs/[runId]/run-detail-client.tsx, line 310-313 (link)Commentary entries accumulate while the sidebar is hidden;
resetis never calledhandleCommentaryEventis dispatched on every SSE event regardless of theshowCommentarytoggle. When a user turns commentary off and later turns it back on, they see up to 24 stale entries from before they re-enabled the sidebar. Thereset()function exported fromuseAgentCommentaryis never called anywhere.If the intended UX is a fresh feed each time the user opts in, call
reset()when the toggle transitions tofalse:onClick={() => setShowCommentary((current) => { if (current) reset(); return !current; }) }
If showing history-on-re-enable is intentional, a brief code comment explaining that design choice would help future maintainers.
Prompt To Fix With AI
This is a comment left during a code review. Path: web/src/app/(workspace)/workspaces/[workspaceId]/runs/[runId]/run-detail-client.tsx Line: 310-313 Comment: **Commentary entries accumulate while the sidebar is hidden; `reset` is never called** `handleCommentaryEvent` is dispatched on every SSE event regardless of the `showCommentary` toggle. When a user turns commentary off and later turns it back on, they see up to 24 stale entries from before they re-enabled the sidebar. The `reset()` function exported from `useAgentCommentary` is never called anywhere. If the intended UX is a fresh feed each time the user opts in, call `reset()` when the toggle transitions to `false`: ```ts onClick={() => setShowCommentary((current) => { if (current) reset(); return !current; }) } ``` If showing history-on-re-enable is intentional, a brief code comment explaining that design choice would help future maintainers. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: web/src/components/arena/live-commentary-sidebar.tsx
Line: 44
Comment:
**Display limit doesn't match bounded store size**
The sidebar renders only the last 12 entries via `entries.slice(-12)`, but `MAX_COMMENTARY_ENTRIES` is 24. This means `useAgentCommentary` stores up to 24 entries in memory, but only half are ever visible in the sidebar. The scrollable `max-h-[34rem]` container with `space-y-2` has room for more entries. Either the display slice should match `MAX_COMMENTARY_ENTRIES` (or a named constant derived from it), or the store limit should be set to 12 to avoid the mismatch.
```suggestion
const recent = entries.slice(-MAX_COMMENTARY_ENTRIES);
```
(Requires importing `MAX_COMMENTARY_ENTRIES` from `@/hooks/use-agent-commentary`.)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/src/components/arena/live-commentary-sidebar.tsx
Line: 21-27
Comment:
**`formatClock` renders timestamps in the user's local timezone**
`d.getHours()` / `d.getMinutes()` / `d.getSeconds()` use the browser's local timezone. The `occurredAt` timestamps from the backend are UTC ISO-8601 strings. Users in non-UTC timezones will see clock values that are offset from the actual event time, and there is no timezone indicator displayed.
Consider using UTC accessors (`getUTCHours`, `getUTCMinutes`, `getUTCSeconds`) plus a "UTC" label, or `Intl.DateTimeFormat` with `timeZone: "UTC"` (or the user's explicit preference):
```suggestion
function formatClock(iso: string): string {
const d = new Date(iso);
const h = d.getUTCHours().toString().padStart(2, "0");
const m = d.getUTCMinutes().toString().padStart(2, "0");
const s = d.getUTCSeconds().toString().padStart(2, "0");
return `${h}:${m}:${s}`;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/src/app/(workspace)/workspaces/[workspaceId]/runs/[runId]/run-detail-client.tsx
Line: 310-313
Comment:
**Commentary entries accumulate while the sidebar is hidden; `reset` is never called**
`handleCommentaryEvent` is dispatched on every SSE event regardless of the `showCommentary` toggle. When a user turns commentary off and later turns it back on, they see up to 24 stale entries from before they re-enabled the sidebar. The `reset()` function exported from `useAgentCommentary` is never called anywhere.
If the intended UX is a fresh feed each time the user opts in, call `reset()` when the toggle transitions to `false`:
```ts
onClick={() =>
setShowCommentary((current) => {
if (current) reset();
return !current;
})
}
```
If showing history-on-re-enable is intentional, a brief code comment explaining that design choice would help future maintainers.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(web): fix live arena SSE and add co..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| className, | ||
| }: LiveCommentarySidebarProps) { | ||
| const scrollRef = useRef<HTMLDivElement>(null); | ||
| const recent = entries.slice(-12); |
Contributor
There was a problem hiding this comment.
The sidebar renders only the last 12 entries via entries.slice(-12), but MAX_COMMENTARY_ENTRIES is 24. This means useAgentCommentary stores up to 24 entries in memory, but only half are ever visible in the sidebar. The scrollable max-h-[34rem] container with space-y-2 has room for more entries. Either the display slice should match MAX_COMMENTARY_ENTRIES (or a named constant derived from it), or the store limit should be set to 12 to avoid the mismatch.
Suggested change
| const recent = entries.slice(-12); | |
| const recent = entries.slice(-MAX_COMMENTARY_ENTRIES); |
(Requires importing MAX_COMMENTARY_ENTRIES from @/hooks/use-agent-commentary.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/components/arena/live-commentary-sidebar.tsx
Line: 44
Comment:
**Display limit doesn't match bounded store size**
The sidebar renders only the last 12 entries via `entries.slice(-12)`, but `MAX_COMMENTARY_ENTRIES` is 24. This means `useAgentCommentary` stores up to 24 entries in memory, but only half are ever visible in the sidebar. The scrollable `max-h-[34rem]` container with `space-y-2` has room for more entries. Either the display slice should match `MAX_COMMENTARY_ENTRIES` (or a named constant derived from it), or the store limit should be set to 12 to avoid the mismatch.
```suggestion
const recent = entries.slice(-MAX_COMMENTARY_ENTRIES);
```
(Requires importing `MAX_COMMENTARY_ENTRIES` from `@/hooks/use-agent-commentary`.)
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+21
to
+27
| function formatClock(iso: string): string { | ||
| const d = new Date(iso); | ||
| const h = d.getHours().toString().padStart(2, "0"); | ||
| const m = d.getMinutes().toString().padStart(2, "0"); | ||
| const s = d.getSeconds().toString().padStart(2, "0"); | ||
| return `${h}:${m}:${s}`; | ||
| } |
Contributor
There was a problem hiding this comment.
formatClock renders timestamps in the user's local timezone
d.getHours() / d.getMinutes() / d.getSeconds() use the browser's local timezone. The occurredAt timestamps from the backend are UTC ISO-8601 strings. Users in non-UTC timezones will see clock values that are offset from the actual event time, and there is no timezone indicator displayed.
Consider using UTC accessors (getUTCHours, getUTCMinutes, getUTCSeconds) plus a "UTC" label, or Intl.DateTimeFormat with timeZone: "UTC" (or the user's explicit preference):
Suggested change
| function formatClock(iso: string): string { | |
| const d = new Date(iso); | |
| const h = d.getHours().toString().padStart(2, "0"); | |
| const m = d.getMinutes().toString().padStart(2, "0"); | |
| const s = d.getSeconds().toString().padStart(2, "0"); | |
| return `${h}:${m}:${s}`; | |
| } | |
| function formatClock(iso: string): string { | |
| const d = new Date(iso); | |
| const h = d.getUTCHours().toString().padStart(2, "0"); | |
| const m = d.getUTCMinutes().toString().padStart(2, "0"); | |
| const s = d.getUTCSeconds().toString().padStart(2, "0"); | |
| return `${h}:${m}:${s}`; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/components/arena/live-commentary-sidebar.tsx
Line: 21-27
Comment:
**`formatClock` renders timestamps in the user's local timezone**
`d.getHours()` / `d.getMinutes()` / `d.getSeconds()` use the browser's local timezone. The `occurredAt` timestamps from the backend are UTC ISO-8601 strings. Users in non-UTC timezones will see clock values that are offset from the actual event time, and there is no timezone indicator displayed.
Consider using UTC accessors (`getUTCHours`, `getUTCMinutes`, `getUTCSeconds`) plus a "UTC" label, or `Intl.DateTimeFormat` with `timeZone: "UTC"` (or the user's explicit preference):
```suggestion
function formatClock(iso: string): string {
const d = new Date(iso);
const h = d.getUTCHours().toString().padStart(2, "0");
const m = d.getUTCMinutes().toString().padStart(2, "0");
const s = d.getUTCSeconds().toString().padStart(2, "0");
return `${h}:${m}:${s}`;
}
```
How can I resolve this? If you propose a fix, please make it concise.Checkpoint: completes reviewed step 3 from testing/codex-live-view-commentary-sidebar.md.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Why
The live arena could show the run as
Livewhile each lane stayed stuck because the frontend was readingEventID/RunAgentID/EventType, but the backend SSE stream is emitted asevent_id/run_agent_id/event_type. As a result, events parsed but the live lane reducer saw empty fields and never advanced lane state.User impact
Validation
cd web && npm test -- src/hooks/use-run-events.test.ts src/hooks/use-agent-commentary.test.ts 'src/app/(workspace)/workspaces/[workspaceId]/runs/[runId]/run-detail-client.test.tsx'cd web && npm run lint -- src/hooks/use-run-events.ts src/hooks/use-run-events.test.ts src/hooks/use-agent-commentary.ts src/hooks/use-agent-commentary.test.ts src/components/arena/live-commentary-sidebar.tsx 'src/app/(workspace)/workspaces/[workspaceId]/runs/[runId]/run-detail-client.tsx' 'src/app/(workspace)/workspaces/[workspaceId]/runs/[runId]/run-detail-client.test.tsx'Notes