Repository navigation
Conversation
Av1anJay
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested — 1 navigation regression and 1 failing-test regression.
- On desktop, opening a category changes the enclosing
DialogRoutepop disposition. Scrim clicks and Escape therefore return to the category catalog instead of dismissing Settings; users must dismiss twice. See the inline comment. - The full CI suite is red:
test/bus_app_locale_scale_test.dart:100-101and:174-180still search for the locale dropdown immediately after opening Settings. It now exists only after entering Appearance. Update both tests to open Appearance first. The Verify job reports 477 passed, 2 failed, and every build job is skipped: https://github.com/YetAnotherBusDeveloper/yetanotherbusapp/actions/runs/37173920461/job/111352445847
The new mobile/desktop category navigation, state persistence, scroll restoration, platform visibility, and large-text tests otherwise pass.
| ); | ||
|
|
||
| return PopScope<void>( | ||
| canPop: category == null, |
There was a problem hiding this comment.
DialogRoute report doNotPop. showDialog dismisses its scrim and Escape shortcut through Navigator.maybePop, so both actions are vetoed and onPopInvokedWithResult below merely returns to the category list. Repro at width ≥1100: open Settings → Appearance → click outside the dialog or press Escape; the dialog remains open, requiring a second dismissal. Please handle category-back without changing the dialog route's pop disposition, or explicitly distinguish desktop dismissal attempts from in-dialog back navigation.
Av1anJay
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved — both blockers from the previous review are resolved on 60bd488d.
- Desktop dismissal:
canPop: isDialog || category == nullno longer vetoesNavigator.maybePop, so scrim clicks and Escape close the settings dialog while the AppBarBackButtonstill returns to the category catalog. The new cases cover Escape and scrim both on the catalog and inside a category, plus full-page category back at desktop width. - Locale test regression: both cases in
test/bus_app_locale_scale_test.dartnow enter Appearance before looking for the locale dropdown, and the assertions still check the same item lists.
Checks: this host has no Flutter/Dart toolchain, so I could not run flutter analyze/flutter test locally — this is a static review of the full diff plus surrounding code, backed by CI. On 60bd488d the Verify job is green (Analyze ✅, Test ✅ full suite, Web route-geometry ✅); the platform build jobs were still queued when this review was written.
One trade-off, non-blocking: at width ≥1100 settings renders in a dialog, so on a wide Android tablet/Chromebook the system back gesture now dismisses the whole dialog instead of returning to the category list. That is inherent to the fix (system back and scrim/Escape all route through maybePop), and in-dialog category navigation is still covered by the AppBar back button.
| return PopScope<void>( | ||
| // Scrim clicks and Escape dismiss the dialog. Category back is handled | ||
| // by the app bar there, and by system back on full-page settings. | ||
| canPop: isDialog || category == null, |
There was a problem hiding this comment.
Non-blocking: this is true for any dialog route, so on a wide Android device (settings opens in a dialog at width ≥1100) the system back gesture now dismisses the entire dialog instead of returning to the category catalog. System back cannot be distinguished from scrim/Escape here because all three go through Navigator.maybePop, so the trade-off is reasonable — flagging it only so the behaviour change on that device class is a conscious choice. In-dialog category navigation is still available through the AppBar back button.
Settings previously displayed every control in one long list. Replace it with a category list that opens each section, with back navigation and restored scroll positions on both mobile and the desktop settings dialog.
The back arrow returns to the category list. Clicking outside the desktop dialog or pressing Escape closes Settings immediately; full-page settings retain system-back navigation through the category list.
Preserve the existing account and database pages, platform-specific options, and ad-toggle state. Add English and Traditional Chinese category descriptions, and move About version chips below the header so they fit narrow screens with larger text.
Validation:
flutter analyze --no-pubpassed.