feat(api-graphql): add WebSocket connection health monitoring#14563
feat(api-graphql): add WebSocket connection health monitoring#14563anivar wants to merge 2 commits intoaws-amplify:mainfrom
Conversation
bobbor
left a comment
There was a problem hiding this comment.
Thanks for opening the PR.
Please take a look at the comments
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
packages/api-graphql/src/Providers/AWSWebSocketProvider/index.ts
Outdated
Show resolved
Hide resolved
d1cf448 to
9c832cd
Compare
|
Hi @bobbor - I've addressed all the review feedback and made additional improvements to code quality. Review feedback addressed:
Additional refactoring:
Test coverage:
No breaking changes. Ready for re-review. |
f801f49 to
d96ad88
Compare
🦋 Changeset detectedLatest commit: f8c906a The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Add getConnectionHealth() and isConnected() to AWSWebSocketProvider for real-time connection diagnostics. getConnectionHealth() reports whether the connection is healthy based on connection state and keep-alive staleness (65s threshold). isConnected() checks raw WebSocket readyState. Removed broken reconnect(), over-engineered persistent storage, and unused WebSocketControl interface from original implementation.
d96ad88 to
253b44c
Compare
|
Hi @bobbor — thanks for the thorough review. I've refactored significantly based on your feedback. Most points were addressed by simplifying the scope: Your review points → resolution:
What remains (139 lines, down from 571):
|
Description of changes
Add read-only WebSocket connection health diagnostics to
AWSWebSocketProvider:getConnectionHealth()— returnsWebSocketHealthStatewith:isHealthy:truewhen connection state isConnectedAND last keep-alive was received within 65 secondsconnectionState: currentConnectionStateenum valuelastKeepAliveTime: timestamp of most recent keep-alivetimeSinceLastKeepAlive: milliseconds since last keep-aliveisConnected()— checks raw WebSocketreadyState === OPENThese methods let consumers detect stale connections and build health indicators without modifying any internal state.
Removed from original implementation:
reconnect()method (calledclose()which destroys subscription state and reconnection monitor, making recovery impossible)require()side effect for AsyncStorage, localStorage I/O on every keep-alive, useless after app restart)WebSocketControlinterface (never implemented)getPersistentConnectionHealth()(depended on removed persistence)Issue #, if available
N/A
Description of how you validated changes
AWSAppSyncRealTimeProvidertests still passChecklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.