feat: add motion polish across navigation, dashboard, and coach chat - #65
Conversation
Establish shared Motion tokens and route-specific page transitions. Animate tab switches, readiness rings, progress bars, charts, and coach messages. Add Hero transitions for coach and run detail, expandable card sizing, and smooth dark mode theme lerp. Co-authored-by: Youri Bontekoe <YKDBontekoe@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 38 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 (19)
📝 WalkthroughWalkthroughThis PR introduces centralized motion design tokens and reusable page transition presets, then applies them across the router, shell navigation, dashboard widgets, character screens, and coach chat components. It adds several new animated widgets (progress bars, async content switcher, message entrance, suggestion chips, streaming pulse) and Hero-based shared-element transitions, along with supporting tests. ChangesMotion tokens, page transitions, and router/shell wiring
Shared animated widgets and motion token adoption
Dashboard animated components and hero transitions
Character trail map and quest card animations
Coach chat animated widgets
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 |
Add const pageTransitionsTheme builders, sort imports per directives_ordering, and fix import order in coach chat and bottom nav widgets. 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/character/presentation/widgets/trail_map.dart (1)
60-65: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winKey causes full remount of every dot on each advance, defeating the new
didUpdateWidgetlogic.
session.currentIndexis embedded in every dot's key, so when the user advances (currentIndex changes), all node keys change simultaneously. Flutter then disposes and recreates every_TrailNodeDotState(and itsAnimationController) instead of reusing existing elements and runningdidUpdateWidget. This means the_syncPulse/didUpdateWidgetpath added below effectively never fires for the scenario it was built for, and every dot's controller gets torn down and rebuilt on each advance — wasted work that scales withsession.nodes.length.Key on the node's own stable identity instead, and let
isCurrentprop changes drivedidUpdateWidget.⚡ Proposed fix
return _TrailNodeDot( - key: ValueKey<String>('${node.index}-${session.currentIndex}'), + key: ValueKey<int>(node.index), node: node, isCurrent: isCurrent, isPast: isPast, );🤖 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/trail_map.dart` around lines 60 - 65, The dot key in `TrailMap` is too volatile because `session.currentIndex` is included in every `_TrailNodeDot` `ValueKey`, causing all dots to remount on each advance and bypass `_TrailNodeDotState.didUpdateWidget`. Update the keying in the `return _TrailNodeDot(...)` block to use a stable per-node identity from `node` itself so existing states and `AnimationController`s are reused, and let changes to `isCurrent`/`isPast` drive the `didUpdateWidget` sync path.
🧹 Nitpick comments (10)
lib/shared/widgets/insight_expandable_card.dart (1)
85-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
AnimatedSizeexpand/collapse block.
_InsightExpandableCardStateand_InsightTextExpandableCardStatenow both wrap near-identicalAnimatedSize(duration: Motion.medium, curve: Motion.curve, alignment: Alignment.topCenter, child: _expanded ? ... : SizedBox.shrink())scaffolding. Consider extracting a small shared_ExpandableSectionwrapper widget to avoid drift between the two copies as motion tuning evolves.Also applies to: 200-216
🤖 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/insight_expandable_card.dart` around lines 85 - 131, The expand/collapse UI in `_InsightExpandableCardState` is duplicated in the same `AnimatedSize`/`_expanded` scaffold as the matching text card, so extract the shared wrapper into a small reusable widget such as `_ExpandableSection` and use it from both card states. Keep the shared animation settings (`Motion.medium`, `Motion.curve`, `Alignment.topCenter`) and only pass the varying child content so the two implementations stay in sync.lib/shared/widgets/animated_progress_bar.dart (1)
33-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
coloralongsidevalueColor.Both
valueColorandcolorare set onLinearProgressIndicator. Per Flutter'sProgressIndicator,valueColortakes precedence when both are supplied, makingcolordead/ignored. Drop one to avoid confusion.🧹 Suggested cleanup
return LinearProgressIndicator( value: animatedValue, minHeight: minHeight, backgroundColor: backgroundColor, valueColor: AlwaysStoppedAnimation(valueColor), - color: valueColor, );🤖 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/animated_progress_bar.dart` around lines 33 - 39, The LinearProgressIndicator in AnimatedProgressBar sets both valueColor and color, but valueColor already takes precedence so color is redundant. Update the LinearProgressIndicator construction in AnimatedProgressBar to keep only one source of bar color, using the existing valueColor/AlwaysStoppedAnimation path and removing the ignored color argument.lib/features/character/presentation/widgets/trail_map.dart (1)
106-143: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winController is created for every dot regardless of
isCurrent, anddidUpdateWidgetrestarts the pulse unconditionally.Two related inefficiencies:
initStatealways creates anAnimationControllereven for non-current dots, so a trail with many nodes spins up that many tickers, most idle.didUpdateWidgetcalls_syncPulse()on every rebuild without checkingoldWidget.isCurrent != widget.isCurrent, so if the element is reused (e.g. once the key fix above is applied) while remaining current,repeat(reverse: true)is re-invoked on each rebuild, potentially restarting the pulse mid-cycle.♻️ Proposed guard
`@override` void didUpdateWidget(_TrailNodeDot oldWidget) { super.didUpdateWidget(oldWidget); - _syncPulse(); + if (oldWidget.isCurrent != widget.isCurrent) { + _syncPulse(); + } }🤖 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/trail_map.dart` around lines 106 - 143, Guard the pulse controller lifecycle in _TrailNodeDotState so it only starts and restarts when isCurrent actually changes. In initState and _syncPulse, avoid creating or repeating the AnimationController for non-current dots, and in didUpdateWidget compare oldWidget.isCurrent with widget.isCurrent before calling _syncPulse(). Keep the existing controller fields in _TrailNodeDotState, but make _syncPulse() a no-op when the current state has not changed so repeated rebuilds do not restart the animation.lib/app/shell_page.dart (1)
89-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnimation direction/index bookkeeping looks correct.
_directionis computed against_previousIndexbefore it's overwritten, and the controller is reset viaforward(from: 0)on each tab change — no race between successive rapid taps sinceAnimationController.forwardrestarts cleanly.One edge case: rapid back-to-back tab switches while the shell hasn't settled will call
_buildAnimations()and restart the same controller instance, discarding the in-flight animation value abruptly (visible as a jump-cut rather than smooth redirection). This is a minor, self-recovering visual glitch given animation durations are short (Motion.medium= 280ms).🤖 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/shell_page.dart` around lines 89 - 119, The tab transition animation in _AnimatedShellBody can jump when tabIndex changes again before the current AnimationController finishes. Update didUpdateWidget and/or _buildAnimations so in-flight direction changes are handled smoothly instead of rebuilding tweens and restarting the same controller abruptly; reuse the current animation progress or otherwise transition the existing controller state cleanly when _direction changes.lib/core/theme/app_theme.dart (1)
179-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDesktop platforms (Linux/Windows/Fuchsia) fall back to the default builder.
Only
android,iOS, andmacOSare mapped; unspecified platforms silently fall back toFadeUpwardsPageTransitionsBuilder(Android-style) rather than an intentional choice, which is a minor gap in the stated "platform-adaptive" behavior if the app targets desktop/web builds.🤖 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/core/theme/app_theme.dart` around lines 179 - 185, The PageTransitionsTheme in AppTheme only defines builders for android, iOS, and macOS, so desktop/web platforms still fall back to the default transition builder. Update the builders map to explicitly handle the remaining target platforms you support, using the appropriate PageTransitionsBuilder choices, and keep the change localized to the pageTransitionsTheme setup in app_theme.dart.test/app/shell_page_test.dart (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise the new animated behavior.
This asserts only that
ShellPageis aType, which is always true and provides no coverage of_AnimatedShellBody's slide/fade transition, direction logic, orIndexedStackstate preservation described in the PR. Consider a widget test that pumpsShellPageinside aProviderScope/MaterialAppwith a fakeStatefulNavigationShelland verifies transition behavior on tab change, per the repo's stated testing guideline for widget tests.As per coding guidelines, "widget tests must use
ProviderScopewith overridden fakes rather than real repositories."🤖 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/app/shell_page_test.dart` around lines 1 - 8, The current ShellPage test only checks that the type exists and does not cover the animated shell behavior. Update the test around ShellPage and _AnimatedShellBody to be a widget test that pumps the page inside a ProviderScope and MaterialApp with overridden fakes, using a fake StatefulNavigationShell to drive tab changes. Verify the slide/fade transition, the direction logic when switching tabs, and that the IndexedStack preserves state across navigation instead of only asserting type availability.Source: Coding guidelines
lib/core/theme/kynos_theme_extension.dart (1)
206-208: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winShadow lerp still snaps instead of interpolating.
Colors and text styles now interpolate smoothly via
t, butcardShadow/metricTileShadow/navBarShadowstill snap discretely att < 0.5. Given the PR explicitly targets "smooth dark mode theme interpolation," the shadow snap will visually pop mid-transition while everything else fades smoothly.BoxShadow.lerpListis available for exactly this case.♻️ Proposed fix
- cardShadow: t < 0.5 ? cardShadow : other.cardShadow, - metricTileShadow: t < 0.5 ? metricTileShadow : other.metricTileShadow, - navBarShadow: t < 0.5 ? navBarShadow : other.navBarShadow, + cardShadow: BoxShadow.lerpList(cardShadow, other.cardShadow, t) ?? cardShadow, + metricTileShadow: + BoxShadow.lerpList(metricTileShadow, other.metricTileShadow, t) ?? + metricTileShadow, + navBarShadow: + BoxShadow.lerpList(navBarShadow, other.navBarShadow, t) ?? navBarShadow,🤖 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/core/theme/kynos_theme_extension.dart` around lines 206 - 208, The shadow fields in the theme interpolation still switch abruptly at t < 0.5 instead of blending smoothly. Update the interpolation logic in the Kynos theme extension’s lerp method for cardShadow, metricTileShadow, and navBarShadow to use BoxShadow.lerpList (or equivalent shadow interpolation) between the current and other values, keeping the rest of the theme transition behavior unchanged.test/features/dashboard/activity_ring_test.dart (1)
25-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extending coverage for the new animation logic.
Current test only checks that
AnimatedActivityRingbuilds and passes throughringProgresses; it doesn't exercise the staggered animation completing (tester.pumpAndSettle()), thedidUpdateWidgetre-forward-on-change path, or the new publicringSemanticsLabelhelper — all flagged as high-complexity additions in this diff.🤖 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/dashboard/activity_ring_test.dart` around lines 25 - 44, The current AnimatedActivityRing test only verifies construction and prop passthrough; extend it to cover the new animation behavior and helper API. Add assertions around the staggered animation reaching its final state with pumpAndSettle, verify didUpdateWidget re-forwards when ringProgresses changes, and include coverage for the new ringSemanticsLabel helper using AnimatedActivityRing and its semantics-related behavior.lib/features/coach_chat/presentation/widgets/coach_chat_app_bar.dart (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCross-widget-file import just to reach a shared constant.
This file imports
message_list.dartsolely to useCoachHeroTags, coupling the app bar to an otherwise-unrelated list widget file. Same pattern repeats withRunHeroTagsliving inrun_card.dartand being imported byrun_route_page.dart. Consider extracting these tag constants to a small shared file (e.g.lib/shared/widgets/hero_tags.dart) to avoid widgets importing each other purely for constants.Also applies to: 39-46
🤖 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/coach_chat_app_bar.dart` at line 7, The app bar is importing an unrelated widget file just to access shared hero tag constants, which creates unnecessary cross-widget coupling. Move `CoachHeroTags` out of `message_list.dart` into a small shared constants file and update `coach_chat_app_bar.dart` to import that shared location instead. Apply the same extraction for `RunHeroTags` currently living in `run_card.dart`, then update `run_route_page.dart` to use the shared constants file instead of importing the card widget.lib/features/coach_chat/presentation/widgets/streaming_text_pulse.dart (1)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded duration/curve instead of shared
Motiontokens.Other new widgets in this cohort (
AnimatedMessageEntrance,GlassSuggestionChip) consistently useMotion.medium/Motion.fastandMotion.curvefrom the centralized motion system this PR introduces, but this widget hardcodesDuration(milliseconds: 900)andCurves.easeInOutlocally. Consider adding a dedicated token (e.g.Motion.pulse) to keep the streaming pulse consistent with the rest of the motion design system.🤖 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/streaming_text_pulse.dart` around lines 26 - 32, The streaming pulse animation in streaming_text_pulse.dart hardcodes its timing and easing instead of using the shared motion system. Update the AnimationController setup in the relevant widget to use a centralized Motion token rather than a local Duration(milliseconds: 900) and Curves.easeInOut, and add a dedicated Motion.pulse token if needed so the pulse stays consistent with AnimatedMessageEntrance and GlassSuggestionChip.
🤖 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/core/theme/app_theme.dart`:
- Around line 179-185: The Android predictive-back setup is incomplete even
though AppTheme uses PredictiveBackPageTransitionsBuilder. Update the
AndroidManifest application entry to enable the back-invoked callback so the
Android transition builder is actually used; locate the manifest under the
Android app configuration and add the needed application-level flag alongside
the existing app settings.
In `@lib/features/coach_chat/presentation/widgets/coach_chat_app_bar.dart`:
- Around line 39-46: The CoachChatAppBar Hero is reusing CoachHeroTags.sparkle
in a screen where CoachChatEmptyState can be mounted at the same time, which can
cause a duplicate Hero tag conflict. Update the Hero in CoachChatAppBar (and, if
needed, the matching Hero in CoachChatEmptyState) so only the intended
transition pair shares CoachHeroTags.sparkle, or assign one of them a distinct
tag to avoid collisions.
In `@lib/features/coach_chat/presentation/widgets/glass_suggestion_chip.dart`:
- Around line 55-79: The GlassSuggestionChip build method uses a bare
GestureDetector, so it does not expose button semantics or keyboard/switch
activation. Update the widget in glass_suggestion_chip.dart to wrap the tappable
area with Semantics using widget.label as the accessible label and button role,
and replace or augment GestureDetector with a focusable control such as
FocusableActionDetector or InkWell so it can be activated from keyboard/switch
input while keeping the existing tap handlers (_handleTapDown, _handleTapEnd,
_handleTapCancel).
In `@lib/features/coach_chat/presentation/widgets/message_list.dart`:
- Around line 98-105: The Hero tag `CoachHeroTags.sparkle` is duplicated in
`CoachChatEmptyState`, which conflicts with the existing flight used by
`CoachChatAppBar` on `CoachChatPage`. Update the `message_list.dart` empty-state
Hero so it either removes the Hero wrapper entirely or uses a separate tag,
keeping `CoachHeroTags.sparkle` reserved for the dashboard button ↔ app bar
transition. Locate the `Hero` around the empty-state icon in
`CoachChatEmptyState` and adjust that widget accordingly.
In `@lib/features/coach_chat/presentation/widgets/streaming_text_pulse.dart`:
- Around line 36-49: The pulse animation is being restarted on every rebuild
because StreamingTextPulse.didUpdateWidget always calls _syncAnimation() even
when isActive has not changed. Update didUpdateWidget to compare
oldWidget.isActive with widget.isActive and only invoke _syncAnimation() when
the flag flips; keep the existing _syncAnimation() behavior for
starting/stopping the controller based on widget.isActive.
In `@lib/features/dashboard/presentation/pages/run_route_page.dart`:
- Around line 69-80: The Hero transition for the run date is using different
text formats between the route page and RunCard, so the shared-element animation
appears inconsistent. Update _runRouteAppBar (and the matching RunCard label
source) to use the same date formatting helper or shared formatter for
RunHeroTags.date(run.id), ensuring both Hero endpoints render identical labels
before and during the transition.
In `@lib/features/dashboard/presentation/widgets/activity_ring.dart`:
- Around line 97-108: The AnimatedActivityRing update check is using exact
double equality via listEquals, which can restart the animation on tiny
floating-point drift. Update didUpdateWidget in AnimatedActivityRing to compare
oldTargets and _targetProgresses with a small tolerance instead of strict ==, so
the controller only forwards when the ring progress changes meaningfully. Use
the existing oldWidget.ringProgresses and _targetProgresses values as the
comparison inputs, and keep the current animation restart behavior only for real
data changes.
In `@lib/features/dashboard/presentation/widgets/dashboard_header_sliver.dart`:
- Line 6: Dashboard is importing a `coach_chat` presentation widget file just to
use `CoachHeroTags`, which breaks the feature boundary. Move `CoachHeroTags` out
of `message_list.dart` into a shared location such as a shared widgets/constants
file, then update `dashboard_header_sliver.dart` and any `coach_chat` usage to
import that shared symbol instead of reaching into another feature’s internals.
In `@lib/shared/widgets/animated_async_content.dart`:
- Around line 27-39: The data-state key in AnimatedAsyncContent is using
identityHashCode(item), which makes the key change on every new AsyncValue.data
emission and retriggers AnimatedSwitcher unnecessarily. Update the data branch
in the value.when call to use a stable, non-identity-based key (similar to the
existing loading/error keys) so refreshes of the same logical data do not replay
the crossfade; keep the change localized to AnimatedAsyncContent and its
KeyedSubtree handling.
In `@lib/shared/widgets/run_card.dart`:
- Around line 10-13: The Hero date text is formatted inconsistently between the
two endpoints, causing a visible jump during the transition. Update the date
rendering in both RunHeroTags-related views, specifically the run card and the
run route page, to use the same date format string so the shared Hero label
matches exactly.
In `@test/features/coach_chat/coach_chat_error_retry_test.dart`:
- Around line 9-38: The MessageList widget is being pumped without a Riverpod
scope, which prevents MessageBubble (a ConsumerWidget) from mounting in the
test. Update the test setup in coach_chat_error_retry_test by wrapping the
MaterialApp/Scaffold tree with ProviderScope so the MessageList and its child
widgets can resolve Riverpod dependencies during the widget test.
---
Outside diff comments:
In `@lib/features/character/presentation/widgets/trail_map.dart`:
- Around line 60-65: The dot key in `TrailMap` is too volatile because
`session.currentIndex` is included in every `_TrailNodeDot` `ValueKey`, causing
all dots to remount on each advance and bypass
`_TrailNodeDotState.didUpdateWidget`. Update the keying in the `return
_TrailNodeDot(...)` block to use a stable per-node identity from `node` itself
so existing states and `AnimationController`s are reused, and let changes to
`isCurrent`/`isPast` drive the `didUpdateWidget` sync path.
---
Nitpick comments:
In `@lib/app/shell_page.dart`:
- Around line 89-119: The tab transition animation in _AnimatedShellBody can
jump when tabIndex changes again before the current AnimationController
finishes. Update didUpdateWidget and/or _buildAnimations so in-flight direction
changes are handled smoothly instead of rebuilding tweens and restarting the
same controller abruptly; reuse the current animation progress or otherwise
transition the existing controller state cleanly when _direction changes.
In `@lib/core/theme/app_theme.dart`:
- Around line 179-185: The PageTransitionsTheme in AppTheme only defines
builders for android, iOS, and macOS, so desktop/web platforms still fall back
to the default transition builder. Update the builders map to explicitly handle
the remaining target platforms you support, using the appropriate
PageTransitionsBuilder choices, and keep the change localized to the
pageTransitionsTheme setup in app_theme.dart.
In `@lib/core/theme/kynos_theme_extension.dart`:
- Around line 206-208: The shadow fields in the theme interpolation still switch
abruptly at t < 0.5 instead of blending smoothly. Update the interpolation logic
in the Kynos theme extension’s lerp method for cardShadow, metricTileShadow, and
navBarShadow to use BoxShadow.lerpList (or equivalent shadow interpolation)
between the current and other values, keeping the rest of the theme transition
behavior unchanged.
In `@lib/features/character/presentation/widgets/trail_map.dart`:
- Around line 106-143: Guard the pulse controller lifecycle in
_TrailNodeDotState so it only starts and restarts when isCurrent actually
changes. In initState and _syncPulse, avoid creating or repeating the
AnimationController for non-current dots, and in didUpdateWidget compare
oldWidget.isCurrent with widget.isCurrent before calling _syncPulse(). Keep the
existing controller fields in _TrailNodeDotState, but make _syncPulse() a no-op
when the current state has not changed so repeated rebuilds do not restart the
animation.
In `@lib/features/coach_chat/presentation/widgets/coach_chat_app_bar.dart`:
- Line 7: The app bar is importing an unrelated widget file just to access
shared hero tag constants, which creates unnecessary cross-widget coupling. Move
`CoachHeroTags` out of `message_list.dart` into a small shared constants file
and update `coach_chat_app_bar.dart` to import that shared location instead.
Apply the same extraction for `RunHeroTags` currently living in `run_card.dart`,
then update `run_route_page.dart` to use the shared constants file instead of
importing the card widget.
In `@lib/features/coach_chat/presentation/widgets/streaming_text_pulse.dart`:
- Around line 26-32: The streaming pulse animation in streaming_text_pulse.dart
hardcodes its timing and easing instead of using the shared motion system.
Update the AnimationController setup in the relevant widget to use a centralized
Motion token rather than a local Duration(milliseconds: 900) and
Curves.easeInOut, and add a dedicated Motion.pulse token if needed so the pulse
stays consistent with AnimatedMessageEntrance and GlassSuggestionChip.
In `@lib/shared/widgets/animated_progress_bar.dart`:
- Around line 33-39: The LinearProgressIndicator in AnimatedProgressBar sets
both valueColor and color, but valueColor already takes precedence so color is
redundant. Update the LinearProgressIndicator construction in
AnimatedProgressBar to keep only one source of bar color, using the existing
valueColor/AlwaysStoppedAnimation path and removing the ignored color argument.
In `@lib/shared/widgets/insight_expandable_card.dart`:
- Around line 85-131: The expand/collapse UI in `_InsightExpandableCardState` is
duplicated in the same `AnimatedSize`/`_expanded` scaffold as the matching text
card, so extract the shared wrapper into a small reusable widget such as
`_ExpandableSection` and use it from both card states. Keep the shared animation
settings (`Motion.medium`, `Motion.curve`, `Alignment.topCenter`) and only pass
the varying child content so the two implementations stay in sync.
In `@test/app/shell_page_test.dart`:
- Around line 1-8: The current ShellPage test only checks that the type exists
and does not cover the animated shell behavior. Update the test around ShellPage
and _AnimatedShellBody to be a widget test that pumps the page inside a
ProviderScope and MaterialApp with overridden fakes, using a fake
StatefulNavigationShell to drive tab changes. Verify the slide/fade transition,
the direction logic when switching tabs, and that the IndexedStack preserves
state across navigation instead of only asserting type availability.
In `@test/features/dashboard/activity_ring_test.dart`:
- Around line 25-44: The current AnimatedActivityRing test only verifies
construction and prop passthrough; extend it to cover the new animation behavior
and helper API. Add assertions around the staggered animation reaching its final
state with pumpAndSettle, verify didUpdateWidget re-forwards when ringProgresses
changes, and include coverage for the new ringSemanticsLabel helper using
AnimatedActivityRing and its semantics-related behavior.
🪄 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: 4321c504-570a-4a2d-89ce-68f983eba097
📒 Files selected for processing (37)
lib/app/page_transitions.dartlib/app/router.dartlib/app/shell_page.dartlib/core/theme/app_theme.dartlib/core/theme/kynos_theme_extension.dartlib/core/theme/motion.dartlib/core/theme/theme.dartlib/features/character/presentation/widgets/quest_card.dartlib/features/character/presentation/widgets/trail_map.dartlib/features/coach_chat/presentation/widgets/animated_message_entrance.dartlib/features/coach_chat/presentation/widgets/assistant_bubble.dartlib/features/coach_chat/presentation/widgets/coach_chat_app_bar.dartlib/features/coach_chat/presentation/widgets/glass_suggestion_chip.dartlib/features/coach_chat/presentation/widgets/message_list.dartlib/features/coach_chat/presentation/widgets/streaming_text_pulse.dartlib/features/dashboard/presentation/pages/run_route_page.dartlib/features/dashboard/presentation/widgets/activity_ring.dartlib/features/dashboard/presentation/widgets/character_glance_card.dartlib/features/dashboard/presentation/widgets/coach_insight_card.dartlib/features/dashboard/presentation/widgets/dashboard_header_sliver.dartlib/features/dashboard/presentation/widgets/hrv_sparkline.dartlib/features/dashboard/presentation/widgets/readiness_card.dartlib/features/dashboard/presentation/widgets/week_momentum_card.dartlib/shared/widgets/animated_async_content.dartlib/shared/widgets/animated_progress_bar.dartlib/shared/widgets/charts/hrv_chart.dartlib/shared/widgets/charts/load_chart.dartlib/shared/widgets/insight_expandable_card.dartlib/shared/widgets/kynos_bottom_nav.dartlib/shared/widgets/kynos_page_dots.dartlib/shared/widgets/liquid_glass_button.dartlib/shared/widgets/run_card.dartlib/shared/widgets/widgets.darttest/app/shell_page_test.darttest/core/theme/motion_test.darttest/features/coach_chat/coach_chat_error_retry_test.darttest/features/dashboard/activity_ring_test.dart
| Hero( | ||
| tag: CoachHeroTags.sparkle, | ||
| child: Icon( | ||
| Icons.auto_awesome_rounded, | ||
| size: 20, | ||
| color: kynos.purple, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the coach chat page to confirm whether CoachChatAppBar and
# CoachChatEmptyState/MessageList are composed in the same Scaffold/route.
fd -e dart | xargs rg -n "CoachChatAppBar|CoachChatEmptyState" -C3Repository: YKDBontekoe/KYNOS
Length of output: 3496
Separate the two CoachHeroTags.sparkle heroes
CoachChatAppBar and CoachChatEmptyState are mounted together when the conversation is empty, so reusing the same Hero tag here can trigger Flutter’s duplicate-tag runtime error during a flight. Give one of them a different tag, or ensure only the intended dashboard↔coach transition pair uses CoachHeroTags.sparkle.
🤖 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/coach_chat_app_bar.dart` around
lines 39 - 46, The CoachChatAppBar Hero is reusing CoachHeroTags.sparkle in a
screen where CoachChatEmptyState can be mounted at the same time, which can
cause a duplicate Hero tag conflict. Update the Hero in CoachChatAppBar (and, if
needed, the matching Hero in CoachChatEmptyState) so only the intended
transition pair shares CoachHeroTags.sparkle, or assign one of them a distinct
tag to avoid collisions.
| Hero( | ||
| tag: CoachHeroTags.sparkle, | ||
| child: Icon( | ||
| Icons.chat_bubble_outline_rounded, | ||
| size: 48, | ||
| color: context.kynosTheme.stand, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== CoachHeroTags usages ==\n'
rg -n "CoachHeroTags\.sparkle|Hero\(" lib/features/coach_chat lib/features/dashboard -g '*.dart'
printf '\n== message_list.dart excerpt ==\n'
nl -ba lib/features/coach_chat/presentation/widgets/message_list.dart | sed -n '85,120p'
printf '\n== coach_chat_app_bar.dart excerpt ==\n'
nl -ba lib/features/coach_chat/presentation/widgets/coach_chat_app_bar.dart | sed -n '1,90p'
printf '\n== dashboard_header_sliver.dart excerpt ==\n'
nl -ba lib/features/dashboard/presentation/widgets/dashboard_header_sliver.dart | sed -n '1,90p'
printf '\n== CoachHeroTags definition ==\n'
rg -n "abstract final class CoachHeroTags|static const sparkle" lib/features -g '*.dart' -A 5 -B 2Repository: YKDBontekoe/KYNOS
Length of output: 863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list around coach_chat presentation widgets ==\n'
git ls-files 'lib/features/coach_chat/presentation/widgets/*.dart' 'lib/features/dashboard/presentation/widgets/*.dart'
printf '\n== search for CoachHeroTags class ==\n'
rg -n "class CoachHeroTags|abstract final class CoachHeroTags" lib -g '*.dart' -A 10 -B 2
printf '\n== search for HeroController / HeroMode / tags in coach chat route files ==\n'
rg -n "HeroMode|HeroController|Hero\(" lib/features/coach_chat lib/features/dashboard lib -g '*.dart' -A 3 -B 3Repository: YKDBontekoe/KYNOS
Length of output: 7667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== message_list.dart relevant lines ==\n'
sed -n '85,150p' lib/features/coach_chat/presentation/widgets/message_list.dart
printf '\n== coach_chat_app_bar.dart relevant lines ==\n'
sed -n '1,120p' lib/features/coach_chat/presentation/widgets/coach_chat_app_bar.dart
printf '\n== coach chat page / route references ==\n'
rg -n "CoachChatAppBar|MessageList|message_list|coach_chat_app_bar" lib/features/coach_chat -g '*.dart' -A 4 -B 4
printf '\n== dashboard header sliver relevant lines ==\n'
sed -n '35,80p' lib/features/dashboard/presentation/widgets/dashboard_header_sliver.dartRepository: YKDBontekoe/KYNOS
Length of output: 14634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== coach chat presentation pages/widgets tree ==\n'
git ls-files 'lib/features/coach_chat/presentation/**/*.dart'
printf '\n== coach chat page/route files referencing app bar and message list ==\n'
rg -n "CoachChatAppBar|MessageList|ChatAppBar|EmptyState|message list|coach chat" lib/features/coach_chat/presentation -g '*.dart' -A 3 -B 3
printf '\n== current route/page for coach chat ==\n'
rg -n "GoRoute|ShellRoute|GoRouter|coach_chat" lib -g '*.dart' -A 4 -B 4Repository: YKDBontekoe/KYNOS
Length of output: 30499
Give CoachChatEmptyState its own hero tag
CoachChatAppBar and CoachChatEmptyState both render on CoachChatPage, so CoachHeroTags.sparkle is duplicated in one subtree. Keep that tag for the dashboard button ↔ app bar flight and remove the Hero from the empty state or assign it a separate tag.
🤖 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/message_list.dart` around lines
98 - 105, The Hero tag `CoachHeroTags.sparkle` is duplicated in
`CoachChatEmptyState`, which conflicts with the existing flight used by
`CoachChatAppBar` on `CoachChatPage`. Update the `message_list.dart` empty-state
Hero so it either removes the Hero wrapper entirely or uses a separate tag,
keeping `CoachHeroTags.sparkle` reserved for the dashboard button ↔ app bar
transition. Locate the `Hero` around the empty-state icon in
`CoachChatEmptyState` and adjust that widget accordingly.
Move hero tag constants to shared, unify run date Hero labels, fix duplicate coach Hero tags, improve accessibility and animation lifecycle guards, and enable Android predictive back in the manifest. 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 📦🚀 |
2 participants
Summary
Implements the full KYNOS Motion & Visual Polish plan across four phases using Flutter built-in animations only (no new dependencies).
What changed
Phase 1 — Foundation
Motiontokens (lib/core/theme/motion.dart) for durations and curvesKynosPageTransitionswith route-specific transitions (modal up, horizontal drill, standard push, fade-through)router.darttopageBuilderwith intentional transitions per route typeShellPage(preserves IndexedStack state)pageTransitionsThemebaseline inAppThemePhase 2 — Today tab hero moments
AnimatedActivityRingwith staggered ring sweep on load/refreshAnimatedProgressBarapplied to week momentum, character glance, and quest cardsKynosPageDotswith onboarding-style morphAnimatedAsyncContentcrossfade for readiness and coach insight loading statesFadeInOnAppearfor character glance cardPhase 3 — Coach chat
Phase 4 — Delight
AnimatedSizeon expandable insight cardsKynosThemeExtension.lerpfor smooth dark mode crossfadeAnimatedSwitchercheckmarkValidation
flutter analyze— pass (info-level lints only)flutter test— 135 tests passflutter build web— succeedsVisual proof
Motion changes cover tab switches, route pushes, readiness ring sweep, coach message entrance, and run detail Hero — verify in a device/simulator build of this branch.
Summary by CodeRabbit
New Features
Bug Fixes