Skip to content

feat: add ScoreSyncService for batch score uploads #200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hexivine wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: add ScoreSyncService for batch score uploads #200

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3438431
feat: add ScoreSyncService for batch score uploads
hexivine Jun 14, 2026
587ed21
chore: trigger re-review after credentials fix
hexivine Jun 14, 2026
3726fef
chore: trigger re-review after secrets binding fix
hexivine Jun 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions lib/services/score_sync_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// E2E test trigger — re-review after secrets binding fix

@codepeel codepeel Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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});
Suggested change
// 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.

import 'package:cloud_firestore/cloud_firestore.dart';

/// Syncs scores between local cache and Firestore.
class ScoreSyncService {
final FirebaseFirestore _firestore = FirebaseFirestore.instance;

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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;

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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.

return score;
}
Comment on lines +8 to +16

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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.


/// 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');

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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: Replace print('Pushing ${scores.length} scores') with dev.log('Pushing ${scores.length} scores', name: 'ScoreSyncService', level: 800).
  • lib/services/score_sync_service.dart#L29-L29: Replace print('Clearing cache for $userId') with dev.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

// BUG 3: raw Firestore write — violates 'Firestore writes must use batch or transaction'
for (final entry in scores.entries) {
await _firestore.collection('scores').doc(entry.key).set({'score': entry.value});
}
Comment on lines +23 to +25

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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.

Suggested change
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.

}

/// Clears the local cache for a user.
Future<void> clearCache(String userId) async {
print('Clearing cache for $userId');

@codepeel codepeel Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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');
Suggested change
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.

}
}