Fix/network url and body handling - #13
Conversation
…ce tags in event logs
…ee, JSON, and platform-specific code support
…ifecycle-based invalidation for JSON highlights
…ze large JSON rendering
…gles in storage and console views
…capture stability, and standardized byte formatting
…tio for higher resolution captures
…on state logic into the widget
…ending for over 10 minutes
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (40)
📝 WalkthroughWalkthroughThis PR spans two platforms: the React Native SDK reworks fetch/XHR header and body interception, while the Flutter app adds backend service detection, stale network-request cleanup, an LRU-based JSON highlight cache with deferred/async rendering widgets, redesigned screenshot naming across detail panels, a new SDK-tips widget, and extensive localization additions. ChangesReact Native SDK
Estimated code review effort: 3 (Moderate) | ~25 minutes Flutter DevConnect App
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several performance optimizations and UI enhancements to the DevConnect tool, including lazy tab loading, asynchronous JSON parsing, an LRU cache for syntax highlighting, and a redesigned storage detail panel. It also adds backend service detection for network requests and a 'Clear stale' utility. The code review identified several critical issues: a potential header corruption bug in the React Native client when handling array-based headers, a double-unit formatting bug in the binary body size display, a memory leak from an unclosed Riverpod subscription in the storage viewer, and a Flutter anti-pattern involving state mutation inside a build method. Additionally, the reviewer recommended enhancing service action detection to support parsed JSON maps and replacing hardcoded English strings with existing localization templates.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const headers = init.headers; | ||
| if (headers && typeof headers === 'object' && !(headers instanceof Headers)) { |
There was a problem hiding this comment.
The check typeof headers === 'object' evaluates to true for arrays. If headers is passed as an array of key-value pairs (e.g., string[][]), it will incorrectly enter this block and be wrapped in a Proxy designed for a flat record. This will corrupt the headers when they are spread or read.
Adding !Array.isArray(headers) ensures that array-based headers are correctly handled by the fallback block, which is already supported by readFinalHeaders.
| const headers = init.headers; | |
| if (headers && typeof headers === 'object' && !(headers instanceof Headers)) { | |
| const headers = init.headers; | |
| if (headers && typeof headers === 'object' && !Array.isArray(headers) && !(headers instanceof Headers)) { |
| S.of(context).binaryBodySize( | ||
| AppConstants.formatBytes(sizeBytes), | ||
| sizeBytes, | ||
| ), | ||
| style: TextStyle( |
There was a problem hiding this comment.
The translation template for binaryBodySize expects a raw number/string representing the size in KB (e.g., "{kb} KB ({bytes} bytes)"). Passing AppConstants.formatBytes(sizeBytes) (which already formats the value with a unit like '4.2 KB' or '512 B') will result in a double unit display (e.g., '4.2 KB KB' or '512 B KB').
Passing (sizeBytes / 1024).toStringAsFixed(1) fixes this formatting bug.
| S.of(context).binaryBodySize( | |
| AppConstants.formatBytes(sizeBytes), | |
| sizeBytes, | |
| ), | |
| style: TextStyle( | |
| Text( | |
| S.of(context).binaryBodySize( | |
| (sizeBytes / 1024).toStringAsFixed(1), | |
| sizeBytes, | |
| ), |
| ref.listenManual<String?>( | ||
| selectedStorageIdProvider, | ||
| (_, _) { | ||
| _generation++; | ||
| setState(() {}); | ||
| }, | ||
| fireImmediately: false, | ||
| ); |
There was a problem hiding this comment.
Using ref.listenManual in initState without storing and closing the returned ProviderSubscription in dispose causes a memory leak. If the provider emits a change after this widget is unmounted, the callback will still run, call setState, and throw an exception.
Instead of manually managing the subscription lifecycle, use the standard ref.listen inside the build method. Riverpod will automatically manage and dispose of the subscription when the widget is unmounted.
| ref.listenManual<String?>( | |
| selectedStorageIdProvider, | |
| (_, _) { | |
| _generation++; | |
| setState(() {}); | |
| }, | |
| fireImmediately: false, | |
| ); | |
| // Selection changes are handled via ref.listen in build |
| final devices = ref.watch(connectedDevicesProvider); | ||
| final ids = devices.map((d) => d.deviceId).toSet(); | ||
| if (_knownDeviceIds != null && ids.any((id) => !_knownDeviceIds!.contains(id))) { | ||
| HighlightCacheLifecycleObserver.instance.clearCache(); | ||
| } | ||
| _knownDeviceIds = ids; |
There was a problem hiding this comment.
Mutating state variables (like _knownDeviceIds) and triggering side effects (like clearing a cache) directly inside the build method is a Flutter anti-pattern. It can lead to inconsistent state or unnecessary rebuilds.
Instead, use ref.listen to reactively listen to changes in connectedDevicesProvider and trigger the cache invalidation side-effect. This also allows you to completely remove the _knownDeviceIds state variable from the class.
| final devices = ref.watch(connectedDevicesProvider); | |
| final ids = devices.map((d) => d.deviceId).toSet(); | |
| if (_knownDeviceIds != null && ids.any((id) => !_knownDeviceIds!.contains(id))) { | |
| HighlightCacheLifecycleObserver.instance.clearCache(); | |
| } | |
| _knownDeviceIds = ids; | |
| ref.listen(connectedDevicesProvider, (previous, next) { | |
| if (previous != null) { | |
| final prevIds = previous.map((d) => d.deviceId).toSet(); | |
| final nextIds = next.map((d) => d.deviceId).toSet(); | |
| if (nextIds.any((id) => !prevIds.contains(id))) { | |
| HighlightCacheLifecycleObserver.instance.clearCache(); | |
| } | |
| } | |
| }); |
| if (body is String) { | ||
| final sigMatch = RegExp(r'"Action"\s*:\s*"([^"]+)"').firstMatch(body); | ||
| if (sigMatch != null) return sigMatch.group(1); | ||
| } |
There was a problem hiding this comment.
The body parameter can be a parsed JSON payload (such as a Map or List) rather than a raw String. If body is a Map, the check body is String will fail, and the action name will not be extracted.
Adding a check for body is Map allows direct lookup of the Action or action keys, improving the reliability of service action detection.
if (body is Map) {
final action = body['Action'] ?? body['action'];
if (action is String) return action;
}
if (body is String) {
final sigMatch = RegExp(r'"Action"\s*:\s*"([^"]+)"').firstMatch(body);
if (sigMatch != null) return sigMatch.group(1);
}| content: Text( | ||
| 'Cleared $removed stale request${removed == 1 ? '' : 's'} ' | ||
| '(pending > 10min)', | ||
| ), |
There was a problem hiding this comment.
This user-facing string is hardcoded. Since the localization key clearStaleSnackbar is already defined in the ARB files, it should be used here to support internationalization.
| content: Text( | |
| 'Cleared $removed stale request${removed == 1 ? '' : 's'} ' | |
| '(pending > 10min)', | |
| ), | |
| content: Text( | |
| S.of(context).clearStaleSnackbar(removed), | |
| ), |
| return Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 6), | ||
| child: TextComponent( | ||
| '$type payload ($bytes bytes) — binary, cannot be inspected.\nIdentify the action via the X-Amz-Target header.', |
There was a problem hiding this comment.
This screenshot helper uses hardcoded English strings. It should use the localized strings binaryBody, binaryBodySize, and binaryBodyHint to ensure consistency across all supported locales.
| return Padding( | |
| padding: const EdgeInsets.symmetric(vertical: 6), | |
| child: TextComponent( | |
| '$type payload ($bytes bytes) — binary, cannot be inspected.\nIdentify the action via the X-Amz-Target header.', | |
| final sizeLabel = (bytes / 1024).toStringAsFixed(1); | |
| return Padding( | |
| padding: const EdgeInsets.symmetric(vertical: 6), | |
| child: TextComponent( | |
| '${S.of(context).binaryBody(type)} (${S.of(context).binaryBodySize(sizeLabel, bytes)}) — binary, cannot be inspected.\\n${S.of(context).binaryBodyHint}', |
1 participant
Description
Related Issue
Type of Change
Testing
Screenshots (if applicable)
Summary by CodeRabbit
New Features
Bug Fixes