Claude/fix UI performance

[jeremyeder]

No description provided.

[jeremyeder]

Reviewer: the guts of this change are here

bfa0e8f#diff-8d67cd8823b6dd8fd3997ca62cf8edfb0a8b23b161edc0ab4ed9567dccdcd77fR53

[github-actions]

Claude Code Review

Summary

This PR targets two frontend performance issues during AG-UI streaming sessions:

1. use-agui-stream.ts — Introduces requestAnimationFrame-based event batching to collapse ~20-50 SSE events/frame into a single React render cycle, and stabilises the sendMessage callback via a state snapshot ref.
2. page.tsx — Removes two redundant setInterval polling effects (superseded by useSession's existing 500–1000ms transitional-state polling) and trims an over-specified useEffect dependency array.

No backend, operator, or security-sensitive code is touched. No new any types are introduced. The changes are frontend-only and performance-focused.

---

Issues by Severity

🚫 Blocker Issues

None.

🔴 Critical Issues

None.

🟡 Major Issues

None.

🔵 Minor Issues

1. Connection-check semantics changed in sendMessage (use-agui-stream.ts ~line 349)

- if (!eventSourceRef.current) {
+ if (stateSnapshotRef.current.status !== 'connected') {

These two conditions are not equivalent:

State	Old behaviour	New behaviour
idle (no EventSource)	Calls connect() ✅	Calls connect() ✅
connecting (EventSource exists)	Does not call connect() ✅	Calls connect() again ⚠️
error / completed (EventSource cleaned up)	May not call connect()	Calls connect() ✅

During the connecting → connected transition (typically milliseconds), a sendMessage call will now invoke connect() a second time, potentially tearing down and re-creating the in-flight EventSource. In practice the race window is very narrow and user-initiated messages are unlikely to land in it, but the semantics diverge from the old guard in a non-obvious way.

Suggestion: Consider stateSnapshotRef.current.status === 'idle' || stateSnapshotRef.current.status === 'error' — or keep !eventSourceRef.current since it was the correct pre-condition for "no SSE connection exists at all".

---

2. Trailing blank line introduced (use-agui-stream.ts ~line 309)

      }),
    ))
+
+
    try {

Two blank lines where the rest of the file uses one. Trivial nit but breaks visual consistency.

---

3. No test coverage for the rAF batching path

The new flushEventBuffer / scheduleFlush / processEventRef triad is the most complex addition in the PR. There are no unit tests validating:

• that events buffered before the rAF fires are all processed on flush
• that the buffer is fully drained and cleared correctly
• that cleanup cancels a pending rAF on unmount/disconnect

This is not a blocker (the existing hook likely lacks tests already), but given this is a performance-critical path for the core streaming feature, a small set of Jest/RTL tests would significantly improve confidence.

---

Positive Highlights

rAF batching is the right tool here. Collapsing N SSE events per animation frame into one React render pass is a well-established pattern and is particularly effective under React 18's automatic batching of synchronous setState calls. The implementation is clean:

• eventBufferRef drains atomically (assigned [] before the loop, not after)
• rafIdRef is set to null before processing, preventing double-scheduling
• Both unmount (useEffect cleanup) and disconnect() cancel pending frames and clear the buffer — no leaks

stateSnapshotRef correctly stabilises sendMessage. Removing state.threadId and state.runId from the sendMessage useCallback deps while keeping the ref in sync via a dedicated useEffect is the standard "ref as escape hatch" pattern. The dep array accurately reflects the callback's actual capture ([projectName, sessionName, connect]).

Removing redundant polling is the right call. The two removed setInterval(refetchSession, 2000) effects were slower than useSession's 500–1000ms polling during transitional phases and were creating unnecessary network churn. The inline // NOTE: comments clearly explain the reasoning for future maintainers.

Trimming the initialPrompt effect deps is correct. The effect only updates a ref and the only meaningful input is session?.spec?.initialPrompt. The previous inclusion of session?.status?.phase, aguiState.messages.length, and aguiState.status caused the effect to re-evaluate on every streamed token with zero functional benefit.

Unmount/disconnect cleanup is thorough. rafIdRef, eventBufferRef, reconnectTimeoutRef, and eventSourceRef are all properly nulled out in both the unmount guard and disconnect().

---

Recommendations

1. Address the sendMessage connection guard (item Outcome: Reduce Refinement Time with agent System #1) — decide whether the intent is "no EventSource exists" or "not in a fully connected state" and pick the condition that matches. If 'error' recovery is the desired improvement, document the intent explicitly.
2. Fix the double blank line — cosmetic but easy.
3. Add tests when the hook test suite is established (not blocking this PR, but recommended as follow-up).

Overall this is a well-reasoned, focused performance PR with no security concerns and no violations of project standards. The rAF batching approach directly addresses the root cause of streaming jank.

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.

[github-actions]

Claude Code Review

Summary

This PR targets three root causes of UI jank during agent streaming: per-token React re-renders from raw SSE handling, a frequently-recreated sendMessage callback, and two redundant polling intervals that duplicated work already done by useSession. The approach is technically sound and the reduction from ~50 renders/sec to ≤60fps is meaningful. Most issues are minor.

---

Issues by Severity

🚫 Blocker Issues

None.

🔴 Critical Issues

None.

🟡 Major Issues

1. Removed polling depends on an undocumented invariant (page.tsx:309–336)

Both removed setInterval effects are replaced with comments claiming useSession already polls at 500–1000ms during transitional states. That claim is almost certainly true today, but it is now load-bearing: if useSession's polling cadence or transitional-state detection changes, queued workflows and messages will silently stop working while tests continue to pass (because no test covers this interaction).

Recommendation: add a refetchInterval assertion in a test or, at minimum, add an inline reference to the specific hook+option that provides the coverage (e.g., // useSession: refetchInterval: 500 in Pending/Creating/Stopping — see use-session.ts:42).

---

🔵 Minor Issues

2. stateSnapshotRef sync has a one-render stale window (use-agui-stream.ts:80–86)

useEffect(() => {
  stateSnapshotRef.current = { threadId: ..., runId: ..., status: ... }
}, [state.threadId, state.runId, state.status])

useEffect fires after the render in which state changed. If sendMessage is called in the same event loop tick as a state change (e.g., rapid successive sends), the snapshot will hold the previous values for that one render cycle. In practice this is unlikely to cause observable bugs, but the standard mitigation is to write directly to the ref inside the reducer/setState callback rather than syncing it via useEffect. Worth a comment acknowledging the accepted trade-off.

3. Extra blank lines (cosmetic, use-agui-stream.ts)

Two extra blank lines snuck in — after the stateSnapshotRef declaration block (~line 63 of the diff) and after setIsRunActive(true) (~line 330). npm run build won't flag these but they add noise.

4. No new tests for the rAF batching path

The flushEventBuffer/scheduleFlush path is the most mechanically novel code in this PR. A unit test that fires N events on the mock SSE and asserts render count ≤ N/frame-budget would document the invariant and catch regressions. This hook already appears testable in isolation.

5. flushEventBuffer useCallback dependency array comment (use-agui-stream.ts:115–126)

The empty [] dep array is correct because every value accessed inside (rafIdRef, mountedRef, eventBufferRef, processEventRef) is a ref. A short inline comment — // all accessed values are refs; stable by definition — would help the next reader understand why ESLint's exhaustive-deps is satisfied here.

---

Positive Highlights

• rAF batching is the right tool here. React 18 automatic batching helps within event handlers but not across separate SSE onmessage callbacks. The buffer-then-rAF pattern correctly collapses N concurrent token events into a single synchronous pass that React 18 can batch into one render. The implementation is clean and the cleanup on both disconnect() and unmount is handled correctly.

• processEventRef avoids stale closure correctly. Keeping a ref to the latest processEvent callback and accessing it inside the rAF callback prevents the class of bug where the rAF fires after a reconnect with a stale closure over the previous processEvent.

• Connection check semantics improved. Replacing !eventSourceRef.current with stateSnapshotRef.current.status !== 'connected' is more semantically correct — the old check would pass even if the EventSource object existed but was in CONNECTING or CLOSED state.

• Effect dependency reduction is well-justified. Narrowing [session?.spec?.initialPrompt, session?.status?.phase, aguiState.messages.length, aguiState.status] → [session?.spec?.initialPrompt] is correct: the effect only updates a useRef value and has no side effects that require awareness of phase or message count. The commit message reasoning is clear.

• Callback dependency chain is stable end-to-end. flushEventBuffer (stable) → scheduleFlush (stable) → connect (stable). This means connect won't be recreated on every state change, which is a secondary jank fix.

---

Recommendations

1. Add an inline cross-reference to the useSession hook's polling configuration in both comment blocks that replaced the polling intervals — this makes the invariant explicit and greppable.
2. Consider a test for flushEventBuffer behavior: fire 20 events synchronously, assert processEvent called 20 times but setState batched into 1 render.
3. Minor: clean up the two extra blank lines before merge.
4. Minor: document the one-render stale window in stateSnapshotRef sync with a comment.

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.