feat: implement 35 quality-of-life improvements across KYNOS - #64
Merged
Merged
Conversation
Add consistent error recovery with KynosInlineErrorCard, confirmation dialogs for destructive actions, pull-to-refresh on major tabs, and health data refresh on app resume. Improve coach chat with persistence, stop streaming, copy messages, and AI reconnect after background. Support deep-linkable run routes, global quest auto-complete, onboarding polish, accessibility labels, and friendlier permission error copy. Co-authored-by: Youri Bontekoe <YKDBontekoe@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (38)
📝 WalkthroughWalkthroughThis PR adds deep-linked run route resolution, coach chat cancellation/persistence/reconnect handling with phase-based model setup, relocates measurable quest sync into a global provider, adds pull-to-refresh and standardized error cards across dashboard/training/character pages, and adds confirmation dialogs to onboarding, settings, and health import flows. ChangesRun Route Deep Linking
Coach Chat Cancellation, Persistence & UI Polish
AI Reconnect Lifecycle & Measurable Quest Sync Relocation
Dashboard/Training Refresh & Standardized Error States
Onboarding & Settings Confirmation Flows
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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/features/settings/presentation/pages/settings_page.dart (1)
30-30: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile exceeds the ~250-line guideline for hand-written Dart files.
_SettingsPageStatespans well beyond 250 lines (shown through Line 474). As per coding guidelines, "Keep hand-written files under ~250 lines; split larger code intopresentation/widgets/." Consider extracting the "Health & Data", "AI & Cloud", and "Legal"KynosCardsections into separate widgets underlib/features/settings/presentation/widgets/.🤖 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/presentation/pages/settings_page.dart` at line 30, _SettingsPageState in SettingsPage is too large and should be split into smaller hand-written widgets to meet the ~250-line guideline. Extract the existing “Health & Data”, “AI & Cloud”, and “Legal” KynosCard sections into separate widget classes under presentation/widgets/, then compose them back inside build so the page stays thin. Keep the main SettingsPage and _SettingsPageState focused on layout and navigation wiring, and move each card’s UI into its own reusable widget with clear names.Source: Coding guidelines
lib/features/settings/presentation/pages/openrouter_model_picker_page.dart (2)
194-210: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSnackBar likely won't be visible — shown right before popping its own page.
ScaffoldMessenger.of(context).showSnackBar(...)is attached to this page's Scaffold, butcontext.pop()is called immediately after, popping that same page/Scaffold off the stack. The confirmation message this PR intends to add will likely be dismissed before the user can see it.🩹 Proposed fix (show the confirmation on the previous page)
if (ctx.mounted) { Navigator.pop(ctx); } - if (context.mounted) { - ScaffoldMessenger.of(context).showSnackBar( - SnackBar(content: Text('Selected ${model.name}')), - ); - context.pop(); - } + if (context.mounted) { + context.pop(); + rootScaffoldMessengerKey.currentState?.showSnackBar( + SnackBar(content: Text('Selected ${model.name}')), + ); + }Alternatively, pass a result back via
context.pop(model)and show the snackbar in the caller after navigation completes, or use an app-scopedScaffoldMessengerKeythat outlives the popped route.🤖 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/presentation/pages/openrouter_model_picker_page.dart` around lines 194 - 210, The confirmation snackbar in the model picker flow is being shown on the same route that is immediately popped, so it will not stay visible. Update the `FilledButton` handler in `openrouter_model_picker_page.dart` to avoid calling `ScaffoldMessenger.of(context).showSnackBar` right before `context.pop()`, and instead return the selected model/result from this page (for example via `Navigator.pop`/`context.pop`) so the caller can show the snackbar after navigation, or use an app-scoped messenger that survives the pop. Refer to the `onPressed` callback, `Navigator.pop(ctx)`, and `context.pop()` flow when making the change.
90-102: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid surfacing raw OpenRouter exception text
lib/infrastructure/ai/openrouter/openrouter_api_client.dartreturnse.message ?? e.toString(), soKynosInlineErrorCardcan expose transport/exception details to users. Map this to a generic message here and keep the raw error out of the UI.🤖 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/presentation/pages/openrouter_model_picker_page.dart` around lines 90 - 102, The OpenRouter error handling in openrouter_model_picker_page.dart is passing raw exception text from result.error into KynosInlineErrorCard. Update the data branch in the page’s builder to map result.error to a generic user-facing message instead of displaying the underlying transport/exception details, while keeping the retry action unchanged. Use the existing openRouterCatalogDataProvider and KynosInlineErrorCard entry point to ensure only sanitized UI text is shown.lib/features/coach_chat/providers/model_setup_provider.dart (1)
1-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep
GemmaRuntimeout oflib/features/imports.lib/features/coach_chat/providers/model_setup_provider.dartstill pullsGemmaRuntimedirectly frominfrastructure/ai/gemma/gemma_runtime.dart; expose it throughshared/providers/instead.🤖 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/providers/model_setup_provider.dart` around lines 1 - 9, The provider setup still imports GemmaRuntime directly from infrastructure, which keeps feature code coupled to that layer. Update model_setup_provider to depend on a shared provider export instead of importing gemma_runtime.dart directly, and move any GemmaRuntime access behind shared/providers so the feature only references the provider symbols it needs.Source: Coding guidelines
🧹 Nitpick comments (8)
lib/features/coach_chat/presentation/widgets/assistant_bubble.dart (2)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLong-press copy affordance isn't discoverable/announced.
The long-press-to-copy gesture has no visual or semantic hint (e.g. tooltip or
Semanticshint like "double tap and hold to copy"), so sighted users and screen-reader users alike have no cue this action exists.🤖 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/assistant_bubble.dart` around lines 38 - 39, The long-press copy action in AssistantBubble is not discoverable because the GestureDetector only wires onLongPress without any visible or semantic cue. Update the assistant bubble widget to expose a clear hint using Semantics and/or a tooltip-style affordance around the GestureDetector so users know they can long-press to copy, and keep the hint tied to the content.isEmpty gating and _copyMessage(context) behavior.
23-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated clipboard-copy logic across widgets.
_copyMessagehere is nearly identical to the one added inlib/shared/widgets/kynos_user_bubble.dart(Lines 14-20). Consider extracting a shared helper (e.g.copyToClipboardWithFeedback(BuildContext, String)) intoshared/to avoid duplicating the clipboard + snackbar logic and keep the messaging consistent between assistant and user bubbles.♻️ Example shared helper
// lib/shared/utils/clipboard_copy.dart Future<void> copyTextWithFeedback(BuildContext context, String text) async { if (text.isEmpty) return; await Clipboard.setData(ClipboardData(text: text)); if (!context.mounted) return; ScaffoldMessenger.of(context).showSnackBar( const SnackBar(content: Text('Message copied')), ); }🤖 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/assistant_bubble.dart` around lines 23 - 30, The clipboard-copy flow is duplicated between assistant_bubble’s _copyMessage and the matching logic in kynos_user_bubble, so extract the shared Clipboard + SnackBar behavior into a reusable helper under shared/ (for example, a utility like copyTextWithFeedback) and update both widgets to call it. Keep the empty-text guard, mounted check, and copy confirmation message in the shared helper so assistant and user bubbles stay consistent and the logic lives in one place.lib/features/nexus_lab/presentation/nexus_lab_page.dart (1)
23-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
tooltipto the closeIconButtonfor accessibility.Without a tooltip, screen readers announce this button with no descriptive label. Given this PR's focus on accessibility (semantics/tooltips added elsewhere), consider adding one here too.
♿ Proposed fix
leading: IconButton( icon: const Icon(Icons.close_rounded), + tooltip: 'Close', onPressed: () => popOrGo(context, Routes.training), ),🤖 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/nexus_lab/presentation/nexus_lab_page.dart` around lines 23 - 31, The close IconButton in NexusLabPage’s AppBar is missing an accessibility tooltip, so add a descriptive tooltip to the leading IconButton alongside the existing close icon and popOrGo navigation action. Keep the change localized to the AppBar leading control in nexus_lab_page.dart so screen readers announce the button purpose clearly and consistently with the other accessibility updates in this PR.lib/features/dashboard/presentation/widgets/activity_ring.dart (1)
31-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSemantics wiring looks correct.
CustomPaintdoesn't inject its own semantics nodes, so wrapping with a singleSemantics(label: ...)here won't cause duplicate announcements. Generic "Ring 1/2/3" labels are a minor readability limitation since the widget has no context of what each ring represents, but that's a nice-to-have, not a defect.♻️ Optional: accept per-ring labels for richer semantics
const ActivityRing({ super.key, this.progress, this.ringProgresses, + this.ringLabels, required this.size, required this.strokeWidth, required this.colors, }); + + /// Optional human-readable names for each ring, outer ring first. + final List<String>? ringLabels;String _semanticsLabel(List<double> progresses) { final parts = <String>[]; for (var i = 0; i < progresses.length; i++) { final pct = (progresses[i] * 100).round(); - parts.add('Ring ${i + 1} $pct percent'); + final name = (ringLabels != null && i < ringLabels!.length) + ? ringLabels![i] + : 'Ring ${i + 1}'; + parts.add('$name $pct percent'); } return parts.isEmpty ? 'Activity rings' : 'Activity rings: ${parts.join(', ')}'; }🤖 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/activity_ring.dart` around lines 31 - 54, The current Semantics wiring in ActivityRing is fine, but the per-ring labels in _semanticsLabel are generic and lack context. If you want richer accessibility, update ActivityRing to accept optional per-ring label text and use that in _semanticsLabel instead of “Ring 1/2/3”, while keeping the single Semantics wrapper around CustomPaint. Preserve the existing fallback behavior when no labels are provided.lib/features/onboarding/presentation/onboarding_page.dart (1)
82-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirmation dialog and mounted guard look correct.
The dialog flow and
mountedcheck before_finishOnboarding()correctly guard against usingBuildContextafter an unmounted widget's async gap. As per coding guidelines,lib/**requires "Do not useBuildContextacross async gaps without a mounted guard."One consolidation opportunity: this same
showDialog<bool>+AlertDialog(title/content/Cancel/Confirm actions) boilerplate is repeated near-verbatim insettings_page.dart(_replayOnboarding,_confirmClearImportedData) andapple_health_export_preview_card.dart(_confirmImport). Extracting a sharedFuture<bool?> showConfirmDialog({title, content, confirmLabel, cancelLabel})helper (e.g., inshared/widgets/) would remove this duplication.♻️ Example shared helper
Future<bool> showConfirmDialog( BuildContext context, { required String title, required String content, String cancelLabel = 'Cancel', String confirmLabel = 'Confirm', }) async { final confirmed = await showDialog<bool>( context: context, builder: (context) => AlertDialog( title: Text(title), content: Text(content), actions: [ TextButton( onPressed: () => Navigator.pop(context, false), child: Text(cancelLabel), ), FilledButton( onPressed: () => Navigator.pop(context, true), child: Text(confirmLabel), ), ], ), ); return confirmed ?? false; }🤖 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/onboarding/presentation/onboarding_page.dart` around lines 82 - 105, The _skipOnboarding dialog flow and mounted guard are fine; the remaining issue is duplicated confirmation-dialog boilerplate. Extract the repeated showDialog<bool>/AlertDialog pattern used by _skipOnboarding, _replayOnboarding, _confirmClearImportedData, and _confirmImport into a shared helper (for example a confirm dialog utility in shared/widgets/) that accepts title, content, and button labels. Update those call sites to use the helper so the dialog behavior stays consistent and the duplication is removed.Source: Coding guidelines
lib/shared/providers/health_providers.dart (1)
88-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBroad
on Objectcatch silently swallows all errors, including bugs.Catching
Objectmasks unrelated failures (network issues, bugs,Errorsubtypes) as "permission not granted," and there's no logging to distinguish real errors from actual denial. Consider narrowing the catch and logging unexpected exceptions vialogger.🔧 Proposed fix
Future<bool> build() async { if (kIsWeb) return false; try { final repo = ref.read(healthRepositoryProvider); final result = await repo.getToday(); return result.failure == null; - } on Object { + } on Object catch (e, st) { + logger.w('Failed to determine health permission status', error: e, stackTrace: st); return false; } }🤖 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/providers/health_providers.dart` around lines 88 - 97, The broad on Object catch in build() is swallowing all failures in HealthProviders, which can hide bugs and non-permission issues. Update the catch around repo.getToday() to only handle the expected denial case, and add logging for unexpected exceptions using the existing logger so real errors are distinguishable from a false “permission not granted” result.lib/features/character/presentation/pages/character_page.dart (1)
185-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent use of
HealthPermissionFeedbackwithin the same handler.The
errorbranch now usesHealthPermissionFeedback.connectionFailedMessage(), but thedatabranch just above still hardcodes'$platform connected.'/'$platform permission not granted. ...'instead of reusingHealthPermissionFeedback.connectedMessage(platform)/.permissionDeniedMessage(platform), which produce identical strings and are already used this way inconnect_healthkit_card.dart.♻️ Proposed fix
ref.read(healthPermissionsProvider).whenOrNull( data: (granted) { - final message = granted - ? '$platform connected.' - : '$platform permission not granted. ${HealthPlatformLabels.settingsHint()}'; + final message = granted + ? HealthPermissionFeedback.connectedMessage(platform) + : HealthPermissionFeedback.permissionDeniedMessage(platform); ScaffoldMessenger.of(context).showSnackBar( SnackBar(content: Text(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/character/presentation/pages/character_page.dart` around lines 185 - 203, The handler in character_page.dart is mixing hardcoded snackbar text with HealthPermissionFeedback, so update the data branch in the ref.read(healthPermissionsProvider).whenOrNull callback to reuse the same message helpers as the rest of the app. Replace the inline '$platform connected.' and '$platform permission not granted. ...' strings with HealthPermissionFeedback.connectedMessage(platform) and HealthPermissionFeedback.permissionDeniedMessage(platform), keeping the existing error branch unchanged. This keeps the messaging consistent with connect_healthkit_card.dart and centralizes the wording in HealthPermissionFeedback.lib/features/coach_chat/utils/chat_history_codec.dart (1)
30-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize
ChatMessageserialization
lib/features/coach_chat/utils/chat_history_codec.dart:30-49is the persistence contract forChatMessage, so keeping the field mapping here means any model change has to be updated in two places. Move the JSON mapping ontoChatMessageitself, or generate it, so storage stays aligned as fields evolve.🤖 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/utils/chat_history_codec.dart` around lines 30 - 51, Centralize the ChatMessage persistence mapping instead of keeping field-by-field JSON handling in ChatHistoryCodec’s _toMap and _fromMap helpers. Move the serialization/deserialization logic onto ChatMessage itself, or replace these helpers with generated mapping, and update ChatHistoryCodec to delegate to that single source of truth so future ChatMessage field changes only need one update point.
🤖 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/router.dart`:
- Around line 109-120: The nested `GoRoute` for `:runId` is ignoring the `extra`
payload that `RunCard._openRoute` already provides, so the page always falls
back to an async lookup. Update the `builder` for the `:runId` route in the
router to check `state.extra` first and use the passed `WorkoutSession` when
available, then fall back to
`state.pathParameters['runId']`/`workoutSessionByIdProvider` only if no usable
extra exists. Keep the existing `RunRouteMissingPage` path for missing IDs and
preserve the current `RunRoutePage` flow.
In `@lib/features/character/presentation/pages/character_page.dart`:
- Around line 30-38: The _refreshCharacter flow invalidates nexusLabProvider but
does not wait for it to reload, so the refresh can complete too early. Update
the Future.wait in _refreshCharacter to include the nexusLabProvider future
alongside runnerCharacterProvider and dailyQuestsProvider, using the existing
provider names in character_page.dart so the pull-to-refresh stays active until
all three data sources finish reloading.
In `@lib/features/coach_chat/presentation/pages/coach_chat_page.dart`:
- Around line 106-118: The reconnect listener in coach_chat_page.dart can
trigger a second model setup while an existing setup/download is still in
progress. Update the aiReconnectStateProvider listener in build() to skip
calling modelSetupProvider.notifier.checkAndInstall() when model setup is
already loading/downloading, or add an in-flight guard inside checkAndInstall()
so it is re-entrant safe; use the existing aiReconnectStateProvider and
modelSetupProvider symbols to keep the reconnect path from overlapping with the
initial post-frame setup.
In `@lib/features/coach_chat/presentation/widgets/assistant_bubble.dart`:
- Around line 38-64: Move the copy action out of the outer GestureDetector in
assistant_bubble.dart and into SelectableText so selection and copy don’t
compete in the gesture arena. Update the widget around content and
_copyMessage(context) to use SelectableText.contextMenuBuilder for the “Copy
message” action, or remove SelectableText entirely and keep GestureDetector only
if text selection is not needed. Keep the existing content.isEmpty behavior and
preserve the current styling and error coloring while making the copy affordance
part of SelectableText.
In `@lib/features/coach_chat/providers/coach_chat_provider.dart`:
- Around line 60-77: The persistence writes in coach_chat_provider are racing
because cancelGeneration() fires an unawaited _persist() while
sendMessage/_runInference may start another _persist() for the same
ChatHistoryCodec.prefsKey. Update the persistence flow so writes are serialized
in order, for example by chaining all _persist() calls through a single shared
Future or queue. Keep the fix centered on cancelGeneration(), _persist(), and
the sendMessage/_runInference path so every state change waits for the previous
disk write to finish.
In `@lib/features/coach_chat/providers/model_setup_state.dart`:
- Around line 7-17: ModelSetupState is still a ձեռ-written value object and
should be converted to a freezed data model so it gets immutable equality,
hashCode, and copyWith behavior. Update the ModelSetupState type in the
coach_chat provider to use `@freezed` and keep its existing fields and derived
isReady getter aligned with the generated model conventions. After the
conversion, regenerate the freezed output with build_runner so any references to
ModelSetupState continue to compile against the generated class.
In `@lib/features/coach_chat/utils/chat_history_codec.dart`:
- Around line 9-20: The chat history decode path in chat_history_codec.dart is
all-or-nothing and silently drops the entire list on any bad entry. Update
ChatHistoryCodec.decode and the _fromMap flow so each item is parsed
independently, skip only malformed messages, and keep valid ones. Add logging
for decode failures with enough context to diagnose unknown MessageRole values,
missing keys, or malformed timestamps instead of swallowing everything in the
blanket catch.
In `@lib/features/dashboard/presentation/pages/dashboard_page.dart`:
- Line 27: The dashboard page is importing a provider directly from the training
feature, which breaks the cross-feature boundary. Update dashboard_page.dart to
stop importing training_insights_provider.dart from the training feature and
instead use a shared-provider entry point under shared/providers; if needed,
move or re-export trainingInsightsStateProvider there so dashboard_page.dart can
import it consistently with the other providers.
- Around line 156-194: The SnackBarAction callback in dashboard_page.dart’s
debrief listener uses BuildContext and ref after the SnackBar is shown, so it
needs its own mounted safeguard. Update the “Read debrief” onPressed inside the
next.whenOrNull data handler to check mounted/context.mounted again before
calling coachChatSeedProvider, context.push(Routes.coachChat), and
postRunDebriefProvider.dismiss(). Keep the existing guard for showing the
SnackBar, but also prevent the callback from using context/ref if DashboardPage
has been disposed.
In `@lib/features/dashboard/presentation/pages/run_route_page.dart`:
- Around line 34-77: The loading, error, and not-found states in
run_route_page.dart do not provide a deep-link-safe way to navigate back when
the route is opened on a cold start. Extract the same AppBar back-navigation
fallback already used by _RunRouteScaffold into a shared helper such as
_runRouteAppBar(context), and reuse it in the loading, error, and session ==
null branches so the app bar can go back when possible or fall back to
context.go(Routes.dashboard) when it cannot pop.
In `@lib/features/training/presentation/pages/training_page.dart`:
- Around line 29-39: The `_refreshTraining` method invalidates
`nexusLabProvider` but does not wait for its refresh to complete, so add its
future to the same `Future.wait` block used for `healthHistoryProvider`,
`recentRunsProvider`, and `trainingInsightsStateProvider`. Update the
`training_page.dart` refresh flow so the `RefreshIndicator` only completes after
`nexusLabProvider` has finished reloading, keeping `GaitModelCardAsync` in sync
with the other training data.
In `@lib/shared/providers/health_providers.dart`:
- Around line 92-93: The permission check in HealthPermissionsNotifier.build()
is incorrectly inferring authorization from repo.getToday() by treating any
failure as “permission denied.” Update the logic to use an explicit
permission/authorization result from the health repository instead of
result.failure == null, and only map permission-specific failures to the denied
state. Keep the change localized around HealthPermissionsNotifier and the
getToday() call site so transient HealthKit/store errors do not affect the
permission UX.
In `@lib/shared/providers/measurable_quest_sync_provider.dart`:
- Around line 18-51: Serialize the fire-and-forget sync flow in
MeasurableQuestSyncProvider so overlapping calls from build() and the two
ref.listen callbacks cannot complete the same quest twice; add a
single-flight/lock around _syncMeasurableQuests() and ensure
questProvider.notifier.completeQuest is still guarded against re-completion.
Also wrap the unawaited _syncMeasurableQuests() invocations with error handling
so failures are caught and logged instead of becoming unhandled async
exceptions.
In `@lib/shared/providers/workout_session_lookup_provider.dart`:
- Around line 9-15: The deep-linked lookup in workoutSessionById is still
constrained by recentRunsProvider(days: 365, limit: 200), so older
shared/bookmarked runs can’t be found. Update workoutSessionById in
workout_session_lookup_provider.dart to perform a direct get-by-id lookup
through the health repository instead of scanning recent runs, using the
existing Ref-based access pattern so the result is not limited by the recent
window.
In `@lib/shared/widgets/ai_lifecycle_guard.dart`:
- Around line 58-64: The `_onAppResumed` flow is using an unsafe `ref as Ref`
cast when calling `invalidateHealthProviders`, which can fail at runtime and
prevent the subsequent reconnect refresh from running. Update
`ai_lifecycle_guard.dart` by removing that cast from `_onAppResumed`; either
inline the health invalidation using `WidgetRef` APIs in this method or
introduce a widget-safe helper that accepts `WidgetRef`, and keep
`markNeedsReconnect()` executing after the health refresh logic.
In `@lib/shared/widgets/metric_tile.dart`:
- Around line 39-47: The Semantics wrapper in MetricTile is announcing a
composed label while the child Text widgets remain in the semantics tree,
causing duplicate screen-reader output. Update the MetricTile build logic so the
outer Semantics excludes the descendant semantics, keeping only the composed
label/button state from the existing valueLabel/onTap handling and preventing
the inner label/value/unit/sublabel Text nodes from being re-announced.
---
Outside diff comments:
In `@lib/features/coach_chat/providers/model_setup_provider.dart`:
- Around line 1-9: The provider setup still imports GemmaRuntime directly from
infrastructure, which keeps feature code coupled to that layer. Update
model_setup_provider to depend on a shared provider export instead of importing
gemma_runtime.dart directly, and move any GemmaRuntime access behind
shared/providers so the feature only references the provider symbols it needs.
In `@lib/features/settings/presentation/pages/openrouter_model_picker_page.dart`:
- Around line 194-210: The confirmation snackbar in the model picker flow is
being shown on the same route that is immediately popped, so it will not stay
visible. Update the `FilledButton` handler in
`openrouter_model_picker_page.dart` to avoid calling
`ScaffoldMessenger.of(context).showSnackBar` right before `context.pop()`, and
instead return the selected model/result from this page (for example via
`Navigator.pop`/`context.pop`) so the caller can show the snackbar after
navigation, or use an app-scoped messenger that survives the pop. Refer to the
`onPressed` callback, `Navigator.pop(ctx)`, and `context.pop()` flow when making
the change.
- Around line 90-102: The OpenRouter error handling in
openrouter_model_picker_page.dart is passing raw exception text from
result.error into KynosInlineErrorCard. Update the data branch in the page’s
builder to map result.error to a generic user-facing message instead of
displaying the underlying transport/exception details, while keeping the retry
action unchanged. Use the existing openRouterCatalogDataProvider and
KynosInlineErrorCard entry point to ensure only sanitized UI text is shown.
In `@lib/features/settings/presentation/pages/settings_page.dart`:
- Line 30: _SettingsPageState in SettingsPage is too large and should be split
into smaller hand-written widgets to meet the ~250-line guideline. Extract the
existing “Health & Data”, “AI & Cloud”, and “Legal” KynosCard sections into
separate widget classes under presentation/widgets/, then compose them back
inside build so the page stays thin. Keep the main SettingsPage and
_SettingsPageState focused on layout and navigation wiring, and move each card’s
UI into its own reusable widget with clear names.
---
Nitpick comments:
In `@lib/features/character/presentation/pages/character_page.dart`:
- Around line 185-203: The handler in character_page.dart is mixing hardcoded
snackbar text with HealthPermissionFeedback, so update the data branch in the
ref.read(healthPermissionsProvider).whenOrNull callback to reuse the same
message helpers as the rest of the app. Replace the inline '$platform
connected.' and '$platform permission not granted. ...' strings with
HealthPermissionFeedback.connectedMessage(platform) and
HealthPermissionFeedback.permissionDeniedMessage(platform), keeping the existing
error branch unchanged. This keeps the messaging consistent with
connect_healthkit_card.dart and centralizes the wording in
HealthPermissionFeedback.
In `@lib/features/coach_chat/presentation/widgets/assistant_bubble.dart`:
- Around line 38-39: The long-press copy action in AssistantBubble is not
discoverable because the GestureDetector only wires onLongPress without any
visible or semantic cue. Update the assistant bubble widget to expose a clear
hint using Semantics and/or a tooltip-style affordance around the
GestureDetector so users know they can long-press to copy, and keep the hint
tied to the content.isEmpty gating and _copyMessage(context) behavior.
- Around line 23-30: The clipboard-copy flow is duplicated between
assistant_bubble’s _copyMessage and the matching logic in kynos_user_bubble, so
extract the shared Clipboard + SnackBar behavior into a reusable helper under
shared/ (for example, a utility like copyTextWithFeedback) and update both
widgets to call it. Keep the empty-text guard, mounted check, and copy
confirmation message in the shared helper so assistant and user bubbles stay
consistent and the logic lives in one place.
In `@lib/features/coach_chat/utils/chat_history_codec.dart`:
- Around line 30-51: Centralize the ChatMessage persistence mapping instead of
keeping field-by-field JSON handling in ChatHistoryCodec’s _toMap and _fromMap
helpers. Move the serialization/deserialization logic onto ChatMessage itself,
or replace these helpers with generated mapping, and update ChatHistoryCodec to
delegate to that single source of truth so future ChatMessage field changes only
need one update point.
In `@lib/features/dashboard/presentation/widgets/activity_ring.dart`:
- Around line 31-54: The current Semantics wiring in ActivityRing is fine, but
the per-ring labels in _semanticsLabel are generic and lack context. If you want
richer accessibility, update ActivityRing to accept optional per-ring label text
and use that in _semanticsLabel instead of “Ring 1/2/3”, while keeping the
single Semantics wrapper around CustomPaint. Preserve the existing fallback
behavior when no labels are provided.
In `@lib/features/nexus_lab/presentation/nexus_lab_page.dart`:
- Around line 23-31: The close IconButton in NexusLabPage’s AppBar is missing an
accessibility tooltip, so add a descriptive tooltip to the leading IconButton
alongside the existing close icon and popOrGo navigation action. Keep the change
localized to the AppBar leading control in nexus_lab_page.dart so screen readers
announce the button purpose clearly and consistently with the other
accessibility updates in this PR.
In `@lib/features/onboarding/presentation/onboarding_page.dart`:
- Around line 82-105: The _skipOnboarding dialog flow and mounted guard are
fine; the remaining issue is duplicated confirmation-dialog boilerplate. Extract
the repeated showDialog<bool>/AlertDialog pattern used by _skipOnboarding,
_replayOnboarding, _confirmClearImportedData, and _confirmImport into a shared
helper (for example a confirm dialog utility in shared/widgets/) that accepts
title, content, and button labels. Update those call sites to use the helper so
the dialog behavior stays consistent and the duplication is removed.
In `@lib/shared/providers/health_providers.dart`:
- Around line 88-97: The broad on Object catch in build() is swallowing all
failures in HealthProviders, which can hide bugs and non-permission issues.
Update the catch around repo.getToday() to only handle the expected denial case,
and add logging for unexpected exceptions using the existing logger so real
errors are distinguishable from a false “permission not granted” result.
🪄 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: a3bdcd04-fefd-4d79-aa58-5debd9c8e1d8
📒 Files selected for processing (43)
lib/app/router.dartlib/features/character/presentation/pages/character_page.dartlib/features/character/presentation/widgets/quest_card.dartlib/features/character/providers/adventure_provider.dartlib/features/character/providers/adventure_provider.g.dartlib/features/coach_chat/presentation/pages/coach_chat_page.dartlib/features/coach_chat/presentation/widgets/assistant_bubble.dartlib/features/coach_chat/presentation/widgets/chat_input_bar.dartlib/features/coach_chat/presentation/widgets/model_setup_screen.dartlib/features/coach_chat/providers/coach_chat_provider.dartlib/features/coach_chat/providers/coach_chat_provider.g.dartlib/features/coach_chat/providers/model_setup_provider.dartlib/features/coach_chat/providers/model_setup_provider.g.dartlib/features/coach_chat/providers/model_setup_state.dartlib/features/coach_chat/utils/chat_history_codec.dartlib/features/dashboard/presentation/pages/dashboard_page.dartlib/features/dashboard/presentation/pages/run_history_page.dartlib/features/dashboard/presentation/pages/run_route_page.dartlib/features/dashboard/presentation/widgets/activity_ring.dartlib/features/dashboard/presentation/widgets/connect_healthkit_card.dartlib/features/dashboard/presentation/widgets/daily_quest_teaser.dartlib/features/dashboard/presentation/widgets/last_run_preview.dartlib/features/dashboard/presentation/widgets/week_momentum_card.dartlib/features/nexus_lab/presentation/nexus_lab_page.dartlib/features/onboarding/presentation/onboarding_page.dartlib/features/settings/presentation/pages/openrouter_model_picker_page.dartlib/features/settings/presentation/pages/settings_page.dartlib/features/settings/presentation/widgets/apple_health_export_preview_card.dartlib/features/training/presentation/pages/training_page.dartlib/features/training/presentation/widgets/training_insight_cards.dartlib/shared/providers/ai_reconnect_provider.dartlib/shared/providers/ai_reconnect_provider.g.dartlib/shared/providers/health_providers.dartlib/shared/providers/health_providers.g.dartlib/shared/providers/measurable_quest_sync_provider.dartlib/shared/providers/measurable_quest_sync_provider.g.dartlib/shared/providers/workout_session_lookup_provider.dartlib/shared/providers/workout_session_lookup_provider.g.dartlib/shared/utils/health_permission_feedback.dartlib/shared/widgets/ai_lifecycle_guard.dartlib/shared/widgets/kynos_user_bubble.dartlib/shared/widgets/metric_tile.dartlib/shared/widgets/run_card.dart
💤 Files with no reviewable changes (1)
- lib/features/character/providers/adventure_provider.dart
Verify and fix still-valid review comments: router extra passthrough, refresh await gaps, coach chat persistence/reconnect guards, health permission probe, workout lookup by id, cross-feature import boundary, accessibility duplicates, and settings picker snackbar timing. Co-authored-by: Youri Bontekoe <YKDBontekoe@users.noreply.github.com>
Co-authored-by: Youri Bontekoe <YKDBontekoe@users.noreply.github.com>
|
🎉 This PR is included in version 1.13.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
This was referenced Jul 6, 2026
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
2 participants
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.
Summary
Implements all 35 QoL tasks from the improvement plan across error handling, confirmations, data freshness, coach AI UX, navigation, gamification, accessibility, and onboarding.
Highlights
Error handling & feedback
KynosInlineErrorCard+ retry on Training insights, Character, Run Route, Last Run Preview, and OpenRouter pickerHealthPermissionFeedback(no raw$errorin snackbars)AiInferenceErrorPolicyConfirmations
Data freshness
AiLifecycleGuardCoach AI UX
ChatHistoryCodec)Navigation & gamification
/run-route/:runIdRunCardtappable; Nexus Lab AppBar with close buttonmeasurableQuestSyncProviderOnboarding & empty states
Accessibility
Validation
flutter analyze— no errors (3 info-level import ordering hints)flutter test— 130 tests passedflutter build web— succeededSummary by CodeRabbit
New Features
Bug Fixes