Fix Android stutter: async updateWidget using compute#403
Fix Android stutter: async updateWidget using compute#403Aroojsarfraz90 wants to merge 1 commit intoABausG:mainfrom
Conversation
|
To view this pull requests documentation preview, visit the following URL: docs.page/abausg/home_widget~403 Documentation is deployed and generated using docs.page. |
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/home_widget/lib/src/home_widget.dart`:
- Line 196: Add an explicit null check for the byteData returned from
image.toByteData() before using the forced unwrap; in the method where you call
await file.writeAsBytes(byteData!.buffer.asUint8List()), verify byteData != null
and handle the null case (throw a clear exception or return an error) so you
don't dereference a null value—use the existing byteData, file, and image
variables (or the containing function) to locate and implement this check and
provide a descriptive error message if encoding fails.
- Around line 27-52: The use of compute in updateWidget adds isolate overhead
for a trivial map copy; change updateWidget to build the args synchronously
(either inline or by calling _prepareUpdateArgs directly) and then call
_channel.invokeMethod('updateWidget', args) without compute, and remove
_prepareUpdateArgs if it becomes unused; additionally, check any
native/platform-channel code if stutter persists.
- Around line 198-201: The platform channel call to save widget data is
fire-and-forget; update the caller (likely renderFlutterWidget) to await the
result of _channel.invokeMethod<bool>('saveWidgetData', { 'id': key, 'data':
path }) so persistence completes before returning: make the function async (or
return a Future), await the invokeMethod, and handle failures (check the
returned bool or catch PlatformException) to log or propagate errors instead of
allowing silent failures.
| /// --- Updated to async + compute to avoid stutter on Android --- | ||
| static Future<bool?> updateWidget({ | ||
| String? name, | ||
| String? androidName, | ||
| String? iOSName, | ||
| String? qualifiedAndroidName, | ||
| }) { | ||
| return _channel.invokeMethod('updateWidget', { | ||
| }) async { | ||
| final Map<String, dynamic> args = await compute(_prepareUpdateArgs, { | ||
| 'name': name, | ||
| 'android': androidName, | ||
| 'ios': iOSName, | ||
| 'qualifiedAndroidName': qualifiedAndroidName, | ||
| }); | ||
|
|
||
| return _channel.invokeMethod('updateWidget', args); | ||
| } | ||
|
|
||
| static Map<String, dynamic> _prepareUpdateArgs(Map<String, dynamic> input) { | ||
| // Heavy computation can go here in future if needed | ||
| return { | ||
| 'name': input['name'], | ||
| 'android': input['android'], | ||
| 'ios': input['ios'], | ||
| 'qualifiedAndroidName': input['qualifiedAndroidName'], | ||
| }; | ||
| } |
There was a problem hiding this comment.
compute overhead likely exceeds benefit for trivial map copying.
compute spawns a new isolate with significant overhead (isolate creation, serialization/deserialization of arguments and results). The _prepareUpdateArgs function only copies a map with no actual computation, so this adds latency rather than reducing it.
If Android stutter originates from the platform channel call itself, this change won't help since _channel.invokeMethod still runs on the main isolate. Consider:
- Investigating the actual source of stutter (likely native side or platform channel)
- Removing the
computewrapper if there's no CPU-intensive work to offload - If heavy computation is needed in the future, add
computeat that time
Suggested simplification (revert to direct call)
- /// --- Updated to async + compute to avoid stutter on Android ---
static Future<bool?> updateWidget({
String? name,
String? androidName,
String? iOSName,
String? qualifiedAndroidName,
- }) async {
- final Map<String, dynamic> args = await compute(_prepareUpdateArgs, {
+ }) {
+ return _channel.invokeMethod<bool>('updateWidget', {
'name': name,
'android': androidName,
'ios': iOSName,
'qualifiedAndroidName': qualifiedAndroidName,
});
-
- return _channel.invokeMethod('updateWidget', args);
}
-
- static Map<String, dynamic> _prepareUpdateArgs(Map<String, dynamic> input) {
- // Heavy computation can go here in future if needed
- return {
- 'name': input['name'],
- 'android': input['android'],
- 'ios': input['ios'],
- 'qualifiedAndroidName': input['qualifiedAndroidName'],
- };
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// --- Updated to async + compute to avoid stutter on Android --- | |
| static Future<bool?> updateWidget({ | |
| String? name, | |
| String? androidName, | |
| String? iOSName, | |
| String? qualifiedAndroidName, | |
| }) { | |
| return _channel.invokeMethod('updateWidget', { | |
| }) async { | |
| final Map<String, dynamic> args = await compute(_prepareUpdateArgs, { | |
| 'name': name, | |
| 'android': androidName, | |
| 'ios': iOSName, | |
| 'qualifiedAndroidName': qualifiedAndroidName, | |
| }); | |
| return _channel.invokeMethod('updateWidget', args); | |
| } | |
| static Map<String, dynamic> _prepareUpdateArgs(Map<String, dynamic> input) { | |
| // Heavy computation can go here in future if needed | |
| return { | |
| 'name': input['name'], | |
| 'android': input['android'], | |
| 'ios': input['ios'], | |
| 'qualifiedAndroidName': input['qualifiedAndroidName'], | |
| }; | |
| } | |
| static Future<bool?> updateWidget({ | |
| String? name, | |
| String? androidName, | |
| String? iOSName, | |
| String? qualifiedAndroidName, | |
| }) { | |
| return _channel.invokeMethod<bool>('updateWidget', { | |
| 'name': name, | |
| 'android': androidName, | |
| 'ios': iOSName, | |
| 'qualifiedAndroidName': qualifiedAndroidName, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/home_widget/lib/src/home_widget.dart` around lines 27 - 52, The use
of compute in updateWidget adds isolate overhead for a trivial map copy; change
updateWidget to build the args synchronously (either inline or by calling
_prepareUpdateArgs directly) and then call _channel.invokeMethod('updateWidget',
args) without compute, and remove _prepareUpdateArgs if it becomes unused;
additionally, check any native/platform-channel code if stutter persists.
| if (!await file.exists()) { | ||
| await file.create(recursive: true); | ||
| } | ||
| await file.writeAsBytes(byteData!.buffer.asUint8List()); |
There was a problem hiding this comment.
Add explicit null check for byteData before force unwrap.
image.toByteData() can return null if encoding fails. The force unwrap will throw, but adding an explicit check provides a clearer error message.
Suggested fix
+ if (byteData == null) {
+ throw Exception('Failed to encode image to PNG');
+ }
- await file.writeAsBytes(byteData!.buffer.asUint8List());
+ await file.writeAsBytes(byteData.buffer.asUint8List());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await file.writeAsBytes(byteData!.buffer.asUint8List()); | |
| if (byteData == null) { | |
| throw Exception('Failed to encode image to PNG'); | |
| } | |
| await file.writeAsBytes(byteData.buffer.asUint8List()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/home_widget/lib/src/home_widget.dart` at line 196, Add an explicit
null check for the byteData returned from image.toByteData() before using the
forced unwrap; in the method where you call await
file.writeAsBytes(byteData!.buffer.asUint8List()), verify byteData != null and
handle the null case (throw a clear exception or return an error) so you don't
dereference a null value—use the existing byteData, file, and image variables
(or the containing function) to locate and implement this check and provide a
descriptive error message if encoding fails.
| _channel.invokeMethod<bool>('saveWidgetData', { | ||
| 'id': key, | ||
| 'data': path, | ||
| }); |
There was a problem hiding this comment.
Platform channel call is not awaited, causing potential race condition.
The invokeMethod call is fire-and-forget. Callers may expect that widget data is persisted when renderFlutterWidget completes, but without await, the save operation may still be in progress (or fail silently) when the function returns.
Suggested fix
- _channel.invokeMethod<bool>('saveWidgetData', {
+ await _channel.invokeMethod<bool>('saveWidgetData', {
'id': key,
'data': path,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _channel.invokeMethod<bool>('saveWidgetData', { | |
| 'id': key, | |
| 'data': path, | |
| }); | |
| await _channel.invokeMethod<bool>('saveWidgetData', { | |
| 'id': key, | |
| 'data': path, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/home_widget/lib/src/home_widget.dart` around lines 198 - 201, The
platform channel call to save widget data is fire-and-forget; update the caller
(likely renderFlutterWidget) to await the result of
_channel.invokeMethod<bool>('saveWidgetData', { 'id': key, 'data': path }) so
persistence completes before returning: make the function async (or return a
Future), await the invokeMethod, and handle failures (check the returned bool or
catch PlatformException) to log or propagate errors instead of allowing silent
failures.
ABausG
left a comment
ABausG
left a comment
There was a problem hiding this comment.
Please provide more Info on why you would want this change. Also I don't see any optimization in moving the map creation into a dedicated compute.
Plus please do not delete documentation
2 participants
Description
Replace this text.
Checklist
exampleor documentation.Breaking Change?
Related Issues