Skip to content

Commit 0c59565

Browse files
HermesCoderwernerhp
authored andcommitted
refactor(puzzle): isolate offline queue fill from the solve loop
Address review feedback on #3454. The previous approach tangled a background fill into the solve/sync path, where the single-flight guard was inert (the controller builds a fresh PuzzleService per call) and a within-pass stale solve snapshot could clobber offline results. - Revert the sync path to the original single-request logic, unchanged. - Add PuzzleQueueFiller: a stable NotifierProvider that fills the offline queue to the configured count with sequential, single-flight, select-only requests. Re-reads storage each pass, dedups by id, never submits solves, and is safe offline. - Trigger the fill as a one-time action on the offline-count setting change and on difficulty change, with a spinner on the settings tile while running. - Raise the minimum offline queue to 100 (and the default to 100) per review. Tests: PuzzleQueueFiller covered in isolation (fill-to-count over multiple requests, deficit-only, no-op when full, server-exhausted stop, offline-safe, select-only, single-flight). Screen test asserts the settings change triggers the fill. Removed the obsolete background-fill service tests.
1 parent 31906d9 commit 0c59565

8 files changed

Lines changed: 498 additions & 262 deletions

File tree

lib/src/model/puzzle/puzzle_controller.dart

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import 'package:lichess_mobile/src/model/common/uci.dart';
1515
import 'package:lichess_mobile/src/model/puzzle/puzzle.dart';
1616
import 'package:lichess_mobile/src/model/puzzle/puzzle_difficulty.dart';
1717
import 'package:lichess_mobile/src/model/puzzle/puzzle_preferences.dart';
18+
import 'package:lichess_mobile/src/model/puzzle/puzzle_queue_filler.dart';
1819
import 'package:lichess_mobile/src/model/puzzle/puzzle_repository.dart';
1920
import 'package:lichess_mobile/src/model/puzzle/puzzle_service.dart';
2021
import 'package:lichess_mobile/src/model/puzzle/puzzle_session.dart';
@@ -205,6 +206,15 @@ class PuzzleController extends Notifier<PuzzleState> {
205206

206207
state = state.copyWith(isChangingDifficulty: false);
207208

209+
// Difficulty invalidates the queue, so resetBatch only refetched one batch
210+
// (capped at 50 by the server). Top the queue back up to the configured
211+
// count in the background, matching the settings-change behaviour.
212+
unawaited(
213+
ref
214+
.read(puzzleQueueFillerProvider.notifier)
215+
.fill(userId: initialContext.userId, angle: initialContext.angle),
216+
);
217+
208218
return nextPuzzle;
209219
}
210220

lib/src/model/puzzle/puzzle_preferences.dart

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,13 @@ class PuzzlePreferences extends Notifier<PuzzlePrefs> with SessionPreferencesSto
5656
}
5757

5858
/// Minimum number of puzzles kept in the offline queue.
59-
const kMinOfflinePuzzles = 50;
59+
const kMinOfflinePuzzles = 100;
6060

6161
/// Maximum number of puzzles kept in the offline queue.
6262
const kMaxOfflinePuzzles = 500;
6363

6464
/// Available choices for the size of the offline puzzle queue.
65-
const kOfflinePuzzlesChoices = [50, 100, 150, 200, 300, 500];
65+
const kOfflinePuzzlesChoices = [100, 150, 200, 300, 500];
6666

6767
@Freezed(fromJson: true, toJson: true)
6868
sealed class PuzzlePrefs with _$PuzzlePrefs implements Serializable {
@@ -80,16 +80,16 @@ sealed class PuzzlePrefs with _$PuzzlePrefs implements Serializable {
8080
@Default(true) bool rated,
8181

8282
/// Number of puzzles to keep downloaded in the offline queue.
83-
/// Defaults to `50`.
84-
@Default(50) int nbOfflinePuzzles,
83+
/// Defaults to `100`.
84+
@Default(100) int nbOfflinePuzzles,
8585
}) = _PuzzlePrefs;
8686

8787
factory PuzzlePrefs.defaults({UserId? id}) => PuzzlePrefs(
8888
id: id,
8989
difficulty: PuzzleDifficulty.normal,
9090
autoNext: false,
9191
rated: true,
92-
nbOfflinePuzzles: 50,
92+
nbOfflinePuzzles: 100,
9393
);
9494

9595
factory PuzzlePrefs.fromJson(Map<String, dynamic> json) => _$PuzzlePrefsFromJson(json);
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
import 'dart:math' show max;
2+
3+
import 'package:fast_immutable_collections/fast_immutable_collections.dart';
4+
import 'package:flutter_riverpod/flutter_riverpod.dart';
5+
import 'package:lichess_mobile/src/model/common/id.dart';
6+
import 'package:lichess_mobile/src/model/puzzle/puzzle_angle.dart';
7+
import 'package:lichess_mobile/src/model/puzzle/puzzle_batch_storage.dart';
8+
import 'package:lichess_mobile/src/model/puzzle/puzzle_preferences.dart';
9+
import 'package:lichess_mobile/src/model/puzzle/puzzle_repository.dart';
10+
import 'package:lichess_mobile/src/model/puzzle/puzzle_theme.dart';
11+
import 'package:lichess_mobile/src/network/http.dart';
12+
import 'package:logging/logging.dart';
13+
import 'package:meta/meta.dart';
14+
15+
/// The server caps each batch request at 50 puzzles regardless of the requested
16+
/// `nb` (`nb.atMost(50)` in lila's `Puzzle` controller), so a queue larger than
17+
/// this is filled with several sequential requests, each asking for the current
18+
/// deficit and receiving at most 50.
19+
20+
/// Whether a one-time offline-queue fill is currently running.
21+
///
22+
/// This is a top-level (stable) provider, not per-[PuzzleService] state, so the
23+
/// single-flight guard actually holds: the puzzle service factory builds a fresh
24+
/// service on every read, but this notifier is a single instance.
25+
final puzzleQueueFillerProvider = NotifierProvider<PuzzleQueueFiller, bool>(
26+
PuzzleQueueFiller.new,
27+
name: 'PuzzleQueueFillerProvider',
28+
);
29+
30+
class PuzzleQueueFiller extends Notifier<bool> {
31+
final Logger _log = Logger('PuzzleQueueFiller');
32+
33+
@override
34+
bool build() => false;
35+
36+
/// Fills the offline queue for [angle] up to the configured
37+
/// `nbOfflinePuzzles`, one server batch at a time.
38+
///
39+
/// This is a one-time, select-only action triggered when the offline puzzle
40+
/// count or the difficulty changes. It never submits solves, so it can't race
41+
/// with the solve/sync path over the stored `solved` list. Re-reads storage
42+
/// on every pass and merges by puzzle id so a solve made while the fill runs
43+
/// is never clobbered. Guarded by [state] so overlapping calls can't
44+
/// double-fetch. Never throws: network and storage failures stop the fill.
45+
Future<void> fill({
46+
required UserId? userId,
47+
PuzzleAngle angle = const PuzzleTheme(PuzzleThemeKey.mix),
48+
@visibleForTesting int? queueLengthOverride,
49+
}) async {
50+
if (state) return;
51+
state = true;
52+
try {
53+
final queueLength =
54+
queueLengthOverride ?? ref.read(puzzlePreferencesProvider).nbOfflinePuzzles;
55+
final difficulty = ref.read(puzzlePreferencesProvider).difficulty;
56+
final batchStorage = await ref.read(puzzleBatchStorageProvider.future);
57+
58+
// Tracks the stored queue length across passes. Stop when a pass fails to
59+
// grow the queue (server exhausted or a silent no-op write), not when it
60+
// shrinks: a solve made mid-fill lowers the count and legitimately widens
61+
// the deficit, so that pass should still top up.
62+
int lastLength = -1;
63+
while (true) {
64+
final current = await batchStorage.fetch(userId: userId, angle: angle);
65+
final unsolved = current?.unsolved ?? IList(const []);
66+
67+
if (unsolved.length == lastLength) break;
68+
lastLength = unsolved.length;
69+
70+
final deficit = max(0, queueLength - unsolved.length);
71+
if (deficit <= 0) break;
72+
73+
final PuzzleBatchResponse response;
74+
try {
75+
response = await ref.withClient(
76+
(client) => PuzzleRepository(
77+
client,
78+
).selectBatch(nb: deficit, angle: angle, difficulty: difficulty),
79+
);
80+
} catch (e, st) {
81+
// Offline or server error: stop the fill, keep what we have.
82+
_log.warning('Offline puzzle queue fill failed', e, st);
83+
break;
84+
}
85+
86+
// Server has no more puzzles: stop rather than spin forever below an
87+
// unreachable `queueLength`.
88+
if (response.puzzles.isEmpty) break;
89+
90+
// Skip ids already stored so a re-read after a mid-fill solve can't
91+
// reintroduce a duplicate.
92+
final existingIds = unsolved.map((p) => p.puzzle.id).toISet();
93+
final additions = response.puzzles.where((p) => !existingIds.contains(p.puzzle.id));
94+
if (additions.isEmpty) break;
95+
96+
await batchStorage.save(
97+
userId: userId,
98+
angle: angle,
99+
data: PuzzleBatch(
100+
solved: current?.solved ?? IList(const []),
101+
unsolved: IList([...unsolved, ...additions]),
102+
),
103+
);
104+
}
105+
} finally {
106+
state = false;
107+
}
108+
}
109+
}

0 commit comments

Comments
 (0)