Run the safety review automatically after a backup restore - #916
Merged
Conversation
Adds PostRestoreSafetyReview, which runs the whole-library safety sweep in a short-lived ProviderContainer built against the restored database rather than the live one, whose settings were loaded from the database being replaced. Two supporting fixes in SettingsNotifier, both reachable independently of backups: - Expose initialLoad. State starts at the AppSettings defaults and is replaced asynchronously, so any consumer reading gradient factors before the first load completes silently grades against defaults. - Guard the post-await state assignment with a mounted check. Tearing down a ProviderScope mid-load (restartApp's soft restart, or the sweep's throwaway container) previously threw 'Tried to use SettingsNotifier after dispose'.
Contributor
There was a problem hiding this comment.
Pull request overview
Automatically runs a safety-review sweep after restoring a backup so restored logbooks immediately show safety findings/badges without requiring the user to manually run “Analyze all dives” in Settings. This fits into the backup/restore flow by adding a post-restore analysis phase with progress UI and cancellation.
Changes:
- Extracts the “Analyze all dives” logic into a reusable
SafetyReviewSweepprovider and reuses it from both Settings and post-restore flow. - Adds a post-restore runner that executes the sweep in a short-lived
ProviderContainerwith shared root overrides, and surfaces progress + Skip on the restore barrier. - Fixes
SettingsNotifierlifecycle/initial-load issues by exposinginitialLoadand guardingstatewrites after async reads.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/helpers/mock_providers.dart | Updates mock settings notifier to implement initialLoad. |
| test/features/statistics/presentation/pages/records_page_test.dart | Updates local settings mock to implement initialLoad. |
| test/features/settings/presentation/pages/settings_page_test.dart | Updates local settings mock to implement initialLoad. |
| test/features/settings/presentation/pages/settings_page_shared_data_test.dart | Updates local settings mock to implement initialLoad. |
| test/features/dive_log/presentation/providers/safety_review_sweep_test.dart | New unit tests for SafetyReviewSweep behavior (scope, progress, cancellation, failures). |
| test/features/backup/presentation/widgets/restore_barrier_test.dart | Adds widget coverage for sweep progress UI + Skip button rendering. |
| test/features/backup/presentation/providers/post_restore_safety_review_test.dart | New tests ensuring post-restore sweep uses restored DB settings and reports progress/cancellation. |
| test/features/backup/presentation/providers/backup_providers_restore_test.dart | Extends restore notifier tests to verify sweep invocation, progress publication, skip propagation, and failure-swallowing. |
| test/core/providers/root_overrides_test.dart | New test for shared root override helper. |
| lib/main.dart | Switches root ProviderScope overrides to rootProviderOverrides(...). |
| lib/core/providers/root_overrides.dart | New shared helper producing the root provider overrides used by both app scope and scratch containers. |
| lib/features/settings/presentation/providers/settings_providers.dart | Adds SettingsNotifier.initialLoad and mounted guard after async settings read. |
| lib/features/settings/presentation/pages/safety_settings_page.dart | Delegates manual “Analyze all dives” to SafetyReviewSweep. |
| lib/features/dive_log/presentation/providers/safety_review_sweep.dart | New reusable sweep runner for safety-review analysis. |
| lib/features/backup/presentation/providers/post_restore_safety_review.dart | New post-restore runner that uses a scratch container and awaits settings initial load. |
| lib/features/backup/presentation/providers/backup_providers.dart | Wires post-restore sweep into restore flows; publishes structured progress; adds skip support. |
| lib/features/backup/presentation/widgets/restore_barrier.dart | Shows localized determinate progress + Skip button when sweep is running. |
| lib/l10n/arb/app_en.arb | Adds restore-safety-sweep title/progress/skip strings. |
| lib/l10n/arb/app_de.arb | Adds restore-safety-sweep title/progress/skip strings (de). |
| lib/l10n/arb/app_es.arb | Adds restore-safety-sweep title/progress/skip strings (es). |
| lib/l10n/arb/app_fr.arb | Adds restore-safety-sweep title/progress/skip strings (fr). |
| lib/l10n/arb/app_it.arb | Adds restore-safety-sweep title/progress/skip strings (it). |
| lib/l10n/arb/app_hu.arb | Adds restore-safety-sweep title/progress/skip strings (hu). |
| lib/l10n/arb/app_he.arb | Adds restore-safety-sweep title/progress/skip strings (he). |
| lib/l10n/arb/app_nl.arb | Adds restore-safety-sweep title/progress/skip strings (nl). |
| lib/l10n/arb/app_pt.arb | Adds restore-safety-sweep title/progress/skip strings (pt). |
| lib/l10n/arb/app_zh.arb | Adds restore-safety-sweep title/progress/skip strings (zh). |
| lib/l10n/arb/app_ar.arb | Adds restore-safety-sweep title/progress/skip strings (ar). |
| lib/l10n/arb/app_localizations.dart | Updates generated localization interface for new keys. |
| lib/l10n/arb/app_localizations_en.dart | Updates generated en localizations for new keys. |
| lib/l10n/arb/app_localizations_de.dart | Updates generated de localizations for new keys. |
| lib/l10n/arb/app_localizations_es.dart | Updates generated es localizations for new keys. |
| lib/l10n/arb/app_localizations_fr.dart | Updates generated fr localizations for new keys. |
| lib/l10n/arb/app_localizations_it.dart | Updates generated it localizations for new keys. |
| lib/l10n/arb/app_localizations_hu.dart | Updates generated hu localizations for new keys. |
| lib/l10n/arb/app_localizations_he.dart | Updates generated he localizations for new keys. |
| lib/l10n/arb/app_localizations_nl.dart | Updates generated nl localizations for new keys. |
| lib/l10n/arb/app_localizations_pt.dart | Updates generated pt localizations for new keys. |
| lib/l10n/arb/app_localizations_zh.dart | Updates generated zh localizations for new keys. |
| lib/l10n/arb/app_localizations_ar.dart | Updates generated ar localizations for new keys. |
| docs/superpowers/specs/2026-08-08-post-restore-safety-review-design.md | New design/spec documenting restore-triggered safety sweep. |
| docs/superpowers/plans/2026-08-08-post-restore-safety-review.md | New implementation plan + as-built deviations/verification checklist. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
|
📦 Build artifacts for this PR · commit
Artifacts expire in 7 days. Downloading requires being signed in to GitHub. macOS needs two extractions: unzip the downloaded artifact, then unzip the Updated automatically on each push. |
…preserve sweep progress Addresses PR #916 review. Decompression settings are per-diver (diver_settings.gf_low/gf_high, ppO2 ceilings, deco stop increment), and computeAnalysisForProfile falls back to gfLowProvider/gfHighProvider whenever a dive carries no dive-specific GFs. A single all-divers pass therefore graded every non-active diver's dives with the ACTIVE diver's gradient factors and persisted the result stamped with the current engineVersion, so it would never be recomputed. The sweep now runs one pass per diver, each in a container whose settingsProvider is pinned to that diver via a new SettingsNotifier.preloaded constructor. Overriding the single root provider covers every derived provider (gradient factors, ppO2 ceilings, deco stop increment, ProfileLegend's metric-source defaults) instead of enumerating a dozen overrides that would rot as the analysis pipeline grows. Dives with a null diver_id get a trailing pass. Settings are read with getSettingsForDiver, not getOrCreateSettingsForDiver: the latter writes a defaults row, and a restore must not mint rows that would sync out as real edits. BackupOperationState.copyWith now preserves sweepProgress when omitted and takes an explicit clearSweepProgress flag.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/features/backup/presentation/providers/post_restore_safety_review.dart:52
run()emitsonProgress(0, total)immediately after counting all dives, before knowing whether any sweep pass will actually run. BecauseSafetyReviewSweep.run()returns early whensafetyReviewEnabledProvideris false (and does not callonProgress), a restore can still publishsweepProgressand show the “Running the safety review” UI even when the restored diver has the safety review disabled, and progress may never reachtotalwhen some passes are skipped due to per-diver settings.
Consider computing the progress total as the number of dives that will actually be swept (based on each diver’s safetyReviewEnabled), and if that computed total is 0, return SafetyReviewSweepResult.empty without calling onProgress so the restore barrier stays in its normal restore-spinner mode.
final diveRepo = _ref.read(diveRepositoryProvider);
final allIds = await diveRepo.getOrderedDiveIds();
final total = allIds.length;
onProgress?.call(0, total);
if (total == 0) return SafetyReviewSweepResult.empty;
github-project-automation
Bot
moved this from In review
to Done
in Submersion Release Tracker
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
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.
Summary
Restoring a backup left the safety review empty for the entire logbook. No dive
showed findings and no dive-list row showed a finding badge until the user found
Settings > Safety > "Analyze all dives" and ran it by hand.
The safety review has only two triggers, and a restore is neither: lazy compute
when a dive detail page is opened (
safetyReviewProvider), and the manualSettings sweep. A backup is a whole-file SQLite copy, so the
dive_safety_reviews/dive_safety_findingstables travel with it — butcarrying only whatever rows the source database happened to have. A backup that
predates the feature restores those tables empty (the migration ladder creates
them), and dives the source device never opened were never analyzed. Either way
DiveSummary.safetyFindingCountreports zero across the library.This runs the sweep automatically as the last step of a restore, over every
diver, with progress and a Skip button on the restore barrier.
Changes
safety_settings_page.dartinto ashared
SafetyReviewSweepprovider. Both Settings and the restore path useit, so the load-bearing
ref.invalidate()-before-readinvariant (neededbecause
safetyReviewProvideris notautoDisposeand would otherwise returna stale cached
AsyncValue) lives in one place.BackupOperationNotifier— the single seam every restore entrypoint funnels through, including the setup wizard — between the existing
active-diver realignment and the restore-complete transition.
ProviderContainer(PostRestoreSafetyReview).The live container still holds settings loaded from the database being
replaced: gradient factors shape the ceiling curve that the
missedDecoStopand
highSurfaceGfrules grade against, andProfileLegend's metric-sourcedefaults feed
overlayComputerDecoData. Persisting findings computed fromthose would stamp the current
engineVersiononto wrong results that thennever recompute.
rootProviderOverridesso the scratch container cannot drift from thereal
ProviderScope(logFileServiceProviderthrows unless overridden).is lossless: unswept dives still compute lazily on first view, and the
Settings sweep remains available.
swap and sync re-baseline have already succeeded, so errors are logged and
swallowed.
Two latent bugs fixed along the way, both independent of backups
SettingsNotifierstarts at theAppSettingsdefaults and replaces itsstate asynchronously, so any consumer reading gradient factors before that
first load completes silently grades against defaults. Added
initialLoad,which the sweep awaits before running.
_loadSettingsassignedstateafter anawaitwith nomountedcheck,throwing "Tried to use SettingsNotifier after dispose" whenever a
ProviderScopeis torn down mid-load — reachable today viarestartApp()'ssoft restart, not just the new container. Added the guard.
Notes
SafetyReviewService.engineVersionchange.rebaselineAfterRestorehas alreadycleared the sync position, so every row is pending regardless; the sweep's
parent-dive HLC bumps add no meaningful extra push.
Design and implementation notes, including an as-built deviations section, are
in
docs/superpowers/specs/2026-08-08-post-restore-safety-review-design.mdanddocs/superpowers/plans/2026-08-08-post-restore-safety-review.md.Test Plan
flutter testpasses — 15,724 passing, 15 skipped, 0 failuresflutter analyzepasses — clean across the whole projectNew coverage:
safety_review_sweep_test.dart(new, 7 cases): sweeps every diver whendiverIdis null; scopes correctly when set; stops onisCancelled; counts afailing dive without aborting; no-ops when the master toggle is off; reports
monotonic progress; handles an empty logbook.
post_restore_safety_review_test.dart(new, 3 cases): includingreads settings from the restored database, not the defaults, which switchesthe master toggle off in the restored diver's row and asserts the sweep
no-ops — the assertion that actually proves the scratch container reads
restored settings rather than defaults.
backup_providers_restore_test.dart(+5): both restore entry points run thesweep; progress reaches the barrier with
isRestoringstill true; a throwingsweep still reaches
restoreComplete; the skip flag reaches the running sweep.restore_barrier_test.dart(+2): progress label and Skip button render; theplain spinner is unchanged when no sweep is running.
root_overrides_test.dart(new).Suggested manual check: restore a backup created before the safety review
feature existed. The barrier should show "Running the safety review" with a
moving determinate bar, then the restore-complete screen. After the restart,
dive-list rows should show finding badges without opening each dive first.