From fbbd822cdbe323fe5719f9b457575b81ebc21af4 Mon Sep 17 00:00:00 2001 From: r3econ Date: Tue, 14 Apr 2026 22:58:15 +0200 Subject: [PATCH] Link to daily puzzle screen from puzzle widget (#2984) * Link to puzzle screen in the app from puzzle widget * Extract constants * Fix build problem in widget * Fetch first daily to avoid extra request; add tests --------- Co-authored-by: Vincent Velociter --- CLAUDE.md | 35 +++++ .../DailyPuzzleEntry.swift | 15 +- .../Views/DailyPuzzleWidgetView.swift | 3 +- ios/LichessWidgets/Deeplinks.swift | 18 ++- lib/src/app_links_service.dart | 58 ++++++++ test/app_links_service_test.dart | 129 ++++++++++++++++++ 6 files changed, 246 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5975f806b..2886a6ac1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,6 +63,41 @@ flutter test test/model/engine/engine_test.dart flutter test test/model/engine/engine_test.dart --name "test name" ``` +### Testing Strategy: Prefer HTTP Mocking Over Provider Overrides + +**Always mock at the HTTP layer** (override `httpClientFactoryProvider`) rather than overriding Riverpod providers directly. Reasons: + +1. **`autoDispose` + `ref.read` interaction**: Many providers are `FutureProvider.autoDispose` and use `ref.withClientCacheFor` which calls `keepAlive()` to prevent premature disposal. Overriding the provider directly bypasses `keepAlive()`, so the provider can be disposed before its future resolves — causing silent test failures where navigation never happens. + +2. **Tests provider logic, not mocks**: Most providers contain real logic (caching, fallback, data transformation) that should be exercised in tests. Replacing a provider with `(_) async => fakeValue` skips all that logic. + +**Pattern to use:** +```dart +overrides: { + httpClientFactoryProvider: httpClientFactoryProvider.overrideWith((ref) { + return FakeHttpClientFactory( + () => MockClient((request) async { + if (request.url.path == '/api/puzzle/daily') { + return http.Response(mockDailyPuzzleResponse, 200); + } + return http.Response('', 404); + }), + ); + }), +}, +``` + +Direct provider overrides are acceptable for **non-network providers** (repositories backed by mocks, services with no HTTP, etc.) where the provider has no `keepAlive` dependency and the override doesn't skip meaningful logic. + +### Analysis Rules (CRITICAL) + +**Always run `flutter analyze` on every file you edit, including test files, before finishing.** + +Two rules the analyzer enforces that are easy to miss: + +- **`const` constructors**: use `const` (not `final`) when constructing a const-capable class. The analyzer will flag `prefer_const_constructors`. This applies everywhere, including test files. +- **No leading underscores for local identifiers**: local variables and functions must not start with `_`. Reserve `_` for library-private top-level or class members. + ### Code Quality Checks ```bash # Static analysis diff --git a/ios/LichessWidgets/Daily Puzzle Widget/DailyPuzzleEntry.swift b/ios/LichessWidgets/Daily Puzzle Widget/DailyPuzzleEntry.swift index 8b216342a..28fb3d7cd 100644 --- a/ios/LichessWidgets/Daily Puzzle Widget/DailyPuzzleEntry.swift +++ b/ios/LichessWidgets/Daily Puzzle Widget/DailyPuzzleEntry.swift @@ -16,13 +16,14 @@ struct DailyPuzzleEntry: TimelineEntry { return parts[1] == "w" } - var puzzleURL: URL? { - if let id = puzzleId { - return LichessAppGroup.lichessURL(path: "/training/\(id)") - } else { - return LichessAppGroup.lichessURL(path: "/training/daily") - } - } + /// Custom-scheme deeplink handled by the Flutter app to open the native + /// daily-puzzle screen (titled "Daily Puzzle"). See `AppLinksService`. + /// + /// Includes the specific puzzle id when known so the app opens the same + /// puzzle the widget is showing (the widget caches the daily puzzle for up + /// to 6 hours, so a plain `/training/daily` deeplink could otherwise open + /// a different puzzle than the one tapped). + var puzzleURL: URL? { dailyPuzzleDeeplink(puzzleId: puzzleId) } /// A recognisable position (Italian game after initial moves) /// used as placeholder while the real puzzle loads. diff --git a/ios/LichessWidgets/Daily Puzzle Widget/Views/DailyPuzzleWidgetView.swift b/ios/LichessWidgets/Daily Puzzle Widget/Views/DailyPuzzleWidgetView.swift index 1d219834a..4a69694ec 100644 --- a/ios/LichessWidgets/Daily Puzzle Widget/Views/DailyPuzzleWidgetView.swift +++ b/ios/LichessWidgets/Daily Puzzle Widget/Views/DailyPuzzleWidgetView.swift @@ -47,8 +47,7 @@ struct DailyPuzzleWidgetView: View { ) } .padding(.horizontal, DailyPuzzleWidgetLayout.horizontalPadding) - .padding(.top, DailyPuzzleWidgetLayout.topPadding) - .padding(.bottom, DailyPuzzleWidgetLayout.bottomPadding) + .padding(.vertical, DailyPuzzleWidgetLayout.verticalPadding) } @ViewBuilder diff --git a/ios/LichessWidgets/Deeplinks.swift b/ios/LichessWidgets/Deeplinks.swift index 31a171931..6beb0d39a 100644 --- a/ios/LichessWidgets/Deeplinks.swift +++ b/ios/LichessWidgets/Deeplinks.swift @@ -1,12 +1,24 @@ import Foundation -// Note: puzzle widgets use plain HTTPS URLs (e.g. lichess.org/training/{id}) -// so the app can handle them as universal links and open the native puzzle screen directly. +// Note: the daily-puzzle widget uses the `org.lichess.mobile://training/daily` +// custom scheme (optionally suffixed with `/{puzzleId}`) so the Flutter app +// always opens the native "Daily Puzzle" screen for the tapped puzzle. + +private let kScheme = "org.lichess.mobile" + +/// Builds `org.lichess.mobile://training/daily[/{puzzleId}]` deeplinks handled +/// by the Flutter app to open the native daily-puzzle screen. See `AppLinksService`. +func dailyPuzzleDeeplink(puzzleId: String?) -> URL? { + if let puzzleId { + return URL(string: "\(kScheme)://training/daily/\(puzzleId)") + } + return URL(string: "\(kScheme)://training/daily") +} extension String { /// Encodes the string into the custom scheme the app listens for to open it in the in-app browser. var lichessWebURL: URL? { guard let encoded = addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) else { return nil } - return URL(string: "org.lichess.mobile://open-web?url=\(encoded)") + return URL(string: "\(kScheme)://open-web?url=\(encoded)") } } diff --git a/lib/src/app_links_service.dart b/lib/src/app_links_service.dart index b8976dd28..83c0e2d58 100644 --- a/lib/src/app_links_service.dart +++ b/lib/src/app_links_service.dart @@ -13,7 +13,10 @@ import 'package:lichess_mobile/src/model/challenge/challenge_repository.dart'; import 'package:lichess_mobile/src/model/challenge/challenge_service.dart'; import 'package:lichess_mobile/src/model/common/id.dart'; import 'package:lichess_mobile/src/model/game/game_repository.dart'; +import 'package:lichess_mobile/src/model/puzzle/puzzle.dart'; import 'package:lichess_mobile/src/model/puzzle/puzzle_angle.dart'; +import 'package:lichess_mobile/src/model/puzzle/puzzle_providers.dart'; +import 'package:lichess_mobile/src/model/puzzle/puzzle_theme.dart'; import 'package:lichess_mobile/src/model/user/user.dart'; import 'package:lichess_mobile/src/tab_scaffold.dart'; import 'package:lichess_mobile/src/view/analysis/analysis_screen.dart'; @@ -31,6 +34,11 @@ import 'package:url_launcher/url_launcher.dart'; final _logger = Logger('AppLinks'); +// Deeplink host/path for the iOS daily-puzzle widget tap. +// Must stay in sync with Deeplinks.swift in the iOS widget extension. +const _kDailyPuzzleDeeplinkHost = 'training'; +const _kDailyPuzzleDeeplinkPath = 'daily'; + final appLinksServiceProvider = Provider((ref) { final service = AppLinksService(ref); ref.onDispose(() => service.dispose()); @@ -61,6 +69,14 @@ class AppLinksService { return; } final context = ref.read(currentNavigatorKeyProvider).currentContext; + if (uri.scheme == kLichessUriScheme && + uri.host == _kDailyPuzzleDeeplinkHost && + uri.pathSegments.firstOrNull == _kDailyPuzzleDeeplinkPath) { + if (context != null && context.mounted) { + await handleDailyPuzzleLink(context, uri.pathSegments.elementAtOrNull(1)); + } + return; + } if (context != null && context.mounted) { await handleAppLink(context, uri); } @@ -150,6 +166,48 @@ class AppLinksService { } } + /// Opens the native daily-puzzle screen (same path as tapping the daily-puzzle + /// card on the puzzle tab) in response to `org.lichess.mobile://training/daily` + /// or `org.lichess.mobile://training/daily/{id}` deeplinks emitted by the iOS + /// home-screen widget. + /// + /// Always fetches the current daily puzzle first (cached, so no extra request + /// in the common case). When [puzzleId] matches today's daily, it is used + /// directly. When it differs (widget cached a stale id), that specific puzzle + /// is fetched but NOT flagged as the daily so the user isn't confused when + /// navigating back to the puzzle tab. + @visibleForTesting + Future handleDailyPuzzleLink(BuildContext context, String? puzzleId) async { + try { + Puzzle puzzle; + final dailyPuzzle = await ref.read(dailyPuzzleProvider.future); + if (puzzleId == null || dailyPuzzle.puzzle.id == PuzzleId(puzzleId)) { + puzzle = dailyPuzzle; + } else { + // Widget cached a different puzzle than today's daily — fetch it, but + // don't mark as daily to avoid confusing the user. + try { + puzzle = await ref.read(puzzleProvider(PuzzleId(puzzleId)).future); + } catch (e, st) { + // Fall back to the current daily puzzle rather than leaving the tap + // as a no-op when the widget's cached id is stale or unreachable. + _logger.info('Failed to load widget puzzle id $puzzleId, falling back: $e', e, st); + puzzle = dailyPuzzle; + } + } + if (!context.mounted) return; + await Navigator.of(context, rootNavigator: true).push( + PuzzleScreen.buildRoute( + context, + angle: const PuzzleTheme(PuzzleThemeKey.mix), + puzzle: puzzle, + ), + ); + } catch (e, st) { + _logger.severe('Failed to open daily puzzle from widget: $e\n$st'); + } + } + Future _tryResolveChallengeLink(BuildContext context, Uri appLinkUri) async { try { final challengeId = ChallengeId(appLinkUri.pathSegments[0]); diff --git a/test/app_links_service_test.dart b/test/app_links_service_test.dart index 732d61c20..b754a800f 100644 --- a/test/app_links_service_test.dart +++ b/test/app_links_service_test.dart @@ -3,6 +3,8 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:flutter_riverpod/misc.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:http/http.dart' as http; +import 'package:http/testing.dart'; import 'package:lichess_mobile/src/app_links_service.dart'; import 'package:lichess_mobile/src/model/challenge/challenge.dart'; import 'package:lichess_mobile/src/model/challenge/challenge_repository.dart'; @@ -14,6 +16,7 @@ import 'package:lichess_mobile/src/model/game/game_repository.dart'; import 'package:lichess_mobile/src/model/game/game_status.dart'; import 'package:lichess_mobile/src/model/game/player.dart'; import 'package:lichess_mobile/src/model/user/user.dart'; +import 'package:lichess_mobile/src/network/http.dart'; import 'package:lichess_mobile/src/view/analysis/analysis_screen.dart'; import 'package:lichess_mobile/src/view/broadcast/broadcast_game_screen.dart'; import 'package:lichess_mobile/src/view/broadcast/broadcast_player_results_screen.dart'; @@ -26,13 +29,38 @@ import 'package:mocktail/mocktail.dart'; import 'example_data.dart'; import 'model/game/game_socket_example_data.dart'; +import 'model/puzzle/mock_server_responses.dart'; +import 'network/fake_http_client_factory.dart'; import 'network/fake_websocket_channel.dart'; import 'test_provider_scope.dart'; +// Mock response for a cached widget puzzle with a different id than the daily. +// Built from the daily mock data to share the same valid game/pgn structure. +final _mockStalePuzzleJson = mockDailyPuzzleResponse.trim().replaceFirst( + '"id":"0XqV2"', + '"id":"stale1"', +); + class MockGameRepository extends Mock implements GameRepository {} class MockChallengeRepository extends Mock implements ChallengeRepository {} +class _DailyPuzzleLinkTestWidget extends ConsumerWidget { + const _DailyPuzzleLinkTestWidget({this.puzzleId}); + + final String? puzzleId; + + @override + Widget build(BuildContext context, WidgetRef ref) { + return ElevatedButton( + onPressed: () async { + await ref.read(appLinksServiceProvider).handleDailyPuzzleLink(context, puzzleId); + }, + child: const Text('test daily link'), + ); + } +} + class _TestWidget extends ConsumerWidget { const _TestWidget({required this.uri}); @@ -64,7 +92,108 @@ Future triggerAppLink( await tester.tap(find.text('test link')); } +Future triggerDailyPuzzleLink( + WidgetTester tester, + String? puzzleId, { + Map? overrides, +}) async { + final app = await makeTestProviderScopeApp( + tester, + overrides: overrides, + home: _DailyPuzzleLinkTestWidget(puzzleId: puzzleId), + ); + await tester.pumpWidget(app); + await tester.tap(find.text('test daily link')); +} + void main() { + group('handleDailyPuzzleLink', () { + // Builds an httpClientFactoryProvider override whose mock client routes + // puzzle API endpoints to controlled responses. + Override puzzleHttpOverride({bool failStaleFetch = false}) { + return httpClientFactoryProvider.overrideWith((ref) { + return FakeHttpClientFactory( + () => MockClient((request) async { + if (request.url.path == '/api/puzzle/daily') { + return http.Response(mockDailyPuzzleResponse.trim(), 200); + } + if (request.url.path == '/api/puzzle/stale1') { + if (failStaleFetch) return http.Response('Server error', 500); + return http.Response(_mockStalePuzzleJson, 200); + } + return http.Response('', 200); + }), + ); + }); + } + + testWidgets("no puzzle id: opens today's daily puzzle", (tester) async { + await triggerDailyPuzzleLink( + tester, + null, + overrides: {httpClientFactoryProvider: puzzleHttpOverride()}, + ); + await tester.pumpAndSettle(); + expect( + tester.widget(find.byType(PuzzleScreen)), + isA() + .having((s) => s.puzzle?.puzzle.id, 'puzzle id', const PuzzleId('0XqV2')) + .having((s) => s.puzzle?.isDailyPuzzle, 'is daily', true), + ); + }); + + testWidgets('puzzle id matches daily: opens daily puzzle without extra fetch', (tester) async { + // PuzzleRepository.fetch() does not set isDailyPuzzle, so if puzzleProvider + // were called (wrongly) the assertion on isDailyPuzzle: true would fail. + await triggerDailyPuzzleLink( + tester, + '0XqV2', // same id as the daily puzzle + overrides: {httpClientFactoryProvider: puzzleHttpOverride()}, + ); + await tester.pumpAndSettle(); + expect( + tester.widget(find.byType(PuzzleScreen)), + isA() + .having((s) => s.puzzle?.puzzle.id, 'puzzle id', const PuzzleId('0XqV2')) + .having((s) => s.puzzle?.isDailyPuzzle, 'is daily', true), + ); + }); + + testWidgets('puzzle id differs from daily: opens specific puzzle not flagged as daily', ( + tester, + ) async { + await triggerDailyPuzzleLink( + tester, + 'stale1', + overrides: {httpClientFactoryProvider: puzzleHttpOverride()}, + ); + await tester.pumpAndSettle(); + expect( + tester.widget(find.byType(PuzzleScreen)), + isA() + .having((s) => s.puzzle?.puzzle.id, 'puzzle id', const PuzzleId('stale1')) + .having((s) => s.puzzle?.isDailyPuzzle, 'is daily', isNot(true)), + ); + }); + + testWidgets('puzzle id differs from daily and fetch fails: falls back to daily puzzle', ( + tester, + ) async { + await triggerDailyPuzzleLink( + tester, + 'stale1', + overrides: {httpClientFactoryProvider: puzzleHttpOverride(failStaleFetch: true)}, + ); + await tester.pumpAndSettle(); + expect( + tester.widget(find.byType(PuzzleScreen)), + isA() + .having((s) => s.puzzle?.puzzle.id, 'puzzle id', const PuzzleId('0XqV2')) + .having((s) => s.puzzle?.isDailyPuzzle, 'is daily', true), + ); + }); + }); + group('resolveAppLinkUri', () { testWidgets('Nothing happens for an empty path', (WidgetTester tester) async { final uri = Uri.parse('https://lichess.org/');