feat: added timestamp fields (MR-78) - #260
Merged
Merged
Conversation
miguelccodev
requested review from
ashwinnair-chimple,
bernhardccodev,
dz4va and
janfb-codev
May 28, 2026 17:34
📝 WalkthroughWalkthroughAdds a public payload ChangesPayload metadata, SDK/desugaring, and IDE config
🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
miguelccodev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java (1)
147-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve metadata when updating existing summary docs.
storeSummaryPayloadupdates an existing document withset(record)whererecordonly containscr_user_id,app_id,data, andupdated_at. Since Firestoreset(map)overwrites the whole document by default (noSetOptions.merge), the first update will delete metadata fields thatcreateNewSummaryDocinitially writes:collection,created_at, andschema_version.🛠️ Suggested fix
Map<String, Object> record = new HashMap<>(); record.put("cr_user_id", payload.cr_user_id); record.put("app_id", payload.app_id); + record.put("collection", payload.collection); + record.put( + "schema_version", + payload.schema_version != null && !payload.schema_version.trim().isEmpty() + ? payload.schema_version + : "unknown" + ); query.get() .addOnSuccessListener(querySnapshot -> { if (!querySnapshot.isEmpty()) { @@ Map<String, Object> mergedData = mergeData(existingDoc, payload); record.put("data", mergedData); + Object createdAt = existingDoc.get("created_at"); + if (createdAt != null) { + record.put("created_at", createdAt); + } record.put("updated_at", Instant.now().toString()); db.collection(payload.collection) .document(existingDoc.getId()) .set(record)🤖 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 147 - 166, storeSummaryPayload currently overwrites existing summary docs because it calls db.collection(...).document(existingDoc.getId()).set(record) with a map that lacks metadata; change this to merge the fields instead of replacing the document by calling set with merge options so metadata written by createNewSummaryDoc (fields like collection, created_at, schema_version) are preserved; locate the call in DefaultAppEventPayloadHandler (method storeSummaryPayload) and replace the plain set(...) with the Firestore set that uses SetOptions.merge (or equivalent merge parameter) while still writing updated_at and the merged data produced by mergeData.
🧹 Nitpick comments (1)
.idea/gradle.xml (1)
7-7: 💤 Low valueConsider removing or ignoring
.idea/gradle.xml(testRunner preference)
.idea/gradle.xmlis tracked and is not ignored by git. The repo also has.idea/.gitignore, but it ignoresworkspace.xmlandmodules.xmlwhilegradle.xml(and some other.idea/*files) remain committed—suggesting this change is likely per-developer/auto-generated. If it’s accidental, remove it and update ignore rules; if the team intentionally wants a shared test runner setting, call out the intent.🤖 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 @.idea/gradle.xml at line 7, The committed .idea/gradle.xml containing the testRunner preference (option name="testRunner" value="CHOOSE_PER_TEST") appears to be an IDE-specific, per-developer file and should be removed from version control or explicitly curated; either remove the file from the repo and add gradle.xml (and other non-shared .idea files) to the repository ignore rules, or if this setting is intended to be shared, add a short note in the repo/team guidelines explaining that option and why it must be versioned. To fix: delete .idea/gradle.xml from the index (so it’s no longer tracked), update .idea/.gitignore to include gradle.xml (and any other unintended .idea files), and commit the ignore change, or conversely keep the file and add documentation indicating the deliberate shared setting for testRunner.
🤖 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.
Outside diff comments:
In
`@app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java`:
- Around line 147-166: storeSummaryPayload currently overwrites existing summary
docs because it calls
db.collection(...).document(existingDoc.getId()).set(record) with a map that
lacks metadata; change this to merge the fields instead of replacing the
document by calling set with merge options so metadata written by
createNewSummaryDoc (fields like collection, created_at, schema_version) are
preserved; locate the call in DefaultAppEventPayloadHandler (method
storeSummaryPayload) and replace the plain set(...) with the Firestore set that
uses SetOptions.merge (or equivalent merge parameter) while still writing
updated_at and the merged data produced by mergeData.
---
Nitpick comments:
In @.idea/gradle.xml:
- Line 7: The committed .idea/gradle.xml containing the testRunner preference
(option name="testRunner" value="CHOOSE_PER_TEST") appears to be an
IDE-specific, per-developer file and should be removed from version control or
explicitly curated; either remove the file from the repo and add gradle.xml (and
other non-shared .idea files) to the repository ignore rules, or if this setting
is intended to be shared, add a short note in the repo/team guidelines
explaining that option and why it must be versioned. To fix: delete
.idea/gradle.xml from the index (so it’s no longer tracked), update
.idea/.gitignore to include gradle.xml (and any other unintended .idea files),
and commit the ignore change, or conversely keep the file and add documentation
indicating the deliberate shared setting for testRunner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 037c125a-edbe-4dd1-8ba1-f5b4b95dd8d8
📒 Files selected for processing (5)
.idea/gradle.xml.idea/misc.xmlapp/build.gradleapp/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.javaapp/src/main/java/org/curiouslearning/container/core/subapp/payload/AppEventPayload.java
💤 Files with no reviewable changes (1)
- .idea/misc.xml
janfb-codev
requested changes
May 29, 2026
bernhardccodev
approved these changes
May 29, 2026
janfb-codev
approved these changes
Jun 3, 2026
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 (2)
app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java (2)
45-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the shared validator if
timestampis no longer required.Line 45 relaxes the handler-side checks, but
WebApp.javastill callsAppEventPayloadValidator.validate()first, and that validator rejects any payload with a blanktimestamp. On the main path, clients that stop sendingtimestampwill still be dropped beforehandle()runs.🤖 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 45 - 48, The handler relaxed checks for timestamp in DefaultAppEventPayloadHandler, but AppEventPayloadValidator.validate() still rejects blank timestamps; update the shared validator so it no longer requires a non-blank timestamp (or treats timestamp as optional) to match the handler logic: modify AppEventPayloadValidator.validate() to skip or allow empty/null payload.timestamp (and update any error messages/tests that assert timestamp presence) so WebApp.java’s pre-validate step won't drop payloads before DefaultAppEventPayloadHandler.handle() runs.
179-182:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon't create a new summary document when the lookup fails.
Lines 179-182 treat any Firestore query error the same as “no existing record” and immediately call
createNewSummaryDoc(). On transient failures, that can create a secondsummary_datadocument for the same(cr_user_id, app_id)pair; after that,limit(1)updates an arbitrary copy and the summary state diverges across duplicates.🤖 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 - 182, The failure handler in DefaultAppEventPayloadHandler currently treats any Firestore query error as "no record" and calls createNewSummaryDoc; remove that call from the addOnFailureListener and instead only log the error (or surface it to the caller) so new summary documents are created exclusively from the addOnSuccessListener when the query returns empty results. Update the listener attached to the Firestore lookup so createNewSummaryDoc(db, payload, record) is invoked only in the success path when no documents are found, and ensure addOnFailureListener(e -> { Log.w(TAG, "...", e); /* propagate/return or handle error */ }) does not create new documents.
🤖 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-167: The update path in DefaultAppEventPayloadHandler that
merges mergedData into record and writes to payload.collection for existingDoc
(document id from existingDoc.getId()) does not set schema_version, so summaries
retain stale/unknown values; modify that update block to set
record.put("schema_version", <the same source used by createNewSummaryDoc(),
e.g., payload.schemaVersion or whatever field createNewSummaryDoc uses>) before
calling .set(record, SetOptions.merge()) so both new and existing summary docs
have an up-to-date schema_version.
---
Outside diff comments:
In
`@app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java`:
- Around line 45-48: The handler relaxed checks for timestamp in
DefaultAppEventPayloadHandler, but AppEventPayloadValidator.validate() still
rejects blank timestamps; update the shared validator so it no longer requires a
non-blank timestamp (or treats timestamp as optional) to match the handler
logic: modify AppEventPayloadValidator.validate() to skip or allow empty/null
payload.timestamp (and update any error messages/tests that assert timestamp
presence) so WebApp.java’s pre-validate step won't drop payloads before
DefaultAppEventPayloadHandler.handle() runs.
- Around line 179-182: The failure handler in DefaultAppEventPayloadHandler
currently treats any Firestore query error as "no record" and calls
createNewSummaryDoc; remove that call from the addOnFailureListener and instead
only log the error (or surface it to the caller) so new summary documents are
created exclusively from the addOnSuccessListener when the query returns empty
results. Update the listener attached to the Firestore lookup so
createNewSummaryDoc(db, payload, record) is invoked only in the success path
when no documents are found, and ensure addOnFailureListener(e -> { Log.w(TAG,
"...", e); /* propagate/return or handle error */ }) does not create new
documents.
🪄 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: 3c1b71fd-63f1-4bbe-adb9-91d4393454b2
📒 Files selected for processing (1)
app/src/main/java/org/curiouslearning/container/core/subapp/handler/DefaultAppEventPayloadHandler.java
Comment on lines
162
to
+167
| record.put("data", mergedData); | ||
|
|
||
| record.put("updated_at", Instant.now().toString()); | ||
|
|
||
| db.collection(payload.collection) | ||
| .document(existingDoc.getId()) | ||
| .set(record) | ||
| .set(record, SetOptions.merge()) |
There was a problem hiding this comment.
Persist schema_version on the summary update path as well.
Lines 162-167 only update data and updated_at. New summary docs get schema_version in createNewSummaryDoc(...), but existing docs never refresh it, so older records keep stale or "unknown" values even after newer payloads arrive.
Suggested fix
record.put("data", mergedData);
record.put("updated_at", Instant.now().toString());
+ record.put(
+ "schema_version",
+ payload.schema_version != null
+ ? payload.schema_version
+ : "unknown"
+ );
db.collection(payload.collection)
.document(existingDoc.getId())
.set(record, SetOptions.merge())🤖 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 162 - 167, The update path in DefaultAppEventPayloadHandler that
merges mergedData into record and writes to payload.collection for existingDoc
(document id from existingDoc.getId()) does not set schema_version, so summaries
retain stale/unknown values; modify that update block to set
record.put("schema_version", <the same source used by createNewSummaryDoc(),
e.g., payload.schemaVersion or whatever field createNewSummaryDoc uses>) before
calling .set(record, SetOptions.merge()) so both new and existing summary docs
have an up-to-date schema_version.
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
How to test
Ref: MR-78
Summary by CodeRabbit
Chores
Enhancements