From ac7a7afa0881e8ddeaaa1d3eb9ce92df107b8173 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Sat, 15 Aug 2026 22:28:02 +0200 Subject: [PATCH] Extract WorkoutSyncSession from workout_sync_service.dart (322 -> 218) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The one file on the list that could not be fixed by moving code, so the approach was agreed with the user first: extract a real collaborator rather than change the public surface. `push`, `pushManual` and `syncNow` were three near-identical bodies — same configured-or-bail guard, same GitHubClient/Firebase/MirrorStore construction, same getFileText/mergeLogs/syncLog round-trip, same catch-everything-and-report handler, same close-both finally. They differed in the record they merged in (one, one, none) and two strings. That shared body is now WorkoutSyncSession.run(), and the three methods are thin wrappers holding only their own guard and their own wording. Why a collaborator and not a `part` extension: extension methods dispatch statically. Four fakes in _home_test_fixtures.dart override `syncNow`, and manual_workout_screen_test.dart overrides `pushManual` and `readMergedManualPayloads` — with those in an extension the real implementations would run while the fakes sat unused, which is how four tests hung on pumpAndSettle in an earlier attempt. All four remain ordinary methods on WorkoutSyncService, so every override still binds. `successReason` and `failurePrefix` are passed in rather than derived from one another, so the exact strings the tests assert on ('pushed' / 'push failed', 'synced' / 'sync failed') stay under the callers' control. Unlike the part files, this is a real library with its own lcov SF: entry, so it has to carry its own coverage — it does. The total drops 2751 -> 2728 covered lines, which is the duplication the extraction removed. 402 tests pass; dart analyze clean; coverage 100%. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EveB813BQ8abAP68VSqZ5Y --- .../lib/services/workout_sync_service.dart | 172 ++++-------------- .../lib/services/workout_sync_session.dart | 138 ++++++++++++++ 2 files changed, 172 insertions(+), 138 deletions(-) create mode 100644 stronglift_replacement/workout_app/lib/services/workout_sync_session.dart diff --git a/stronglift_replacement/workout_app/lib/services/workout_sync_service.dart b/stronglift_replacement/workout_app/lib/services/workout_sync_service.dart index 0735735..f537301 100644 --- a/stronglift_replacement/workout_app/lib/services/workout_sync_service.dart +++ b/stronglift_replacement/workout_app/lib/services/workout_sync_service.dart @@ -13,6 +13,7 @@ import 'package:workout_app/services/firebase_backend.dart'; import 'package:workout_app/services/sync_device_id.dart'; import 'package:workout_app/services/sync_settings.dart'; import 'package:workout_app/services/sync_state_factory.dart'; +import 'package:workout_app/services/workout_sync_session.dart'; part 'workout_sync_service_read.dart'; @@ -118,64 +119,33 @@ class WorkoutSyncService { log('WorkoutSyncService.push skipped: $reason', level: 900); return const PushResult(pushed: false, reason: reason); } - - final github = GitHubClient( - owner: owner, - repo: repo, - token: settings.token, - httpClient: _httpClient, - ); - final firebase = await _openFirebase(); - final client = firebase == null - ? github - : MirrorStore(primary: firebase, mirror: github); - try { - final path = '$_pathPrefix/$currentSyncDeviceId/$_logFilename'; - final existingText = await client.getFileText(path); - final existingLog = existingText == null - ? {} - : _decode(existingText); - final record = Record( + return _session(settings.token).run( + operation: 'push', + successReason: 'pushed', + failurePrefix: 'push failed', + addition: Record( id: session.startTime.toIso8601String(), fields: { 'payload': (session.toJson(), Hlc.newTick(currentSyncDeviceId)), }, - ); - final localLog = mergeLogs(existingLog, {record.id: record}); - await syncLog( - client: client, - deviceId: currentSyncDeviceId, - legacyDeviceId: legacySyncDeviceId, - pathPrefix: _pathPrefix, - localLog: localLog, - encode: _encode, - decode: _decode, - // Without this every push re-uploads the whole log and every pull - // re-downloads every peer's, regardless of change -- the traffic the - // Firebase free tier's monthly budget depends on not happening. - stateStore: await _openStateStore(), - ); - return const PushResult(pushed: true, reason: 'pushed'); - } on Object catch (error, stackTrace) { - // Deliberately broad: a GitHubSyncError was the only case handled - // before, so a Firebase error, an auth failure or a dropped connection - // vanished with no trace at all. Still swallowed rather than rethrown - // -- a sync failure must not cost the user their finished workout -- - // but now it is reported, not hidden. - final reason = 'push failed: $error'; - log( - 'WorkoutSyncService.push failed', - level: 1000, - error: error, - stackTrace: stackTrace, - ); - return PushResult(pushed: false, reason: reason); - } finally { - github.close(); - firebase?.close(); - } + ), + ); } + /// Builds the collaborator that owns one round-trip's client lifecycle. + WorkoutSyncSession _session(String token) => WorkoutSyncSession( + owner: owner, + repo: repo, + token: token, + pathPrefix: _pathPrefix, + logFilename: _logFilename, + encode: _encode, + decode: _decode, + openFirebaseClient: _openFirebase, + openStateStore: _openStateStore, + httpClient: _httpClient, + ); + /// Runs a sync tick with no new record, purely to find out whether sync /// works right now. /// @@ -193,51 +163,16 @@ class WorkoutSyncService { log('WorkoutSyncService.syncNow skipped: $reason', level: 900); return const PushResult(pushed: false, reason: reason); } - - final github = GitHubClient( - owner: owner, - repo: repo, - token: settings.token, - httpClient: _httpClient, + // No addition: whatever this device already has, unchanged. The point is + // the round trip, not the payload. + return _session(settings.token).run( + operation: 'syncNow', + successReason: 'synced', + failurePrefix: 'sync failed', ); - final firebase = await _openFirebase(); - final client = firebase == null - ? github - : MirrorStore(primary: firebase, mirror: github); - try { - final path = '$_pathPrefix/$currentSyncDeviceId/$_logFilename'; - final existingText = await client.getFileText(path); - final existingLog = existingText == null - ? {} - : _decode(existingText); - await syncLog( - client: client, - deviceId: currentSyncDeviceId, - legacyDeviceId: legacySyncDeviceId, - pathPrefix: _pathPrefix, - // No new record: whatever this device already has, unchanged. The - // point is the round trip, not the payload. - localLog: existingLog, - encode: _encode, - decode: _decode, - stateStore: await _openStateStore(), - ); - return const PushResult(pushed: true, reason: 'synced'); - } on Object catch (error, stackTrace) { - final reason = 'sync failed: $error'; - log( - 'WorkoutSyncService.syncNow failed', - level: 1000, - error: error, - stackTrace: stackTrace, - ); - return PushResult(pushed: false, reason: reason); - } finally { - github.close(); - firebase?.close(); - } } + /// Pushes a pre-built manual-workout [record] to this device's log. /// /// Same never-throw, always-report contract as [push]. @@ -248,51 +183,12 @@ class WorkoutSyncService { log('WorkoutSyncService.pushManual skipped: $reason', level: 900); return const PushResult(pushed: false, reason: reason); } - - final github = GitHubClient( - owner: owner, - repo: repo, - token: settings.token, - httpClient: _httpClient, + return _session(settings.token).run( + operation: 'pushManual', + successReason: 'pushed', + failurePrefix: 'push failed', + addition: record, ); - final firebase = await _openFirebase(); - final client = firebase == null - ? github - : MirrorStore(primary: firebase, mirror: github); - try { - final path = '$_pathPrefix/$currentSyncDeviceId/$_logFilename'; - final existingText = await client.getFileText(path); - final existingLog = existingText == null - ? {} - : _decode(existingText); - final localLog = mergeLogs(existingLog, {record.id: record}); - await syncLog( - client: client, - deviceId: currentSyncDeviceId, - legacyDeviceId: legacySyncDeviceId, - pathPrefix: _pathPrefix, - localLog: localLog, - encode: _encode, - decode: _decode, - // Without this every push re-uploads the whole log and every pull - // re-downloads every peer's, regardless of change -- the traffic the - // Firebase free tier's monthly budget depends on not happening. - stateStore: await _openStateStore(), - ); - return const PushResult(pushed: true, reason: 'pushed'); - } on Object catch (error, stackTrace) { - final reason = 'push failed: $error'; - log( - 'WorkoutSyncService.pushManual failed', - level: 1000, - error: error, - stackTrace: stackTrace, - ); - return PushResult(pushed: false, reason: reason); - } finally { - github.close(); - firebase?.close(); - } } /// Returns every device's manual-workout payloads, merged and deduped by id diff --git a/stronglift_replacement/workout_app/lib/services/workout_sync_session.dart b/stronglift_replacement/workout_app/lib/services/workout_sync_session.dart new file mode 100644 index 0000000..2c25260 --- /dev/null +++ b/stronglift_replacement/workout_app/lib/services/workout_sync_session.dart @@ -0,0 +1,138 @@ +/// One sync round-trip: open the backends, merge a log, close them again. +library; + +import 'dart:developer'; + +import 'package:crdt_sync/crdt_sync.dart'; +import 'package:http/http.dart' as http; +import 'package:workout_app/services/sync_device_id.dart'; +import 'package:workout_app/services/workout_sync_service.dart'; + +/// Runs a single sync tick against GitHub, Firebase, or both. +/// +/// `push`, `pushManual` and `syncNow` on [WorkoutSyncService] were three +/// near-identical bodies: the same configured-or-bail guard, the same +/// GitHub/Firebase/[MirrorStore] construction, the same try/catch/finally, and +/// the same never-throw-always-report contract. Only the log they submit and +/// the success wording differed. +/// +/// This is a collaborator the service delegates to rather than a `part` +/// extension, deliberately. Test fakes bind to `push` / `pushManual` / +/// `syncNow` with `extends WorkoutSyncService`, and extension methods dispatch +/// statically — moving those into an extension would mean the real +/// implementation ran while the fake sat unused. Keeping them as thin methods +/// on the class preserves every override; only the shared body moved here. +class WorkoutSyncSession { + /// Creates a session bound to one repo and one pair of backends. + const WorkoutSyncSession({ + required this.owner, + required this.repo, + required this.token, + required this.pathPrefix, + required this.logFilename, + required this.encode, + required this.decode, + required this.openFirebaseClient, + required this.openStateStore, + this.httpClient, + }); + + /// The repo owner/org to sync with. + final String owner; + + /// The repo name to sync with. + final String repo; + + /// The GitHub token. Empty when only Firebase is configured, which is what + /// [SyncSettings] reports and what the mirror treats as unauthenticated. + final String token; + + /// Directory prefix every device's log lives under. + final String pathPrefix; + + /// Filename of a device's log within its directory. + final String logFilename; + + /// Serializes a log for storage. + final String Function(Log log) encode; + + /// Parses a stored log. + final Log Function(String text) decode; + + /// Opens the Firebase backend, or returns null when not set up. + final Future Function() openFirebaseClient; + + /// Opens the revision cache that lets an unchanged peer be skipped. + final Future Function() openStateStore; + + /// Overrides the HTTP client; tests point this at a mock. + final http.Client? httpClient; + + /// Runs one tick, merging [addition] into this device's log if given. + /// + /// Pass null for [addition] to round-trip the existing log unchanged — that + /// is how a connectivity check gets a real answer without inventing a + /// record to push. + /// + /// Never throws. [operation] names the caller in log lines; [successReason] + /// and [failurePrefix] are passed in rather than derived, so the three + /// callers keep the exact strings their tests assert on. + Future run({ + required String operation, + required String successReason, + required String failurePrefix, + Record? addition, + }) async { + final github = GitHubClient( + owner: owner, + repo: repo, + token: token, + httpClient: httpClient, + ); + final firebase = await openFirebaseClient(); + final client = firebase == null + ? github + : MirrorStore(primary: firebase, mirror: github); + try { + final path = '$pathPrefix/$currentSyncDeviceId/$logFilename'; + final existingText = await client.getFileText(path); + final existingLog = existingText == null + ? {} + : decode(existingText); + final localLog = addition == null + ? existingLog + : mergeLogs(existingLog, {addition.id: addition}); + await syncLog( + client: client, + deviceId: currentSyncDeviceId, + legacyDeviceId: legacySyncDeviceId, + pathPrefix: pathPrefix, + localLog: localLog, + encode: encode, + decode: decode, + // Without this every push re-uploads the whole log and every pull + // re-downloads every peer's, regardless of change -- the traffic the + // Firebase free tier's monthly budget depends on not happening. + stateStore: await openStateStore(), + ); + return PushResult(pushed: true, reason: successReason); + } on Object catch (error, stackTrace) { + // Deliberately broad: a GitHubSyncError was the only case handled + // before, so a Firebase error, an auth failure or a dropped connection + // vanished with no trace at all. Still swallowed rather than rethrown + // -- a sync failure must not cost the user their finished workout -- + // but now it is reported, not hidden. + final reason = '$failurePrefix: $error'; + log( + 'WorkoutSyncService.$operation failed', + level: 1000, + error: error, + stackTrace: stackTrace, + ); + return PushResult(pushed: false, reason: reason); + } finally { + github.close(); + firebase?.close(); + } + } +}