From 7564a6569fda280c9c8064fd86049cdbd1a00a14 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Thu, 16 Jul 2026 17:58:41 +0200 Subject: [PATCH] Surface sync outcomes so devices can't silently diverge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The desktop and phone split-brained for 5 days (39 vs 30 notes) after the crdt_sync cutover because one device kept running the pre-migration build — and nothing in the UI ever said a sync had failed or skipped a peer: - SyncResult now reports peer files that were listed but couldn't be fetched/decoded (skippedFiles) instead of dropping them silently. - Auto-sync no longer swallows errors: every sync (manual or automatic) records a persisted last-sync status shown in a status line under the editor, with failures in the error color. - Desktop actually background-syncs now: focus loss (inactive/hidden, debounced 5s) triggers a sync — Flutter Linux rarely delivers the `paused` event the old code relied on. `paused` still syncs immediately for Android. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01K5C5jZ2c4mTaGedK7C1dDv --- lib/sync/sync_service.dart | 32 ++++++-- lib/ui/capture_screen.dart | 145 ++++++++++++++++++++++++++++++---- test/capture_screen_test.dart | 108 ++++++++++++++++++++++++- test/sync_service_test.dart | 23 ++++++ 4 files changed, 281 insertions(+), 27 deletions(-) diff --git a/lib/sync/sync_service.dart b/lib/sync/sync_service.dart index f6a392c..cadae3a 100644 --- a/lib/sync/sync_service.dart +++ b/lib/sync/sync_service.dart @@ -4,7 +4,11 @@ import '../data/note_repository.dart'; /// Outcome of a sync run, for surfacing in the UI. class SyncResult { - const SyncResult({required this.mergedDevices, required this.pushed}); + const SyncResult({ + required this.mergedDevices, + required this.pushed, + this.skippedFiles = const [], + }); /// How many other devices' logs were pulled and merged. final int mergedDevices; @@ -12,9 +16,15 @@ class SyncResult { /// Whether this device pushed its own log. final bool pushed; + /// Peer files that were listed but could not be fetched or decoded + /// (vanished, corrupt, or foreign format) and were left out of the merge. + /// Surfaced so a device silently dropping out of sync is visible. + final List skippedFiles; + @override String toString() => - 'SyncResult(mergedDevices: $mergedDevices, pushed: $pushed)'; + 'SyncResult(mergedDevices: $mergedDevices, pushed: $pushed, ' + 'skippedFiles: $skippedFiles)'; } /// Synchronises a [NoteRepository] with a GitHub repo used as dumb storage. @@ -43,12 +53,18 @@ class SyncService { // 1. Pull: list all device log files, merge everyone else's. final names = await github.listDirectory(notesDir); var merged = 0; + final skipped = []; for (final name in names) { if (name == ownFileName) continue; // our own file; skip it. final text = await github.getFileText('$notesDir/$name'); - if (text == null) continue; - final remote = _decodeLog(text); - if (remote == null) continue; // Skip a corrupt/foreign file. + final remote = text == null ? null : _decodeLog(text); + if (remote == null) { + // A vanished or undecodable peer file must not abort the sync, but + // it must not vanish silently either — that's how a device drops + // out of sync unnoticed. Report it in the result instead. + skipped.add(name); + continue; + } await repo.importLog(remote); merged++; } @@ -61,7 +77,11 @@ class SyncService { message: 'sync: $nodeId @ ${DateTime.now().toUtc().toIso8601String()}', ); - return SyncResult(mergedDevices: merged, pushed: true); + return SyncResult( + mergedDevices: merged, + pushed: true, + skippedFiles: skipped, + ); } /// Parses a remote note log, returning `null` for a corrupt or wrong-shape diff --git a/lib/ui/capture_screen.dart b/lib/ui/capture_screen.dart index ac39999..27db511 100644 --- a/lib/ui/capture_screen.dart +++ b/lib/ui/capture_screen.dart @@ -4,6 +4,7 @@ import 'dart:io'; import 'package:flutter/material.dart'; import 'package:http/http.dart' as http; import 'package:path_provider/path_provider.dart'; +import 'package:shared_preferences/shared_preferences.dart'; import 'package:uuid/uuid.dart'; import '../data/note.dart'; @@ -43,6 +44,10 @@ class CaptureScreen extends StatefulWidget { /// file-backed instance is created); tests pass a fake with in-memory IO. final LocalBackup? localBackup; + /// How long focus must stay lost before a background sync fires. Public so + /// tests can pump exactly past it. + static const autoSyncDebounce = Duration(seconds: 5); + @override State createState() => _CaptureScreenState(); } @@ -85,6 +90,19 @@ class _CaptureScreenState extends State SyncSettings? _settings; bool _syncing = false; + /// Outcome of the most recent sync attempt (manual or automatic), driving + /// the tiny status line under the editor. Persisted so a failure that + /// happened just before the app closed is still visible on next launch. + final ValueNotifier _lastSync = ValueNotifier(null); + + /// Debounces focus-loss sync triggers so desktop alt-tab flicker doesn't + /// hammer GitHub with a request per focus change. + Timer? _autoSyncDebounce; + + static const _kLastSyncTime = 'sync.lastTime'; + static const _kLastSyncOk = 'sync.lastOk'; + static const _kLastSyncDetail = 'sync.lastDetail'; + /// Hides the Priority/Status row while the editor's own bare-guided chrome /// (template/mode selectors) is also hidden, so the two stay in lockstep. bool _chromeVisible = true; @@ -106,6 +124,7 @@ class _CaptureScreenState extends State // fires on writes), so a deleted/stale BACKLOG.md is regenerated even if // the user makes no edits this session. _localBackup.schedule(); + _restoreSyncStatus(); SyncSettings.load().then((s) { if (!mounted) return; setState(() => _settings = s); @@ -116,12 +135,49 @@ class _CaptureScreenState extends State @override void dispose() { WidgetsBinding.instance.removeObserver(this); + _autoSyncDebounce?.cancel(); _changesSub?.cancel(); _localBackup.dispose(); _lastSavedAt.dispose(); + _lastSync.dispose(); super.dispose(); } + /// Reloads the persisted last-sync outcome so the status line survives a + /// restart (a failure just before closing must not disappear). + Future _restoreSyncStatus() async { + final prefs = await SharedPreferences.getInstance(); + final time = DateTime.tryParse(prefs.getString(_kLastSyncTime) ?? ''); + if (!mounted || time == null || _lastSync.value != null) return; + _lastSync.value = SyncStatus( + time: time, + ok: prefs.getBool(_kLastSyncOk) ?? true, + detail: prefs.getString(_kLastSyncDetail) ?? '', + ); + } + + /// Records a sync outcome in the UI notifier and persists it. + Future _recordSyncStatus({ + required bool ok, + required String detail, + }) async { + final status = SyncStatus(time: DateTime.now(), ok: ok, detail: detail); + if (mounted) _lastSync.value = status; + final prefs = await SharedPreferences.getInstance(); + await prefs.setString(_kLastSyncTime, status.time.toIso8601String()); + await prefs.setBool(_kLastSyncOk, ok); + await prefs.setString(_kLastSyncDetail, detail); + } + + /// Human-readable one-liner for a sync outcome, shared by the snackbar and + /// the status line. Skipped files are called out so a peer device silently + /// dropping out of the merge is visible. + static String _describe(SyncResult result) { + final skipped = result.skippedFiles.length; + return 'merged ${result.mergedDevices} device(s)' + '${skipped == 0 ? '' : ', skipped $skipped unreadable file(s)'}'; + } + /// Restores notes from the local backup file, but only into an empty DB so a /// stale backup never clobbers existing notes (merge-by-id stays safe too). Future _recoverFromBackup() async { @@ -161,12 +217,23 @@ class _CaptureScreenState extends State @override void didChangeAppLifecycleState(AppLifecycleState state) { // Push on background so the remote (the durable store) stays near-current. - if (state == AppLifecycleState.paused) _autoSync(); + if (state == AppLifecycleState.paused) { + // Immediate: Android may kill the app before a debounce timer fires. + _autoSyncDebounce?.cancel(); + _autoSync(); + } else if (state == AppLifecycleState.inactive || + state == AppLifecycleState.hidden) { + // Focus loss — the only background signal Flutter Linux delivers + // reliably (`paused` rarely arrives on desktop). Debounced so alt-tab + // flicker doesn't fire a request per focus change. + _autoSyncDebounce?.cancel(); + _autoSyncDebounce = Timer(CaptureScreen.autoSyncDebounce, _autoSync); + } } - /// Best-effort background sync: silent, skips when unconfigured, and never - /// overlaps itself. Failures are swallowed — the manual Sync button is the - /// place that surfaces errors. + /// Best-effort background sync: no snackbar, skips when unconfigured, and + /// never overlaps itself. Failures are not swallowed silently — the outcome + /// lands in the status line so drift can't go unnoticed for days. Future _autoSync() async { final settings = _settings; if (_autoSyncing || settings == null || !settings.isConfigured) return; @@ -178,9 +245,12 @@ class _CaptureScreenState extends State httpClient: widget.httpClient, ); try { - await _syncService.sync(widget.repository, client); - } catch (_) { - // Best-effort: ignore (offline, transient GitHub errors, etc.). + final result = await _syncService.sync(widget.repository, client); + await _recordSyncStatus(ok: true, detail: _describe(result)); + } catch (e) { + // Offline or a transient GitHub error: still no snackbar (this path + // must never interrupt capture), but the status line shows it. + await _recordSyncStatus(ok: false, detail: '$e'); } finally { client.close(); _autoSyncing = false; @@ -233,8 +303,11 @@ class _CaptureScreenState extends State ); try { final result = await _syncService.sync(widget.repository, client); - _showSnack('Synced: merged ${result.mergedDevices} device(s)'); + final detail = _describe(result); + await _recordSyncStatus(ok: true, detail: detail); + _showSnack('Synced: $detail'); } catch (e) { + await _recordSyncStatus(ok: false, detail: '$e'); _showSnack('Sync failed: $e'); } finally { client.close(); @@ -419,17 +492,37 @@ class _CaptureScreenState extends State ), ), const SizedBox(height: 8), - // Leave room so the Save FAB doesn't cover the save indicator. + // Leave room so the Save FAB doesn't cover the indicators. Padding( padding: const EdgeInsets.only(right: 96), - child: ValueListenableBuilder( - valueListenable: _lastSavedAt, - builder: (context, savedAt, _) => Text( - savedAt == null - ? 'Autosaves as you type' - : 'Saved locally at ${_formatTime(savedAt)}', - style: theme.textTheme.bodySmall, - ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + ValueListenableBuilder( + valueListenable: _lastSavedAt, + builder: (context, savedAt, _) => Text( + savedAt == null + ? 'Autosaves as you type' + : 'Saved locally at ${_formatTime(savedAt)}', + style: theme.textTheme.bodySmall, + ), + ), + ValueListenableBuilder( + valueListenable: _lastSync, + builder: (context, status, _) { + if (status == null) return const SizedBox.shrink(); + return Text( + '${status.ok ? 'Synced' : 'Sync failed'} at ' + '${_formatTime(status.time)} · ${status.detail}', + style: theme.textTheme.bodySmall?.copyWith( + color: status.ok ? null : theme.colorScheme.error, + ), + maxLines: 1, + overflow: TextOverflow.ellipsis, + ); + }, + ), + ], ), ), ], @@ -452,6 +545,24 @@ class _CaptureScreenState extends State } } +/// Outcome of a sync attempt, shown in the capture screen's status line. +class SyncStatus { + const SyncStatus({ + required this.time, + required this.ok, + required this.detail, + }); + + /// When the attempt finished. + final DateTime time; + + /// Whether the sync succeeded. + final bool ok; + + /// One-line summary: merged/skipped counts on success, the error otherwise. + final String detail; +} + /// A compact labelled dropdown for picking an enum value (priority/status). /// /// Generic over the enum type [T] so the same control drives both pickers diff --git a/test/capture_screen_test.dart b/test/capture_screen_test.dart index 4d55847..a5a4f93 100644 --- a/test/capture_screen_test.dart +++ b/test/capture_screen_test.dart @@ -1,3 +1,5 @@ +import 'dart:convert'; + import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; @@ -234,7 +236,40 @@ void main() { await tester.pump(); await tester.pump(); - expect(find.textContaining('Sync failed'), findsOneWidget); + // 'Sync failed:' is the snackbar; the status line says 'Sync failed at'. + expect(find.textContaining('Sync failed:'), findsOneWidget); + expect(find.textContaining('Sync failed at'), findsOneWidget); + }); + + testWidgets('manual sync reports peer files it could not decode', ( + tester, + ) async { + // One peer file exists but holds garbage → merged 0, skipped 1. + final mock = MockClient((req) async { + if (req.method == 'PUT') return http.Response('{}', 200); + if (!req.url.path.contains('/contents/')) return http.Response('{}', 200); + if (req.url.path.endsWith('/todo-sync/notes')) { + return http.Response('[{"name": "bad.json"}]', 200); + } + if (req.url.path.endsWith('/bad.json')) { + return http.Response( + '{"content": "${base64Encode(utf8.encode('{not a log'))}"}', + 200, + ); + } + return http.Response('', 404); // own-file sha probe → create fresh + }); + await pumpCapture(tester, prefs: configuredPrefs, httpClient: mock); + + await tester.tap(find.byTooltip('Sync')); + await tester.pump(); + await tester.pump(); + await tester.pump(); + + expect( + find.textContaining('skipped 1 unreadable file(s)'), + findsWidgets, // snackbar and status line both call it out + ); }); testWidgets('returning from settings adopts the saved configuration', ( @@ -311,14 +346,79 @@ void main() { expect(methods, contains('PUT')); }); - testWidgets('auto-sync failure is silent (no snackbar)', (tester) async { + testWidgets('auto-sync failure shows no snackbar but flags the status line', ( + tester, + ) async { final mock = MockClient((_) async => throw Exception('offline')); await pumpCapture(tester, prefs: configuredPrefs, httpClient: mock); await tester.pump(); await tester.pump(); - expect(find.textContaining('Sync failed'), findsNothing); - expect(find.textContaining('Synced'), findsNothing); + // Capture must never be interrupted by a snackbar… + expect(find.byType(SnackBar), findsNothing); + // …but the failure is no longer swallowed: the status line shows it. + expect(find.textContaining('Sync failed at'), findsOneWidget); + expect(find.textContaining('offline'), findsOneWidget); + }); + + testWidgets('auto-sync success shows when and what was merged', ( + tester, + ) async { + final methods = []; + await pumpCapture( + tester, + prefs: configuredPrefs, + httpClient: recordingMock(methods), + ); + await tester.pump(); + await tester.pump(); + + expect(find.textContaining('Synced at'), findsOneWidget); + expect(find.textContaining('merged 0 device(s)'), findsOneWidget); + }); + + testWidgets('restores the last sync outcome on launch', (tester) async { + // Unconfigured (no token) so no launch auto-sync overwrites the restored + // status. A failure recorded on a previous run must still be visible. + await pumpCapture( + tester, + prefs: { + 'sync.lastTime': DateTime(2026, 7, 16, 12).toIso8601String(), + 'sync.lastOk': false, + 'sync.lastDetail': 'Exception: rate limited', + }, + ); + await tester.pump(); + + expect(find.textContaining('Sync failed at 12:00:00'), findsOneWidget); + expect(find.textContaining('rate limited'), findsOneWidget); + }); + + testWidgets('losing focus triggers a debounced auto-sync', (tester) async { + final methods = []; + await pumpCapture( + tester, + prefs: configuredPrefs, + httpClient: recordingMock(methods), + ); + await tester.pump(); + await tester.pump(); + methods.clear(); + + tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.inactive); + // A second signal (hidden also counts as focus loss) resets the debounce + // instead of double-firing. + tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.hidden); + await tester.pump( + CaptureScreen.autoSyncDebounce - const Duration(seconds: 1), + ); + expect(methods, isEmpty); // still inside the debounce window + + await tester.pump(const Duration(seconds: 1)); + await tester.pump(); + await tester.pump(); + + expect(methods, contains('PUT')); }); testWidgets('recovers notes from the local backup into an empty DB', ( diff --git a/test/sync_service_test.dart b/test/sync_service_test.dart index caed892..4444d1f 100644 --- a/test/sync_service_test.dart +++ b/test/sync_service_test.dart @@ -128,5 +128,28 @@ void main() { final result = await const SyncService().sync(local, _github(httpClient)); expect(result.mergedDevices, 0); // both bad files skipped expect(putCalls, hasLength(1)); + // The skips are reported, not silent: that's how a peer device dropping + // out of the merge becomes visible in the UI. + expect(result.skippedFiles, ['bad1.json', 'bad2.json']); + expect(result.toString(), contains('bad1.json')); + }); + + test('a listed-but-unfetchable peer file is reported as skipped', () async { + // The file appears in the directory listing but its GET 404s (deleted + // between list and fetch, or contents unavailable). + final (:httpClient, :putCalls) = _mockGitHub( + contentResponses: { + 'todo-sync/notes': _response(200, [ + {'name': 'gone.json'}, + ]), + }, + ); + final local = await NoteRepository.openInMemory(nodeId: 'localNode'); + addTearDown(local.close); + + final result = await const SyncService().sync(local, _github(httpClient)); + expect(result.mergedDevices, 0); + expect(result.skippedFiles, ['gone.json']); + expect(putCalls, hasLength(1)); // the push still happened }); }