Skip to content

Commit beb830e

Browse files
committed
fix(sync): address PR review round 2
- Rebuild flow now reflects the actual outcome: on failure (e.g. no epoch marker) it shows the error message instead of a misleading success snackbar. - resolveSyncTempDir's FlutterError fallback is scoped to the specific 'Binding has not yet been initialized' message; any other FlutterError rethrows so real problems surface. - deleteLeftoverBaseTempFiles sweeps only the 'ssv1_' sync prefix (covers base export + assembled parts) instead of any '*.base' file, so unrelated temp files in the shared dir are never deleted.
1 parent cdc8ec7 commit beb830e

5 files changed

Lines changed: 60 additions & 11 deletions

File tree

lib/core/services/sync/changeset_log/sync_temp_dir.dart

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,14 @@ Future<Directory> resolveSyncTempDir() async {
2525
return await getTemporaryDirectory();
2626
} on MissingPluginException {
2727
return Directory.systemTemp;
28-
} on FlutterError {
29-
return Directory.systemTemp;
28+
} on FlutterError catch (e) {
29+
// ONLY the "no test binding" case (a unit test that never called
30+
// TestWidgetsFlutterBinding.ensureInitialized, so ServicesBinding.instance
31+
// throws). Any other FlutterError is a real problem and must surface, not
32+
// silently fall back to /tmp.
33+
if (e.toString().contains('Binding has not yet been initialized')) {
34+
return Directory.systemTemp;
35+
}
36+
rethrow;
3037
}
3138
}

lib/core/services/sync/sync_service.dart

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2072,15 +2072,18 @@ class SyncService {
20722072
}
20732073

20742074
/// Best-effort sweep of leftover streaming-base temp files (base export
2075-
/// `ssv1_base_*` and assembled `*.base` parts) from the app temp dir. Failure
2076-
/// is logged and ignored -- a stale temp file is harmless.
2075+
/// base export `ssv1_base_*.json` and assembled `ssv1_*.base` / `ssv1_adopt_*`
2076+
/// parts) from the app temp dir. Every sync temp file is prefixed `ssv1_`, so
2077+
/// the sweep matches ONLY that prefix -- never an unrelated app temp file
2078+
/// (the dir is a shared, general-purpose temp location). Failure is logged
2079+
/// and ignored; a stale temp file is harmless.
20772080
Future<void> deleteLeftoverBaseTempFiles() async {
20782081
try {
20792082
final dir = await resolveSyncTempDir();
20802083
await for (final entity in dir.list(followLinks: false)) {
20812084
if (entity is! File) continue;
20822085
final name = entity.uri.pathSegments.last;
2083-
if (name.startsWith('ssv1_base_') || name.endsWith('.base')) {
2086+
if (name.startsWith('ssv1_')) {
20842087
try {
20852088
await entity.delete();
20862089
} catch (_) {

lib/features/settings/presentation/pages/troubleshoot_sync_page.dart

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,11 +119,21 @@ class TroubleshootSyncPage extends ConsumerWidget {
119119
);
120120
if (ok != true) return;
121121
await ref.read(syncStateProvider.notifier).rebuildBackendFromThisDevice();
122-
if (context.mounted) {
123-
ScaffoldMessenger.of(context).showSnackBar(
124-
const SnackBar(content: Text('Rebuilt backend from this device')),
125-
);
126-
}
122+
if (!context.mounted) return;
123+
// The rebuild can fail (e.g. no epoch marker to rebuild from), in which case
124+
// the notifier surfaces SyncStatus.error -- reflect that instead of always
125+
// claiming success.
126+
final state = ref.read(syncStateProvider);
127+
final failed = state.status == SyncStatus.error;
128+
ScaffoldMessenger.of(context).showSnackBar(
129+
SnackBar(
130+
content: Text(
131+
failed
132+
? (state.message ?? 'Rebuild failed')
133+
: 'Rebuilt backend from this device',
134+
),
135+
),
136+
);
127137
}
128138

129139
Future<void> _confirmRemoveThisDevice(

test/core/services/sync/sync_service_repair_test.dart

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,16 @@ void main() {
7070
await epochStore.setPendingReplace(marker);
7171
final leftover = File('${fakeAppTemp.path}/ssv1_base_dev_0.abc.json');
7272
await leftover.writeAsString('stale');
73+
// An unrelated temp file (the dir is shared) must be left untouched.
74+
final unrelated = File('${fakeAppTemp.path}/user_photo.jpg');
75+
await unrelated.writeAsString('keep');
7376

7477
await buildService().repairLocalSyncState();
7578

7679
expect(epochStore.lastAcceptedMarker, isNull);
7780
expect(epochStore.pendingReplace, isNull);
7881
expect(leftover.existsSync(), isFalse);
82+
expect(unrelated.existsSync(), isTrue);
7983
},
8084
);
8185

test/features/settings/presentation/pages/troubleshoot_sync_page_test.dart

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@ class _FakeSyncNotifier extends StateNotifier<SyncState>
1717
int wipeAllCalls = 0;
1818
int rebuildCalls = 0;
1919

20+
/// When set, [rebuildBackendFromThisDevice] models a failure by leaving the
21+
/// state in [SyncStatus.error] with this message (mirrors the real notifier).
22+
String? rebuildFailureMessage;
23+
2024
@override
2125
Future<void> repairSync() async => repairSyncCalls++;
2226

@@ -27,7 +31,13 @@ class _FakeSyncNotifier extends StateNotifier<SyncState>
2731
Future<void> wipeAllCloudSyncData() async => wipeAllCalls++;
2832

2933
@override
30-
Future<void> rebuildBackendFromThisDevice() async => rebuildCalls++;
34+
Future<void> rebuildBackendFromThisDevice() async {
35+
rebuildCalls++;
36+
final msg = rebuildFailureMessage;
37+
if (msg != null) {
38+
state = SyncState(status: SyncStatus.error, message: msg);
39+
}
40+
}
3141

3242
@override
3343
dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
@@ -155,6 +165,21 @@ void main() {
155165
expect(find.text('Rebuilt backend from this device'), findsOneWidget);
156166
});
157167

168+
testWidgets('Rebuild failure shows the error, not a success snackbar', (
169+
tester,
170+
) async {
171+
final fake = await _pump(tester);
172+
fake.rebuildFailureMessage = 'No library replacement to rebuild from';
173+
174+
await tester.tap(find.text('Rebuild backend from this device'));
175+
await tester.pumpAndSettle();
176+
await tester.tap(find.widgetWithText(FilledButton, 'Rebuild'));
177+
await tester.pumpAndSettle();
178+
179+
expect(find.text('No library replacement to rebuild from'), findsOneWidget);
180+
expect(find.text('Rebuilt backend from this device'), findsNothing);
181+
});
182+
158183
testWidgets('Remove-this-device confirm calls the notifier', (tester) async {
159184
final fake = await _pump(tester);
160185

0 commit comments

Comments
 (0)