Websock retry logic #1578
Websock retry logic #1578
Conversation
WalkthroughA new custom React hook, Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useWebSocketConnection
participant react-use-websocket
Component->>useWebSocketConnection: Call useWebSocketConnection(endpoint, options)
useWebSocketConnection->>react-use-websocket: Establish WebSocket connection with options
react-use-websocket-->>useWebSocketConnection: Provide sendJsonMessage, lastMessage, setSocketUrl
useWebSocketConnection-->>Component: Expose sendJsonMessage, lastMessage, setSocketUrl, disconnect
react-use-websocket-->>useWebSocketConnection: On error event
useWebSocketConnection-->>Component: Call handleError or log error
Component->>useWebSocketConnection: Call setSocketUrl or disconnect as needed
useWebSocketConnection->>react-use-websocket: Update or close socket
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/customHooks/useWebsocketConnection.ts (1)
40-44: Consider adding dependency to cleanup effect.The cleanup effect has an empty dependency array but calls
disconnect()which referencesgetWebSocket(). While this works for unmount cleanup, consider if any dependencies should be included for completeness.useEffect(() => { return () => { disconnect(); }; - }, []); + }, [getWebSocket]);src/components/Connections/CreateConnectionForm.tsx (1)
15-15: Remove unused import.The
setimport fromreact-hook-formis not used anywhere in the component and should be removed.-import { Controller, set, useForm } from 'react-hook-form'; +import { Controller, useForm } from 'react-hook-form';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/Connections/CreateConnectionForm.tsx(2 hunks)src/components/Destinations/DestinationForm.tsx(2 hunks)src/components/Destinations/__tests__/CreateDestinationForm.test.tsx(1 hunks)src/components/Destinations/__tests__/EditDestinationForm.test.tsx(1 hunks)src/components/Sources/SourceForm.tsx(2 hunks)src/components/Sources/__tests__/CreateSourceForm.test.tsx(1 hunks)src/components/Sources/__tests__/EditSourceForm.test.tsx(1 hunks)src/customHooks/useWebsocketConnection.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
src/components/Destinations/DestinationForm.tsx (3)
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/components/Sources/__tests__/EditSourceForm.test.tsx:427-437
Timestamp: 2025-06-17T17:12:43.766Z
Learning: In src/components/Sources/__tests__/EditSourceForm.test.tsx, the test expectation for the host value 'localhostinvalid-host' in the connection failure test should be kept as-is, even though it appears to be a concatenation of default and input values.
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1556
File: src/components/Connections/Connections.tsx:692-701
Timestamp: 2025-06-11T08:52:45.594Z
Learning: In `src/components/Connections/Connections.tsx`, the intended behavior is for `filterConnections` to match the search term against connection name and source name only; destination name should not be included in the filter criteria.
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/helpers/connectorConfig/FormField.tsx:343-359
Timestamp: 2025-06-17T03:30:42.337Z
Learning: In the FormField component (src/helpers/connectorConfig/FormField.tsx), the addItem function for array fields only initializes explicit default values for top-level subFields, but nested required fields are correctly rendered dynamically in the UI at render time. This approach works properly for complex configurations like S3 buckets, as confirmed by user testing.
src/components/Destinations/__tests__/EditDestinationForm.test.tsx (2)
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/components/Sources/__tests__/EditSourceForm.test.tsx:427-437
Timestamp: 2025-06-17T17:12:43.766Z
Learning: In src/components/Sources/__tests__/EditSourceForm.test.tsx, the test expectation for the host value 'localhostinvalid-host' in the connection failure test should be kept as-is, even though it appears to be a concatenation of default and input values.
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1556
File: src/components/Connections/Connections.tsx:692-701
Timestamp: 2025-06-11T08:52:45.594Z
Learning: In `src/components/Connections/Connections.tsx`, the intended behavior is for `filterConnections` to match the search term against connection name and source name only; destination name should not be included in the filter criteria.
src/components/Destinations/__tests__/CreateDestinationForm.test.tsx (2)
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/components/Sources/__tests__/EditSourceForm.test.tsx:427-437
Timestamp: 2025-06-17T17:12:43.766Z
Learning: In src/components/Sources/__tests__/EditSourceForm.test.tsx, the test expectation for the host value 'localhostinvalid-host' in the connection failure test should be kept as-is, even though it appears to be a concatenation of default and input values.
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1556
File: src/components/Connections/Connections.tsx:692-701
Timestamp: 2025-06-11T08:52:45.594Z
Learning: In `src/components/Connections/Connections.tsx`, the intended behavior is for `filterConnections` to match the search term against connection name and source name only; destination name should not be included in the filter criteria.
src/components/Sources/__tests__/CreateSourceForm.test.tsx (2)
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/components/Sources/__tests__/EditSourceForm.test.tsx:427-437
Timestamp: 2025-06-17T17:12:43.766Z
Learning: In src/components/Sources/__tests__/EditSourceForm.test.tsx, the test expectation for the host value 'localhostinvalid-host' in the connection failure test should be kept as-is, even though it appears to be a concatenation of default and input values.
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1556
File: src/components/Connections/Connections.tsx:692-701
Timestamp: 2025-06-11T08:52:45.594Z
Learning: In `src/components/Connections/Connections.tsx`, the intended behavior is for `filterConnections` to match the search term against connection name and source name only; destination name should not be included in the filter criteria.
src/components/Sources/SourceForm.tsx (3)
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/components/Sources/__tests__/EditSourceForm.test.tsx:427-437
Timestamp: 2025-06-17T17:12:43.766Z
Learning: In src/components/Sources/__tests__/EditSourceForm.test.tsx, the test expectation for the host value 'localhostinvalid-host' in the connection failure test should be kept as-is, even though it appears to be a concatenation of default and input values.
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1556
File: src/components/Connections/Connections.tsx:692-701
Timestamp: 2025-06-11T08:52:45.594Z
Learning: In `src/components/Connections/Connections.tsx`, the intended behavior is for `filterConnections` to match the search term against connection name and source name only; destination name should not be included in the filter criteria.
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/helpers/connectorConfig/FormField.tsx:343-359
Timestamp: 2025-06-17T03:30:42.337Z
Learning: In the FormField component (src/helpers/connectorConfig/FormField.tsx), the addItem function for array fields only initializes explicit default values for top-level subFields, but nested required fields are correctly rendered dynamically in the UI at render time. This approach works properly for complex configurations like S3 buckets, as confirmed by user testing.
src/components/Connections/CreateConnectionForm.tsx (3)
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/components/Sources/__tests__/EditSourceForm.test.tsx:427-437
Timestamp: 2025-06-17T17:12:43.766Z
Learning: In src/components/Sources/__tests__/EditSourceForm.test.tsx, the test expectation for the host value 'localhostinvalid-host' in the connection failure test should be kept as-is, even though it appears to be a concatenation of default and input values.
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1556
File: src/components/Connections/Connections.tsx:692-701
Timestamp: 2025-06-11T08:52:45.594Z
Learning: In `src/components/Connections/Connections.tsx`, the intended behavior is for `filterConnections` to match the search term against connection name and source name only; destination name should not be included in the filter criteria.
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/helpers/connectorConfig/FormField.tsx:343-359
Timestamp: 2025-06-17T03:30:42.337Z
Learning: In the FormField component (src/helpers/connectorConfig/FormField.tsx), the addItem function for array fields only initializes explicit default values for top-level subFields, but nested required fields are correctly rendered dynamically in the UI at render time. This approach works properly for complex configurations like S3 buckets, as confirmed by user testing.
src/components/Sources/__tests__/EditSourceForm.test.tsx (1)
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/components/Sources/__tests__/EditSourceForm.test.tsx:427-437
Timestamp: 2025-06-17T17:12:43.766Z
Learning: In src/components/Sources/__tests__/EditSourceForm.test.tsx, the test expectation for the host value 'localhostinvalid-host' in the connection failure test should be kept as-is, even though it appears to be a concatenation of default and input values.
🧬 Code Graph Analysis (3)
src/components/Destinations/DestinationForm.tsx (1)
src/customHooks/useWebsocketConnection.ts (1)
useWebSocketConnection(9-47)
src/components/Sources/SourceForm.tsx (1)
src/customHooks/useWebsocketConnection.ts (1)
useWebSocketConnection(9-47)
src/components/Connections/CreateConnectionForm.tsx (1)
src/customHooks/useWebsocketConnection.ts (1)
useWebSocketConnection(9-47)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: checks
🔇 Additional comments (12)
src/customHooks/useWebsocketConnection.ts (2)
1-47: Well-implemented WebSocket abstraction with proper retry logic.The custom hook effectively centralizes WebSocket management with exponential backoff retry mechanism. The implementation looks solid overall.
20-20: Verify the exponential backoff calculation.The exponential backoff formula looks correct, but let's confirm the sequence:
- Attempt 0:
Math.min(1000 * 2^0, 30000) = 1000ms- Attempt 1:
Math.min(1000 * 2^1, 30000) = 2000ms- Attempt 2:
Math.min(1000 * 2^2, 30000) = 4000ms- And so on, capped at 30 seconds
#!/bin/bash # Verify the exponential backoff calculation produces expected intervals python3 -c " import math for attempt in range(6): interval = min(1000 * math.pow(2, attempt), 30000) print(f'Attempt {attempt}: {interval}ms ({interval/1000}s)') "src/components/Sources/__tests__/CreateSourceForm.test.tsx (1)
6-6: Import change correctly implements the WebSocket refactor.The import has been properly updated to use the custom hook while maintaining interface compatibility through aliasing. The test logic remains unchanged, which demonstrates good abstraction.
src/components/Sources/__tests__/EditSourceForm.test.tsx (1)
7-7: Consistent refactor implementation across test files.The import change follows the same pattern as other test files in this refactor, properly switching to the custom hook while maintaining interface compatibility.
src/components/Destinations/__tests__/CreateDestinationForm.test.tsx (1)
7-7: Consistent application of WebSocket refactor to destination tests.The import change maintains the same pattern used across all test files in this refactor, ensuring consistent adoption of the custom WebSocket hook.
src/components/Destinations/__tests__/EditDestinationForm.test.tsx (1)
7-7: Completes the comprehensive WebSocket refactor across test files.This final import change ensures all test files consistently use the custom WebSocket hook, completing the refactor objective of centralizing WebSocket connection management.
src/components/Destinations/DestinationForm.tsx (2)
13-13: LGTM: Import updated to use custom WebSocket hook.The import change to the custom
useWebSocketConnectionhook is part of the centralization effort to standardize WebSocket usage across components.
53-53: LGTM: Hook usage properly simplified.The change removes the explicit error handling options and relies on the custom hook's default error handling (console.error). The returned interface (
sendJsonMessage,lastMessage) remains consistent with the original implementation.src/components/Sources/SourceForm.tsx (2)
12-12: LGTM: Import updated to use centralized WebSocket hook.The import change aligns with the refactor to use the custom
useWebSocketConnectionhook across all components.
84-84: LGTM: Enhanced hook usage with dynamic URL management.The change properly utilizes the custom hook's
setSocketUrlfunctionality, allowing dynamic WebSocket URL updates. This is a good improvement over static URL management.src/components/Connections/CreateConnectionForm.tsx (2)
23-23: LGTM: Import updated to use custom WebSocket hook.The import change maintains consistency with the WebSocket refactoring across other components.
217-222: LGTM: Hook usage properly maintains error handling.The change correctly uses the custom hook's
handleErroroption to preserve the existing error handling behavior while benefiting from the centralized retry logic and connection management.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Sources/__tests__/CreateSourceForm.test.tsx (1)
182-190: Remove debug console logs and improve test clarity.The console.log statements appear to be leftover debugging code and should be removed for cleaner test output.
- console.log('generateWebsocketUrl calls:', generateWebsocketUrlMock.mock.calls); - console.log('setSocketUrl calls:', setSocketUrlMock.mock.calls); - expect(generateWebsocketUrlMock).toHaveBeenCalledWith( 'airbyte/source/check_connection', expect.objectContaining({ user: { email: 'a' } }) ); - expect(setSocketUrlMock).toHaveBeenCalledWith(socketUrlEnpoint);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Sources/__tests__/CreateSourceForm.test.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/components/Sources/__tests__/CreateSourceForm.test.tsx (1)
Learnt from: himanshudube97
PR: DalgoT4D/webapp#1550
File: src/components/Sources/__tests__/EditSourceForm.test.tsx:427-437
Timestamp: 2025-06-17T17:12:43.766Z
Learning: In src/components/Sources/__tests__/EditSourceForm.test.tsx, the test expectation for the host value 'localhostinvalid-host' in the connection failure test should be kept as-is, even though it appears to be a concatenation of default and input values.
🧬 Code Graph Analysis (1)
src/components/Sources/__tests__/CreateSourceForm.test.tsx (1)
src/customHooks/useWebsocketConnection.ts (1)
useWebSocketConnection(9-47)
🪛 GitHub Check: checks
src/components/Sources/__tests__/CreateSourceForm.test.tsx
[failure] 103-103:
Require statement not part of import statement
🪛 ESLint
src/components/Sources/__tests__/CreateSourceForm.test.tsx
[error] 103-103: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🪛 GitHub Actions: Dalgo CI
src/components/Sources/__tests__/CreateSourceForm.test.tsx
[error] 103-103: ESLint: Require statement not part of import statement (@typescript-eslint/no-var-requires)
🔇 Additional comments (6)
src/components/Sources/__tests__/CreateSourceForm.test.tsx (6)
6-6: LGTM: Import updated for custom hook.The import has been correctly updated to use the new custom hook as part of the WebSocket refactor.
15-15: LGTM: Added helper function import.The import of
generateWebsocketUrlis necessary for the new WebSocket architecture.
27-30: LGTM: Mock setup updated correctly.The Jest mock has been properly updated to mock the named export
useWebSocketConnectioninstead of the external package.
84-87: LGTM: Helper function mock added.The mock for
generateWebsocketUrlis properly set up to support the new testing approach.
106-111: LGTM: Mock return value matches new hook interface.The mock return value correctly includes all the methods exposed by the
useWebSocketConnectionhook:sendJsonMessage,lastMessage,disconnect, andsetSocketUrl.
177-191: No changes required; test aligns with component behavior.
The CreateSourceForm test correctly mocksgenerateWebsocketUrlanduseWebSocketConnectionand asserts thatsetSocketUrlis called with the value returned bygenerateWebsocketUrl, matching theSourceForm’s useEffect logic.
|
working on the test cases |
1 participant
This PR refactors the socket logic to use a common function/hook. And this hook now also does retry with exponential backoff of some sort.
Summary by CodeRabbit
New Features
Refactor
Tests