RTL5l: detach immediately if disconnected and detach() called

[SimonWoolf]

ably/specification#411

Summary by CodeRabbit

• Bug Fixes
  • Enhanced channel detachment to gracefully handle disconnected connection states. Channels now immediately transition to detached status instead of raising an error when detaching during an inactive connection.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Modified RealtimeChannel.detach() to gracefully handle disconnected connection states by immediately notifying the channel as detached instead of throwing an error. Added test case RTL5l to validate this behavior when detaching during disconnected states.

Changes

Cohort / File(s)	Summary
Core Logic src/common/lib/client/realtimechannel.ts	Removed pre-check that threw on non-connected states in detach(). Now immediately notifies channel as detached and returns when connection is not in 'connected' state. Added inline comments (RTL5j, RTL5b, RTL5l) documenting control-flow behavior.
Test Coverage test/realtime/channel.test.js	Added new test case RTL5l validating that detaching with a disconnected connection immediately transitions the channel to detached state. Uses async/await with proper cleanup and connection state simulation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hops of grace through detached states,
No errors thrown at closing gates,
When connections slip away so quick,
Detach with ease—a gentle trick!
Tests confirm the path so bright, ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title clearly and specifically describes the main change: detaching a channel immediately when the connection is disconnected, which matches the implementation in the code changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch detach-immediately

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

test/realtime/channel.test.js (1)

1655-1678: Well-structured test, but consider testing other non-connected states.

The test correctly verifies the RTL5l behavior for the 'disconnected' state using modern async/await patterns and proper cleanup. However, the implementation at lines 406-410 in realtimechannel.ts handles all non-connected states (connecting, disconnected, closing, closed, failed, suspended), not just 'disconnected'.

Consider adding test cases for other connection states to ensure comprehensive coverage, particularly:

• 'connecting' (detach while connection is establishing)
• 'closing' (detach while connection is closing)

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between eef8ed2 and 1917d9a.

📒 Files selected for processing (2)

• src/common/lib/client/realtimechannel.ts
• test/realtime/channel.test.js

🧰 Additional context used

🧬 Code graph analysis (2)

test/realtime/channel.test.js (2)

test/realtime/init.test.js (1)

• realtime (279-279)

test/realtime/connection.test.js (1)

• realtime (363-363)

src/common/lib/client/realtimechannel.ts (1)

src/common/lib/types/errorinfo.ts (1)

• ErrorInfo (35-66)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: test-browser (chromium)
• GitHub Check: test-npm-package
• GitHub Check: test-browser (webkit)
• GitHub Check: test-browser (firefox)
• GitHub Check: test-node (20.x)
• GitHub Check: test-node (16.x)
• GitHub Check: test-node (18.x)

🔇 Additional comments (1)

src/common/lib/client/realtimechannel.ts (1)

406-410: RTL5l implementation is correct; the 'detaching' state is properly handled.

The RTL5l check (lines 406-410) correctly handles non-connected states for channels not already in a terminal state. While channels already in 'detaching' bypass this check and go directly to the case at line 413, this is not an issue.

The propogateConnectionInterruption() method in baserealtime.ts explicitly includes 'detaching' in its fromChannelStates array (line 187), ensuring that when the connection transitions to closing, closed, failed, or suspended states, channels stuck in 'detaching' are properly transitioned to their corresponding channel states. This handles the edge case and prevents channels from remaining stuck in 'detaching' due to connection issues.