Open
Conversation
…checking, and encapsulation
📝 WalkthroughWalkthroughThis PR integrates native app shortcuts (Quick Actions) via the Changes
Sequence DiagramsequenceDiagram
actor User
participant OS as Android/iOS OS
participant App as App (DeepLinkService)
participant HomeCtrl as HomeController
participant UI as UI Layer<br/>(AddTaskSheet/<br/>Reports)
User->>OS: Long-press app icon<br/>select Quick Action
OS->>App: Trigger shortcut URI<br/>(add_task/reports)
alt HomeController ready (context exists)
App->>HomeCtrl: Check context status
HomeCtrl-->>App: Ready ✓
App->>UI: Execute action<br/>(show sheet or navigate)
UI-->>User: Display UI
else HomeController not ready
App->>App: Queue URI in queuedUri
Note over App: Await HomeController<br/>readiness
HomeCtrl->>App: consumePendingActions()
App->>UI: Execute queued action
UI-->>User: Display UI
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/Runner/Assets.xcassets/plus.imageset/Contents.json`:
- Around line 1-21: The imageset references 2x and 3x variants but only provides
"plus.png" for 1x, so add high-resolution assets named exactly "plus@2x.png" and
"plus@3x.png" into the plus.imageset folder and update the imageset entry if
necessary so the "filename" fields for the 2x and 3x entries point to
"plus@2x.png" and "plus@3x.png" respectively; ensure the files are properly
sized (2x and 3x of the base) and included in the Xcode asset catalog.
In `@ios/Runner/Assets.xcassets/report.imageset/Contents.json`:
- Around line 1-21: The Contents.json for the report.imageset declares 2x and 3x
scales but lacks filenames, causing iOS to upscale the 1x asset; add
high-resolution files named report@2x.png and report@3x.png into
ios/Runner/Assets.xcassets/report.imageset/ and update the Contents.json entries
for the objects with "scale": "2x" and "scale": "3x" to include "filename":
"report@2x.png" and "filename": "report@3x.png" respectively (leave the existing
"report.png" for "1x"), then verify the imageset renders correctly in Xcode.
In `@lib/app/services/deep_link_service.dart`:
- Around line 44-47: The shortcut branch for 'shortcut_add_task' should not
reuse the 'taskwarrior://addclicked' dialog flow; update the if branch (where
shortcutType == 'shortcut_add_task') to invoke a dedicated bottom-sheet action
instead of calling _handleWidgetUri(Uri.parse('taskwarrior://addclicked')).
Implement or call a method such as _showAddTaskBottomSheet() that opens the Add
Task bottom sheet with Get.bottomSheet(...) (rather than relying on
_executeAction() / Get.dialog(...) which handles the 'addclicked' host), and
ensure any existing _executeAction() handling for 'addclicked' remains unchanged
so the shortcut routes to the new bottom-sheet flow.
- Around line 112-117: The reports quick-action is being dropped when
Get.context is null in _handleWidgetUri()/_executeAction(); instead of guarding
the entire reports branch on Get.context, always schedule the navigation for the
next frame (or enqueue the action until the UI is ready) so the route is
executed after the first frame. Locate the branch that checks uri.host ==
"reports" in deep_link_service.dart and replace the Get.context conditional with
a call to WidgetsBinding.instance.addPostFrameCallback((_) {
Get.toNamed(Routes.REPORTS); }); (or push the reports action onto your existing
startup queue to be processed once the UI is available).
- Around line 18-22: The app currently calls DeepLinkService.init() which
unconditionally invokes _initQuickActions() and crashes on macOS because
quick_actions supports only Android/iOS; modify init() to import dart:io and
wrap the call to _initQuickActions() (and any quick_actions setup inside
_initQuickActions()) with a runtime platform guard using Platform.isAndroid ||
Platform.isIOS so quick actions are only initialized on mobile; ensure the guard
references the _initQuickActions() method and keep other deep-linking logic
intact for non-mobile platforms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e8b7e91-3ba4-4439-9cc5-fcb05c2572fa
⛔ Files ignored due to path filters (5)
android/app/src/main/res/drawable/plus.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable/report.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/plus.imageset/plus.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/report.imageset/report.pngis excluded by!**/*.pngpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
ios/Runner/Assets.xcassets/plus.imageset/Contents.jsonios/Runner/Assets.xcassets/report.imageset/Contents.jsonlib/app/services/deep_link_service.dartlib/main.dartpubspec.yaml
Comment on lines
+1
to
+21
| { | ||
| "images" : [ | ||
| { | ||
| "idiom" : "universal", | ||
| "filename" : "plus.png", | ||
| "scale" : "1x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "2x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "3x" | ||
| } | ||
| ], | ||
| "info" : { | ||
| "version" : 1, | ||
| "author" : "xcode" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for `@2x` and `@3x` variants of plus icon
echo "Checking for plus icon variants in iOS asset catalog..."
fd -t f "plus" ios/Runner/Assets.xcassets/plus.imageset/
echo -e "\nExpected files for optimal display quality:"
echo " - plus.png (1x)"
echo " - plus@2x.png (2x) - for retina displays"
echo " - plus@3x.png (3x) - for super retina displays"Repository: CCExtractor/taskwarrior-flutter
Length of output: 335
Add high-resolution image variants to the asset catalog.
The asset catalog defines 2x and 3x scale variants but only provides a filename for the 1x scale image. The plus@2x.png and plus@3x.png files are missing from this imageset directory. Without them, iOS will upscale the 1x image to fill higher resolutions, resulting in blurry or pixelated icons on retina displays.
Provide the following files to ensure crisp display across all iOS devices:
plus@2x.pngfor 2x scale (retina displays)plus@3x.pngfor 3x scale (super retina displays)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Runner/Assets.xcassets/plus.imageset/Contents.json` around lines 1 - 21,
The imageset references 2x and 3x variants but only provides "plus.png" for 1x,
so add high-resolution assets named exactly "plus@2x.png" and "plus@3x.png" into
the plus.imageset folder and update the imageset entry if necessary so the
"filename" fields for the 2x and 3x entries point to "plus@2x.png" and
"plus@3x.png" respectively; ensure the files are properly sized (2x and 3x of
the base) and included in the Xcode asset catalog.
Comment on lines
+1
to
+21
| { | ||
| "images": [ | ||
| { | ||
| "idiom": "universal", | ||
| "filename": "report.png", | ||
| "scale": "1x" | ||
| }, | ||
| { | ||
| "idiom": "universal", | ||
| "scale": "2x" | ||
| }, | ||
| { | ||
| "idiom": "universal", | ||
| "scale": "3x" | ||
| } | ||
| ], | ||
| "info": { | ||
| "version": 1, | ||
| "author": "xcode" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for `@2x` and `@3x` variants of report icon
echo "Checking for report icon variants in iOS asset catalog..."
fd -t f "report" ios/Runner/Assets.xcassets/report.imageset/
echo -e "\nExpected files for optimal display quality:"
echo " - report.png (1x)"
echo " - report@2x.png (2x) - for retina displays"
echo " - report@3x.png (3x) - for super retina displays"Repository: CCExtractor/taskwarrior-flutter
Length of output: 347
Add missing high-resolution image variants.
The imageset directory contains only report.png (1x scale), but the Contents.json declares support for 2x and 3x scales without providing filenames. iOS will upscale the 1x image on retina displays, resulting in blurry or pixelated rendering. Add report@2x.png and report@3x.png to the ios/Runner/Assets.xcassets/report.imageset/ directory, then update the Contents.json with the corresponding filenames.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Runner/Assets.xcassets/report.imageset/Contents.json` around lines 1 -
21, The Contents.json for the report.imageset declares 2x and 3x scales but
lacks filenames, causing iOS to upscale the 1x asset; add high-resolution files
named report@2x.png and report@3x.png into
ios/Runner/Assets.xcassets/report.imageset/ and update the Contents.json entries
for the objects with "scale": "2x" and "scale": "3x" to include "filename":
"report@2x.png" and "filename": "report@3x.png" respectively (leave the existing
"report.png" for "1x"), then verify the imageset renders correctly in Xcode.
Collaborator
|
Icons for shortcuts do not look good I can't see icons at all. Improve that ! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/app/modules/home/views/add_task_bottom_sheet_new.dart (2)
472-476:⚠️ Potential issue | 🟡 MinorInconsistent platform check—missing iOS.
Same issue as line 393: only checks
Platform.isAndroidwhile other locations check both Android and iOS.Suggested fix
- if (Platform.isAndroid) { + if (Platform.isAndroid || Platform.isIOS) { WidgetController widgetController = Get.put(WidgetController()); widgetController.fetchAllData(); widgetController.update(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/home/views/add_task_bottom_sheet_new.dart` around lines 472 - 476, The platform check currently only runs on Android (Platform.isAndroid) causing inconsistent behavior; change the conditional in the block that creates and uses WidgetController (the Get.put(WidgetController()), widgetController.fetchAllData(), widgetController.update()) to run for both Android and iOS (e.g., Platform.isAndroid || Platform.isIOS) so iOS executes the same logic as Android.
393-397:⚠️ Potential issue | 🟡 MinorInconsistent platform check—missing iOS.
This block only checks
Platform.isAndroid, but other locations (line 427) checkPlatform.isAndroid || Platform.isIOS. This inconsistency means iOS users won't get widget refresh when saving TaskC tasks.Suggested fix
- if (Platform.isAndroid) { + if (Platform.isAndroid || Platform.isIOS) { WidgetController widgetController = Get.put(WidgetController()); widgetController.fetchAllData(); widgetController.update(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/home/views/add_task_bottom_sheet_new.dart` around lines 393 - 397, The platform check currently only uses Platform.isAndroid before creating and updating WidgetController (WidgetController, fetchAllData, update), which omits iOS; change the conditional to match the other usage (use Platform.isAndroid || Platform.isIOS) so iOS also triggers widgetController.fetchAllData() and widgetController.update(), keeping the logic consistent with the other check.
🧹 Nitpick comments (2)
lib/app/modules/home/views/tasks_builder.dart (1)
229-235: Minor formatting inconsistency.Line 229 uses
Platform.isAndroid||Platform.isIOSwithout spaces around||, while line 200 uses proper spacingPlatform.isAndroid || Platform.isIOS. This is a minor style inconsistency.Suggested fix
- if (Platform.isAndroid||Platform.isIOS) { + if (Platform.isAndroid || Platform.isIOS) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/home/views/tasks_builder.dart` around lines 229 - 235, Fix the spacing inconsistency around the logical OR: change the condition in the tasks_builder.dart block from Platform.isAndroid||Platform.isIOS to use spaces like Platform.isAndroid || Platform.isIOS so it matches the style used elsewhere; update the condition near the WidgetController instantiation (WidgetController(), widgetController.fetchAllData(), widgetController.update()) accordingly to maintain consistent formatting.lib/app/modules/detailRoute/controllers/detail_route_controller.dart (1)
97-101: Platform-specific widget refresh pattern is correct but has minor timing concern.The
fetchAllData()method isasync void(fire-and-forget) andupdate()is called synchronously after it. SincefetchAllData()internally callssendAndUpdate()without awaiting, theupdate()call here may execute before the widget data is fully refreshed. This works in practice becauseupdate()triggers a reactive rebuild, but the timing is non-deterministic.This same pattern appears in 4+ other files (
show_tasks.dart,show_tasks_replica.dart,tasks_builder.dart,add_task_bottom_sheet_new.dart). Consider extracting this into a helper method onWidgetControlleror a utility function to reduce duplication and centralize any future fixes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/detailRoute/controllers/detail_route_controller.dart` around lines 97 - 101, The current pattern calls WidgetController().fetchAllData() (async void) then update() synchronously, causing nondeterministic timing because fetchAllData() internally calls sendAndUpdate() without awaiting; fix by adding a new awaitable helper on WidgetController (e.g., Future<void> refreshAllData() or ensure fetchAllData returns Future) that awaits the internal sendAndUpdate() call and then calls update(), and replace the existing fire-and-forget calls (instances in detail_route_controller.dart, show_tasks.dart, show_tasks_replica.dart, tasks_builder.dart, add_task_bottom_sheet_new.dart) to await widgetController.refreshAllData() (or await fetchAllData()) so update runs after data is fully refreshed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/app/modules/home/views/home_page_floating_action_button.dart`:
- Around line 31-51: The bottom sheet dismissal handler uses the returned value
to decide whether to call controller.isNeededtoSyncOnStart, but a swipe-dismiss
returns null so the current check (value != "cancel") can trigger unwanted
syncs; update the contract so AddTaskBottomSheet explicitly returns a clear
success token (e.g., "saved" or true) on successful save and "cancel" on user
cancel, and change the .then callback after Get.bottomSheet(...) to only trigger
controller.isNeededtoSyncOnStart(context) when value equals that success token
(and still respect controller.isSyncNeeded.value) so swipe-dismiss/null does not
cause a sync.
In `@lib/app/services/deep_link_service.dart`:
- Around line 123-156: The then() handler on Get.bottomSheet currently treats
any non-"cancel" result (including null from swipe-dismiss) as a trigger to call
controller.isNeededtoSyncOnStart; update the flow so only an explicit successful
save triggers sync: either (A) make AddTaskBottomSheet return an explicit
"cancel" when dismissed without saving (ensure it pops with
Navigator.pop(context, "cancel") on swipe/close paths) or (B) change the
.then(...) check to require an explicit success token (e.g., value == "saved" or
value != null && value == "saved") before calling
controller.isNeededtoSyncOnStart; reference Get.bottomSheet, AddTaskBottomSheet,
controller.isSyncNeeded, isNeededtoSyncOnStart and the then((value) { ... })
callback when making the change.
---
Outside diff comments:
In `@lib/app/modules/home/views/add_task_bottom_sheet_new.dart`:
- Around line 472-476: The platform check currently only runs on Android
(Platform.isAndroid) causing inconsistent behavior; change the conditional in
the block that creates and uses WidgetController (the
Get.put(WidgetController()), widgetController.fetchAllData(),
widgetController.update()) to run for both Android and iOS (e.g.,
Platform.isAndroid || Platform.isIOS) so iOS executes the same logic as Android.
- Around line 393-397: The platform check currently only uses Platform.isAndroid
before creating and updating WidgetController (WidgetController, fetchAllData,
update), which omits iOS; change the conditional to match the other usage (use
Platform.isAndroid || Platform.isIOS) so iOS also triggers
widgetController.fetchAllData() and widgetController.update(), keeping the logic
consistent with the other check.
---
Nitpick comments:
In `@lib/app/modules/detailRoute/controllers/detail_route_controller.dart`:
- Around line 97-101: The current pattern calls
WidgetController().fetchAllData() (async void) then update() synchronously,
causing nondeterministic timing because fetchAllData() internally calls
sendAndUpdate() without awaiting; fix by adding a new awaitable helper on
WidgetController (e.g., Future<void> refreshAllData() or ensure fetchAllData
returns Future) that awaits the internal sendAndUpdate() call and then calls
update(), and replace the existing fire-and-forget calls (instances in
detail_route_controller.dart, show_tasks.dart, show_tasks_replica.dart,
tasks_builder.dart, add_task_bottom_sheet_new.dart) to await
widgetController.refreshAllData() (or await fetchAllData()) so update runs after
data is fully refreshed.
In `@lib/app/modules/home/views/tasks_builder.dart`:
- Around line 229-235: Fix the spacing inconsistency around the logical OR:
change the condition in the tasks_builder.dart block from
Platform.isAndroid||Platform.isIOS to use spaces like Platform.isAndroid ||
Platform.isIOS so it matches the style used elsewhere; update the condition near
the WidgetController instantiation (WidgetController(),
widgetController.fetchAllData(), widgetController.update()) accordingly to
maintain consistent formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d96df78-61bb-42a6-9f3e-0728115aa833
⛔ Files ignored due to path filters (8)
android/app/src/main/res/drawable/ic_shortcut_add.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable/ic_shortcut_reports.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/ic_shortcut_add.imageset/add_1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/ic_shortcut_add.imageset/add_2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/ic_shortcut_add.imageset/add_3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/ic_shortcut_reports.imageset/reports_1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/ic_shortcut_reports.imageset/reports_2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/ic_shortcut_reports.imageset/reports_3x.pngis excluded by!**/*.png
📒 Files selected for processing (9)
ios/Runner/Assets.xcassets/ic_shortcut_add.imageset/Contents.jsonios/Runner/Assets.xcassets/ic_shortcut_reports.imageset/Contents.jsonlib/app/modules/detailRoute/controllers/detail_route_controller.dartlib/app/modules/home/views/add_task_bottom_sheet_new.dartlib/app/modules/home/views/home_page_floating_action_button.dartlib/app/modules/home/views/show_tasks.dartlib/app/modules/home/views/show_tasks_replica.dartlib/app/modules/home/views/tasks_builder.dartlib/app/services/deep_link_service.dart
✅ Files skipped from review due to trivial changes (2)
- ios/Runner/Assets.xcassets/ic_shortcut_reports.imageset/Contents.json
- ios/Runner/Assets.xcassets/ic_shortcut_add.imageset/Contents.json
Comment on lines
+31
to
+51
| onPressed: () { | ||
| Get.bottomSheet( | ||
| AddTaskBottomSheet( | ||
| homeController: controller, | ||
| forTaskC: controller.taskchampion.value, | ||
| forReplica: controller.taskReplica.value, | ||
| ), | ||
| backgroundColor: tColors.dialogBackgroundColor, | ||
| isScrollControlled: true, | ||
| ignoreSafeArea: true, | ||
| shape: const RoundedRectangleBorder( | ||
| borderRadius: BorderRadius.only( | ||
| topLeft: Radius.circular(0), | ||
| topRight: Radius.circular(0), | ||
| ), | ||
| builder: (context) => AddTaskBottomSheet( | ||
| homeController: controller, | ||
| forTaskC: controller.taskchampion.value, | ||
| forReplica: controller.taskReplica.value, | ||
| ), | ||
| ).then((value) { | ||
| if (controller.isSyncNeeded.value && value != "cancel") { | ||
| controller.isNeededtoSyncOnStart(context); | ||
| } | ||
| })) | ||
|
|
||
| // .then((value) { | ||
| ), | ||
| ).then((value) { | ||
| if (controller.isSyncNeeded.value && value != "cancel") { | ||
| controller.isNeededtoSyncOnStart(context); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Get.bottomSheet implementation looks good, but sync trigger on dismissal may cause unexpected UX.
The migration to Get.bottomSheet with ignoreSafeArea: true and isScrollControlled: true properly supports keyboard insets. However, the .then() callback (lines 47-50) will trigger isNeededtoSyncOnStart() when the user swipe-dismisses the sheet without saving, since value will be null (not "cancel").
Per the context snippet, isNeededtoSyncOnStart() performs a full synchronization and displays a SnackBar, which would be unexpected behavior for users who dismiss without creating a task.
This is the same pattern used in deep_link_service.dart (lines 151-155). Consider:
- Having
AddTaskBottomSheetexplicitly return"cancel"on dismissal without save, or - Checking for a truthy success indicator (e.g.,
value == trueorvalue == "saved")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/app/modules/home/views/home_page_floating_action_button.dart` around
lines 31 - 51, The bottom sheet dismissal handler uses the returned value to
decide whether to call controller.isNeededtoSyncOnStart, but a swipe-dismiss
returns null so the current check (value != "cancel") can trigger unwanted
syncs; update the contract so AddTaskBottomSheet explicitly returns a clear
success token (e.g., "saved" or true) on successful save and "cancel" on user
cancel, and change the .then callback after Get.bottomSheet(...) to only trigger
controller.isNeededtoSyncOnStart(context) when value equals that success token
(and still respect controller.isSyncNeeded.value) so swipe-dismiss/null does not
cause a sync.
Comment on lines
+123
to
156
| } else if (uri.host == "addclicked" || uri.host == "shortcut_add") { | ||
| if (Get.context != null) { | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| Get.dialog( | ||
| Material( | ||
| child: AddTaskBottomSheet( | ||
| homeController: controller, | ||
| forTaskC: isTaskChampion, | ||
| forReplica: isReplica, | ||
| // If the bottom sheet is already open, doing nothing prevents async stacking issues! | ||
| // Get.back() animating at the exact same time as a new Get.bottomSheet() causes | ||
| // Get.isBottomSheetOpen to desync inside GetX state management on the third instance. | ||
| if (Get.isBottomSheetOpen ?? false) { | ||
| return; | ||
| } | ||
|
|
||
| TaskwarriorColorTheme tColors = | ||
| Theme.of(Get.context!).extension<TaskwarriorColorTheme>()!; | ||
|
|
||
| Get.bottomSheet( | ||
| AddTaskBottomSheet( | ||
| homeController: controller, | ||
| forTaskC: isTaskChampion, | ||
| forReplica: isReplica, | ||
| ), | ||
| backgroundColor: tColors.dialogBackgroundColor, | ||
| isScrollControlled: true, | ||
| ignoreSafeArea: true, // TRUE allows MediaQuery.viewInsets (software keyboard) to reach inner Padding | ||
| shape: const RoundedRectangleBorder( | ||
| borderRadius: BorderRadius.only( | ||
| topLeft: Radius.circular(0), | ||
| topRight: Radius.circular(0), | ||
| ), | ||
| ), | ||
| ); | ||
| ).then((value) { | ||
| if (controller.isSyncNeeded.value && value != "cancel") { | ||
| controller.isNeededtoSyncOnStart(Get.context!); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Bottom sheet stacking guard is good, but sync-on-dismiss may cause unexpected behavior.
The guard at lines 129-131 preventing stacked bottom sheets is excellent defensive coding. However, the .then() callback (lines 151-155) triggers isNeededtoSyncOnStart() when value != "cancel". If the user swipe-dismisses the sheet without saving, value will be null (not "cancel"), still triggering a sync operation. This may cause unexpected sync behavior on casual dismissal.
Consider returning "cancel" explicitly when the sheet is dismissed without saving, or checking for a truthy value indicating successful task creation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/app/services/deep_link_service.dart` around lines 123 - 156, The then()
handler on Get.bottomSheet currently treats any non-"cancel" result (including
null from swipe-dismiss) as a trigger to call controller.isNeededtoSyncOnStart;
update the flow so only an explicit successful save triggers sync: either (A)
make AddTaskBottomSheet return an explicit "cancel" when dismissed without
saving (ensure it pops with Navigator.pop(context, "cancel") on swipe/close
paths) or (B) change the .then(...) check to require an explicit success token
(e.g., value == "saved" or value != null && value == "saved") before calling
controller.isNeededtoSyncOnStart; reference Get.bottomSheet, AddTaskBottomSheet,
controller.isSyncNeeded, isNeededtoSyncOnStart and the then((value) { ... })
callback when making the change.
Contributor
Author
|
@SGI-CAPP-AT2 My bad for bundling two issues into one PR. The routing logic for the new shortcuts and the old widgets were completely tangled up in the exact same deep link service. Fixing the bottom sheet collisions naturally required rewriting the state management for the widgets too, so it just made sense to drop it all here. Bottom sheet routing and widget caching fixes are wired up exactly as discussed. Also swapped the shortcut icons to crisp Material ones per the feedback. Everything should be running smoothly now! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements native home screen App Shortcuts (long-press on the app icon) to give users immediate access to frequently used features, as requested in the linked issue.
Closes #614 #627
Changes Made:
quick_actionspackage topubspec.yaml.action_add_taskandaction_reportssafely.plus.pngandreport.png) to their respective Android (res/drawable/) and iOS (Assets.xcassets/) directories.reports.mp4
tasks.mp4
Summary by CodeRabbit
New Features
Style