feat(fdc): Handle disconnects and reconnects#18157
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements WebSocket-based streaming for Firebase Data Connect, introducing WebSocketTransport and a _RoutingTransport to manage communication. It includes protocol definitions for bi-directional streaming, updates QueryRef to handle server-pushed data, and deprecates the gRPC transport. Review feedback highlights a critical type cast issue that could cause runtime errors, identifies performance bottlenecks due to redundant asynchronous calls in loops, and notes style guide violations concerning Future handling and function length.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/firebase_data_connect/firebase_data_connect/lib/src/core/ref.dart (383)
The cast e as Error is problematic because many runtime issues in Dart (including DataConnectError and other network-related issues) are Exceptions, not Errors. This cast will cause a TypeError at runtime when an exception is caught. Since publishErrorToStream accepts an Object, the cast is unnecessary.
publishErrorToStream(e);
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart (305-318)
The App Check token is being fetched asynchronously inside a loop. This is inefficient as it triggers multiple redundant token retrieval calls. Fetch the token once before the loop starts.
final appCheckToken = appCheck == null ? null : await appCheck!.getToken();
for (final sub in _pendingSubscriptions.values) {
final reqId = _activeSubscriptions[sub.operationId];
if (reqId == null) continue;
final headers = _buildHeaders(authToken, appCheckToken);
final request = StreamRequest(
authToken: authToken,
appCheckToken: appCheckToken,
requestId: reqId,
requestKind: RequestKind.subscribe,
subscribe: ExecuteRequest(sub.queryName, sub.variables),
headers: headers,
);
_channel!.sink.add(jsonEncode(request.toJson()));
}
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart (322-345)
The App Check token is being fetched asynchronously inside a nested loop. This is inefficient. Fetch the token once before the loop starts.
final appCheckToken = appCheck == null ? null : await appCheck!.getToken();
for (final entry in _unaryListeners.entries) {
final reqId = entry.key;
final kept = <_PendingUnary>[];
for (final p in entry.value) {
if (p.isMutation) {
p.completer.completeError(DataConnectError(
DataConnectErrorCode.other,
'Network reconnected; mutations cannot be safely retried.'));
} else {
kept.add(p);
final headers = _buildHeaders(authToken, appCheckToken);
final request = StreamRequest(
authToken: authToken,
appCheckToken: appCheckToken,
requestId: reqId,
requestKind: RequestKind.execute,
execute: ExecuteRequest(p.operationName, p.variables),
headers: headers,
);
_channel!.sink.add(jsonEncode(request.toJson()));
}
}
if (kept.isNotEmpty) {
unariesToReplay[reqId] = kept;
}
}packages/firebase_data_connect/firebase_data_connect/lib/src/core/ref.dart (337-349)
Adhering to the repository style guide (line 24), async/await and try-catch should be used for Future handling instead of .then() and .catchError().
Future.microtask(() async {
if (dataConnect.cacheManager != null) {
try {
await _executeFromCache(QueryFetchPolicy.cacheOnly);
} catch (err) {
log("Error fetching from cache during subscribe $err");
// Ignore cache misses here, server stream will provide latest data
}
}
// Initiate Web Socket stream
_streamFromServer();
});
References
- Use async/await for Futures. Catch all errors with try-catch. (link)
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart (277-351)
The function _performReconnect is approximately 75 lines long, which exceeds the 20-line limit recommended by the repository style guide (line 22). Consider refactoring the logic into smaller helper methods.
References
- Functions <20 lines. Avoid verbosity. (link)
73815b7 to
0c3db68
Compare
352baea to
56f8f5b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a robust reconnection mechanism for the WebSocket transport, including exponential backoff and state management for pending operations. It also refactors connection logic to handle authentication changes and idle states more effectively. Feedback focuses on adhering to the repository style guide by avoiding the ! operator and keeping functions under 20 lines, as well as replacing magic numbers with named constants and ensuring consistent use of jsonDecode.
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart
Outdated
Show resolved
Hide resolved
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart
Show resolved
Hide resolved
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart
Show resolved
Hide resolved
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart
Outdated
Show resolved
Hide resolved
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart
Outdated
Show resolved
Hide resolved
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart
Show resolved
Hide resolved
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart
Show resolved
Hide resolved
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart
Outdated
Show resolved
Hide resolved
packages/firebase_data_connect/firebase_data_connect/lib/src/network/websocket_transport.dart
Outdated
Show resolved
Hide resolved
32658fa to
b3a8c75
Compare
|
Merging. The test errors are not related to data connect. Resolved the App Check error FDC was running into initially. |
Handle network or auth disconnects and reconnect logic.