feat: [CR] Summary Data Improvements (MR-78)#256
Closed
miguelccodev wants to merge 5 commits into
Closed
Conversation
MR-72 Update DefaultAppEventPayloadHandler.java
📝 WalkthroughWalkthroughHandler changes standardize created_at/updated_at/schema_version handling and route "summary_data" explicitly; summary document creation is delegated to createNewSummaryDoc. Also adds AppEventPayload.schema_version and raises app minSdk to 26. ChangesMetadata field standardization in event payload handler
Build config and payload schema
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java (3)
44-51:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSyntax error: Incomplete condition removal.
The
timestampvalidation was removed but left invalid syntax—line 46 ends with||and line 47 has only a closing). This will not compile, as confirmed by the static analysis parse error.🐛 Proposed fix
if (payload.cr_user_id == null || payload.cr_user_id.trim().isEmpty() || payload.app_id == null || payload.app_id.trim().isEmpty() || - normalizedCollection == null || normalizedCollection.isEmpty() || - ) { + normalizedCollection == null || normalizedCollection.isEmpty()) { Log.e(TAG, "Invalid payload — missing or blank required fields"); return; }🤖 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 `@app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java` around lines 44 - 51, The if-condition in DefaultAppEventPayloadHandler has a trailing logical operator and an extra parenthesis causing a syntax error; edit the conditional that references payload.cr_user_id, payload.app_id and normalizedCollection to remove the stray "||" and the unmatched ")" so the expression is a single well-formed boolean check (or, if you intended to validate payload.timestamp, reintroduce that check properly instead of leaving dangling tokens); keep the existing Log.e(TAG, ...) and return behavior unchanged.
186-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVariable shadowing:
recordparameter is unused.Line 201 declares a new
Map<String, Object> recordthat shadows the method parameter. The parameter passed from lines 176 and 182 is completely ignored.Since the method now builds its own complete record, remove the parameter entirely:
🔧 Proposed fix
private void createNewSummaryDoc( FirebaseFirestore db, - AppEventPayload payload, - Map<String, Object> record + AppEventPayload payload ) { if (!(payload.data instanceof Map)) {Then update the call sites:
- createNewSummaryDoc(db, payload, record); + createNewSummaryDoc(db, payload);🤖 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 `@app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java` around lines 186 - 215, The createNewSummaryDoc method currently declares and populates a new local Map named record that shadows and never uses the method parameter record; remove the unused parameter from the method signature (delete the Map<String, Object> record parameter) and update all call sites that pass a record to call createNewSummaryDoc(FirebaseFirestore db, AppEventPayload payload) instead; ensure the method still constructs the local record Map, uses payload fields (cr_user_id, app_id, collection, data, schema_version) and performs db.collection(payload.collection).add(record) unchanged.
179-183:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCompilation error:
recordis out of scope in failure callback.The
recordvariable is defined at line 150 inside theaddOnSuccessListenerlambda, but line 182 attempts to reference it from theaddOnFailureListenerlambda—a separate scope whererecorddoes not exist.🐛 Proposed fix
Create the record inside the failure handler or extract it to the enclosing method scope:
.addOnFailureListener(e -> { Log.w(TAG, "Query failed — creating new summary record", e); - createNewSummaryDoc(db, payload, record); + Map<String, Object> record = new HashMap<>(); + record.put("cr_user_id", payload.cr_user_id); + record.put("app_id", payload.app_id); + createNewSummaryDoc(db, payload, record); });Alternatively, since
createNewSummaryDocnow builds its own record (lines 201-207), consider whether therecordparameter is still needed.🤖 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 `@app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java` around lines 179 - 183, The failure callback in DefaultAppEventPayloadHandler's addOnFailureListener references the local variable record that is only defined inside the addOnSuccessListener lambda, causing a compile error; fix by moving creation of the SummaryRecord (the same object named record) into the addOnFailureListener scope before calling createNewSummaryDoc, or if createNewSummaryDoc already builds its own record (per lines ~201-207), remove the record parameter from createNewSummaryDoc and all callers (including the addOnSuccessListener) and update its signature so callers simply call createNewSummaryDoc(db, payload) from the failure handler; update references to the record variable accordingly.
🤖 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
`@app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java`:
- Around line 162-171: The current update in DefaultAppEventPayloadHandler is
using document(...).set(record) which overwrites the whole document and will
drop existing fields like created_at and schema_version; change this to either
merge the new fields into the existing document by calling set(record,
SetOptions.merge()) or explicitly copy created_at and schema_version from
existingDoc into record before calling set, ensuring you still update updated_at
and data while preserving metadata; update the code around
db.collection(payload.collection).document(existingDoc.getId()).set(...)
accordingly.
---
Outside diff comments:
In
`@app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java`:
- Around line 44-51: The if-condition in DefaultAppEventPayloadHandler has a
trailing logical operator and an extra parenthesis causing a syntax error; edit
the conditional that references payload.cr_user_id, payload.app_id and
normalizedCollection to remove the stray "||" and the unmatched ")" so the
expression is a single well-formed boolean check (or, if you intended to
validate payload.timestamp, reintroduce that check properly instead of leaving
dangling tokens); keep the existing Log.e(TAG, ...) and return behavior
unchanged.
- Around line 186-215: The createNewSummaryDoc method currently declares and
populates a new local Map named record that shadows and never uses the method
parameter record; remove the unused parameter from the method signature (delete
the Map<String, Object> record parameter) and update all call sites that pass a
record to call createNewSummaryDoc(FirebaseFirestore db, AppEventPayload
payload) instead; ensure the method still constructs the local record Map, uses
payload fields (cr_user_id, app_id, collection, data, schema_version) and
performs db.collection(payload.collection).add(record) unchanged.
- Around line 179-183: The failure callback in DefaultAppEventPayloadHandler's
addOnFailureListener references the local variable record that is only defined
inside the addOnSuccessListener lambda, causing a compile error; fix by moving
creation of the SummaryRecord (the same object named record) into the
addOnFailureListener scope before calling createNewSummaryDoc, or if
createNewSummaryDoc already builds its own record (per lines ~201-207), remove
the record parameter from createNewSummaryDoc and all callers (including the
addOnSuccessListener) and update its signature so callers simply call
createNewSummaryDoc(db, payload) from the failure handler; update references to
the record variable accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26bbc3f6-b89e-4889-978b-398e30964746
📒 Files selected for processing (2)
app/fastlane/debug_apks/app-debug.apkapp/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java
dz4va
approved these changes
May 22, 2026
miguelccodev
changed the title
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
4 participants
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.
Changes
Ref: MR-78
Summary by CodeRabbit
Refactor
Chore
New Fields