[ECO-5361] Make DETACHING channels become DETACHED when connection becomes SUSPENDED
#2033
Conversation
Implements the changes of 34e53e5 from [1]. [1] ably/specification#318
WalkthroughThe updates correct a method name typo from Changes
Sequence Diagram(s)sequenceDiagram
participant ConnectionManager
participant Channels
participant Channel
ConnectionManager->>Channels: propagateConnectionInterruption(connectionState, reason)
alt For each Channel in relevant states
Channels->>Channel: Handle connection interruption (switch-case on states)
alt Connection state is FAILED
Channel->>Channel: Transition to FAILED (propagate reason)
else Connection state is CLOSED
Channel->>Channel: Transition to DETACHED (propagate reason)
else Connection state is SUSPENDED
alt Channel state is ATTACHING or ATTACHED
Channel->>Channel: Transition to SUSPENDED (propagate reason)
else Channel state is DETACHING
Channel->>Channel: Transition to DETACHED (do not propagate reason)
end
end
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ 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 (
|
lawrence-forooghian
changed the title
DETACHING channels become DETACHED when connection becomes SUSPENDEDDETACHING channels become DETACHED when connection becomes SUSPENDED
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/common/lib/client/baserealtime.ts (1)
181-205: Add a defensivedefaultbranch to future-proof the switchAt the moment every known connection state is handled explicitly, but if the spec evolves a new state could slip through silently.
Logging an error (or throwing in dev builds) makes debugging far easier.case 'suspended': … break; + default: + this.logger.logAction( + Logger.LOG_ERROR, + 'Channels.propagateConnectionInterruption()', + `Unhandled connection state: ${connectionState}`, + ); + break;test/realtime/channel.test.js (3)
1586-1638: Deduplicate setup code across the three new detaching-state testsEach of the RTL3g/RTL3h tests repeats identical scaffolding (create realtime, override
transport.onProtocolMessage, initiatedetach, etc.). Extracting that into a helper such aswithDetachingChannel(...)will:
- reduce ~120 lines of duplication,
- make individual tests easier to read,
- ensure future fixes only need to be applied once.
No behavioural change required, purely a maintainability gain.
1641-1680: Use a scenario-specific channel name for the CLOSED testRe-using
'detached_channel_when_connection_enters_failed'in the CLOSED test is confusing when reading logs or debugging failures.- const channel = realtime.channels.get('detached_channel_when_connection_enters_failed'); + const channel = realtime.channels.get('detached_channel_when_connection_enters_closed');
1682-1723: Rename channel in SUSPENDED test for claritySimilarly, give the SUSPENDED test its own descriptive channel name:
- const channel = realtime.channels.get('detached_channel_when_connection_enters_failed'); + const channel = realtime.channels.get('detached_channel_when_connection_enters_suspended');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/common/lib/client/baserealtime.ts(1 hunks)src/common/lib/transport/connectionmanager.ts(1 hunks)test/realtime/auth.test.js(1 hunks)test/realtime/channel.test.js(5 hunks)test/realtime/resume.test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/realtime/channel.test.js (3)
test/realtime/resume.test.js (3)
helper(10-10)helper(616-616)helper(681-681)test/realtime/auth.test.js (4)
helper(32-32)helper(513-513)helper(1217-1217)helper(1257-1257)test/common/modules/objects_helper.js (1)
createPM(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
🔇 Additional comments (3)
test/realtime/auth.test.js (1)
771-772: Typo fix in test description
Corrects "propogated" to "propagated" to align terminology with the source code change. This is a harmless, consistency-only update.test/realtime/resume.test.js (1)
451-451: Typo fix in assertion message
Updates the test expectation message from "propogated" to "propagated" to match the corrected method name. No functional change.src/common/lib/transport/connectionmanager.ts (1)
1306-1307: Invoke correct interruption propagation method
Fixes the typo in the method call from the oldpropogateConnectionInterruptionto the correctly namedpropagateConnectionInterruption, ensuring that channels are notified of connection interruptions (including SUSPENDED) as intended.
Implements the changes of
34e53e5from ably/specification#318.Summary by CodeRabbit
Bug Fixes
Tests