fix: disconnect EventSource when navigating away from session detail #512
Conversation
When navigating from session detail page to sessions list, the EventSource connection wasn't being cleaned up. This caused the connection to remain open and block the browser's connection pool (6 concurrent connections per domain), leading to hung network requests on subsequent page loads. The fix adds a cleanup function in the useEffect that calls aguiStream.disconnect() when the component unmounts, properly closing the EventSource connection. Fixes frontend hanging on navigation between session detail and session list.
Claude Code ReviewSummaryThis PR fixes a critical browser connection pool exhaustion issue by properly cleaning up EventSource (SSE) connections when unmounting the session detail page. The fix is minimal, focused, and correct - a textbook example of an MVP fix. Verdict: ✅ APPROVED with minor recommendations Issues by Severity🚫 Blocker IssuesNone - This PR is ready to merge. 🔴 Critical IssuesNone - The implementation follows all critical patterns correctly. 🟡 Major IssuesNone - The fix is sound and well-implemented. 🔵 Minor Issues1. Dependency Array Completeness (Minor Correctness)Location: The dependency array now includes
Current code: }, [projectName, sessionName, aguiStream]);Analysis: Looking at the hook implementation ( Recommendation: // Option 1: Extract disconnect to avoid including entire object
const aguiDisconnect = aguiStream.disconnect;
useEffect(() => {
// ... connect logic
return () => {
console.log('[Session Detail] Unmounting, disconnecting AG-UI stream');
aguiDisconnect();
hasConnectedRef.current = false;
};
}, [projectName, sessionName, aguiDisconnect]);
// Option 2: Keep as-is and add comment explaining why aguiStream is included
}, [projectName, sessionName, aguiStream]); // aguiStream needed for disconnect in cleanupImpact: Low - Current code works correctly, but could be slightly optimized. 2. Console Logging in Production Code (Code Quality)Location: console.log('[Session Detail] Unmounting, disconnecting AG-UI stream');Issue: Console logs remain in production builds and clutter browser console. Recommendation: Either remove or use conditional logging: if (process.env.NODE_ENV === 'development') {
console.log('[Session Detail] Unmounting, disconnecting AG-UI stream');
}Impact: Very Low - Debug logging is helpful but should be conditional. 3. Missing Test Coverage (Testing)Observation: No test file exists for this component ( Recommendation: Add a test to verify cleanup behavior: // __tests__/session-detail-cleanup.test.tsx
import { render, cleanup } from '@testing-library/react'
import { describe, it, expect, vi } from 'vitest'
describe('Session Detail Cleanup', () => {
it('should disconnect aguiStream on unmount', () => {
const mockDisconnect = vi.fn()
const mockAguiStream = {
disconnect: mockDisconnect,
connect: vi.fn(),
// ... other methods
}
const { unmount } = render(<SessionDetail aguiStream={mockAguiStream} />)
expect(mockDisconnect).not.toHaveBeenCalled()
unmount()
expect(mockDisconnect).toHaveBeenCalledTimes(1)
})
})Impact: Low - Test coverage would prevent regression, but the fix is straightforward. Positive Highlights✅ Excellent Problem DiagnosisThe PR description clearly identifies:
This demonstrates strong debugging skills and clear communication. ✅ Minimal, Focused FixThe fix follows the MVP principle perfectly:
✅ Proper React Patternsreturn () => {
aguiStream.disconnect();
hasConnectedRef.current = false;
};This is exactly how React cleanup functions should work:
✅ Architecture AlignmentThe fix aligns with the hook's design (
The hook was designed for this cleanup pattern - this PR uses it correctly. ✅ Clear Impact StatementThe PR description explicitly states:
This transparency is valuable for reviewers and future maintainers. RecommendationsPriority 1: Merge as-is ✅This fix is correct, minimal, and urgently needed. The minor issues below can be addressed in follow-up PRs if desired. Priority 2: Consider Follow-up Improvements
Priority 3: DocumentationConsider adding a comment explaining why this cleanup is critical: // CRITICAL: Disconnect when navigating away to prevent hung connections
// EventSource holds one of the browser's 6 concurrent HTTP/2 connections per domain.
// If not closed, it blocks the connection pool and all subsequent requests hang.
// Ref: https://developer.mozilla.org/en-US/docs/Web/API/EventSource
return () => {
console.log('[Session Detail] Unmounting, disconnecting AG-UI stream');
aguiStream.disconnect();
hasConnectedRef.current = false;
};Compliance ChecklistReviewing against Frontend Development Standards from
Security ReviewNo security concerns:
Final VerdictAPPROVE ✅ This is a production-critical fix that solves a real user-facing bug with minimal, focused changes. The implementation is correct, follows React patterns, and aligns with the existing architecture. Recommended merge strategy: Squash and merge with commit message: Great work identifying and fixing this subtle browser connection pool issue! 🎉 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
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. |
1 participant
Problem
When navigating from a session detail page to the sessions list page, the frontend would hang with all network requests stuck in "pending" state. This only occurred in OpenShift deployments, not locally.
The root cause: the EventSource (SSE) connection wasn't being cleaned up when unmounting the session detail component. This left the connection open and blocked the browser's connection pool (6 concurrent connections per domain).
Solution
Added a cleanup function in the useEffect hook that calls
aguiStream.disconnect()when the component unmounts. This properly closes the EventSource connection.Changes
Testing
Impact