fix(mobile): in-progress feedback for the Clear local data action#2439
Conversation
The reset- and delete-account tiles already disable and show a spinner while their network calls are in flight, but "Clear local data" (an async op that wipes offline storage and clears the category/merchant/tag providers) had no in-progress feedback. The settings list stayed fully interactive during the wipe, so the row could be tapped again. Mirror the existing reset/delete pattern: add an _isClearingData flag (set before the work, cleared in a finally with a mounted guard) and give the tile a trailing CircularProgressIndicator plus enabled/onTap guards while it runs. Completes the in-progress feedback across all destructive async actions in Settings. flutter analyze: no new issues; full suite (166) green.
📝 WalkthroughWalkthrough
ChangesUnified Destructive Action State Management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mobile/lib/screens/settings_screen.dart`:
- Around line 760-763: The Clear local data tile and its handler
_handleClearLocalData are guarded only by the _isClearingData flag, which allows
concurrent execution with other destructive actions like reset and delete flows
that likely have their own separate flags. Create a single shared "destructive
action in progress" guard flag instead of separate flags for each destructive
action, then use this shared flag to disable all three destructive action tiles
(Clear local data, Reset, and Delete) and gate all three corresponding handlers
to prevent concurrent execution.
🪄 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: 9c52d096-bf82-4066-8029-c7d5d62390fa
📒 Files selected for processing (1)
mobile/lib/screens/settings_screen.dart
Replace three independent boolean flags (_isClearingData, _isResettingAccount, _isDeletingAccount) with a single _activeDestructiveAction string. All three danger-zone tiles now disable together whenever any destructive operation is running, preventing concurrent destructive actions.
jjmata
left a comment
There was a problem hiding this comment.
Good fix. The consolidation into a single String? _activeDestructiveAction discriminator is cleaner than three independent booleans, and it naturally prevents two actions from running simultaneously without extra logic.
The main bug this fixes — the missing finally block on _handleClearLocalData that would have left _activeDestructiveAction stuck as 'clear' on error — is now resolved. The reset and delete paths already had finally blocks, so this brings clear into alignment.
One minor note: the three string literals 'clear', 'reset', 'delete' are scattered across the widget. If a new destructive action were added, it's easy to introduce a typo. Private constants (e.g., static const String _kClear = 'clear') would make this safer, but for three values in one widget file it's not urgent.
Generated by Claude Code
Address review feedback (jjmata): replace the scattered 'clear' / 'reset' / 'delete' string literals used for _activeDestructiveAction with private constants (_clearAction / _resetAction / _deleteAction), defined once. The discriminator can no longer drift via a mistyped literal across the handlers and tiles — a typo'd constant name fails to compile. No behavior change.
|
Thanks @jjmata — done in 7cfc725. Replaced the scattered |
Summary
Closes #1045 (no in-progress feedback during destructive async operations). The reset- and delete-account tiles already disable and show a spinner while their requests run — but Clear local data, which asynchronously wipes offline storage and clears the category/merchant/tag providers, had no feedback. The settings list stayed fully interactive during the wipe, inviting a second tap.
This completes the in-progress feedback across all destructive async actions in Settings.
Change
In
mobile/lib/screens/settings_screen.dart, mirror the established reset/delete pattern:_isClearingDataflag, set before the work and cleared in afinallywith amountedguard;ListTilea trailingCircularProgressIndicatorandenabled/onTapguards while it runs.Screenshots
Before – no feedback, tile fully interactive
After – spinner + tile disabled while clearing
Before – no feedback, tile fully interactive
After – spinner + tile disabled while clearing
Test plan
flutter analyze --no-fatal-infos— no new issuesflutter test— green (166)(The reset/delete loading states already in
mainshipped without a dedicated widget test; this mirrors that exact pattern in the same screen.SettingsScreenrequires a large provider/biometric/upgrader harness to pump, so no new test is added — consistent with existing coverage.)Summary by CodeRabbit