fix: persist daily activity completions to Supabase on toggle and screen exit#218
fix: persist daily activity completions to Supabase on toggle and screen exit#218Varadraj75 wants to merge 3 commits into
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughImplements debounce-based persistence for task completions, adds save-on-exit lifecycle and navigation hooks, and ensures pending completion updates are flushed to the backend on app pause, back navigation, or explicit pop. Changes
Sequence DiagramsequenceDiagram
participant User
participant Screen as DailyActivitiesScreen
participant Binding as WidgetsBinding
participant Provider as TaskProvider
participant DB as Supabase
User->>Screen: Toggle task checkbox
Screen->>Provider: toggleTaskCompletion(activityId, isCompleted)
Provider->>Provider: Mutate _allTasks
Provider->>Provider: Cancel existing debounce timer
Provider->>Provider: Start 1500ms debounce timer
Provider-->>Screen: notifyListeners()
Screen->>Screen: Rebuild UI
alt Debounce timer fires
Provider->>Provider: Debounced callback
Provider->>DB: updateActivityCompletion(_allTasks)
DB-->>Provider: Ack
else User navigates away or presses back
User->>Screen: Back / Pop
Screen->>Provider: saveAndFlush()
Provider->>Provider: Cancel debounce timer
Provider->>DB: updateActivityCompletion(_allTasks)
DB-->>Provider: Ack
Screen->>Screen: Navigator.pop()
else App pauses
Binding->>Screen: didChangeAppLifecycleState(paused)
Screen->>Provider: saveAndFlush()
Provider->>DB: updateActivityCompletion(_allTasks)
DB-->>Provider: Ack
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
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.
Pull request overview
Fixes loss of patient daily activity completion history by ensuring checkbox toggles are persisted to Supabase, and pending changes are flushed when leaving the screen or backgrounding the app.
Changes:
- Debounced persistence on
toggleTaskCompletion()plus asaveAndFlush()method to force-save immediately on exit. - Ensures date switching saves current completion state before fetching the next day’s activities.
- Adds exit/background save hooks in the Daily Activities UI (system back, AppBar back, app pause).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
patient/lib/provider/task_provider.dart |
Adds debounce timer + flush/dispose behavior; awaits completion update before date fetch. |
patient/lib/presentation/activities/daily_activities_screen.dart |
Flushes pending completion updates on back navigation and app lifecycle pause via PopScope and WidgetsBindingObserver. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void setSelectedDate(DateTime date) async { | ||
| _selectedDate = date; | ||
| notifyListeners(); | ||
| if(_allTasks.isNotEmpty) { | ||
| updateActivityCompletion(_allTasks); | ||
| if (_allTasks.isNotEmpty) { | ||
| await updateActivityCompletion(_allTasks); | ||
| } | ||
| getTodayActivities(date: date); | ||
| } |
There was a problem hiding this comment.
setSelectedDate is now marked async but still returns void. This will trigger the avoid_void_async lint and also makes it impossible for callers to await completion/error propagation. Consider changing the signature to Future<void> setSelectedDate(...) and (optionally) awaiting the subsequent getTodayActivities(date: date) call inside it.
There was a problem hiding this comment.
Fixed. Changed signature to Future<void> and added debounce cancellation before the flush so a pending timer from a recent toggle cannot fire after the date-change save.
| _debounceTimer?.cancel(); | ||
| _debounceTimer = Timer(const Duration(milliseconds: 1500), () { | ||
| updateActivityCompletion(_allTasks); | ||
| }); |
There was a problem hiding this comment.
The debounce Timer callback invokes the async updateActivityCompletion without awaiting/handling its Future. That can lead to unhandled async errors and overlapping in-flight updates. Consider using unawaited(updateActivityCompletion(snapshot)) (or making the callback async and awaiting), and pass an immutable snapshot of the task list. Also, updateActivityCompletion(List<PatientTaskModel> tasks) currently ignores its tasks parameter and always sends _allTasks, which makes passing a snapshot ineffective—use the method argument when calling the repository.
There was a problem hiding this comment.
Fixed. Passing an immutable snapshot of _allTasks to the timer callback and wrapping the call with unawaited() so the Future is explicitly discarded.
| void didChangeAppLifecycleState(AppLifecycleState state) { | ||
| if (state == AppLifecycleState.paused) { | ||
| context.read<TaskProvider>().saveAndFlush(); | ||
| } |
There was a problem hiding this comment.
didChangeAppLifecycleState calls saveAndFlush() (async) without awaiting or explicitly discarding the Future. Since this callback can’t be awaited by the framework, consider wrapping the call with unawaited(...) and adding error handling/logging so failures don’t get dropped silently.
There was a problem hiding this comment.
Fixed. Wrapped with unawaited() to make the intent explicit.
![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 `@patient/lib/presentation/activities/daily_activities_screen.dart`:
- Around line 67-83: PopScope is created with canPop: false which prevents both
system and programmatic pops, so the Navigator.of(context).pop() calls in
onPopInvokedWithResult and the AppBar leading IconButton will not dismiss the
route; update those handlers to enable popping before calling pop (for example
toggle the PopScope's canPop state to true via the same state backing PopScope,
e.g., change PopScope(canPop: _canPop) and in onPopInvokedWithResult and the
IconButton onPressed call await context.read<TaskProvider>().saveAndFlush(); set
_canPop = true (via setState or equivalent) and then call
Navigator.of(context).pop()); alternatively call Navigator.maybePop() after
saveAndFlush instead of pop if you prefer not to mutate canPop. Ensure you
modify the PopScope control variable and the handlers (onPopInvokedWithResult,
AppBar leading IconButton onPressed) so the route can actually pop.
In `@patient/lib/provider/task_provider.dart`:
- Around line 95-99: dispose() currently just cancels _debounceTimer which can
drop a pending completion update if the provider is disposed before the timer
fires; before cancelling the timer and calling super.dispose(), ensure any
pending completion is synchronously committed — e.g. implement and call a helper
like _flushPendingCompletion() (or directly call the same commit/save logic the
debounce would run) to persist the pending completion update, then cancel
_debounceTimer and proceed with super.dispose().
- Around line 62-69: The setSelectedDate path can trigger an immediate save via
updateActivityCompletion while a pending debounce timer may still fire later and
cause a second write for the wrong date; modify setSelectedDate to cancel any
pending debounce before performing the immediate save and fetching the new day's
activities (e.g., cancel _debounceTimer or call the debounce-cancel helper used
elsewhere), then await updateActivityCompletion(_allTasks) and call
getTodayActivities(date: date) — ensure you reference and cancel the same
debounce timer/handler used by the code that schedules the debounced write so
the old timer cannot fire after the date change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d6bc999-5936-4c96-8453-5595f375fdf2
📒 Files selected for processing (2)
patient/lib/presentation/activities/daily_activities_screen.dartpatient/lib/provider/task_provider.dart
| return PopScope( | ||
| canPop: false, | ||
| onPopInvokedWithResult: (didPop, result) async { | ||
| if (didPop) return; | ||
| await context.read<TaskProvider>().saveAndFlush(); | ||
| if (context.mounted) Navigator.of(context).pop(); | ||
| }, | ||
| child: Scaffold( | ||
| appBar: AppBar( | ||
| title: const Text('Daily Activities'), | ||
| centerTitle: true, | ||
| leading: IconButton( | ||
| icon: const Icon(Icons.arrow_back_ios_new_rounded), | ||
| onPressed: () { | ||
| // context.read<TaskProvider>().updateActivityInBackground(); // TODO: Uncomment this when the backend is ready | ||
| Navigator.pop(context); | ||
| onPressed: () async { | ||
| await context.read<TaskProvider>().saveAndFlush(); | ||
| if (context.mounted) Navigator.of(context).pop(); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Flutter PopScope canPop false Navigator.pop programmatic behavior
💡 Result:
When PopScope has canPop: false, calling Navigator.pop(context) programmatically on that route's context is blocked. The pop attempt fails (the route does not pop), onPopInvokedWithResult is called with didPop: false, and the route remains in the navigator stack. Key behaviors from official documentation: - canPop: false explicitly "blocks the current route from being popped" via any means, including programmatic Navigator.pop. - "Programmatically attempting pop navigation will also result in a call to onPopInvokedWithResult, with didPop indicating success or failure." - For system gestures: "If canPop is false, then a system back gesture will not pop the route... onPopInvokedWithResult will still be called, and didPop will be false." - Same logic applies to programmatic pops, as PopScope registers a PopEntry that prevents popping regardless of initiation method. Example pattern for conditional programmatic pop after user confirmation: PopScope( canPop: false, onPopInvokedWithResult: (didPop, result) async { if (didPop) return; // Already popped // Show confirmation dialog final shouldPop = await showDialog(...); if (shouldPop == true && context.mounted) { Navigator.pop(context); // This will now succeed, as it's after confirmation } }, child: Scaffold(...), ) Direct Navigator.pop without such handling will fail silently (no pop occurs). Note: Multiple nested PopScope widgets require ALL canPop to be true for the route to pop. Works consistently with standard Navigator; some routing packages (e.g., go_router) may have edge cases on root routes.
Citations:
- 1: https://api.flutter.dev/flutter/widgets/PopScope/canPop.html
- 2: https://api.flutter.dev/flutter/widgets/PopScope-class.html
- 3: https://api.flutter.dev/flutter/widgets/PopScope/onPopInvokedWithResult.html
- 4: https://stackoverflow.com/questions/77645236/flutter-popscope-return-balck-screen-on-android-back-button/77646126
- 5: https://stackoverflow.com/questions/78806771/flutter-gorouter-popscope-widget-not-working
🌐 Web query:
Flutter PopScope onPopInvokedWithResult documentation
💡 Result:
The onPopInvokedWithResult is a property of the Flutter PopScope widget (widgets library). It is a callback of type PopInvokedWithResultCallback?, defined as void Function(bool didPop, T? result). It is called after a route pop has been handled, regardless of whether the pop was successful or canceled. You cannot prevent the pop at this stage; use canPop for that. The didPop parameter is true if the back navigation succeeded, false otherwise (e.g., when canPop: false). The result parameter contains the value passed to Navigator.pop(context, result) if any. This replaced the deprecated onPopInvoked callback (deprecated after v3.22.0-12.0.pre, stable in 3.24.0). PopScope is now generic, with T matching the enclosing Route's type for type-safe access to result. Usage example: PopScope<String?>( canPop: false, onPopInvokedWithResult: (bool didPop, String? result) { if (!didPop) { // Pop was blocked, e.g., show confirmation dialog showDialog(...); } else { // Handle successful pop and result } }, child: YourWidget, ) Official API docs: https://api.flutter.dev/flutter/widgets/PopScope/onPopInvokedWithResult.html
Citations:
- 1: https://api.flutter.dev/flutter/widgets/PopScope/onPopInvokedWithResult.html
- 2: https://api.flutter.dev/flutter/widgets/PopScope-class.html
- 3: https://api.flutter.dev/flutter/widgets/PopInvokedWithResultCallback.html
- 4: https://api.flutter.dev/flutter/widgets/PopScope/PopScope.html
- 5: https://docs.flutter.dev/release/breaking-changes/popscope-with-result
Back-navigation calls will be blocked by PopScope(canPop: false).
With PopScope.canPop = false, both system back gestures and programmatic Navigator.pop() calls are prevented. The Navigator.of(context).pop() calls in both onPopInvokedWithResult and the AppBar back button will fail silently—the route will not dismiss. After saveAndFlush() completes, set canPop: true before attempting to pop, or use a different navigation approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patient/lib/presentation/activities/daily_activities_screen.dart` around
lines 67 - 83, PopScope is created with canPop: false which prevents both system
and programmatic pops, so the Navigator.of(context).pop() calls in
onPopInvokedWithResult and the AppBar leading IconButton will not dismiss the
route; update those handlers to enable popping before calling pop (for example
toggle the PopScope's canPop state to true via the same state backing PopScope,
e.g., change PopScope(canPop: _canPop) and in onPopInvokedWithResult and the
IconButton onPressed call await context.read<TaskProvider>().saveAndFlush(); set
_canPop = true (via setState or equivalent) and then call
Navigator.of(context).pop()); alternatively call Navigator.maybePop() after
saveAndFlush instead of pop if you prefer not to mutate canPop. Ensure you
modify the PopScope control variable and the handlers (onPopInvokedWithResult,
AppBar leading IconButton onPressed) so the route can actually pop.
There was a problem hiding this comment.
This is the standard Flutter pattern for async pre-pop operations. With canPop: false,
onPopInvokedWithResult fires with didPop: false, and the manual Navigator.of(context).pop()
inside the callback performs the actual pop after saveAndFlush() completes.
The route dismisses correctly after the flush.
There was a problem hiding this comment.
🧩 Analysis chain
@Varadraj75, you're correct — I apologize for the incorrect flag.
The standard Flutter pattern for async pre-pop operations does in fact work exactly as you describe: with canPop: false, the onPopInvokedWithResult callback fires with didPop: false, and then manually calling navigator.pop() inside that callback performs the actual dismissal after the async operation completes. This is the recommended replacement for the deprecated WillPopScope.
The implementation is correct — saveAndFlush() completes before the manual Navigator.of(context).pop() is issued, which is the intended behavior.
🌐 Web query:
Flutter PopScope canPop false Navigator.pop inside onPopInvokedWithResult does it work
💡 Result:
Yes—it works, and it’s the intended pattern.
- When
PopScope(canPop: false, ...)is used, a system back attempt will not pop the route, butonPopInvokedWithResultstill fires withdidPop == false. From there you can decide (e.g., show a dialog) and then manually callNavigator.pop(context, result)to actually leave. [1][2] - Programmatic pops (like calling
Navigator.pop) also triggeronPopInvokedWithResult, withdidPopindicating whether the pop succeeded. [1] - Guard against double-handling: if you manually pop, the callback may run again with
didPop == true, so typically you early-return whendidPopis true. [3] - iOS caveat: with
CupertinoRouteTransitionMixinandcanPop: false, no back-swipe gesture is detected, so the callback won’t run for that gesture. [1]
Example pattern (same logic shown in Flutter’s migration guide):
PopScope<Object?>(
canPop: false,
onPopInvokedWithResult: (didPop, result) async {
if (didPop) return; // already popped
final ok = await confirmLeave();
if (ok) Navigator.of(context).pop(/* optional result */);
},
child: ...
)Sources: PopScope docs [1], onPopInvokedWithResult docs [2], Flutter predictive-back migration guide [3].
[1] (api.flutter.dev)
[2] (api.flutter.dev)
[3] (docs.flutter.dev)
Citations:
- 1: https://api.flutter.dev/flutter/widgets/PopScope-class.html
- 2: https://api.flutter.dev/flutter/widgets/PopScope/onPopInvokedWithResult.html
- 3: https://docs.flutter.dev/release/breaking-changes/android-predictive-back
@Varadraj75, you're absolutely right — I apologize for the incorrect flag.
The Flutter documentation confirms that with PopScope(canPop: false), onPopInvokedWithResult still fires with didPop == false on a back attempt, and from there you can manually call Navigator.pop(context) to actually dismiss the route — which is exactly what your implementation does. The if (didPop) return; guard you already have is also the correct way to prevent double-handling when the callback runs again after the manual pop succeeds.
The implementation is correct and follows the standard Flutter pattern.
✏️ Learnings added
Learnt from: Varadraj75
Repo: AOSSIE-Org/NeuroTrack PR: 218
File: patient/lib/presentation/activities/daily_activities_screen.dart:69-85
Timestamp: 2026-04-11T00:22:12.766Z
Learning: In Flutter, the standard pattern for async pre-pop operations using PopScope is: set canPop: false, intercept the back gesture in onPopInvokedWithResult (which fires with didPop: false), perform async work, then call navigator.pop() manually. This pattern works correctly — the manual Navigator.pop() inside onPopInvokedWithResult successfully dismisses the route after the async operation completes. This is the recommended replacement for the deprecated WillPopScope.
Learnt from: Varadraj75
Repo: AOSSIE-Org/NeuroTrack PR: 218
File: patient/lib/presentation/activities/daily_activities_screen.dart:69-85
Timestamp: 2026-04-11T00:22:12.766Z
Learning: In Flutter, `PopScope(canPop: false)` + calling `Navigator.pop()` inside `onPopInvokedWithResult` is the **standard pattern** for async pre-pop operations (e.g., saving data before leaving a screen). When `canPop: false`, a system back gesture is blocked but `onPopInvokedWithResult` still fires with `didPop: false`. After completing async work, calling `Navigator.of(context).pop()` inside the callback successfully dismisses the route. An `if (didPop) return;` guard prevents double-handling. This is the recommended Flutter replacement for the deprecated `WillPopScope`. Used in `patient/lib/presentation/activities/daily_activities_screen.dart`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…ebounce on date change, flush on dispose, unawaited lifecycle save
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 (1)
patient/lib/provider/task_provider.dart (1)
45-47:⚠️ Potential issue | 🟠 MajorPersist the method argument, not the mutable field.
Line 47 ignores the
tasksparameter and always writes_allTasks. That makes the debounce snapshot on Lines 84-86 ineffective, so the delayed write can still persist later mutations/reloads instead of the captured state.patient/lib/repository/supabase_patient_repository.dart(Lines 175-192) serializes whichever list it receives.Proposed fix
Future<void> updateActivityCompletion(List<PatientTaskModel> tasks) async { try { - final result = await _patientRepository.updateActivityCompletion(tasks: _allTasks, activityId: _activityId, activitySetId: _activitySetId); + final result = await _patientRepository.updateActivityCompletion( + tasks: tasks, + activityId: _activityId, + activitySetId: _activitySetId, + ); if(result is ActionResultSuccess) { _apiStatus = ApiStatus.success; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patient/lib/provider/task_provider.dart` around lines 45 - 47, The updateActivityCompletion method is persisting the mutable field _allTasks instead of the captured argument tasks, so replace the call to _patientRepository.updateActivityCompletion(..., tasks: _allTasks, ...) with passing the method parameter (tasks: tasks) so the serialized state is the snapshot provided to updateActivityCompletion; ensure the rest of the call still uses _activityId and _activitySetId and that nothing mutates the tasks list before the repository call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patient/lib/provider/task_provider.dart`:
- Around line 98-104: The dispose() implementation fires
updateActivityCompletion(_allTasks) via unawaited which lets its async finally
call notifyListeners() after the ChangeNotifier is disposed — causing an
assertion; fix by either awaiting updateActivityCompletion(_allTasks) before
calling super.dispose() or by adding a disposed guard (e.g. a private bool
_isDisposed set true in dispose()) and checking that flag inside
updateActivityCompletion() (or before any notifyListeners() calls) so
notifyListeners() is skipped once dispose() begins; update references:
dispose(), updateActivityCompletion(), _debounceTimer, _allTasks, and any
notifyListeners() calls in the finally block.
- Around line 62-70: The setSelectedDate(DateTime) method cancels debounce, sets
_selectedDate, notifies listeners, and calls getTodayActivities(date: date)
without awaiting it, causing setSelectedDate to complete before the network
request finishes; update setSelectedDate in task_provider.dart to await the
getTodayActivities(...) call (i.e., change to await getTodayActivities(date:
date)) so the Future returned by setSelectedDate resolves only after
updateActivityCompletion(...) and getTodayActivities complete, preventing
out-of-order responses from overwriting newer data.
---
Outside diff comments:
In `@patient/lib/provider/task_provider.dart`:
- Around line 45-47: The updateActivityCompletion method is persisting the
mutable field _allTasks instead of the captured argument tasks, so replace the
call to _patientRepository.updateActivityCompletion(..., tasks: _allTasks, ...)
with passing the method parameter (tasks: tasks) so the serialized state is the
snapshot provided to updateActivityCompletion; ensure the rest of the call still
uses _activityId and _activitySetId and that nothing mutates the tasks list
before the repository call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62ff0cf6-824c-44a1-8f5f-a1c039deed6f
📒 Files selected for processing (2)
patient/lib/presentation/activities/daily_activities_screen.dartpatient/lib/provider/task_provider.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- patient/lib/presentation/activities/daily_activities_screen.dart
…ctivities, guard notifyListeners after dispose
Closes #217
📝 Description
toggleTaskCompletion()was only updating in-memory state and callingnotifyListeners()with no Supabase write. The only save path in the codebase was a commented-out TODO pointing to a method that never existed. This means every patient's activity completion history was never actually saved - progress reset silently on every back navigation or restart.🔧 Changes Made
updateActivityCompletion()call insidetoggleTaskCompletion()soeach checkbox toggle persists to Supabase after 1.5s
saveAndFlush()method that cancels the debounce timer and immediately awaitsupdateActivityCompletion()for use on screen exitdispose()override inTaskProviderto cancel any pending debounce timer on cleanupsetSelectedDate()toawaitthe completion update before fetching new activities,eliminating the race condition where a fresh DB read could overwrite unsaved state
ScaffoldinPopScopeso system back gesture callssaveAndFlush()before poppingAppBarback button to also callsaveAndFlush()before poppingWidgetsBindingObserverto callsaveAndFlush()when the app is backgroundedupdateActivityInBackgroundTODO line📷 Screenshots or Visual Changes (if applicable)
Not applicable. The fix is in data written to Supabase, not visible in the UI.
🤝 Collaboration
Collaborated with: NONE
✅ Checklist
Summary by CodeRabbit
Bug Fixes
Performance