From 5fc04f14b79f8609ba5142b1aca9745820dbb05b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 11 May 2026 23:54:11 -0400 Subject: [PATCH] fix(flutter): full-player kebab nav + restore artistAlbums SWR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unrelated issues, batched. Full-player kebab → "Go to album/artist" still crashed with _debugCheckDuplicatedPageKeys despite the prior onBeforeNavigate fix. Cause: pop() and push() ran in the same frame, so go_router's page-key reservation table briefly contained both /now-playing AND the destination shell-child, tripping the duplicate-key assert. Refactor: replace onBeforeNavigate with onNavigate(path), where the host receives the path AFTER the sheet pops and owns the navigation entirely. The full player wires: onNavigate: (path) async { await Navigator.of(context).maybePop(); if (context.mounted) GoRouter.of(context).push(path); } Awaiting the pop guarantees /now-playing is fully gone from the navigator's page list before the push starts. Mini player + track rows + everywhere else use the default (no onNavigate) path that does context.push(path) inline — they're already inside the ShellRoute, no race possible. Restored alwaysRefresh: true on artistAlbumsProvider. Dropping it in the cache-loop fix had a side effect: if drift's cachedAlbums held only a subset of an artist's albums (user previously visited just one album by them), the artist detail page rendered that partial list forever — provider only fetched on empty drift, never on partial drift. The metadata prefetcher only mass-warms artistProvider (single row), so re-enabling SWR on artistAlbums won't recreate the storm. --- .../lib/library/library_providers.dart | 8 ++++ .../lib/player/now_playing_screen.dart | 11 ++++-- .../track_actions/track_actions_button.dart | 12 +++--- .../track_actions/track_actions_sheet.dart | 38 ++++++++++++------- 4 files changed, 46 insertions(+), 23 deletions(-) diff --git a/flutter_client/lib/library/library_providers.dart b/flutter_client/lib/library/library_providers.dart index 26ecd296..9482c05c 100644 --- a/flutter_client/lib/library/library_providers.dart +++ b/flutter_client/lib/library/library_providers.dart @@ -101,6 +101,14 @@ final artistAlbumsProvider = isOnline: () async => (await ref .read(connectivityProvider.future) .timeout(const Duration(seconds: 3), onTimeout: () => true)), + // SWR: re-fetch the full album list on every visit. Without this, + // a previously-incomplete drift cache (e.g. user had only opened + // one album by this artist, so cachedAlbums had just that row) + // would render forever as a partial list. The prefetcher only + // warms artistProvider (single row), so this provider isn't + // mass-instantiated and the storm risk that motivated dropping + // alwaysRefresh elsewhere doesn't apply here. + alwaysRefresh: true, tag: 'artistAlbums($artistId)', ); }); diff --git a/flutter_client/lib/player/now_playing_screen.dart b/flutter_client/lib/player/now_playing_screen.dart index e7547b7e..e04a2d64 100644 --- a/flutter_client/lib/player/now_playing_screen.dart +++ b/flutter_client/lib/player/now_playing_screen.dart @@ -400,9 +400,14 @@ class _SecondaryControls extends StatelessWidget { // /now-playing lives outside the ShellRoute. Pushing // /artists/:id or /albums/:id (both shell-children) on top // of it would make go_router try to mount a duplicate - // ShellRoute. Pop ourselves first so the destination route - // mounts cleanly inside the existing shell. - onBeforeNavigate: () => Navigator.of(context).maybePop(), + // ShellRoute (the same _debugCheckDuplicatedPageKeys crash + // we hit before with /queue). Await our own pop fully + // before pushing the destination so go_router never sees + // both pages active in the same frame. + onNavigate: (path) async { + await Navigator.of(context).maybePop(); + if (context.mounted) GoRouter.of(context).push(path); + }, ), ], ); diff --git a/flutter_client/lib/shared/widgets/track_actions/track_actions_button.dart b/flutter_client/lib/shared/widgets/track_actions/track_actions_button.dart index 61b67575..6937e9a9 100644 --- a/flutter_client/lib/shared/widgets/track_actions/track_actions_button.dart +++ b/flutter_client/lib/shared/widgets/track_actions/track_actions_button.dart @@ -11,7 +11,7 @@ class TrackActionsButton extends StatelessWidget { super.key, required this.track, this.hideQueueActions = false, - this.onBeforeNavigate, + this.onNavigate, }); final TrackRef track; @@ -20,10 +20,10 @@ class TrackActionsButton extends StatelessWidget { /// screen where the menu's track IS the currently-playing one. final bool hideQueueActions; - /// Forwarded to TrackActionsSheet.onBeforeNavigate. Use to dismiss - /// the host route before "Go to album" / "Go to artist" pushes the - /// detail screen. - final void Function()? onBeforeNavigate; + /// Forwarded to TrackActionsSheet.onNavigate. The host receives the + /// destination path AFTER the sheet has popped and is responsible + /// for navigating to it (typically: pop self first, then push). + final Future Function(String path)? onNavigate; @override Widget build(BuildContext context) { @@ -35,7 +35,7 @@ class TrackActionsButton extends StatelessWidget { context, track, hideQueueActions: hideQueueActions, - onBeforeNavigate: onBeforeNavigate, + onNavigate: onNavigate, ), ); } diff --git a/flutter_client/lib/shared/widgets/track_actions/track_actions_sheet.dart b/flutter_client/lib/shared/widgets/track_actions/track_actions_sheet.dart index b689f70d..bb5aeef5 100644 --- a/flutter_client/lib/shared/widgets/track_actions/track_actions_sheet.dart +++ b/flutter_client/lib/shared/widgets/track_actions/track_actions_sheet.dart @@ -21,25 +21,27 @@ class TrackActionsSheet extends ConsumerWidget { super.key, required this.track, required this.hideQueueActions, - this.onBeforeNavigate, + this.onNavigate, }); final TrackRef track; final bool hideQueueActions; - /// Optional hook invoked just before "Go to album" / "Go to artist" - /// pushes the destination route. Lets callers that live OUTSIDE the - /// ShellRoute (e.g. the full player at /now-playing) pop themselves - /// first, so go_router doesn't try to mount a second ShellRoute on - /// top of the active top-level route — the same duplicate-page-key - /// failure mode we hit when /queue lived inside the shell. - final void Function()? onBeforeNavigate; + /// Optional hook for "Go to album" / "Go to artist". Called with + /// the destination route path AFTER the sheet has popped. Lets + /// callers that live OUTSIDE the ShellRoute (e.g. the full player + /// at /now-playing) pop themselves first then push, so go_router + /// doesn't try to mount a second ShellRoute on top of the active + /// top-level route. When null, the sheet does its own + /// context.push, which is correct for surfaces already inside the + /// shell (mini player, track rows on detail screens, etc.). + final Future Function(String path)? onNavigate; static Future show( BuildContext context, TrackRef track, { bool hideQueueActions = false, - void Function()? onBeforeNavigate, + Future Function(String path)? onNavigate, }) { return showModalBottomSheet( context: context, @@ -47,7 +49,7 @@ class TrackActionsSheet extends ConsumerWidget { builder: (_) => TrackActionsSheet( track: track, hideQueueActions: hideQueueActions, - onBeforeNavigate: onBeforeNavigate, + onNavigate: onNavigate, ), ); } @@ -152,8 +154,12 @@ class TrackActionsSheet extends ConsumerWidget { label: 'Go to album', onTap: () { Navigator.pop(context); - onBeforeNavigate?.call(); - context.push('/albums/${track.albumId}'); + final path = '/albums/${track.albumId}'; + if (onNavigate != null) { + onNavigate!(path); + } else { + context.push(path); + } }, ), _MenuItem( @@ -162,8 +168,12 @@ class TrackActionsSheet extends ConsumerWidget { label: 'Go to artist', onTap: () { Navigator.pop(context); - onBeforeNavigate?.call(); - context.push('/artists/${track.artistId}'); + final path = '/artists/${track.artistId}'; + if (onNavigate != null) { + onNavigate!(path); + } else { + context.push(path); + } }, ), const _Divider(),