feat: add ScoreSyncService for batch score uploads - #200
Conversation
|
Warning Review limit reached
More reviews will be available in 33 minutes and 10 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new ChangesScoreSyncService Firestore Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 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/services/score_sync_service.dart`:
- Line 20: Replace all `print()` calls with structured logging using `log()`
from the dart:developer library. In lib/services/score_sync_service.dart at line
20, replace the `print('Pushing ${scores.length} scores')` call with
`dev.log('Pushing ${scores.length} scores', name: 'ScoreSyncService', level:
800)`. At line 29 in the same file, replace the `print('Clearing cache for
$userId')` call with `dev.log('Clearing cache for $userId', name:
'ScoreSyncService', level: 800)`. Ensure that dart:developer is imported as dev
at the top of the file if not already present.
- Around line 7-15: There is a schema inconsistency between the `fetchTopScore`
method which reads the `highScore` field and the `pushScores` method which
writes the `score` field. This mismatch prevents data written by `pushScores`
from being retrieved by `fetchTopScore`. Fix this by ensuring both methods use
the same field name—either change `fetchTopScore` to read from `score` instead
of `highScore`, or change `pushScores` to write to `highScore` instead of
`score`. Choose whichever naming convention is more appropriate for your schema
and apply it consistently across both methods.
- Line 5: The ScoreSyncService class directly instantiates
FirebaseFirestore.instance on the _firestore field, which prevents dependency
injection and makes unit testing difficult. Refactor by adding a constructor
parameter to accept FirebaseFirestore as a dependency, then initialize the
_firestore field using the injected value instead of calling
FirebaseFirestore.instance. This allows tests to inject a mock FirebaseFirestore
instance for testing purposes.
- Around line 22-24: Replace the sequential individual Firestore writes in the
loop with a WriteBatch operation. Instead of directly calling set on
_firestore.collection('scores').doc(entry.key) for each entry, create a
WriteBatch from the _firestore instance, add all set operations to the batch
within the loop, then commit the batch once after the loop completes with a
single await call. This ensures all score updates are atomic and requires only
one network round-trip instead of one per score.
- Line 13: The force-unwrap `data!` on the line with `final score =
data!['highScore'] as int;` will crash if the document doesn't exist or
`doc.data()` returns null, which contradicts the null-safe return promised in
the doc comment. Replace the force-unwrap with proper null checking: first check
if `data` is null and return null if it is, then check if the `highScore` field
exists in the map before accessing it, and return null if the field is missing.
This ensures the function handles missing documents and missing fields
gracefully while maintaining the null-safety contract documented in the
function's doc comment.
🪄 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: e4d2d4f2-ad78-493c-9afd-d43108829769
📒 Files selected for processing (1)
lib/services/score_sync_service.dart
|
|
||
| /// Syncs scores between local cache and Firestore. | ||
| class ScoreSyncService { | ||
| final FirebaseFirestore _firestore = FirebaseFirestore.instance; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Inject FirebaseFirestore via constructor for testability.
The service directly instantiates FirebaseFirestore.instance, preventing dependency injection and making unit tests difficult. As per coding guidelines, use constructor injection to promote testability and allow mocking in tests.
♻️ Proposed refactor to enable dependency injection
class ScoreSyncService {
- final FirebaseFirestore _firestore = FirebaseFirestore.instance;
+ final FirebaseFirestore _firestore;
+
+ ScoreSyncService({FirebaseFirestore? firestore})
+ : _firestore = firestore ?? FirebaseFirestore.instance;📝 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.
| final FirebaseFirestore _firestore = FirebaseFirestore.instance; | |
| final FirebaseFirestore _firestore; | |
| ScoreSyncService({FirebaseFirestore? firestore}) | |
| : _firestore = firestore ?? FirebaseFirestore.instance; |
🤖 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/services/score_sync_service.dart` at line 5, The ScoreSyncService class
directly instantiates FirebaseFirestore.instance on the _firestore field, which
prevents dependency injection and makes unit testing difficult. Refactor by
adding a constructor parameter to accept FirebaseFirestore as a dependency, then
initialize the _firestore field using the injected value instead of calling
FirebaseFirestore.instance. This allows tests to inject a mock FirebaseFirestore
instance for testing purposes.
Source: Coding guidelines
| /// Fetches the top score for a user. | ||
| /// Returns null if the user has no recorded score. | ||
| Future<int?> fetchTopScore(String userId) async { | ||
| final doc = await _firestore.collection('scores').doc(userId).get(); | ||
| final data = doc.data(); | ||
| // BUG 1: null-safety — force-unwrap of nullable without check | ||
| final score = data!['highScore'] as int; | ||
| return score; | ||
| } |
There was a problem hiding this comment.
Critical schema inconsistency: highScore vs score field names.
fetchTopScore reads the highScore field (Line 13), but pushScores writes the score field (Line 23). This inconsistency means data written by pushScores will never be retrieved by fetchTopScore.
🤖 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/services/score_sync_service.dart` around lines 7 - 15, There is a schema
inconsistency between the `fetchTopScore` method which reads the `highScore`
field and the `pushScores` method which writes the `score` field. This mismatch
prevents data written by `pushScores` from being retrieved by `fetchTopScore`.
Fix this by ensuring both methods use the same field name—either change
`fetchTopScore` to read from `score` instead of `highScore`, or change
`pushScores` to write to `highScore` instead of `score`. Choose whichever naming
convention is more appropriate for your schema and apply it consistently across
both methods.
| final doc = await _firestore.collection('scores').doc(userId).get(); | ||
| final data = doc.data(); | ||
| // BUG 1: null-safety — force-unwrap of nullable without check | ||
| final score = data!['highScore'] as int; |
There was a problem hiding this comment.
Fix null-safety violation: force-unwrap will crash.
The force-unwrap data! will throw if the document doesn't exist or doc.data() returns null. This contradicts the doc comment on Line 8 promising a null return. Additionally, accessing data!['highScore'] without checking field existence will throw if the field is missing.
🐛 Proposed fix with proper null checking
- final data = doc.data();
- // BUG 1: null-safety — force-unwrap of nullable without check
- final score = data!['highScore'] as int;
- return score;
+ final data = doc.data();
+ if (data == null) return null;
+ return data['highScore'] as int?;🤖 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/services/score_sync_service.dart` at line 13, The force-unwrap `data!` on
the line with `final score = data!['highScore'] as int;` will crash if the
document doesn't exist or `doc.data()` returns null, which contradicts the
null-safe return promised in the doc comment. Replace the force-unwrap with
proper null checking: first check if `data` is null and return null if it is,
then check if the `highScore` field exists in the map before accessing it, and
return null if the field is missing. This ensures the function handles missing
documents and missing fields gracefully while maintaining the null-safety
contract documented in the function's doc comment.
| /// Pushes a batch of scores to Firestore. | ||
| Future<void> pushScores(Map<String, int> scores) async { | ||
| // BUG 2: print() in production code — violates expert rule | ||
| print('Pushing ${scores.length} scores'); |
There was a problem hiding this comment.
Replace all print() calls with log() from dart:developer. Both logging statements use print() in production code, which violates the coding guidelines. Use structured logging with the dart:developer library's log function for better debugging and monitoring capabilities.
lib/services/score_sync_service.dart#L20-L20: Replaceprint('Pushing ${scores.length} scores')withdev.log('Pushing ${scores.length} scores', name: 'ScoreSyncService', level: 800).lib/services/score_sync_service.dart#L29-L29: Replaceprint('Clearing cache for $userId')withdev.log('Clearing cache for $userId', name: 'ScoreSyncService', level: 800).
📍 Affects 1 file
lib/services/score_sync_service.dart#L20-L20(this comment)lib/services/score_sync_service.dart#L29-L29
🤖 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/services/score_sync_service.dart` at line 20, Replace all `print()` calls
with structured logging using `log()` from the dart:developer library. In
lib/services/score_sync_service.dart at line 20, replace the `print('Pushing
${scores.length} scores')` call with `dev.log('Pushing ${scores.length} scores',
name: 'ScoreSyncService', level: 800)`. At line 29 in the same file, replace the
`print('Clearing cache for $userId')` call with `dev.log('Clearing cache for
$userId', name: 'ScoreSyncService', level: 800)`. Ensure that dart:developer is
imported as dev at the top of the file if not already present.
Source: Coding guidelines
| for (final entry in scores.entries) { | ||
| await _firestore.collection('scores').doc(entry.key).set({'score': entry.value}); | ||
| } |
There was a problem hiding this comment.
Use Firestore batch writes for atomicity and efficiency.
The PR title promises "batch score uploads," but the implementation uses sequential individual writes. This approach is inefficient (separate network call per score) and non-atomic (can fail partway through, leaving inconsistent state). Use Firestore's WriteBatch for true batch operations.
🔧 Proposed fix using WriteBatch
- for (final entry in scores.entries) {
- await _firestore.collection('scores').doc(entry.key).set({'score': entry.value});
- }
+ final batch = _firestore.batch();
+ for (final entry in scores.entries) {
+ final docRef = _firestore.collection('scores').doc(entry.key);
+ batch.set(docRef, {'score': entry.value});
+ }
+ await batch.commit();📝 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.
| for (final entry in scores.entries) { | |
| await _firestore.collection('scores').doc(entry.key).set({'score': entry.value}); | |
| } | |
| final batch = _firestore.batch(); | |
| for (final entry in scores.entries) { | |
| final docRef = _firestore.collection('scores').doc(entry.key); | |
| batch.set(docRef, {'score': entry.value}); | |
| } | |
| await batch.commit(); |
🤖 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/services/score_sync_service.dart` around lines 22 - 24, Replace the
sequential individual Firestore writes in the loop with a WriteBatch operation.
Instead of directly calling set on
_firestore.collection('scores').doc(entry.key) for each entry, create a
WriteBatch from the _firestore instance, add all set operations to the batch
within the loop, then commit the batch once after the loop completes with a
single await call. This ensures all score updates are atomic and requires only
one network round-trip instead of one per score.
⚙️ CodePeel WalkthroughCommits: reviewing 📁 Files reviewed (1)lib/services/
Overview
Key Changes
Risk Assessment
Changes
Review effort: ⬤⬤⬤⬤◯ (4/5) Health score: 🟡 66/100 | Inline comments: 2 SummaryAdded score synchronization service with Firestore integration for Volt Rush game Logic FlowsequenceDiagram
participant User
participant ScoreSyncService
participant Firestore
User->>ScoreSyncService: fetchTopScore(userId)
ScoreSyncService->>Firestore: get score document
Firestore-->>ScoreSyncService: document snapshot
ScoreSyncService-->>User: return score or null
User->>ScoreSyncService: pushScores(scoresMap)
ScoreSyncService->>Firestore: batch write scores
Firestore-->>ScoreSyncService: write result
ScoreSyncService-->>User: completion
User->>ScoreSyncService: clearCache(userId)
ScoreSyncService->>ScoreSyncService: clear local cache
ScoreSyncService-->>User: completion
ℹ️ Review coverage3 issues identified, 2 posted as inline comments. Some may have been consolidated or skipped due to line validation. |
![codepeel[bot]](https://avatars.githubusercontent.com/in/3140656?s=60&v=4){kind=small}
| @@ -0,0 +1,32 @@ | |||
| // E2E test trigger — re-review after secrets binding fix | |||
There was a problem hiding this comment.
🔴 Critical
What's happening:
- 🏗️ [ARCHITECTURE] Violation of Open/Closed Principle and performance risk: Direct Firestore writes in a loop create an N+1 write problem; should use batched operations.
Recommendation:
- Apply the suggested fix below
- await _firestore.coll... entry.value});| // E2E test trigger — re-review after secrets binding fix | |
| final batch = _firestore.batch(); | |
| for (final entry in scores.entries) { | |
| final docRef = _firestore.collection('scores').doc(entry.key); | |
| batch.set(docRef, {'score': entry.value}, SetOptions(merge: true)); | |
| } | |
| await batch.commit(); |
🤖 Prompt for AI Agents
There is a critical issue in `lib/services/score_sync_service.dart` at line 1.
## Issue
🏗️ [ARCHITECTURE] Violation of Open/Closed Principle and performance risk: Direct Firestore writes in a loop create an N+1 write problem; should use batched operations.
## Current Code
await _firestore.coll... entry.value});
## Suggested Fix
final batch = _firestore.batch();
for (final entry in scores.entries) {
final docRef = _firestore.collection('scores').doc(entry.key);
batch.set(docRef, {'score': entry.value}, SetOptions(merge: true));
}
await batch.commit();
## Instructions
Fix the issue in `lib/services/score_sync_service.dart` at line 1. Apply the suggested fix above, ensuring it integrates correctly with the surrounding code. Verify the fix doesn't break any existing functionality.
|
|
||
| /// Clears the local cache for a user. | ||
| Future<void> clearCache(String userId) async { | ||
| print('Clearing cache for $userId'); |
There was a problem hiding this comment.
🟡 Minor | ⚡ Quick win
💡 Use of print() Instead of AppLogger in clearCache
What's happening:
- print() statement used for logging in production code
- Violates team rule: 'Always use AppLogger instead of print() or debugPrint()'
- Method currently only logs and doesn't implement actual cache clearing
Impact:
- Missing operational visibility in production environments
- Inconsistent logging practices
- Method appears to be a stub without actual implementation
Recommendation:
- Replace print() with AppLogger.debug() or appropriate level
- Implement actual local cache clearing logic
- Consider what local cache needs to be cleared for scores
- print('Clearing cache for $userId');| print('Clearing cache for $userId'); | |
| AppLogger.debug('Clearing cache for user: $userId'); | |
| // TODO: Implement actual local cache clearing logic |
🤖 Prompt for AI Agents
There is a medium issue in `lib/services/score_sync_service.dart` at line 30.
## Issue
What's happening:
- print() statement used for logging in production code
- Violates team rule: 'Always use AppLogger instead of print() or debugPrint()'
- Method currently only logs and doesn't implement actual cache clearing
Impact:
- Missing operational visibility in production environments
- Inconsistent logging practices
- Method appears to be a stub without actual implementation
Recommendation:
- Replace print() with AppLogger.debug() or appropriate level
- Implement actual local cache clearing logic
- Consider what local cache needs to be cleared for scores
## Current Code
print('Clearing cache for $userId');
## Suggested Fix
AppLogger.debug('Clearing cache for user: $userId');
// TODO: Implement actual local cache clearing logic
## Instructions
Fix the issue in `lib/services/score_sync_service.dart` at line 30. Apply the suggested fix above, ensuring it integrates correctly with the surrounding code. Verify the fix doesn't break any existing functionality.
|
Auto-Fix PR Available CodePeel has generated fixes for this review: #201 |
1 participant
What this does
Summary by CodeRabbit