-
Notifications
You must be signed in to change notification settings - Fork 213
Protocol v13 #984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Protocol v13 #984
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/core/signal_client.dart (1)
461-464: 🛠️ Refactor suggestion | 🟠 MajorExtension method creates empty LeaveRequest without v13 fields.
This extension method creates an empty
LeaveRequest()without thereasonoractionfields set in the class method above. If this method is intentionally kept for some specific use case, it should be documented. Otherwise, consider removing it or aligning it with the updated class method.Suggested fix to align with v13 semantics
`@internal` -void sendLeave() => _sendRequest(lk_rtc.SignalRequest( - leave: lk_rtc.LeaveRequest(), - )); +void sendLeave() => _sendRequest(lk_rtc.SignalRequest( + leave: lk_rtc.LeaveRequest( + reason: lk_models.DisconnectReason.CLIENT_INITIATED, + action: lk_rtc.LeaveRequest_Action.DISCONNECT, + ), + ));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changes/protocol-v13lib/src/core/engine.dartlib/src/core/signal_client.dartlib/src/extensions.dartlib/src/options.dartlib/src/types/other.dart
⏰ 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). (3)
- GitHub Check: Build for Flutter Windows
- GitHub Check: Build for Flutter Android
- GitHub Check: Build for Flutter macOS
🔇 Additional comments (6)
.changes/protocol-v13 (1)
1-1: LGTM!The changelog entry accurately describes the protocol v13 feature addition with LeaveRequest action support.
lib/src/options.dart (1)
64-69: LGTM!The default protocol version is correctly updated to v13. The engine's LeaveEvent handling maintains backward compatibility with v12 servers by still checking
canReconnectwhen the action isDISCONNECT(the default/unset value).lib/src/types/other.dart (1)
26-39: LGTM!The
ProtocolVersion.v13enum member is added following the established pattern. The inline comment accurately documents the key protocol changes (regions in leave request, action obsoleting canReconnect).lib/src/extensions.dart (1)
56-71: LGTM!The string mapping for
ProtocolVersion.v13is correctly added following the established pattern.lib/src/core/engine.dart (1)
1353-1380: Well-structured protocol v13 LeaveEvent handling with proper backward compatibility.The logic correctly prioritizes the new
actionfield while maintaining backward compatibility with v12 servers:
- RESUME (line 1362): Soft reconnect without full restart
- RECONNECT or canReconnect=true (line 1366): Full reconnect - handles both v13
RECONNECTaction and v12canReconnectflag- DISCONNECT (line 1370): Default path for clean disconnect, including v12 servers with
canReconnect=falseThe comment at lines 1359-1361 clearly documents the backward compatibility approach.
lib/src/core/signal_client.dart (1)
200-207: No action needed — the twosendLeave()methods do not cause functional issues.The instance method at lines 200-207 and the extension method at lines 461-464 have different signatures and contexts:
- Instance method (public API):
Future<void> sendLeave() asyncwith v13 fields (reason: CLIENT_INITIATED,action: DISCONNECT)- Extension method (internal API):
@internal void sendLeave()creating an emptyLeaveRequest(), synchronous returnDart's method resolution order ensures the instance method takes precedence when called on a
SignalClientinstance. The extension method's@internalannotation restricts its visibility to internal use only. The code at engine.dart:1387 correctly calls the public instance method viaawait signalClient.sendLeave().
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary by CodeRabbit
New Features
Updates