fix: implement ui/ux bug backlog across navigation, errors, and theme - #63
Conversation
Addresses the full UI/UX audit: P0 data-integrity fixes, visual glitches, navigation/deep links, error retry UX, dark mode, architecture refactors, accessibility polish, and new tests. Co-authored-by: Youri Bontekoe <YKDBontekoe@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR reshapes routing and the dashboard shell, moves health and manual-run logic into providers, adds shared inline error UI, updates theming/layout utilities, and revises onboarding/settings flows and tests. ChangesApp Refresh Cohort
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Co-authored-by: Youri Bontekoe <YKDBontekoe@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/character/presentation/widgets/gamekit_panel.dart (1)
73-100: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winExclude the descendant semantics here
Text(label, ...)still contributes its own accessibility node, so this button can be announced with the label twice. AddexcludeSemantics: trueto theSemanticswrapper so the custom label is the only one exposed.🤖 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/features/character/presentation/widgets/gamekit_panel.dart` around lines 73 - 100, The Semantics wrapper in gamekit_panel.dart is exposing both the custom label and the Text(label, ...) descendant, causing duplicate accessibility announcements. Update the Semantics widget around the GestureDetector/Container in the relevant build method to exclude descendant semantics so only the explicit label is exposed, while keeping the button role and tap behavior unchanged.lib/features/character/providers/quest_provider.dart (1)
26-47: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReward can be lost after a partial save failure. If
saveCharacterfails aftersaveQuestssucceeds, the quest stays completed but the XP/stat gain never lands. The retry action onQuestPanelonly invalidatesquestProvider, so it reloads the already-completed quests and never retries the character update. Make the quest completion and reward grant atomic, or persist a retryable “reward pending” state.🤖 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/features/character/providers/quest_provider.dart` around lines 26 - 47, The quest completion flow in quest_provider.dart is not atomic: `saveQuests` can succeed before `saveCharacter`, leaving the quest completed but the reward unapplied. Update the `completeQuest` logic so the quest state and character reward are committed together, or persist a retryable pending-reward state that survives failures. Use the existing `repo.saveQuests`, `charRepo.saveCharacter`, and `runnerCharacterProvider` invalidation paths to ensure a retry can reapply the XP/stat gain instead of only reloading completed quests.
🧹 Nitpick comments (14)
lib/features/dashboard/presentation/pages/run_route_missing_page.dart (1)
19-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated pop-or-fallback navigation logic.
The same
if (context.canPop()) { context.pop(); } else { context.go(fallback); }block is repeated verbatim here, inmodel_setup_screen.dart, and incoach_chat_app_bar.dart. Consider extracting a small shared helper (e.g. inlib/shared/utils/) such asvoid popOrGo(BuildContext context, String fallbackRoute)to keep this DRY and centralize any future changes (e.g. adding analytics or Semantics consistently).🤖 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/features/dashboard/presentation/pages/run_route_missing_page.dart` around lines 19 - 28, The back-button handler in the route-missing page duplicates the same pop-or-fallback navigation logic used elsewhere. Extract the repeated `context.canPop() / context.pop() / context.go(fallback)` behavior into a shared helper such as `popOrGo(BuildContext context, String fallbackRoute)` in a common utils location, then update the `IconButton` callback here to call that helper so the navigation behavior stays centralized across `run_route_missing_page.dart`, `model_setup_screen.dart`, and `coach_chat_app_bar.dart`.lib/features/coach_chat/presentation/widgets/model_setup_screen.dart (1)
52-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose button missing accessible label.
Unlike the parallel close button added in
coach_chat_app_bar.dart(which wraps theIconButtoninSemantics(label: 'Close coach chat', button: true)and setstooltip: 'Close'), thisIconButtonhas neither atooltipnor aSemanticslabel. Screen readers will announce it as an unlabeled button.♻️ Proposed fix
appBar: showClose ? AppBar( leading: IconButton( icon: const Icon(Icons.close_rounded), + tooltip: 'Close', onPressed: () { if (context.canPop()) { context.pop(); } else { context.go(Routes.dashboard); } }, ), ) : null,🤖 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/features/coach_chat/presentation/widgets/model_setup_screen.dart` around lines 52 - 65, The close control in model_setup_screen.dart is missing an accessible label, unlike the matching button in coach_chat_app_bar.dart. Update the AppBar leading IconButton used when showClose is true to include the same accessibility treatment: add a tooltip and wrap it with a Semantics label/button role so screen readers announce it properly. Use the existing close button implementation in coach_chat_app_bar.dart as the reference for the IconButton setup.lib/app/router.dart (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated SharedPreferences key instead of reusing the provider.
'onboarding_completed'is hardcoded here, duplicating the private_keyinOnboardingCompleted(lib/features/onboarding/providers/onboarding_provider.dart). If that key ever changes,initialLocationsilently desyncs from the actual completion state (redirect would still self-correct on first frame, but causes an avoidable flash of the wrong screen). Prefer reading the existing provider directly instead of re-implementing the read.♻️ Proposed fix
final routerProvider = Provider<GoRouter>((ref) { - final prefs = ref.watch(sharedPreferencesProvider); - final hasCompletedOnboarding = prefs.getBool('onboarding_completed') ?? false; + final hasCompletedOnboarding = ref.read(onboardingCompletedProvider); final refresh = _RouterRefreshNotifier(ref);🤖 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/app/router.dart` around lines 42 - 48, The initial route logic in `GoRouter` is duplicating the onboarding completion key by reading `SharedPreferences` directly instead of reusing the existing `OnboardingCompleted` provider. Update `router.dart` to derive `hasCompletedOnboarding` from the provider used by onboarding state, so `initialLocation` stays in sync with the source of truth and avoids hardcoded key drift. Refer to `GoRouter` setup and the onboarding provider (`OnboardingCompleted`) when wiring the check.lib/features/dashboard/presentation/widgets/character_glance_card.dart (1)
65-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLeftover
GoogleFonts.interusage in a theming-migration PR.This XP/"Train weakest" row still uses
GoogleFonts.inter(fontSize:..., color:...)while the rest of this layer migrates typography toTheme.of(context).textTheme+context.kynosThemecolors (see thecharacter_page.dartandgait_teaser_card.dartchanges in this same PR). Worth aligning for consistency with the stated migration goal.🤖 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/features/dashboard/presentation/widgets/character_glance_card.dart` around lines 65 - 78, The “Train weakest” row in CharacterGlanceCard is still using GoogleFonts.inter, which is inconsistent with the typography/theme migration used elsewhere. Update the Text widgets in character_glance_card.dart to use Theme.of(context).textTheme styles combined with context.kynosTheme colors, matching the patterns used in character_page.dart and gait_teaser_card.dart, and remove the direct GoogleFonts.inter usage from this widget.lib/features/character/presentation/pages/character_page.dart (1)
156-161: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo feedback path for health permission request failure.
ref.read(healthPermissionsProvider.notifier).request()is fired without awaiting or listening for errors/loading. If the permission request fails or is denied, the user gets no indication — the button just does nothing visible. Considerref.listen(healthPermissionsProvider, ...)to surface a snackbar/error state, consistent with the inline-error-card pattern used elsewhere in this PR.🤖 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/features/character/presentation/pages/character_page.dart` around lines 156 - 161, The health permission request in CharacterPage is fire-and-forget, so failures or denials are invisible to the user. Update the `FilledButton` action or surrounding `CharacterPage` logic to observe `healthPermissionsProvider` state and surface errors/loading, using `ref.listen` on `healthPermissionsProvider` to show a snackbar or inline error consistent with the existing error-card pattern. Keep the request call in `healthPermissionsProvider.notifier.request()` but wire its failure path to user-facing feedback.lib/shared/widgets/kynos_page_dots.dart (1)
24-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a single semantics node for the whole indicator.
Wrapping each dot individually may cause screen readers to stop on every dot while swiping through the row. A single
Semanticsaround theRow(e.g.,label: 'Page ${activeIndex + 1} of $count') with children excluded is a more common a11y pattern for page indicators and reduces traversal noise.🤖 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/shared/widgets/kynos_page_dots.dart` around lines 24 - 35, The page indicator currently adds a separate Semantics node for each dot in kynos_page_dots.dart, which creates unnecessary screen reader traversal. Update the page indicator widget to use a single Semantics wrapper around the entire Row in the relevant widget/build method, with the label based on the current active page and the count, and exclude the individual dot children from accessibility so only one announcement is exposed.lib/shared/widgets/gait_model_card.dart (1)
86-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLoading state loses progress semantics for screen readers.
Replacing
LinearProgressIndicator()withKynosLoadingLinedrops the built-in progress-bar semantics that assistive tech relies on to announce a busy/loading state; per the widget's implementation it's a plain shimmerContainerwith noSemanticswrapper (only an optional visible label). This affects every usage ofKynosLoadingLineacross the PR (e.g. alsoweek_momentum_card.dartline 49). Given this cohort is focused on accessibility, consider wrappingKynosLoadingLineitself withSemantics(label: ..., liveRegion: true)or similar at the shared-widget level.🤖 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/shared/widgets/gait_model_card.dart` at line 86, The shared loading widget is missing progress semantics, so screen readers no longer get a busy/loading announcement when KynosLoadingLine is used. Update KynosLoadingLine itself to include appropriate Semantics (for example, a label and liveRegion on the widget’s root) so every caller, including gait_model_card.dart and week_momentum_card.dart, inherits accessible loading behavior without duplicating wrapper logic.lib/shared/widgets/responsive_center.dart (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport tokens via
theme.dartbarrel per guidelines.This shared widget imports
core/theme/layout.dartdirectly rather than thetheme.dartbarrel used elsewhere for tokens/theme access.As per coding guidelines, "Shared widgets must import tokens and theme via
package:kynos/core/theme/theme.dart".🤖 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/shared/widgets/responsive_center.dart` around lines 1 - 3, The shared widget is importing theme tokens directly from core/theme/layout.dart instead of the required theme.dart barrel. Update the imports in ResponsiveCenter to use package:kynos/core/theme/theme.dart for token/theme access, and remove the direct layout.dart import so the widget follows the shared-widget guideline consistently.Source: Coding guidelines
test/features/onboarding/onboarding_page_test.dart (1)
1-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for skip/get-started navigation flows.
This test only checks initial rendering. The new async
_skipOnboarding/_getStartedlogic (with its provider-driven navigation and permission-request branching) has no test coverage. Consider adding tests that tap "Skip" and "Get Started" (overridinghealthPermissionsProvideras needed) and assert the resulting route/provider state.Want me to draft these additional test cases?
🤖 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/features/onboarding/onboarding_page_test.dart` around lines 1 - 27, The onboarding widget test only covers initial rendering and misses the async navigation paths in OnboardingPage, especially the _skipOnboarding and _getStarted flows. Add widget tests that tap the Skip and Get Started actions, override healthPermissionsProvider (and any other required providers) to control the permission branch, and assert the expected navigation or provider-state changes after the taps. Use the existing OnboardingPage and onboarding_provider symbols to locate the test targets.lib/features/dashboard/presentation/widgets/readiness_card.dart (1)
147-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInsights error state has no retry, unlike the readiness summary above.
For consistency with the new inline-error/retry pattern (and
KynosInlineErrorCardused just above in the same widget), consider offering a retry affordance here too, rather than a static "Insights unavailable" message.🤖 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/features/dashboard/presentation/widgets/readiness_card.dart` around lines 147 - 157, The insights error state in readiness_card.dart is static and does not offer a retry path, unlike the readiness summary and the nearby KynosInlineErrorCard pattern. Update the todayInsightsState.when error branch in the ReadinessCard widget to render a retry affordance instead of only the “Insights unavailable” Text, wiring it to the same retry callback/action used by the other inline error state so users can recover consistently.lib/features/settings/providers/health_import_provider.dart (1)
16-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
@freezedforHealthImportState.This is a hand-written state class with manual
copyWith/clear-flags boilerplate. Per guidelines, data models should use@freezedfor immutability,copyWith, equality, andhashCode. Freezed's defaultcopyWithdoesn't support explicit-null clearing, so this may have been a deliberate choice, but consider using freezed with a sentinel-basedcopyWithor a wrapper (e.g.,Wrapped<T?>) to align with the convention.As per coding guidelines, "All data models must use
@freezedfor immutability,copyWith, equality, andhashCode."🤖 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/features/settings/providers/health_import_provider.dart` around lines 16 - 83, HealthImportState is a hand-written mutable-style state model with custom copyWith and clear flags, but it should follow the project convention of using `@freezed` for immutability, equality, and hashCode. Refactor the HealthImportState class in health_import_provider.dart to a Freezed data class, keeping the existing fields and state semantics used by HealthImportState and its copyWith behavior. If explicit null clearing is still needed for fields like error, progressMessage, and importedWorkout, preserve that via a Freezed-friendly sentinel/wrapper approach rather than manual boilerplate.Source: Coding guidelines
lib/features/dashboard/providers/post_run_debrief_provider.dart (1)
79-84: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign
saveCharacterwith the repository contract
CharacterRepository.saveCharacterstill returnsFuture<Failure?>(lib/domain/repositories/character_repository.dart:6-8), while the rest of the repository API uses named records. Update the save methods to the same record-based shape so success/failure handling stays consistent.🤖 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/features/dashboard/providers/post_run_debrief_provider.dart` around lines 79 - 84, The save flow is still using a nullable Failure return from saveCharacter, which is inconsistent with the repository’s record-based API. Update CharacterRepository.saveCharacter and its implementation to return the same named record shape used by the other methods, then adjust PostRunDebriefProvider’s save handling to read the record fields instead of checking for null so success and failure are handled consistently.Source: Coding guidelines
lib/features/character/providers/adventure_provider.dart (1)
144-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated
Failure → AsyncErrorhandling.The same catch block is duplicated in
advance(),performCombatAction(), andretreatFromEncounter(). A small private helper (e.g. wrapping the guarded body in a_runGuarded(Future<void> Function() action)that sets_actionInFlight, awaitsaction(), and mapsFailuretoAsyncError) would remove the triplication and give a single place to add logging (logger) if these failures should be observable.♻️ Sketch of a shared helper
Future<void> _runGuarded(Future<void> Function() action) async { if (_actionInFlight) return; _actionInFlight = true; try { await action(); } on Failure catch (failure, stackTrace) { state = AsyncError(failure, stackTrace); } finally { _actionInFlight = false; } }Also applies to: 216-217, 239-240
🤖 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/features/character/providers/adventure_provider.dart` around lines 144 - 145, The `advance()`, `performCombatAction()`, and `retreatFromEncounter()` methods in `AdventureProvider` duplicate the same `Failure` to `AsyncError` catch handling. Extract that shared guarded execution into a private helper such as `_runGuarded(Future<void> Function() action)` that manages `_actionInFlight`, awaits the action, and converts `Failure` into `state = AsyncError(...)`, then have those three methods call the helper instead of repeating the same try/catch logic.lib/features/dashboard/presentation/pages/run_history_page.dart (1)
118-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFull-page retry error UI duplicated across three files; consider reusing the shared error card.
This
Column/Text/Gap/FilledButtonretry layout is essentially repeated inrun_route_page.dart(Lines 52-69) andcoach_chat_page.dart(Lines 104-144). The PR just addedKynosInlineErrorCardfor exactly this pattern (message + optional retry action); consider reusing/adapting it (or extracting a shared full-page error widget) instead of duplicating the layout three times.Two smaller nits on top:
'Failed to load runs: $e'surfaces the raw exceptiontoString()to end users; consider a friendlier copy with the technical detail only logged (logger).- This page renders the error text in a muted
kynos.secondaryLabel, whilerun_route_page.dartuseskynos.move(the color also used byKynosInlineErrorCard's error icon) — the inconsistency makes this error state visually harder to distinguish from normal text.🤖 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/features/dashboard/presentation/pages/run_history_page.dart` around lines 118 - 135, The full-page retry error layout in run_history_page should not be duplicated; reuse KynosInlineErrorCard or extract a shared full-page error widget for the same message + retry pattern used in run_route_page and coach_chat_page. Update the error text in the relevant builder to avoid showing the raw exception directly to users, and log the technical details separately via logger. Also align the error styling with the shared error treatment by using the same error color convention as KynosInlineErrorCard instead of kynos.secondaryLabel.
🤖 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/app/shell_page.dart`:
- Around line 56-67: The Dashboard shortcuts currently use context.go, which
bypasses the shell branch state and resets the nested navigation stack. Update
DashboardTab to accept branch-switch callbacks from ShellPage, and wire those
callbacks to navigationShell.goBranch(...) for the Training and Character
actions so each branch restores its preserved stack instead of navigating to the
root. Use the DashboardTab and ShellPage symbols to locate the callback wiring
and shortcut handlers.
In `@lib/features/character/presentation/widgets/trail_run_game_panel.dart`:
- Around line 27-30: The TrailRunGamePanel error state is exposing raw exception
text in the user-facing KynosInlineErrorCard message. Update the error handling
in the AdventureSession/TrailRunGamePanel builder to use a static, friendly
message like the other KynosInlineErrorCard usages in this PR, and keep the raw
error out of the UI while preserving the existing retry behavior via
ref.invalidate(adventureSessionProvider).
In `@lib/features/dashboard/presentation/widgets/character_glance_card.dart`:
- Around line 27-33: The Semantics wrapper in character_glance_card should
expose the card as one accessibility node instead of combining with child text;
update the CharacterGlanceCard build path to use excludeSemantics: true on the
Semantics around KynosCard. Also make the label in that widget conditional on
onViewCharacter so “tap to view” is only included when the card is actually
interactive, while keeping button tied to onViewCharacter != null.
In `@lib/features/dashboard/presentation/widgets/gait_teaser_card.dart`:
- Around line 28-86: Update the Semantics wrapper in gait_teaser_card.dart
around the KynosCard so the label matches whether onViewTraining is null: use a
tappable label only when the card is actionable, and a non-action label when
disabled. Also set excludeSemantics on the outer Semantics and ensure the
visible content inside the Column (header, MetricTile widgets, and calibration
text) remains readable without being announced twice.
In `@lib/features/onboarding/presentation/onboarding_page.dart`:
- Around line 79-88: The _getStarted flow in onboarding_page.dart completes
onboarding too early, which lets the router guard redirect away before the
health-import navigation runs. In _getStarted, move completeOnboarding() until
after the handoff to Routes.healthImport, or otherwise bypass the guard during
this transition so context.go(Routes.healthImport) can execute reliably. Use the
_getStarted method and onboardingCompletedProvider.notifier.completeOnboarding()
as the key spots to update.
In `@lib/features/settings/presentation/pages/health_import_page.dart`:
- Around line 21-46: The import error transition is being split across multiple
state updates, so the snackbar listener in healthImportPage can miss failures.
Update confirmImport() in the healthImport provider/notifier so it sets error
and isImporting: false in the same state change, or introduce a distinct error
token that changes per failure, then keep the healthImportProvider listener in
health_import_page reacting to that single visible transition.
In `@lib/features/settings/presentation/pages/manual_run_page.dart`:
- Around line 88-100: The manual run error listener is suppressing repeated
validation feedback because ref.listen in ManualRunPage only calls _showError
when next.error differs from previous?.error. Update the ManualRunPage listener
so every failed submit triggers visible feedback even when the error message is
identical, using another change signal from manualRunProvider/saveRun() or by
tracking a submit attempt/state transition instead of comparing error values
alone.
In `@lib/features/settings/providers/health_import_provider.dart`:
- Around line 223-247: The ZIP import success path in _importZip leaves
zipPreview and pickedFile intact, so the preview stays visible and the same
export can be re-imported. Update the success branch in
health_import_provider.dart to clear the ZIP import state after a successful
import, either by resetting zipPreview and pickedFile in state.copyWith or by
marking the ZIP import as completed so the preview cannot be reused.
In `@lib/features/settings/providers/manual_run_provider.dart`:
- Around line 9-36: ManualRunState is a hand-rolled mutable-style data model and
should be converted to a Freezed model to match project guidelines. Update
ManualRunState in manual_run_provider.dart to use `@freezed` (and
`@JsonSerializable` only if needed), remove the manual copyWith implementation,
and define the state as an immutable union/data class so Freezed generates
copyWith, equality, and hashCode. Keep the existing fields and defaults intact
while moving the state definition to the generated Freezed pattern used
elsewhere in the codebase.
- Around line 51-63: The validation errors in saveRun() are emitted with the
same string each time, so ref.listen in manual_run_page.dart can miss repeated
failures when the message doesn’t change. Update ManualRunProvider.saveRun and
the related state model so each validation failure carries a fresh
identifier/nonce or similar freshness marker, and have the listener key off that
instead of error string equality. Use the existing saveRun method and the
state.copyWith(error: ...) flow to locate the change.
In `@lib/features/settings/providers/settings_provider.dart`:
- Line 3: `settings_provider.dart` is depending on `sharedPreferencesProvider`
from the onboarding feature, which violates the shared-provider boundary and
creates cross-feature imports. Move the `sharedPreferencesProvider` definition
out of `onboarding_provider.dart` into a dedicated shared provider under
`shared/providers/`, then update `settings_provider.dart`, onboarding code,
tests, and the app-level override to import the new shared location. Use the
existing `sharedPreferencesProvider` symbol as the migration target and remove
the onboarding import from settings once the provider is relocated.
In `@lib/shared/utils/url_opener.dart`:
- Around line 5-9: The openExternalUrl function currently only checks the
boolean result from launchUrl, so a PlatformException can escape and bypass the
_openLegalUrl fallback. Update openExternalUrl to wrap the launchUrl call in
try/catch, catch platform launch failures, and return false on any exception so
callers like _openLegalUrl can still trigger showUrlLaunchError.
---
Outside diff comments:
In `@lib/features/character/presentation/widgets/gamekit_panel.dart`:
- Around line 73-100: The Semantics wrapper in gamekit_panel.dart is exposing
both the custom label and the Text(label, ...) descendant, causing duplicate
accessibility announcements. Update the Semantics widget around the
GestureDetector/Container in the relevant build method to exclude descendant
semantics so only the explicit label is exposed, while keeping the button role
and tap behavior unchanged.
In `@lib/features/character/providers/quest_provider.dart`:
- Around line 26-47: The quest completion flow in quest_provider.dart is not
atomic: `saveQuests` can succeed before `saveCharacter`, leaving the quest
completed but the reward unapplied. Update the `completeQuest` logic so the
quest state and character reward are committed together, or persist a retryable
pending-reward state that survives failures. Use the existing `repo.saveQuests`,
`charRepo.saveCharacter`, and `runnerCharacterProvider` invalidation paths to
ensure a retry can reapply the XP/stat gain instead of only reloading completed
quests.
---
Nitpick comments:
In `@lib/app/router.dart`:
- Around line 42-48: The initial route logic in `GoRouter` is duplicating the
onboarding completion key by reading `SharedPreferences` directly instead of
reusing the existing `OnboardingCompleted` provider. Update `router.dart` to
derive `hasCompletedOnboarding` from the provider used by onboarding state, so
`initialLocation` stays in sync with the source of truth and avoids hardcoded
key drift. Refer to `GoRouter` setup and the onboarding provider
(`OnboardingCompleted`) when wiring the check.
In `@lib/features/character/presentation/pages/character_page.dart`:
- Around line 156-161: The health permission request in CharacterPage is
fire-and-forget, so failures or denials are invisible to the user. Update the
`FilledButton` action or surrounding `CharacterPage` logic to observe
`healthPermissionsProvider` state and surface errors/loading, using `ref.listen`
on `healthPermissionsProvider` to show a snackbar or inline error consistent
with the existing error-card pattern. Keep the request call in
`healthPermissionsProvider.notifier.request()` but wire its failure path to
user-facing feedback.
In `@lib/features/character/providers/adventure_provider.dart`:
- Around line 144-145: The `advance()`, `performCombatAction()`, and
`retreatFromEncounter()` methods in `AdventureProvider` duplicate the same
`Failure` to `AsyncError` catch handling. Extract that shared guarded execution
into a private helper such as `_runGuarded(Future<void> Function() action)` that
manages `_actionInFlight`, awaits the action, and converts `Failure` into `state
= AsyncError(...)`, then have those three methods call the helper instead of
repeating the same try/catch logic.
In `@lib/features/coach_chat/presentation/widgets/model_setup_screen.dart`:
- Around line 52-65: The close control in model_setup_screen.dart is missing an
accessible label, unlike the matching button in coach_chat_app_bar.dart. Update
the AppBar leading IconButton used when showClose is true to include the same
accessibility treatment: add a tooltip and wrap it with a Semantics label/button
role so screen readers announce it properly. Use the existing close button
implementation in coach_chat_app_bar.dart as the reference for the IconButton
setup.
In `@lib/features/dashboard/presentation/pages/run_history_page.dart`:
- Around line 118-135: The full-page retry error layout in run_history_page
should not be duplicated; reuse KynosInlineErrorCard or extract a shared
full-page error widget for the same message + retry pattern used in
run_route_page and coach_chat_page. Update the error text in the relevant
builder to avoid showing the raw exception directly to users, and log the
technical details separately via logger. Also align the error styling with the
shared error treatment by using the same error color convention as
KynosInlineErrorCard instead of kynos.secondaryLabel.
In `@lib/features/dashboard/presentation/pages/run_route_missing_page.dart`:
- Around line 19-28: The back-button handler in the route-missing page
duplicates the same pop-or-fallback navigation logic used elsewhere. Extract the
repeated `context.canPop() / context.pop() / context.go(fallback)` behavior into
a shared helper such as `popOrGo(BuildContext context, String fallbackRoute)` in
a common utils location, then update the `IconButton` callback here to call that
helper so the navigation behavior stays centralized across
`run_route_missing_page.dart`, `model_setup_screen.dart`, and
`coach_chat_app_bar.dart`.
In `@lib/features/dashboard/presentation/widgets/character_glance_card.dart`:
- Around line 65-78: The “Train weakest” row in CharacterGlanceCard is still
using GoogleFonts.inter, which is inconsistent with the typography/theme
migration used elsewhere. Update the Text widgets in character_glance_card.dart
to use Theme.of(context).textTheme styles combined with context.kynosTheme
colors, matching the patterns used in character_page.dart and
gait_teaser_card.dart, and remove the direct GoogleFonts.inter usage from this
widget.
In `@lib/features/dashboard/presentation/widgets/readiness_card.dart`:
- Around line 147-157: The insights error state in readiness_card.dart is static
and does not offer a retry path, unlike the readiness summary and the nearby
KynosInlineErrorCard pattern. Update the todayInsightsState.when error branch in
the ReadinessCard widget to render a retry affordance instead of only the
“Insights unavailable” Text, wiring it to the same retry callback/action used by
the other inline error state so users can recover consistently.
In `@lib/features/dashboard/providers/post_run_debrief_provider.dart`:
- Around line 79-84: The save flow is still using a nullable Failure return from
saveCharacter, which is inconsistent with the repository’s record-based API.
Update CharacterRepository.saveCharacter and its implementation to return the
same named record shape used by the other methods, then adjust
PostRunDebriefProvider’s save handling to read the record fields instead of
checking for null so success and failure are handled consistently.
In `@lib/features/settings/providers/health_import_provider.dart`:
- Around line 16-83: HealthImportState is a hand-written mutable-style state
model with custom copyWith and clear flags, but it should follow the project
convention of using `@freezed` for immutability, equality, and hashCode. Refactor
the HealthImportState class in health_import_provider.dart to a Freezed data
class, keeping the existing fields and state semantics used by HealthImportState
and its copyWith behavior. If explicit null clearing is still needed for fields
like error, progressMessage, and importedWorkout, preserve that via a
Freezed-friendly sentinel/wrapper approach rather than manual boilerplate.
In `@lib/shared/widgets/gait_model_card.dart`:
- Line 86: The shared loading widget is missing progress semantics, so screen
readers no longer get a busy/loading announcement when KynosLoadingLine is used.
Update KynosLoadingLine itself to include appropriate Semantics (for example, a
label and liveRegion on the widget’s root) so every caller, including
gait_model_card.dart and week_momentum_card.dart, inherits accessible loading
behavior without duplicating wrapper logic.
In `@lib/shared/widgets/kynos_page_dots.dart`:
- Around line 24-35: The page indicator currently adds a separate Semantics node
for each dot in kynos_page_dots.dart, which creates unnecessary screen reader
traversal. Update the page indicator widget to use a single Semantics wrapper
around the entire Row in the relevant widget/build method, with the label based
on the current active page and the count, and exclude the individual dot
children from accessibility so only one announcement is exposed.
In `@lib/shared/widgets/responsive_center.dart`:
- Around line 1-3: The shared widget is importing theme tokens directly from
core/theme/layout.dart instead of the required theme.dart barrel. Update the
imports in ResponsiveCenter to use package:kynos/core/theme/theme.dart for
token/theme access, and remove the direct layout.dart import so the widget
follows the shared-widget guideline consistently.
In `@test/features/onboarding/onboarding_page_test.dart`:
- Around line 1-27: The onboarding widget test only covers initial rendering and
misses the async navigation paths in OnboardingPage, especially the
_skipOnboarding and _getStarted flows. Add widget tests that tap the Skip and
Get Started actions, override healthPermissionsProvider (and any other required
providers) to control the permission branch, and assert the expected navigation
or provider-state changes after the taps. Use the existing OnboardingPage and
onboarding_provider symbols to locate the test targets.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 56ef8a42-0232-4d98-bef2-36269bf41484
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (77)
README.mdlib/app/not_found_page.dartlib/app/router.dartlib/app/shell_page.dartlib/core/constants/app_constants.dartlib/core/theme/layout.dartlib/features/character/presentation/pages/character_page.dartlib/features/character/presentation/widgets/gamekit_panel.dartlib/features/character/presentation/widgets/quest_card.dartlib/features/character/presentation/widgets/trail_run_game_panel.dartlib/features/character/providers/adventure_provider.dartlib/features/character/providers/adventure_provider.g.dartlib/features/character/providers/character_provider.dartlib/features/character/providers/quest_provider.dartlib/features/character/providers/quest_provider.g.dartlib/features/coach_chat/presentation/pages/coach_chat_page.dartlib/features/coach_chat/presentation/widgets/coach_chat_app_bar.dartlib/features/coach_chat/presentation/widgets/message_list.dartlib/features/coach_chat/presentation/widgets/model_setup_screen.dartlib/features/dashboard/presentation/pages/dashboard_page.dartlib/features/dashboard/presentation/pages/run_history_page.dartlib/features/dashboard/presentation/pages/run_route_missing_page.dartlib/features/dashboard/presentation/pages/run_route_page.dartlib/features/dashboard/presentation/widgets/acwr_guardrail_card.dartlib/features/dashboard/presentation/widgets/character_glance_card.dartlib/features/dashboard/presentation/widgets/connect_healthkit_card.dartlib/features/dashboard/presentation/widgets/daily_quest_teaser.dartlib/features/dashboard/presentation/widgets/gait_teaser_card.dartlib/features/dashboard/presentation/widgets/last_run_preview.dartlib/features/dashboard/presentation/widgets/metric_detail_sheet.dartlib/features/dashboard/presentation/widgets/readiness_card.dartlib/features/dashboard/presentation/widgets/today_insight_cards.dartlib/features/dashboard/presentation/widgets/trend_carousel.dartlib/features/dashboard/presentation/widgets/week_momentum_card.dartlib/features/dashboard/providers/post_run_debrief_provider.dartlib/features/dashboard/providers/post_run_debrief_provider.g.dartlib/features/onboarding/presentation/onboarding_page.dartlib/features/onboarding/providers/onboarding_provider.dartlib/features/onboarding/providers/onboarding_provider.g.dartlib/features/settings/presentation/pages/health_import_page.dartlib/features/settings/presentation/pages/manual_run_page.dartlib/features/settings/presentation/pages/settings_page.dartlib/features/settings/presentation/widgets/health_import_progress_card.dartlib/features/settings/presentation/widgets/settings_appearance_section.dartlib/features/settings/providers/health_import_provider.dartlib/features/settings/providers/health_import_provider.g.dartlib/features/settings/providers/manual_run_provider.dartlib/features/settings/providers/manual_run_provider.g.dartlib/features/settings/providers/settings_provider.dartlib/features/settings/providers/settings_provider.g.dartlib/features/training/presentation/pages/training_page.dartlib/features/training/presentation/widgets/past_runs_list.dartlib/features/training/presentation/widgets/trend_cards.dartlib/infrastructure/health/health_infrastructure_providers.dartlib/main.dartlib/shared/providers/character_providers.dartlib/shared/providers/character_providers.g.dartlib/shared/providers/health_import_providers.dartlib/shared/providers/health_providers.dartlib/shared/providers/health_providers.g.dartlib/shared/utils/health_platform_labels.dartlib/shared/utils/url_opener.dartlib/shared/widgets/charts/chart_placeholder.dartlib/shared/widgets/charts/hrv_chart.dartlib/shared/widgets/charts/load_chart.dartlib/shared/widgets/gait_model_card.dartlib/shared/widgets/kynos_inline_error_card.dartlib/shared/widgets/kynos_page_dots.dartlib/shared/widgets/metric_tile.dartlib/shared/widgets/responsive_center.dartlib/shared/widgets/run_card.dartlib/shared/widgets/widgets.dartpubspec.yamltest/features/character/quest_provider_test.darttest/features/onboarding/onboarding_page_test.darttest/features/settings/health_import_progress_card_test.darttest/features/settings/settings_page_test.dart
💤 Files with no reviewable changes (5)
- test/features/settings/health_import_progress_card_test.dart
- lib/features/dashboard/presentation/widgets/acwr_guardrail_card.dart
- lib/features/settings/presentation/widgets/health_import_progress_card.dart
- lib/features/dashboard/presentation/widgets/today_insight_cards.dart
- lib/infrastructure/health/health_infrastructure_providers.dart
| class ManualRunState { | ||
| const ManualRunState({ | ||
| required this.start, | ||
| this.isSaving = false, | ||
| this.error, | ||
| this.saveSucceeded = false, | ||
| }); | ||
|
|
||
| final DateTime start; | ||
| final bool isSaving; | ||
| final String? error; | ||
| final bool saveSucceeded; | ||
|
|
||
| ManualRunState copyWith({ | ||
| DateTime? start, | ||
| bool? isSaving, | ||
| String? error, | ||
| bool? saveSucceeded, | ||
| bool clearError = false, | ||
| }) { | ||
| return ManualRunState( | ||
| start: start ?? this.start, | ||
| isSaving: isSaving ?? this.isSaving, | ||
| error: clearError ? null : (error ?? this.error), | ||
| saveSucceeded: saveSucceeded ?? this.saveSucceeded, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
ManualRunState should use @freezed per project guidelines.
This is a hand-rolled class with manual copyWith. Per coding guidelines, data models must use @freezed (plus @JsonSerializable where relevant) for immutability, copyWith, equality, and hashCode.
As per coding guidelines, lib/**: "All data models must use @freezed for immutability, copyWith, equality, and hashCode."
🤖 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/features/settings/providers/manual_run_provider.dart` around lines 9 -
36, ManualRunState is a hand-rolled mutable-style data model and should be
converted to a Freezed model to match project guidelines. Update ManualRunState
in manual_run_provider.dart to use `@freezed` (and `@JsonSerializable` only if
needed), remove the manual copyWith implementation, and define the state as an
immutable union/data class so Freezed generates copyWith, equality, and
hashCode. Keep the existing fields and defaults intact while moving the state
definition to the generated Freezed pattern used elsewhere in the codebase.
Source: Coding guidelines
| @@ -1,7 +1,7 @@ | |||
| import 'package:flutter/material.dart'; | |||
| import 'package:kynos/domain/entities/cloud_data_level.dart'; | |||
| import 'package:kynos/features/onboarding/providers/onboarding_provider.dart'; | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Cross-feature provider import: move sharedPreferencesProvider to shared/providers/.
settings_provider.dart now imports sharedPreferencesProvider from the onboarding feature. This is a cross-feature provider dependency and forces other consumers (e.g. the settings widget test) to import onboarding internals for an unrelated, general-purpose dependency.
As per coding guidelines, "Do not import one feature's providers from another feature," and "All providers must live inside the feature they serve, or in shared/providers/."
♻️ Suggested direction
-import 'package:kynos/features/onboarding/providers/onboarding_provider.dart';
+import 'package:kynos/shared/providers/shared_preferences_provider.dart';Move the sharedPreferencesProvider definition itself out of onboarding_provider.dart into a new lib/shared/providers/shared_preferences_provider.dart, then update all importers (onboarding, settings, tests, and the app-level override).
Also applies to: 42-44, 59-80, 89-92, 100-103
🤖 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/features/settings/providers/settings_provider.dart` at line 3,
`settings_provider.dart` is depending on `sharedPreferencesProvider` from the
onboarding feature, which violates the shared-provider boundary and creates
cross-feature imports. Move the `sharedPreferencesProvider` definition out of
`onboarding_provider.dart` into a dedicated shared provider under
`shared/providers/`, then update `settings_provider.dart`, onboarding code,
tests, and the app-level override to import the new shared location. Use the
existing `sharedPreferencesProvider` symbol as the migration target and remove
the onboarding import from settings once the provider is relocated.
Source: Coding guidelines
Wire shell branch navigation for dashboard shortcuts, fix onboarding health-import handoff race, and use errorToken for repeated import/manual-run failures. Move sharedPreferencesProvider to shared/providers, add popOrGo helper, improve inline error cards and accessibility semantics. Co-authored-by: Youri Bontekoe <YKDBontekoe@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/shared/widgets/kynos_page_dots.dart (1)
2-2: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winImport tokens via
theme.dart, not directly fromspacing.dart.As per coding guidelines, "Shared widgets must import tokens and theme via
package:kynos/core/theme/theme.dart, usecontext.kynosTheme, useGapfor spacing...". This file importspackage:kynos/core/theme/spacing.dartdirectly under atokensalias, bypassing thetheme.dartboundary.🤖 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/shared/widgets/kynos_page_dots.dart` at line 2, The shared widget is importing spacing tokens directly from spacing.dart instead of going through the theme boundary. Update kynos_page_dots.dart to import package:kynos/core/theme/theme.dart and use the theme/tokens exposed there rather than the direct tokens alias from spacing.dart, keeping the widget aligned with the shared widget guidelines and using the existing theme access patterns in this file.Source: Coding guidelines
🧹 Nitpick comments (1)
lib/shared/widgets/kynos_page_dots.dart (1)
27-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic-number offsets on token values.
tokens.Spacing.xs + 2andtokens.Spacing.xs - 1obscure the intended dot size/margin. Prefer dedicated named constants (or exact token values) instead of ad-hoc arithmetic on tokens.🤖 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/shared/widgets/kynos_page_dots.dart` around lines 27 - 37, The dot sizing and spacing in kynos_page_dots.dart use ad-hoc arithmetic on tokens.Spacing values, which obscures intent. Update the widget-building logic in the dot Container to replace tokens.Spacing.xs + 2 and tokens.Spacing.xs - 1 with explicit named constants or exact token values, and keep the sizing/margin definitions centralized and readable within the dot rendering code.
🤖 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/features/coach_chat/presentation/widgets/model_setup_screen.dart`:
- Around line 52-64: The AppBar close control in model_setup_screen.dart has
redundant accessibility semantics because IconButton.tooltip already provides
the accessible label. Update the AppBar leading widget so the close action is
exposed by one source only, either remove the outer Semantics wrapper around the
IconButton or remove the tooltip, and keep the close action using the existing
popOrGo(context, Routes.dashboard) handler.
---
Outside diff comments:
In `@lib/shared/widgets/kynos_page_dots.dart`:
- Line 2: The shared widget is importing spacing tokens directly from
spacing.dart instead of going through the theme boundary. Update
kynos_page_dots.dart to import package:kynos/core/theme/theme.dart and use the
theme/tokens exposed there rather than the direct tokens alias from
spacing.dart, keeping the widget aligned with the shared widget guidelines and
using the existing theme access patterns in this file.
---
Nitpick comments:
In `@lib/shared/widgets/kynos_page_dots.dart`:
- Around line 27-37: The dot sizing and spacing in kynos_page_dots.dart use
ad-hoc arithmetic on tokens.Spacing values, which obscures intent. Update the
widget-building logic in the dot Container to replace tokens.Spacing.xs + 2 and
tokens.Spacing.xs - 1 with explicit named constants or exact token values, and
keep the sizing/margin definitions centralized and readable within the dot
rendering code.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ae7c03e7-620f-4592-93b5-d7de2e54590a
📒 Files selected for processing (37)
lib/app/router.dartlib/app/shell_navigation_scope.dartlib/app/shell_page.dartlib/features/character/presentation/pages/character_page.dartlib/features/character/presentation/widgets/gamekit_panel.dartlib/features/character/presentation/widgets/trail_run_game_panel.dartlib/features/character/providers/quest_provider.dartlib/features/character/providers/quest_provider.g.dartlib/features/coach_chat/presentation/widgets/coach_chat_app_bar.dartlib/features/coach_chat/presentation/widgets/model_setup_screen.dartlib/features/dashboard/presentation/pages/run_history_page.dartlib/features/dashboard/presentation/pages/run_route_missing_page.dartlib/features/dashboard/presentation/widgets/character_glance_card.dartlib/features/dashboard/presentation/widgets/gait_teaser_card.dartlib/features/dashboard/presentation/widgets/readiness_card.dartlib/features/onboarding/presentation/onboarding_page.dartlib/features/onboarding/providers/onboarding_provider.dartlib/features/onboarding/providers/onboarding_provider.g.dartlib/features/settings/presentation/pages/health_import_page.dartlib/features/settings/presentation/pages/manual_run_page.dartlib/features/settings/providers/health_import_provider.dartlib/features/settings/providers/health_import_provider.g.dartlib/features/settings/providers/manual_run_provider.dartlib/features/settings/providers/manual_run_provider.g.dartlib/features/settings/providers/settings_provider.dartlib/infrastructure/health/platform_imported_health_store_web.dartlib/main.dartlib/shared/providers/shared_preferences_provider.dartlib/shared/providers/shared_preferences_provider.g.dartlib/shared/utils/navigation_utils.dartlib/shared/utils/url_opener.dartlib/shared/widgets/kynos_loading_line.dartlib/shared/widgets/kynos_page_dots.dartlib/shared/widgets/responsive_center.darttest/features/onboarding/onboarding_page_test.darttest/features/settings/settings_page_test.darttest/widget_test.dart
💤 Files with no reviewable changes (1)
- lib/features/onboarding/providers/onboarding_provider.g.dart
✅ Files skipped from review due to trivial changes (6)
- lib/shared/utils/navigation_utils.dart
- lib/features/character/providers/quest_provider.g.dart
- lib/shared/providers/shared_preferences_provider.g.dart
- lib/infrastructure/health/platform_imported_health_store_web.dart
- lib/features/settings/providers/health_import_provider.g.dart
- lib/features/settings/providers/manual_run_provider.g.dart
🚧 Files skipped from review as they are similar to previous changes (20)
- lib/features/character/presentation/widgets/gamekit_panel.dart
- test/features/settings/settings_page_test.dart
- lib/shared/utils/url_opener.dart
- lib/features/dashboard/presentation/pages/run_route_missing_page.dart
- lib/shared/widgets/responsive_center.dart
- lib/features/character/presentation/widgets/trail_run_game_panel.dart
- lib/features/dashboard/presentation/widgets/gait_teaser_card.dart
- lib/main.dart
- lib/features/dashboard/presentation/widgets/character_glance_card.dart
- lib/features/settings/providers/manual_run_provider.dart
- lib/features/dashboard/presentation/pages/run_history_page.dart
- lib/features/settings/presentation/pages/manual_run_page.dart
- lib/features/character/providers/quest_provider.dart
- lib/features/character/presentation/pages/character_page.dart
- lib/features/settings/providers/settings_provider.dart
- lib/features/settings/presentation/pages/health_import_page.dart
- lib/features/coach_chat/presentation/widgets/coach_chat_app_bar.dart
- test/features/onboarding/onboarding_page_test.dart
- lib/features/onboarding/presentation/onboarding_page.dart
- lib/features/settings/providers/health_import_provider.dart
| appBar: showClose | ||
| ? AppBar( | ||
| leading: Semantics( | ||
| label: 'Close', | ||
| button: true, | ||
| child: IconButton( | ||
| icon: const Icon(Icons.close_rounded), | ||
| tooltip: 'Close', | ||
| onPressed: () => popOrGo(context, Routes.dashboard), | ||
| ), | ||
| ), | ||
| ) | ||
| : null, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Flutter IconButton tooltip semantics duplicate Semantics label Tooltip accessibility
💡 Result:
In Flutter, the IconButton widget's tooltip property serves as the primary mechanism for both providing a visual description on long-press and defining the accessible label for screen readers [1][2][3]. When you provide a string to the tooltip property of an IconButton, Flutter automatically includes that text in the semantics tree for accessibility [2][3][4]. Because the IconButton widget internally handles this mapping, providing an additional Semantics wrapper with a redundant label can lead to accessibility issues, such as duplicate announcements or confusing reading orders [5][6]. Key points regarding accessibility and duplication: 1. Unified Labeling: The tooltip property is intended to act as the accessibility label for the button [7]. Using it effectively replaces the need for a separate semantic label in most cases [8][7]. 2. Avoiding Duplication: If you use both a Tooltip and an explicit Semantics label, screen readers may announce the information twice or in an unexpected order [6]. It is best practice to rely on the tooltip property for icon-only buttons [7]. 3. Internal Implementation: The IconButton widget wraps its internal child in a Tooltip widget (when a tooltip is provided) and then wraps the entire result in a Semantics widget [4]. This structure ensures that the tooltip message is the primary source for screen reader announcements [2]. 4. Troubleshooting Screen Readers: If you encounter issues where a tooltip is not being announced, ensure that the button is not being obscured by other semantics nodes and verify that you are not accidentally overriding the label with an external Semantics wrapper [6]. In summary, for standard icon buttons, you should define the accessibility label directly via the tooltip property and avoid wrapping the widget in additional Semantics nodes unless you need to override the default behavior entirely [7][4][6].
Citations:
- 1: https://api.flutter.dev/flutter/material/IconButton/tooltip.html
- 2: https://api.flutter.dev/flutter/material/Tooltip-class.html
- 3: https://api.flutter.dev/flutter/material/IconButton-class.html
- 4: https://api.flutter.dev/flutter/material/IconButton/build.html
- 5: IconButton has wrong talk back reading order when only set tooltip, not set semanticLabel flutter/flutter#105378
- 6: [Android] Tooltip are not read by the screen reader flutter/flutter#162509
- 7: https://dcm.dev/blog/2025/06/30/accessibility-flutter-practical-tips-tools-code-youll-actually-use/
- 8: IconButton Semantics Label flutter/flutter#148167
Remove the redundant semantics wrapper or tooltip
IconButton.tooltip already contributes the accessible label, so wrapping the button in Semantics(label: 'Close', button: true) can make screen readers announce the control twice. Keep one source of semantics only.
🤖 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/features/coach_chat/presentation/widgets/model_setup_screen.dart` around
lines 52 - 64, The AppBar close control in model_setup_screen.dart has redundant
accessibility semantics because IconButton.tooltip already provides the
accessible label. Update the AppBar leading widget so the close action is
exposed by one source only, either remove the outer Semantics wrapper around the
IconButton or remove the tooltip, and keep the close action using the existing
popOrGo(context, Routes.dashboard) handler.
## [1.12.2](v1.12.1...v1.12.2) (2026-07-06) ### Bug Fixes * implement ui/ux bug backlog across navigation, errors, and theme ([#63](#63)) ([852b07f](852b07f))
|
🎉 This PR is included in version 1.12.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
2 participants
Summary
Implements the full UI/UX bug & improvement backlog (49 tasks) across bugs, visual glitches, navigation, error UX, theme/dark mode, architecture, accessibility, web responsiveness, cleanup, and tests.
P0 — Bugs & data integrity
saveCharacterAsyncErrorin trail panelurl_launcher)P1 — Visual glitches & navigation
ReadinessCard)HealthPlatformLabels) across settings/character/manual runMetricTilevariant to fix nested card stylingStatefulShellRoutefor/,/training,/characterdeep linksRunRouteMissingPagefor cold/run-routeloads;NotFoundPageerror builderP1 — Error UX & onboarding
KynosInlineErrorCardwith retry across training, quests, daily quest teaser, readinessP2 — Polish, a11y, web, architecture
ResponsiveCenterwrappershared/healthSummaryfailure handling; exportinvalidateHealthProvidersAsyncErrorP3 — Cleanup & tests
today_insight_cardsandacwr_guardrail_card/nexus-labroute; link from Training tabSettingsAppearanceSection; add replay onboarding actionValidation
flutter analyze— no errorsflutter test— 130 tests passingflutter build web— succeedsSummary by CodeRabbit
New Features
Bug Fixes