Add connection monitoring and auto-reconnect to data-objectstack adapter#364
Add connection monitoring and auto-reconnect to data-objectstack adapter#364
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…o data-objectstack Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds connection state monitoring, automatic reconnection with exponential backoff, and batch operation progress tracking to the @object-ui/data-objectstack adapter. It also updates the console application's documented spec compliance from v0.8.2 to v0.9.0.
Changes:
- Implements a 5-state connection lifecycle (disconnected → connecting → connected/reconnecting/error) with event-based subscriptions
- Adds auto-reconnect functionality with exponential backoff (default: 3 attempts, starting at 1s delay)
- Provides real-time progress events for bulk operations (create/update/delete)
- Updates console to reflect ObjectStack Spec v0.9.0 support (dashboard and page navigation were already implemented)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-objectstack/src/index.ts | Adds connection state management, auto-reconnect logic, and batch progress tracking to the adapter |
| packages/data-objectstack/src/connection.test.ts | New test file with basic tests for connection monitoring and event subscription |
| packages/data-objectstack/README.md | Documents new connection monitoring, auto-reconnect, and batch progress features |
| apps/console/src/tests/SpecCompliance.test.tsx | Updates spec version reference from v0.8.2 to v0.9.0 |
| apps/console/README.md | Updates documented spec compliance and marks dashboard/page navigation as implemented |
| this.reconnectAttempts = 0; | ||
| this.setConnectionState('connected'); |
There was a problem hiding this comment.
The reconnect counter is not reset properly when manual connect() calls succeed after previous failures. If a user explicitly calls connect() after failed auto-reconnect attempts, the reconnectAttempts counter is reset to 0 at line 127. However, if subsequent connection failures occur, the counter starts from 0 again, which may not reflect the true number of consecutive failures if the connection is unstable.
This may be intentional behavior (resetting on successful connection), but it's not documented. Consider clarifying the expected behavior: should the counter track consecutive failures across manual reconnects, or should it reset on any successful connection?
| completed = ids.length; | ||
| failed = total - completed; | ||
| emitProgress(); |
There was a problem hiding this comment.
The batch progress tracking for the 'delete' operation has a logic error. At line 377, failed is calculated as total - completed, but this doesn't account for items that were missing IDs. Items missing IDs are already counted as failures at line 368, but then at line 377, the calculation would incorrectly mark additional items as failed.
If some items have IDs and some don't, ids.length would be less than total, and failed = total - ids.length would double-count the failures. The correct approach would be to set failed = total - ids.length before the error throw at line 368, and after successful deletion, set failed = 0.
| completed = updated.length; | ||
| failed = total - completed; | ||
| emitProgress(); | ||
| return updated; | ||
| } catch { | ||
| // If updateMany is not supported, fall back to individual updates | ||
| // Silently fallback without logging |
There was a problem hiding this comment.
The batch progress tracking for 'update' operations with updateMany has the same issue as create operations - it assumes all-or-nothing behavior. At lines 391-393, the code sets completed = updated.length and failed = total - completed, but if updateMany can partially succeed, this won't accurately reflect the actual success/failure breakdown.
Additionally, if updateMany throws an exception after partial success, the progress won't reflect the partial completion before the error.
| completed = updated.length; | |
| failed = total - completed; | |
| emitProgress(); | |
| return updated; | |
| } catch { | |
| // If updateMany is not supported, fall back to individual updates | |
| // Silently fallback without logging | |
| // Treat returned items as successfully updated records | |
| completed = updated.length; | |
| failed = total - completed; | |
| emitProgress(); | |
| return updated; | |
| } catch (error: unknown) { | |
| // If updateMany reports a bulk error with partial success, reflect that in progress | |
| const bulkError = error as { | |
| successCount?: number; | |
| failureCount?: number; | |
| }; | |
| if ( | |
| typeof bulkError?.successCount === 'number' || | |
| typeof bulkError?.failureCount === 'number' | |
| ) { | |
| const successCount = | |
| typeof bulkError.successCount === 'number' | |
| ? bulkError.successCount | |
| : undefined; | |
| const failureCount = | |
| typeof bulkError.failureCount === 'number' | |
| ? bulkError.failureCount | |
| : (typeof successCount === 'number' ? total - successCount : undefined); | |
| if (typeof successCount === 'number') { | |
| completed = successCount; | |
| } | |
| if (typeof failureCount === 'number') { | |
| failed = failureCount; | |
| } | |
| emitProgress(); | |
| // Re-throw so callers can inspect the bulk error details | |
| throw error; | |
| } | |
| // If updateMany is not supported or failed without partial counts, | |
| // fall back to individual updates. Silently fallback without logging. |
| describe('Connection State Monitoring', () => { | ||
| let adapter: ObjectStackAdapter; | ||
|
|
||
| beforeEach(() => { | ||
| adapter = new ObjectStackAdapter({ | ||
| baseUrl: 'http://localhost:3000', | ||
| autoReconnect: false, // Disable auto-reconnect for testing | ||
| }); | ||
| }); | ||
|
|
||
| it('should initialize with disconnected state', () => { | ||
| expect(adapter.getConnectionState()).toBe('disconnected'); | ||
| expect(adapter.isConnected()).toBe(false); | ||
| }); | ||
|
|
||
| it('should allow subscribing to connection state changes', () => { | ||
| const listener = vi.fn(); | ||
| const unsubscribe = adapter.onConnectionStateChange(listener); | ||
|
|
||
| expect(typeof unsubscribe).toBe('function'); | ||
| expect(listener).not.toHaveBeenCalled(); | ||
|
|
||
| // Cleanup | ||
| unsubscribe(); | ||
| }); | ||
|
|
||
| it('should allow subscribing to batch progress events', () => { | ||
| const listener = vi.fn(); | ||
| const unsubscribe = adapter.onBatchProgress(listener); | ||
|
|
||
| expect(typeof unsubscribe).toBe('function'); | ||
| expect(listener).not.toHaveBeenCalled(); | ||
|
|
||
| // Cleanup | ||
| unsubscribe(); | ||
| }); | ||
|
|
||
| it('should unsubscribe connection state listener', () => { | ||
| const listener = vi.fn(); | ||
| const unsubscribe = adapter.onConnectionStateChange(listener); | ||
|
|
||
| // Unsubscribe | ||
| unsubscribe(); | ||
|
|
||
| // Listener should not be called after unsubscribe | ||
| // (We can't easily test this without triggering a connection state change) | ||
| }); | ||
|
|
||
| it('should unsubscribe batch progress listener', () => { | ||
| const listener = vi.fn(); | ||
| const unsubscribe = adapter.onBatchProgress(listener); | ||
|
|
||
| // Unsubscribe | ||
| unsubscribe(); | ||
|
|
||
| // Listener should not be called after unsubscribe | ||
| }); | ||
|
|
||
| it('should support auto-reconnect configuration', () => { | ||
| const adapterWithReconnect = new ObjectStackAdapter({ | ||
| baseUrl: 'http://localhost:3000', | ||
| autoReconnect: true, | ||
| maxReconnectAttempts: 5, | ||
| reconnectDelay: 2000, | ||
| }); | ||
|
|
||
| expect(adapterWithReconnect.getConnectionState()).toBe('disconnected'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Batch Progress Events', () => { | ||
| let adapter: ObjectStackAdapter; | ||
|
|
||
| beforeEach(() => { | ||
| adapter = new ObjectStackAdapter({ | ||
| baseUrl: 'http://localhost:3000', | ||
| }); | ||
| }); | ||
|
|
||
| it('should allow subscribing to batch progress', () => { | ||
| const listener = vi.fn(); | ||
| const unsubscribe = adapter.onBatchProgress(listener); | ||
|
|
||
| expect(typeof unsubscribe).toBe('function'); | ||
|
|
||
| // Cleanup | ||
| unsubscribe(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test coverage is insufficient for the new connection state and batch progress features. The tests only verify that:
- Listeners can be subscribed/unsubscribed
- Initial state is 'disconnected'
- Configuration is accepted
The tests do not verify:
- That connection state actually changes through the lifecycle (disconnected → connecting → connected)
- That listeners are actually called when state changes occur
- That auto-reconnect attempts are made on connection failure
- That exponential backoff delays are applied correctly
- That batch progress events are emitted during bulk operations
- That the reconnect counter is reset on successful connection
- Edge cases like maximum reconnect attempts being reached
Consider adding integration tests that mock the ObjectStackClient to test the full connection lifecycle and verify that events are emitted correctly.
| * Check if the adapter is currently connected | ||
| */ | ||
| isConnected(): boolean { | ||
| return this.connected && this.connectionState === 'connected'; |
There was a problem hiding this comment.
The isConnected() method checks both this.connected and this.connectionState === 'connected', which could lead to inconsistency. If these two state variables become out of sync, the method behavior may be unpredictable.
For example, if an error occurs after setting this.connected = true but before emitting the 'connected' state event, or if the connection state is set to 'reconnecting' while this.connected is still true, the two conditions could contradict each other.
Consider:
- Using only
connectionStateas the source of truth and deriving connection status from it - Or, ensuring that
this.connectedis always updated atomically withthis.connectionState - Adding assertions or validation to ensure these two states remain consistent
| return this.connected && this.connectionState === 'connected'; | |
| return this.connectionState === 'connected'; |
| if (this.autoReconnect && this.reconnectAttempts < this.maxReconnectAttempts) { | ||
| await this.attemptReconnect(); | ||
| } else { | ||
| throw connectionError; | ||
| } |
There was a problem hiding this comment.
The auto-reconnect logic contains a potential infinite recursion issue. When attemptReconnect() calls connect(), if connect() fails and autoReconnect is enabled with remaining attempts, it will call attemptReconnect() again. However, the reconnectAttempts counter is incremented at the start of attemptReconnect(), so if the maximum is reached during a reconnect attempt, connect() will throw an error without checking the counter again.
The issue is that if connect() throws after exhausting retries, the error propagates up through the recursion chain. However, there's no clear termination path documented, and the error thrown from line 143 may not be what callers expect during a reconnection attempt.
Consider restructuring to use a while loop instead of recursion to make the termination conditions more explicit and avoid potential stack overflow with many retry attempts.
| if (!this.connected) { | ||
| this.setConnectionState('connecting'); | ||
|
|
||
| try { | ||
| await this.client.connect(); | ||
| this.connected = true; | ||
| this.reconnectAttempts = 0; | ||
| this.setConnectionState('connected'); | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Failed to connect to ObjectStack server'; | ||
| throw new ConnectionError( | ||
| const connectionError = new ConnectionError( | ||
| errorMessage, | ||
| undefined, | ||
| { originalError: error } | ||
| ); | ||
|
|
||
| this.setConnectionState('error', connectionError); | ||
|
|
||
| // Attempt auto-reconnect if enabled | ||
| if (this.autoReconnect && this.reconnectAttempts < this.maxReconnectAttempts) { | ||
| await this.attemptReconnect(); | ||
| } else { | ||
| throw connectionError; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a potential race condition when connect() is called multiple times concurrently. The check at line 121 if (!this.connected) is not atomic with the connection attempt. If two async calls to connect() happen simultaneously, both could pass the check and attempt to connect, resulting in:
- Duplicate connection attempts to the underlying client
- Conflicting connection state updates
- Multiple state change events being fired
Consider adding a connection lock or checking if a connection is already in progress (e.g., checking for connectionState === 'connecting').
| this.reconnectAttempts++; | ||
| this.setConnectionState('reconnecting'); | ||
|
|
||
| // Exponential backoff: delay * 2^(attempts-1) | ||
| const delay = this.reconnectDelay * Math.pow(2, this.reconnectAttempts - 1); | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, delay)); | ||
|
|
||
| this.connected = false; | ||
| await this.connect(); |
There was a problem hiding this comment.
The attemptReconnect() method uses recursion to retry connections, which could lead to a deep call stack if maxReconnectAttempts is set to a high value. While the default is 3, users can configure this to be higher (e.g., 10 or more), which could cause stack overflow issues in JavaScript environments with limited stack depth.
Consider refactoring to use an iterative approach (while loop) instead of recursion.
| this.reconnectAttempts++; | |
| this.setConnectionState('reconnecting'); | |
| // Exponential backoff: delay * 2^(attempts-1) | |
| const delay = this.reconnectDelay * Math.pow(2, this.reconnectAttempts - 1); | |
| await new Promise(resolve => setTimeout(resolve, delay)); | |
| this.connected = false; | |
| await this.connect(); | |
| let lastError: unknown = undefined; | |
| while (this.autoReconnect && this.reconnectAttempts < this.maxReconnectAttempts) { | |
| this.reconnectAttempts++; | |
| this.setConnectionState('reconnecting'); | |
| // Exponential backoff: delay * 2^(attempts-1) | |
| const delay = this.reconnectDelay * Math.pow(2, this.reconnectAttempts - 1); | |
| await new Promise(resolve => setTimeout(resolve, delay)); | |
| this.connected = false; | |
| try { | |
| await this.client.connect(); | |
| this.connected = true; | |
| this.reconnectAttempts = 0; | |
| this.setConnectionState('connected'); | |
| return; | |
| } catch (error: unknown) { | |
| lastError = error; | |
| // Loop will continue if we still have attempts left | |
| } | |
| } | |
| const errorMessage = | |
| lastError instanceof Error ? lastError.message : 'Failed to reconnect to ObjectStack server'; | |
| const connectionError = new ConnectionError( | |
| errorMessage, | |
| undefined, | |
| { originalError: lastError } | |
| ); | |
| this.setConnectionState('error', connectionError); | |
| throw connectionError; |
| completed = created.length; | ||
| failed = total - completed; | ||
| emitProgress(); | ||
| return created; |
There was a problem hiding this comment.
The batch progress tracking for the 'create' operation assumes all items succeed or none do. At line 353, completed is set to created.length, and at line 354, failed is calculated as total - completed. However, if the underlying createMany partially succeeds (creates some records but fails others), this logic won't accurately reflect which items failed.
The current implementation doesn't handle partial failures in batch create operations. If createMany can partially succeed, the progress tracking will be inaccurate. Consider checking if the ObjectStack client's createMany method provides information about partial failures, or document that progress tracking assumes all-or-nothing behavior for create operations.
| private connectionStateListeners: ConnectionStateListener[] = []; | ||
| private batchProgressListeners: BatchProgressListener[] = []; |
There was a problem hiding this comment.
The event listener arrays (connectionStateListeners and batchProgressListeners) could potentially lead to memory leaks if listeners are not properly unsubscribed. While the methods return unsubscribe functions, there's no built-in mechanism to warn users or automatically clean up listeners.
Consider:
- Adding a warning if the listener count exceeds a reasonable threshold (e.g., 10-20 listeners), which might indicate a leak
- Documenting the importance of calling the unsubscribe function in component unmount/cleanup
- Adding a method to remove all listeners (e.g.,
removeAllListeners()) for cleanup scenarios
This is especially important for React components that might re-render frequently and re-subscribe without proper cleanup.
Implements connection state monitoring, automatic reconnection, and batch operation progress tracking for
@object-ui/data-objectstack. Updates console to ObjectStack Spec v0.9.0.Changes
@object-ui/data-objectstack
Connection State Monitoring
disconnected→connecting→connected|reconnecting|erroronConnectionStateChange(listener)getConnectionState(),isConnected()Auto-Reconnect
autoReconnect,maxReconnectAttempts,reconnectDelayBatch Progress Events
onBatchProgress(listener)@object-ui/console
Usage
API Changes
All changes are backward compatible. New features are opt-in via event listeners and configuration parameters.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.