Stop promising cloud-synced folders for the database on Android (#311) - #963
Conversation
The error in #311 ("Cannot write to the selected folder") was already fixed by #300: pickCustomFolder no longer routes Android through FilePicker.getDirectoryPath, which resolved the SAF tree URI to a raw /storage/emulated/0 path and discarded the content:// grant, so the dart:io probe write was denied by scoped storage. What remained was the copy that led the reporter there. On Android a custom folder can only be an app-specific external volume, because a live SQLite file needs a real lockable path for its -wal/-shm byte-range locks and a SAF stream cannot provide one. Nothing under Android/data is readable by any sync client on Android 11+, so three strings were false: - the Custom Folder subtitle, "Choose a synced folder (Dropbox, Google Drive, etc.)" - the storage-settings info banner and the cloud-sync page banner, both crediting "your folder's sync service" with synchronization The banners are the worse pair: selecting a custom folder disables app-managed cloud sync, so an Android user on the SD card was told something else covered a library that was in fact syncing nowhere. Adds StoragePlatformCapabilities.customFolderIsDeviceVolumeOnly (Platform.isAndroid) and uses it to pick honest wording on all three surfaces, pointing users who want a synced copy at the backup location, which does keep its content:// grant through submersion_saf. The volume chooser carries the same pointer. Also corrects the stale "Android: Uses Storage Access Framework (SAF)" comment. Keeping the platform check in the provider is what lets the new widget tests exercise the Android branch on a non-Android host.
There was a problem hiding this comment.
Pull request overview
Updates Settings UX to stop implying Android can host the live SQLite database inside cloud-synced folders (e.g., Dropbox/Drive/Nextcloud), and instead points users to Backup & Restore for cloud copies—preventing the “sync is handled elsewhere” misconception when app-managed sync is disabled.
Changes:
- Introduces
StoragePlatformCapabilities.customFolderIsDeviceVolumeOnly(Android) to select Android-specific copy in Settings / Cloud Sync UI. - Updates subtitles and banners to accurately describe Android custom-folder limitations and direct users to Backup & Restore.
- Adds widget/page tests and new l10n keys across all locales for the updated messaging.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/features/settings/presentation/widgets/storage_volume_chooser_dialog_test.dart | Adds a dialog copy test for the new Backup & Restore guidance. |
| test/features/settings/presentation/pages/storage_settings_reset_test.dart | Updates test overrides to include the new platform capability flag. |
| test/features/settings/presentation/pages/storage_settings_pick_failure_test.dart | Updates test overrides to include the new platform capability flag. |
| test/features/settings/presentation/pages/storage_settings_custom_folder_subtitle_test.dart | New tests for Android-vs-non-Android subtitles and banner copy. |
| test/features/settings/presentation/pages/cloud_sync_page_test.dart | Adds coverage for Android device-volume wording vs folder-sync wording. |
| lib/features/settings/presentation/widgets/storage_volume_chooser_dialog.dart | Displays the Backup & Restore guidance note in the Android volume chooser dialog. |
| lib/features/settings/presentation/providers/storage_providers.dart | Adds customFolderIsDeviceVolumeOnly to platform capabilities and sets it for Android. |
| lib/features/settings/presentation/pages/storage_settings_page.dart | Uses platform capabilities to select accurate Custom Folder subtitle and info banner copy. |
| lib/features/settings/presentation/pages/cloud_sync_page.dart | Uses platform capabilities to select accurate custom-folder-disabled banner content. |
| lib/l10n/arb/app_localizations.dart | Adds new localization getters for the new Settings strings. |
| lib/l10n/arb/app_localizations_en.dart | English strings for the new keys. |
| lib/l10n/arb/app_localizations_de.dart | German strings for the new keys. |
| lib/l10n/arb/app_localizations_es.dart | Spanish strings for the new keys. |
| lib/l10n/arb/app_localizations_fr.dart | French strings for the new keys. |
| lib/l10n/arb/app_localizations_he.dart | Hebrew strings for the new keys. |
| lib/l10n/arb/app_localizations_hu.dart | Hungarian strings for the new keys. |
| lib/l10n/arb/app_localizations_it.dart | Italian strings for the new keys. |
| lib/l10n/arb/app_localizations_nl.dart | Dutch strings for the new keys. |
| lib/l10n/arb/app_localizations_pt.dart | Portuguese strings for the new keys. |
| lib/l10n/arb/app_localizations_zh.dart | Chinese strings for the new keys. |
| lib/l10n/arb/app_en.arb | Adds new English ARB keys. |
| lib/l10n/arb/app_de.arb | Adds new German ARB keys. |
| lib/l10n/arb/app_es.arb | Adds new Spanish ARB keys. |
| lib/l10n/arb/app_fr.arb | Adds new French ARB keys. |
| lib/l10n/arb/app_he.arb | Adds new Hebrew ARB keys. |
| lib/l10n/arb/app_hu.arb | Adds new Hungarian ARB keys. |
| lib/l10n/arb/app_it.arb | Adds new Italian ARB keys. |
| lib/l10n/arb/app_nl.arb | Adds new Dutch ARB keys. |
| lib/l10n/arb/app_pt.arb | Adds new Portuguese ARB keys. |
| lib/l10n/arb/app_zh.arb | Adds new Chinese ARB keys. |
| lib/l10n/arb/app_ar.arb | Adds new Arabic ARB keys. |
| lib/l10n/arb/app_localizations_ar.dart | Arabic strings for the new keys. |
Suppressed comments (2)
test/features/settings/presentation/pages/storage_settings_custom_folder_subtitle_test.dart:60
- Store the original PathProviderPlatform.instance before overwriting it so it can be restored in tearDown; otherwise this fake path provider can affect unrelated tests running later in the same process.
tempDir = Directory.systemTemp.createTempSync('storage_subtitle_test');
PathProviderPlatform.instance = _FakePathProvider(tempDir.path);
});
test/features/settings/presentation/pages/storage_settings_custom_folder_subtitle_test.dart:70
- Restore PathProviderPlatform.instance in tearDown to avoid leaking the fake path provider into subsequent tests.
DatabaseService.instance.resetForTesting();
try {
tempDir.deleteSync(recursive: true);
} catch (_) {}
});
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses the Copilot review on #963. - app_es.arb: "Ubicacion" -> "Ubicación" in the new db_location_backup_note, and in the backup_location_title label that string quotes, so the copy and the menu it points at agree. "Ubicación" is already the spelling used elsewhere in the file; the unaccented label was the outlier. The other ~20 unaccented "Ubicacion" strings in es are pre-existing and left alone. - storage_settings_custom_folder_subtitle_test: capture and restore PathProviderPlatform.instance, matching icloud_storage_provider_test. The fake pointed at a temp dir the same tearDown deletes. - storage_volume_chooser_dialog_test: pin locale to en so the English literals cannot depend on the host default. Asserting through AppLocalizations instead would compare the copy against itself and pass even if the wording were wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/features/settings/presentation/pages/cloud_sync_page_test.dart:691
- Same locale determinism issue here: this test asserts English strings via
find.text(...), but without pinning the test locale the widget may render a different language depending on the host environment.
testWidgets('does not credit a sync service on device-volume-only '
'platforms', (tester) async {
await pumpPage(
tester,
customFolderMode: true,
customFolderIsDeviceVolumeOnly: true,
);
test/features/settings/presentation/pages/cloud_sync_page_test.dart:683
- These tests assert on English banner copy, but the
MaterialAppinpumpPagedoesn’t pin a locale. If the host test environment’s default locale isn’t English, the page can localize to a different language and thesefind.text(...)expectations become flaky. Setting the test locale toenat the start of each test (or once in a shared setup) keeps the assertions deterministic.
This issue also appears on line 685 of the same file.
testWidgets('credits the folder\'s sync service where folders really sync', (
tester,
) async {
await pumpPage(tester, customFolderMode: true);
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
📦 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. |
Addresses the two suppressed comments from the second Copilot review on #963: the banner tests assert English literals via find.text, but the MaterialApp in pumpPage did not pin a locale, so the page could localize to the host default and make those expectations flaky. Pins locale to en on all four app roots in the file, not just the pumpPage one Copilot flagged -- the three MaterialApp.router roots have the same gap and their tests assert English strings too, so pinning one and leaving three would be incoherent. Same reasoning as the chooser-dialog fix in 00ce75c: pin the locale rather than assert through AppLocalizations, which would compare the copy against itself.
|
Picked up the two suppressed comments from the latest review ( Verified the claim before acting: Pinned Same reasoning as the chooser-dialog thread: pinning the locale is preferable to asserting through
|
github-project-automation
Bot
moved this from In review
to Done
in Submersion Release Tracker
Closes #311.
What #311 actually was
The reported error — "Cannot write to the selected folder. Please check permissions." — is already fixed, by #300.
DatabaseLocationService.pickCustomFolder()used to route Android throughFilePicker.getDirectoryPath(), which resolves the SAF tree URI the user just approved into a raw/storage/emulated/0/...path and discards thecontent://grant. The subsequentdart:ioprobe write inverifyFolderAccessible()was then denied by scoped storage — which is also why the reporter saw no file permissions beyond photos and video. Commit 8eb3105 replaced that branch withpath_provider's app-specific external volumes, and it has shipped since v1.5.5.What was still broken
The copy that led the reporter into the flow. On Android a custom folder can only ever be an app-specific external volume: a live SQLite file needs a real lockable path for its
-wal/-shmbyte-range locks, and a SAF stream cannot provide one. Nothing underAndroid/data/is readable by any sync client on Android 11+, so three strings were false there:The two banners are the more consequential pair. Selecting a custom folder sets
isCloudSyncDisabledByCustomFolderProvider, so the app turns its own sync off and then told an Android user that Dropbox or Drive was covering a library that was in fact syncing nowhere.Changes
StoragePlatformCapabilities.customFolderIsDeviceVolumeOnly(Platform.isAndroid). Keeping the platform check in the provider rather than inbuild()is what lets the new widget tests exercise the Android branch on a non-Android host.content://grant through thesubmersion_safplugin. That is the real answer for the reporter's use case.// Android: Uses Storage Access Framework (SAF)comment.Verification
ocr_scan_page_test×2,shearwater_cloud_parser_test) all pass when re-run in isolation and are untouched by this change.flutter analyzeclean on the whole project;dart formatreports no changes.test/l10n/arb_parity_test.dartgreen, confirming the new keys reached every locale.Not covered
Behaviour is unchanged on non-Android platforms, and no Android hardware verification was performed — this PR only changes strings and the platform predicate that selects between them.