feat: add SSE reconnection with exponential backoff and connection he… #281
Conversation
Summary of ChangesHello @jmanhype, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's real-time communication infrastructure by introducing a comprehensive Server-Sent Events (SSE) solution. It provides robust connection management with automatic reconnection strategies, a centralized system for tracking the health of all real-time streams, and a user-friendly UI component to visualize this status. This ensures a more resilient and transparent real-time experience for users, allowing for better handling of network fluctuations and providing clear feedback on data stream availability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust SSE client implementation with exponential backoff and jitter for reconnections, which is a great improvement for real-time features. It also adds a global connection status tracking system using Zustand and a new ConnectionHealthIndicator UI component. The code is well-structured with clear separation of concerns into hooks, context, and UI components. I've provided a few suggestions to improve maintainability and fix an issue with a hook implementation. Overall, this is a solid contribution.
| export function useRegisterStream( | ||
| id: string, | ||
| name: string, | ||
| isConnected: boolean, | ||
| error: string | null | ||
| ) { | ||
| const updateStream = useGlobalStreamStatus((s) => s.updateStream); | ||
| const removeStream = useGlobalStreamStatus((s) => s.removeStream); | ||
|
|
||
| // Update on mount and when status changes | ||
| updateStream(id, name, isConnected, error); | ||
|
|
||
| // Return cleanup function | ||
| return () => removeStream(id); | ||
| } |
There was a problem hiding this comment.
The useRegisterStream function is named as a hook (starts with use) but doesn't follow the Rules of Hooks. It performs side effects (calling updateStream) directly in its body, which runs during the render phase. This can lead to unexpected behavior and performance issues.
This change refactors it into a proper hook that uses useEffect to manage its lifecycle. This will correctly handle registering, updating, and un-registering the stream.
Note: You will also need to import useEffect from react at the top of the file.
| export function useRegisterStream( | |
| id: string, | |
| name: string, | |
| isConnected: boolean, | |
| error: string | null | |
| ) { | |
| const updateStream = useGlobalStreamStatus((s) => s.updateStream); | |
| const removeStream = useGlobalStreamStatus((s) => s.removeStream); | |
| // Update on mount and when status changes | |
| updateStream(id, name, isConnected, error); | |
| // Return cleanup function | |
| return () => removeStream(id); | |
| } | |
| export function useRegisterStream( | |
| id: string, | |
| name: string, | |
| isConnected: boolean, | |
| error: string | null | |
| ) { | |
| const updateStream = useGlobalStreamStatus((s) => s.updateStream); | |
| const removeStream = useGlobalStreamStatus((s) => s.removeStream); | |
| useEffect(() => { | |
| updateStream(id, name, isConnected, error); | |
| // On unmount, or if dependencies change, this cleanup will run. | |
| return () => removeStream(id); | |
| }, [id, name, isConnected, error, updateStream, removeStream]); | |
| } |
| state === 'connected' && 'text-green-600 dark:text-green-400', | ||
| state === 'connecting' && 'text-yellow-600 dark:text-yellow-400', | ||
| state === 'reconnecting' && 'text-yellow-600 dark:text-yellow-400', | ||
| state === 'disconnected' && 'text-red-600 dark:text-red-400' |
There was a problem hiding this comment.
To improve maintainability and centralize state-specific styles, consider moving these text color classes into the stateConfigs object. This will colocate all display logic for each connection state and reduce repetition.
You could add a textColor property to the StateConfig interface and the stateConfigs object, then use it here like config.textColor.
| const streamName = endpoint.includes('tasks/stream') | ||
| ? 'Tasks' | ||
| : endpoint.includes('diff/ws') | ||
| ? 'Diff' | ||
| : endpoint.includes('logs/ws') | ||
| ? 'Logs' | ||
| : endpoint.includes('execution-processes') | ||
| ? 'Processes' | ||
| : endpoint.includes('drafts/stream') | ||
| ? 'Drafts' | ||
| : 'Stream'; |
There was a problem hiding this comment.
This series of ternary operators to determine the streamName works, but it can become difficult to read and maintain as more stream types are added. Consider refactoring this into a map or a helper function to improve readability and scalability.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
frontend/src/hooks/useSSEStream.ts
Outdated
| }, [ | ||
| endpoint, | ||
| enabled, | ||
| reconnectAttempts, | ||
| lastEventId, |
There was a problem hiding this comment.
The SSE effect lists lastEventId as a dependency, but the effect’s cleanup closes the current EventSource. Because onmessage updates lastEventId for every event, any stream that emits an Last-Event-ID header will trigger the effect on each message, closing and recreating the connection repeatedly instead of keeping a stable stream. That defeats SSE continuity and can drop or duplicate events under normal traffic. Consider removing lastEventId from the dependency array (it’s already available when reconnecting via other state changes) so the connection isn’t torn down on every event.
Useful? React with 👍 / 👎.
…alth indicator - Add useSSEStream hook with EventSource API supporting: - Exponential backoff reconnection (configurable initial/max delay) - Jitter to prevent thundering herd - last-event-ID tracking for replay capability - Connection state management (connected/connecting/reconnecting/disconnected) - Add ConnectionHealthIndicator UI component: - Visual indicator with color-coded states - Pulsing animation during connection attempts - Tooltip with detailed status info - Optional label display - Click-to-reconnect support - Add useGlobalStreamStatus Zustand store: - Tracks multiple stream connections - Provides aggregate connection health - Integrates with existing WebSocket streams - Add SSE context provider for shared connection state - Add useRealtimeConnection convenience hook - Integrate health indicator into navbar - Register existing WebSocket streams with global tracker Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
Based on PR automagik-dev#281 review feedback from Gemini Code Assist: 1. **Fix hook implementation issue** (useGlobalStreamStatus): - Refactor useRegisterStream to properly use useEffect - Follows React Hooks rules for side effects and cleanup - Prevents Rules of Hooks violations 2. **Fix SSE dependency issue** (useSSEStream): - Remove lastEventId from dependency array - Prevents unnecessary reconnections when only event ID changes - Improves connection stability 3. **Improve maintainability** (ConnectionHealthIndicator): - Move text color classes into stateConfigs object - Eliminates repetitive conditional styling - Better code organization and extensibility 4. **Improve readability** (useJsonPatchWsStream): - Extract stream name logic into getStreamNameFromEndpoint helper - Replace complex nested ternaries with maintainable map lookup - Easier to add new stream types All changes maintain functionality while improving code quality, maintainability, and following React best practices. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
jmanhype
force-pushed
the
claude/sse-reconnection-backoff-01JuLVY1t4C9zkuvFEw6dkea
branch
from
2412ae4 to
6996dc4
Compare
|
@codex review this pr |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
frontend/src/hooks/useSSEStream.ts
Outdated
| endpoint, | ||
| enabled, | ||
| reconnectAttempts, | ||
| lastEventId, |
There was a problem hiding this comment.
The main effect depends on lastEventId while lastEventId is updated for every incoming event, so each message forces the effect to cleanup the current EventSource and recreate it. On SSE endpoints that send an ID on each event (the common case), the connection will be torn down and re-established for every message, causing constant reconnects and likely dropping events rather than maintaining a stable stream. Removing lastEventId from the dependency list would keep the connection alive while still using the latest ID on actual reconnects.
Useful? React with 👍 / 👎.
| setError(null); | ||
| setReconnectAttempts(0); | ||
| updateConnectionState('connected'); |
There was a problem hiding this comment.
When a reconnect succeeds, onopen resets reconnectAttempts to 0, but because reconnectAttempts is a dependency of the main effect (355–359), that state change triggers the effect cleanup, closing the freshly reconnected EventSource and creating another one. After any network hiccup the hook always tears down the first successful reconnection, potentially discarding the first post-outage events and doubling reconnect churn. The attempt counter should be reset without retriggering the effect that owns the live connection.
Useful? React with 👍 / 👎.
|
@codex review this pr |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Build URL with last-event-id for replay capability | ||
| const url = new URL(endpoint, window.location.origin); | ||
| if (lastEventId) { | ||
| url.searchParams.set('lastEventId', lastEventId); | ||
| } |
There was a problem hiding this comment.
The SSE hook always appends the previously seen lastEventId to whatever endpoint is passed in, but the state is never cleared when callers switch to a different stream or toggle the connection off and back on. If a component reuses the hook with a new endpoint after viewing another feed, the new connection will send an unrelated lastEventId, which many SSE servers reject or treat as an empty replay, so the new stream can come up missing data until the page is reloaded.
Useful? React with 👍 / 👎.
Resolves three critical SSE reconnection issues identified by Codex: 1. Reconnect loop on every message: Removed lastEventId from effect dependencies to prevent effect rerun on each message received. 2. Connection closes on successful reconnect: Use ref for tracking reconnect attempts internally. When connection succeeds and resets attempts, it no longer triggers effect cleanup that closes connection. 3. lastEventId not reset on endpoint change: Added dedicated effect to reset lastEventId when endpoint changes, preventing stale event ID from being sent to new endpoints. Technical changes: - Added reconnectAttemptsRef for internal tracking - Added reconnectTrigger state for controlled effect reruns - Removed lastEventId and reconnectAttempts from effect dependencies - Added endpoint change effect to reset lastEventId 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
3 participants
…alth indicator
Add useSSEStream hook with EventSource API supporting:
Add ConnectionHealthIndicator UI component:
Add useGlobalStreamStatus Zustand store:
Add SSE context provider for shared connection state
Add useRealtimeConnection convenience hook
Integrate health indicator into navbar
Register existing WebSocket streams with global tracker