Ux updates - #36
Ux updates#36
Conversation
📝 WalkthroughWalkthroughChangesRelease, platform, and interface updates
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/levels_uniqueness_test.dart (1)
11-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated asset discovery logic into a shared test helper.
The
AssetManifest.loadFromAssetBundle+listAssets()filtering and level ID extraction pattern appears 3 times across both test files (levels_solvability_test.dartlines 11-24, and here at lines 11-25 and 88-93). Extracting this into a shared helper (e.g.,loadLevelIds()in atest_helpers.dartfile) would eliminate duplication and ensure consistent asset discovery if the path convention changes.♻️ Suggested shared helper
// test/test_helpers.dart import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; Future<List<String>> loadLevelIds() async { final manifest = await AssetManifest.loadFromAssetBundle(rootBundle); final levelPaths = manifest .listAssets() .where((String key) => key.contains('lib/data/levels/level_')) .toList(); final levelIds = levelPaths.map((path) { return path .split('/') .last .replaceAll('level_', '') .replaceAll('.json', ''); }).toList(); levelIds.sort((a, b) { final aNum = double.tryParse(a) ?? 0; final bNum = double.tryParse(b) ?? 0; return aNum.compareTo(bNum); }); return levelIds; } Future<List<String>> loadLevelPaths() async { final manifest = await AssetManifest.loadFromAssetBundle(rootBundle); return manifest .listAssets() .where((String key) => key.contains('lib/data/levels/level_')) .toList(); }Also applies to: 88-93
🤖 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/levels_uniqueness_test.dart` around lines 11 - 25, Extract the duplicated manifest asset filtering and level ID parsing from the affected tests and levels_solvability_test.dart into shared helpers such as loadLevelIds() and loadLevelPaths() in test_helpers.dart. Update all three call sites, including the logic around the levelPaths and levelIds declarations, to use these helpers and preserve the existing sorting and assertions.
🤖 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 `@ios/Runner.xcodeproj/project.pbxproj`:
- Around line 480-481: Replace the invalid AppIcon value assigned to
ASSETCATALOG_COMPILER_GENERATE_SWIFT_ASSET_SYMBOL_EXTENSIONS in the Debug and
Release build configurations with the boolean value YES, matching the existing
Profile configuration. Keep ASSETCATALOG_COMPILER_APPICON_NAME set to AppIcon.
In `@lib/presentation/screens/home/side_menu_screen.dart`:
- Around line 77-88: Increase the Pressable touch target in the side-menu item
builder by enforcing a minimum height of at least 48px while preserving the
existing padding and alignment; update the widget containing onTap, title, and
theme.menuTextStyle rather than relying solely on its current vertical padding.
In `@lib/presentation/widgets/common/edgy.dart`:
- Around line 22-23: Initialize _canScrollEnd to false in the Edgy widget state,
allowing layout/metrics notifications to enable the end fade only when the
content actually overflows.
In `@lib/presentation/widgets/common/pressable.dart`:
- Line 2: Update the motion configuration in the pressable widget to reference
the constant directly: replace `CupertinoMotion.smooth()` with
`CupertinoMotion.smooth` in the relevant `Pressable` configuration.
---
Nitpick comments:
In `@test/levels_uniqueness_test.dart`:
- Around line 11-25: Extract the duplicated manifest asset filtering and level
ID parsing from the affected tests and levels_solvability_test.dart into shared
helpers such as loadLevelIds() and loadLevelPaths() in test_helpers.dart. Update
all three call sites, including the logic around the levelPaths and levelIds
declarations, to use these helpers and preserve the existing sorting and
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a947e18-cb02-4353-b343-f5b8439de544
⛔ Files ignored due to path filters (8)
ios/Podfile.lockis excluded by!**/*.locklib/generated/intl/messages_de.dartis excluded by!**/generated/**lib/generated/intl/messages_en.dartis excluded by!**/generated/**lib/generated/intl/messages_es.dartis excluded by!**/generated/**lib/generated/intl/messages_fr.dartis excluded by!**/generated/**lib/generated/intl/messages_it.dartis excluded by!**/generated/**lib/generated/intl/messages_ru.dartis excluded by!**/generated/**pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
CHANGELOG.mdREADME.mdanalysis_options.yamlandroid/gradle.propertiesandroid/gradle/wrapper/gradle-wrapper.propertiesandroid/settings.gradle.ktsios/Flutter/Debug.xcconfigios/Flutter/Release.xcconfigios/Podfileios/Runner.xcodeproj/project.pbxprojios/Runner.xcworkspace/contents.xcworkspacedatalib/presentation/providers/game_provider.g.dartlib/presentation/providers/settings/language_provider.g.dartlib/presentation/providers/settings/theme_provider.g.dartlib/presentation/screens/home/home_screen.dartlib/presentation/screens/home/side_menu_screen.dartlib/presentation/screens/info/about_screen.dartlib/presentation/screens/info/story_screen.dartlib/presentation/widgets/common/app_bar_widget.dartlib/presentation/widgets/common/edgy.dartlib/presentation/widgets/common/pressable.dartlib/presentation/widgets/game/game_bottom_bar.dartpubspec.yamltest/levels_solvability_test.darttest/levels_uniqueness_test.dart
💤 Files with no reviewable changes (4)
- ios/Flutter/Release.xcconfig
- ios/Runner.xcworkspace/contents.xcworkspacedata
- ios/Flutter/Debug.xcconfig
- ios/Podfile
| "ARCHS[sdk=iphonesimulator*]" = arm64; | ||
| ASSETCATALOG_COMPILER_GENERATE_SWIFT_ASSET_SYMBOL_EXTENSIONS = AppIcon; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix invalid ASSETCATALOG_COMPILER_GENERATE_SWIFT_ASSET_SYMBOL_EXTENSIONS value in Debug and Release configs.
This build setting expects a boolean (YES/NO), but both the Debug (line 481) and Release (line 541) configs now have AppIcon — likely a copy-paste error from ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon. The Profile config (line 354, unchanged) correctly retains YES. This inconsistency could produce build warnings or disable Swift asset symbol extensions in Debug/Release builds.
🔧 Proposed fix for both Debug and Release configs
// Debug config (line 481)
- ASSETCATALOG_COMPILER_GENERATE_SWIFT_ASSET_SYMBOL_EXTENSIONS = AppIcon;
+ ASSETCATALOG_COMPILER_GENERATE_SWIFT_ASSET_SYMBOL_EXTENSIONS = YES;
// Release config (line 541)
- ASSETCATALOG_COMPILER_GENERATE_SWIFT_ASSET_SYMBOL_EXTENSIONS = AppIcon;
+ ASSETCATALOG_COMPILER_GENERATE_SWIFT_ASSET_SYMBOL_EXTENSIONS = YES;Also applies to: 540-541
🤖 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 `@ios/Runner.xcodeproj/project.pbxproj` around lines 480 - 481, Replace the
invalid AppIcon value assigned to
ASSETCATALOG_COMPILER_GENERATE_SWIFT_ASSET_SYMBOL_EXTENSIONS in the Debug and
Release build configurations with the boolean value YES, matching the existing
Profile configuration. Keep ASSETCATALOG_COMPILER_APPICON_NAME set to AppIcon.
| return Pressable( | ||
| onTap: onTap, | ||
| contentPadding: const EdgeInsets.symmetric( | ||
| horizontal: GeneralConsts.horizontalPadding, | ||
| vertical: GeneralConsts.verticalPadding, | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric( | ||
| horizontal: GeneralConsts.horizontalPadding, | ||
| vertical: GeneralConsts.verticalPadding, | ||
| ), | ||
| child: Align( | ||
| alignment: Alignment.centerLeft, | ||
| child: Text(title, style: theme.menuTextStyle), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Touch target height may fall below accessibility minimum.
The previous ListTile enforced a minimum height (~48–56px via materialTapTargetSize). The new Pressable + Padding + Text layout has an estimated height of ~32–40px (16px text + 16px vertical padding), which is below the 48px minimum recommended by WCAG and Material Design guidelines.
♿ Proposed fix: enforce minimum touch target height
return Pressable(
onTap: onTap,
- child: Padding(
+ child: ConstrainedBox(
+ constraints: const BoxConstraints(minHeight: 48),
+ child: Padding(
padding: const EdgeInsets.symmetric(
horizontal: GeneralConsts.horizontalPadding,
vertical: GeneralConsts.verticalPadding,
),
child: Align(
alignment: Alignment.centerLeft,
child: Text(title, style: theme.menuTextStyle),
),
),
),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return Pressable( | |
| onTap: onTap, | |
| contentPadding: const EdgeInsets.symmetric( | |
| horizontal: GeneralConsts.horizontalPadding, | |
| vertical: GeneralConsts.verticalPadding, | |
| child: Padding( | |
| padding: const EdgeInsets.symmetric( | |
| horizontal: GeneralConsts.horizontalPadding, | |
| vertical: GeneralConsts.verticalPadding, | |
| ), | |
| child: Align( | |
| alignment: Alignment.centerLeft, | |
| child: Text(title, style: theme.menuTextStyle), | |
| ), | |
| ), | |
| return Pressable( | |
| onTap: onTap, | |
| child: ConstrainedBox( | |
| constraints: const BoxConstraints(minHeight: 48), | |
| child: Padding( | |
| padding: const EdgeInsets.symmetric( | |
| horizontal: GeneralConsts.horizontalPadding, | |
| vertical: GeneralConsts.verticalPadding, | |
| ), | |
| child: Align( | |
| alignment: Alignment.centerLeft, | |
| child: Text(title, style: theme.menuTextStyle), | |
| ), | |
| ), | |
| ), |
🤖 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/presentation/screens/home/side_menu_screen.dart` around lines 77 - 88,
Increase the Pressable touch target in the side-menu item builder by enforcing a
minimum height of at least 48px while preserving the existing padding and
alignment; update the widget containing onTap, title, and theme.menuTextStyle
rather than relying solely on its current vertical padding.
| bool _canScrollStart = false; | ||
| bool _canScrollEnd = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Default _canScrollEnd = true may show end fade for non-overflowing content.
If the wrapped scrollable content fits entirely within the viewport, no ScrollNotification fires, so _canScrollEnd remains true and the end fade displays incorrectly. Defaulting to false is safer — ScrollMetricsNotification (dispatched on initial layout and metric changes) will set it to true when content actually overflows.
🛡️ Proposed fix
bool _canScrollStart = false;
- bool _canScrollEnd = true;
+ bool _canScrollEnd = false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool _canScrollStart = false; | |
| bool _canScrollEnd = true; | |
| bool _canScrollStart = false; | |
| bool _canScrollEnd = 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/presentation/widgets/common/edgy.dart` around lines 22 - 23, Initialize
_canScrollEnd to false in the Edgy widget state, allowing layout/metrics
notifications to enable the end fade only when the content actually overflows.
| @@ -0,0 +1,46 @@ | |||
| import 'package:flutter/material.dart'; | |||
| import 'package:motor/motor.dart'; | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify motor package is declared and locked, then inspect its API surface.
rg -n 'motor' pubspec.yaml pubspec.lock
# If source is available, check for SingleMotionBuilder and CupertinoMotion
fd -t f 'motor' --exec rg -l 'SingleMotionBuilder|CupertinoMotion' {} \; 2>/dev/null || \
find . -path '*/motor/*' -name '*.dart' -exec rg -l 'SingleMotionBuilder|CupertinoMotion' {} \; 2>/dev/nullRepository: khlebobul/pegma
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pressable.dart =="
if [ -f lib/presentation/widgets/common/pressable.dart ]; then
cat -n lib/presentation/widgets/common/pressable.dart | sed -n '1,220p'
else
echo "missing: lib/presentation/widgets/common/pressable.dart"
fi
echo
echo "== motor dependency =="
rg -n '^\s*motor:|name: motor|version:' pubspec.yaml pubspec.lock || true
echo
echo "== motor API usage in repo =="
rg -n 'SingleMotionBuilder|CupertinoMotion\.smooth\(|CupertinoMotion|MotionBuilder' lib . || trueRepository: khlebobul/pegma
Length of output: 8685
🌐 Web query:
motor 1.1.0 SingleMotionBuilder CupertinoMotion.smooth API pub.dev
💡 Result:
In the motor package (version 1.1.0), CupertinoMotion.smooth is a static constant that provides a smooth spring animation configuration [1][2]. This configuration is designed to match Apple's SwiftUI animation presets, specifically offering a smooth motion with no bounce, which is ideal for subtle UI transitions [1][2]. The SingleMotionBuilder widget is used within the motor package for basic, one-dimensional animations [1][2]. When using CupertinoMotion.smooth, it triggers a physics-based spring animation whenever the target value changes [1][2]. Example usage: SingleMotionBuilder( motion: CupertinoMotion.smooth, value: targetValue, builder: (context, value, child) { return Container( width: value, height: value, color: Colors.blue,); },) CupertinoMotion is a subclass of SpringMotion that offers several other predefined constants mirroring iOS design, including CupertinoMotion, CupertinoMotion.bouncy, CupertinoMotion.snappy, and CupertinoMotion.interactive [1][2].
Citations:
- 1: https://pub.dev/documentation/motor/latest/
- 2: https://raw.githubusercontent.com/whynotmake-it/rivership/main/packages/motor/README.md
Use CupertinoMotion.smooth without ()
motor 1.1.0 exposes CupertinoMotion.smooth as a constant, so const CupertinoMotion.smooth() is invalid here. Update lib/presentation/widgets/common/pressable.dart:36-37 to motion: CupertinoMotion.smooth.
🤖 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/presentation/widgets/common/pressable.dart` at line 2, Update the motion
configuration in the pressable widget to reference the constant directly:
replace `CupertinoMotion.smooth()` with `CupertinoMotion.smooth` in the relevant
`Pressable` configuration.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Technical Improvements