feat: add statistics screen to Flutter app (PCA + K-Means visualization) - #25
Merged
Conversation
Connects recommendation-service's /pca and /clustering endpoints to a new scatter-plot screen, accessible from the languages list. Only tonally-annotated words (nbSyllabes/tone1/tone2) can be projected — currently only Yemba has this annotation — so the repository throws a NotEnoughDataException with a clear message for languages without enough annotated words, rather than crashing. Also fixes a port collision between user-service and recommendation-service (both defaulted to 8086, undetected until both services ran simultaneously for the first time). user-service moved to 8087. Verified manually end to end: all 7 services register on Eureka, PCA + clustering scatter plot renders correctly for Yemba, tapping a point identifies the word and its cluster.
📝 WalkthroughWalkthroughAdds a new mobile "statistics" feature: domain models (StatisticsPoint, WordWithTones), a repository fetching PCA/clustering data via API and combining results, Riverpod providers, a scatter-plot screen with tap selection, and navigation entry points (router route and languages screen button). Also bumps user-service application port from 8086 to 8087. ChangesMobile Statistics Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes User Service Port Configuration
Estimated code review effort: 1 (Trivial) | ~2 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LanguagesScreen
participant StatisticsScreen
participant StatisticsRepository
participant API
User->>LanguagesScreen: tap statistics icon
LanguagesScreen->>StatisticsScreen: navigate with languageId, languageName
StatisticsScreen->>StatisticsRepository: loadWordStatistics(languageId)
StatisticsRepository->>API: fetch words for language
StatisticsRepository->>API: request PCA + clustering
API-->>StatisticsRepository: pca points, cluster assignments
StatisticsRepository-->>StatisticsScreen: List<StatisticsPoint>
StatisticsScreen-->>User: render scatter plot / error message
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (3)
mobile/lib/features/statistics/data/statistics_repository.dart (1)
56-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNull-safe casts on clustering response fields.
entry['word_id'] as Stringandentry['cluster'] as intwill throw if either field is null in the clustering response. Consider using null-safe casts with fallbacks.🛡️ Proposed fix
- for (final entry in clusterData) entry['word_id'] as String: entry['cluster'] as int, + for (final entry in clusterData) + (entry['word_id'] as String?) ?? '': (entry['cluster'] as int?) ?? 0,🤖 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 `@mobile/lib/features/statistics/data/statistics_repository.dart` around lines 56 - 58, The cluster mapping in statistics_repository.dart is using unsafe casts for the clustering response fields, so update the logic around clusterByWordId to handle missing values safely. In the StatisticsRepository code that builds the map from clusterData, replace entry['word_id'] and entry['cluster'] assumptions with null-safe conversion and sensible fallbacks so null or malformed entries are skipped or defaulted instead of throwing.mobile/lib/features/statistics/domain/word_model.dart (1)
29-36: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against null
id/word/translationfrom the API.
nbSyllabes,tone1, andtone2are null-checked, butid,word, andtranslationare cast withas Stringwithout null checks. If the backend ever omits one of these fields, this will throw aTypeErrorinstead of returningnull.🛡️ Proposed fix
return WordWithTones( - id: json['id'] as String, - word: json['word'] as String, - translation: json['translation'] as String, + id: json['id'] as String? ?? '', + word: json['word'] as String? ?? '', + translation: json['translation'] as String? ?? '', nbSyllabes: nbSyllabes, tone1: tone1, tone2: tone2, );🤖 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 `@mobile/lib/features/statistics/domain/word_model.dart` around lines 29 - 36, The WordWithTones JSON parsing path in the WordModel factory is missing null guards for id, word, and translation, so an omitted API field can throw a TypeError instead of yielding null. Update the parsing logic in the WordModel/WordWithTones mapping so these fields are checked safely like nbSyllabes, tone1, and tone2, and only construct the model when all required values are present.mobile/lib/features/statistics/presentation/statistics_screen.dart (1)
120-131: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
_boundsand_projectare recomputed on every tap and every paint frame.
_boundsiterates all points four times (two reduces for x, two for y). For large datasets this is minor, but the bounds are also recomputed separately in_handleTapand_ScatterPainter.paint. Consider computing bounds once and passing them to both.🤖 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 `@mobile/lib/features/statistics/presentation/statistics_screen.dart` around lines 120 - 131, The statistics screen recomputes point bounds separately in both `_handleTap` and `_ScatterPainter.paint`, and `_bounds` itself scans the list multiple times, so the same work is repeated unnecessarily. Compute the bounds once per dataset in the statistics screen flow, reuse the result for tap handling and painting, and pass the cached min/max values into `_project` instead of recalculating them in each call.
🤖 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 `@mobile/lib/features/statistics/data/statistics_repository.dart`:
- Around line 61-72: The StatisticsRepository point mapping currently
force-unwraps wordById[wordId] inside the PCA points transform, which can crash
if the PCA response and annotated words list are out of sync. Update the mapping
in the repository method that builds StatisticsPoint objects to safely look up
the word by word_id, and either skip unmatched points or handle them gracefully
before constructing the point. Keep the clusterByWordId fallback behavior, but
remove the null-check exception risk from the word lookup.
In `@mobile/lib/features/statistics/presentation/statistics_screen.dart`:
- Around line 94-117: The tap hit-testing in _handleTap is using the wrong
render box, so the projected point coordinates do not line up with the painted
scatter plot. Update _StatisticsBody/_handleTap to measure the actual
GestureDetector or CustomPaint area instead of context.findRenderObject() from
the Column, and use that render box size and offset for _project and
localPosition comparisons. A GlobalKey on the GestureDetector is a good way to
retrieve the correct render object and keep the coordinate space consistent.
---
Nitpick comments:
In `@mobile/lib/features/statistics/data/statistics_repository.dart`:
- Around line 56-58: The cluster mapping in statistics_repository.dart is using
unsafe casts for the clustering response fields, so update the logic around
clusterByWordId to handle missing values safely. In the StatisticsRepository
code that builds the map from clusterData, replace entry['word_id'] and
entry['cluster'] assumptions with null-safe conversion and sensible fallbacks so
null or malformed entries are skipped or defaulted instead of throwing.
In `@mobile/lib/features/statistics/domain/word_model.dart`:
- Around line 29-36: The WordWithTones JSON parsing path in the WordModel
factory is missing null guards for id, word, and translation, so an omitted API
field can throw a TypeError instead of yielding null. Update the parsing logic
in the WordModel/WordWithTones mapping so these fields are checked safely like
nbSyllabes, tone1, and tone2, and only construct the model when all required
values are present.
In `@mobile/lib/features/statistics/presentation/statistics_screen.dart`:
- Around line 120-131: The statistics screen recomputes point bounds separately
in both `_handleTap` and `_ScatterPainter.paint`, and `_bounds` itself scans the
list multiple times, so the same work is repeated unnecessarily. Compute the
bounds once per dataset in the statistics screen flow, reuse the result for tap
handling and painting, and pass the cached min/max values into `_project`
instead of recalculating them in each call.
🪄 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: 8ddd6368-2ce8-489b-bf03-e5b693af0692
📒 Files selected for processing (8)
mobile/lib/core/router/app_router.dartmobile/lib/features/languages/presentation/languages_screen.dartmobile/lib/features/statistics/data/statistics_repository.dartmobile/lib/features/statistics/domain/statistics_point.dartmobile/lib/features/statistics/domain/word_model.dartmobile/lib/features/statistics/presentation/statistics_providers.dartmobile/lib/features/statistics/presentation/statistics_screen.dartservices/user-service/src/main/resources/application.yml
Comment on lines
+61
to
+72
| return (pcaData['points'] as List).cast<Map<String, dynamic>>().map((json) { | ||
| final wordId = json['word_id'] as String; | ||
| final word = wordById[wordId]!; | ||
| return StatisticsPoint( | ||
| wordId: wordId, | ||
| word: word.word, | ||
| translation: word.translation, | ||
| x: (json['x'] as num).toDouble(), | ||
| y: (json['y'] as num).toDouble(), | ||
| cluster: clusterByWordId[wordId] ?? 0, | ||
| ); | ||
| }).toList(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against PCA/clustering word_id mismatch with force unwrap.
Line 63 uses wordById[wordId]! — if the PCA response contains a word_id not present in the annotated words list (e.g., backend data inconsistency or stale cache), this will throw a null check exception and crash the screen. Consider filtering out unmatched PCA points or using a safe lookup.
🛡️ Proposed fix
- return (pcaData['points'] as List).cast<Map<String, dynamic>>().map((json) {
- final wordId = json['word_id'] as String;
- final word = wordById[wordId]!;
- return StatisticsPoint(
+ return (pcaData['points'] as List).cast<Map<String, dynamic>>()
+ .map((json) {
+ final wordId = json['word_id'] as String;
+ final word = wordById[wordId];
+ if (word == null) return null;
+ return StatisticsPoint(
wordId: wordId,
word: word.word,
translation: word.translation,
x: (json['x'] as num).toDouble(),
y: (json['y'] as num).toDouble(),
cluster: clusterByWordId[wordId] ?? 0,
);
- }).toList();
+ })
+ .whereType<StatisticsPoint>()
+ .toList();📝 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.
Suggested change
| return (pcaData['points'] as List).cast<Map<String, dynamic>>().map((json) { | |
| final wordId = json['word_id'] as String; | |
| final word = wordById[wordId]!; | |
| return StatisticsPoint( | |
| wordId: wordId, | |
| word: word.word, | |
| translation: word.translation, | |
| x: (json['x'] as num).toDouble(), | |
| y: (json['y'] as num).toDouble(), | |
| cluster: clusterByWordId[wordId] ?? 0, | |
| ); | |
| }).toList(); | |
| return (pcaData['points'] as List).cast<Map<String, dynamic>>() | |
| .map((json) { | |
| final wordId = json['word_id'] as String; | |
| final word = wordById[wordId]; | |
| if (word == null) return null; | |
| return StatisticsPoint( | |
| wordId: wordId, | |
| word: word.word, | |
| translation: word.translation, | |
| x: (json['x'] as num).toDouble(), | |
| y: (json['y'] as num).toDouble(), | |
| cluster: clusterByWordId[wordId] ?? 0, | |
| ); | |
| }) | |
| .whereType<StatisticsPoint>() | |
| .toList(); |
🤖 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 `@mobile/lib/features/statistics/data/statistics_repository.dart` around lines
61 - 72, The StatisticsRepository point mapping currently force-unwraps
wordById[wordId] inside the PCA points transform, which can crash if the PCA
response and annotated words list are out of sync. Update the mapping in the
repository method that builds StatisticsPoint objects to safely look up the word
by word_id, and either skip unmatched points or handle them gracefully before
constructing the point. Keep the clusterByWordId fallback behavior, but remove
the null-check exception risk from the word lookup.
Comment on lines
+94
to
+117
| void _handleTap(TapUpDetails details) { | ||
| final box = context.findRenderObject() as RenderBox?; | ||
| if (box == null || widget.points.isEmpty) return; | ||
|
|
||
| final size = box.size; | ||
| final (minX, maxX, minY, maxY) = _bounds(widget.points); | ||
| const padding = 24.0; | ||
|
|
||
| StatisticsPoint? closest; | ||
| double closestDistance = double.infinity; | ||
|
|
||
| for (final point in widget.points) { | ||
| final offset = _project(point, minX, maxX, minY, maxY, size, padding); | ||
| final distance = (details.localPosition - offset).distance; | ||
| if (distance < closestDistance) { | ||
| closestDistance = distance; | ||
| closest = point; | ||
| } | ||
| } | ||
|
|
||
| if (closest != null && closestDistance < 30) { | ||
| setState(() => _selected = closest); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Tap detection uses wrong render box — coordinates won't match the painted scatter plot.
context.findRenderObject() on line 95 returns the render box of the _StatisticsBodyState's build root (the Column), not the GestureDetector/CustomPaint. However, details.localPosition is relative to the GestureDetector. Since the CustomPaint is nested inside Expanded + Padding within the Column, box.size will be the full body size (including the header text and bottom card), not the paint area. This means _project will map points to a different coordinate space than where they're actually drawn, making tap selection inaccurate or non-functional.
🐛 Proposed fix using a GlobalKey on the GestureDetector
class _StatisticsBodyState extends State<_StatisticsBody> {
StatisticsPoint? _selected;
+ final _gestureKey = GlobalKey();
static const _palette = [
@@
child: GestureDetector(
+ key: _gestureKey,
onTapUp: _handleTap,
@@
void _handleTap(TapUpDetails details) {
- final box = context.findRenderObject() as RenderBox?;
+ final box = _gestureKey.currentContext?.findRenderObject() as RenderBox?;
if (box == null || widget.points.isEmpty) return;📝 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.
Suggested change
| void _handleTap(TapUpDetails details) { | |
| final box = context.findRenderObject() as RenderBox?; | |
| if (box == null || widget.points.isEmpty) return; | |
| final size = box.size; | |
| final (minX, maxX, minY, maxY) = _bounds(widget.points); | |
| const padding = 24.0; | |
| StatisticsPoint? closest; | |
| double closestDistance = double.infinity; | |
| for (final point in widget.points) { | |
| final offset = _project(point, minX, maxX, minY, maxY, size, padding); | |
| final distance = (details.localPosition - offset).distance; | |
| if (distance < closestDistance) { | |
| closestDistance = distance; | |
| closest = point; | |
| } | |
| } | |
| if (closest != null && closestDistance < 30) { | |
| setState(() => _selected = closest); | |
| } | |
| } | |
| void _handleTap(TapUpDetails details) { | |
| final box = _gestureKey.currentContext?.findRenderObject() as RenderBox?; | |
| if (box == null || widget.points.isEmpty) return; | |
| final size = box.size; | |
| final (minX, maxX, minY, maxY) = _bounds(widget.points); | |
| const padding = 24.0; | |
| StatisticsPoint? closest; | |
| double closestDistance = double.infinity; | |
| for (final point in widget.points) { | |
| final offset = _project(point, minX, maxX, minY, maxY, size, padding); | |
| final distance = (details.localPosition - offset).distance; | |
| if (distance < closestDistance) { | |
| closestDistance = distance; | |
| closest = point; | |
| } | |
| } | |
| if (closest != null && closestDistance < 30) { | |
| setState(() => _selected = closest); | |
| } | |
| } |
🤖 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 `@mobile/lib/features/statistics/presentation/statistics_screen.dart` around
lines 94 - 117, The tap hit-testing in _handleTap is using the wrong render box,
so the projected point coordinates do not line up with the painted scatter plot.
Update _StatisticsBody/_handleTap to measure the actual GestureDetector or
CustomPaint area instead of context.findRenderObject() from the Column, and use
that render box size and offset for _project and localPosition comparisons. A
GlobalKey on the GestureDetector is a good way to retrieve the correct render
object and keep the coordinate space consistent.
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
1 participant
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.
Connects recommendation-service's /pca and /clustering endpoints to a new scatter-plot screen, accessible from the languages list. Only tonally-annotated words (nbSyllabes/tone1/tone2) can be projected — currently only Yemba has this annotation — so the repository throws a NotEnoughDataException with a clear message for languages without enough annotated words, rather than crashing.
Also fixes a port collision between user-service and recommendation-service (both defaulted to 8086, undetected until both services ran simultaneously for the first time). user-service moved to 8087.
Verified manually end to end: all 7 services register on Eureka, PCA + clustering scatter plot renders correctly for Yemba, tapping a point identifies the word and its cluster.
Summary by CodeRabbit