Give the loading screen an escape hatch on weak connections#206
Merged
Conversation
The data-loading screen showed an indefinite spinner while GET data ran with no timeout, so a weak connection could hang it forever with no feedback or way out. Now: a 30s timeout falls through to the existing retry/log-out box; after 6s of loading a "This is taking longer than usual…" message appears; and when the user has downloaded songs, a "View Downloads" button drops them into the offline library instead of waiting.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesData loading flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DataLoadingScreen
participant DataProvider
participant DownloadProvider
participant AppState
participant MainScreen
DataLoadingScreen->>DataLoadingScreen: start initialization and timers
DataLoadingScreen->>DataProvider: init()
DataProvider-->>DataLoadingScreen: return result
DataLoadingScreen->>DownloadProvider: check available downloads
DataLoadingScreen->>AppState: set offline mode
DataLoadingScreen->>MainScreen: navigate to MainScreen
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/ui/screens/data_loading.dart (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
catch (_)instead ofcatch (e).The exception variable
eis never used. As per coding guidelines, single-letter variable names are not permitted except fori,j, andh.♻️ Proposed fix
- } catch (e) { + } catch (_) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/screens/data_loading.dart` at line 59, Update the catch clause in the relevant error-handling flow to use an ignored exception binding, `catch (_)`, since the caught value is not referenced. Leave the surrounding handling unchanged.Source: Coding guidelines
test/ui/screens/data_loading_test.dart (1)
58-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for timeout, retry, and error paths.
The three existing tests cover the fresh-loading window, delayed message, and View Downloads navigation. Missing scenarios:
- 30-second timeout: pump past
_loadTimeout, verifyOopsBoxappears with retry/log-out buttons.- Retry after error: complete
initCompleterwith an error, verifyOopsBox, tap Retry, reset stub, complete newinitCompleter, verify navigation toMainScreen.init()throws: configurewhen(dataProvider.init()).thenThrow(...), verifyOopsBoxappears.These cover the critical fallback paths introduced by this PR.
🧪 Example timeout test
testWidgets('shows OopsBox after the load timeout', (tester) async { await mount(tester, downloads: []); await tester.pump(); await tester.pump(const Duration(seconds: 30)); expect(find.text('Oops!'), findsOneWidget); expect(find.text('Retry'), findsOneWidget); expect(find.text('Log Out'), findsOneWidget); await settle(tester); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ui/screens/data_loading_test.dart` around lines 58 - 99, Add widget tests covering the missing fallback paths around the existing loading tests: pump past _loadTimeout and verify OopsBox with Retry and Log Out; complete initCompleter with an error, tap Retry, reset the stub, complete the new initCompleter, and verify MainScreen navigation; configure dataProvider.init() to throw and verify OopsBox appears. Use the existing mount, settle, and test stubbing helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ui/screens/data_loading.dart`:
- Around line 46-72: Update _loadData and _retry to track each loading attempt
with a generation/token, incrementing it when starting a retry and capturing the
current value for each init() future. Before handling success, failure, or
canceling timers, verify the captured generation is still current; ignore
results from superseded attempts while preserving the active attempt’s
navigation and error behavior.
---
Nitpick comments:
In `@lib/ui/screens/data_loading.dart`:
- Line 59: Update the catch clause in the relevant error-handling flow to use an
ignored exception binding, `catch (_)`, since the caught value is not
referenced. Leave the surrounding handling unchanged.
In `@test/ui/screens/data_loading_test.dart`:
- Around line 58-99: Add widget tests covering the missing fallback paths around
the existing loading tests: pump past _loadTimeout and verify OopsBox with Retry
and Log Out; complete initCompleter with an error, tap Retry, reset the stub,
complete the new initCompleter, and verify MainScreen navigation; configure
dataProvider.init() to throw and verify OopsBox appears. Use the existing mount,
settle, and test stubbing helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 427c2d98-3405-445b-a4b7-e3c70337918d
📒 Files selected for processing (3)
lib/ui/screens/data_loading.darttest/ui/screens/data_loading_test.darttest/ui/screens/data_loading_test.mocks.dart
Address review: a load attempt now carries a generation so a stale init() future (e.g. one that resolves after the timeout fired and the user hit Retry) can't navigate or re-error over a newer attempt. Also use catch (_), and add tests for the timeout, failure, and retry paths (including the stale-future case).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DataLoadingScreenranDataProvider.init()(a singleGET data) with no timeout behind an indefinite spinner. On a weak/stalling connection it could hang for minutes with no feedback and no way out — the only exit was if the request eventually errored.Changes
AppMode.offline→ downloaded library), reusing the same pathNoConnectionScreenalready uses. Shown only when downloads exist.Both timers are owned by the state and cancelled in
dispose()/on navigation (replacing the un-cancellableFuture.timeout), and the meaninglessawaitonpushReplacementNamedwas dropped.Tests
data_loading_test.dart:Summary by CodeRabbit
New Features
Bug Fixes
Tests