Feature/time decay reputation#481
Conversation
… trigger idempotency
…ing coverage increased , Cobblering issue addressed
|
Warning Review limit reached
More reviews will be available in 14 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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 Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors the reputation system from simple linear scoring to a time-decay bucket model with monthly aggregation, adds event start timestamps across client and backend, hardens certificate distribution with API-based authorization, introduces pagination-based batch processing for scalability, and migrates to ESLint flat configuration. Changes span app screens, check-in service, Cloud Functions, tests, and documentation. ChangesTime-Decayed Reputation System Refactor
Event Start Timestamp Propagation
Certificate Service Security and API Hardening
Scalability: Pagination and Batching
Admin Security and Rate Limiting
Tooling, Configuration, and Documentation
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested labels: Suggested reviewers:
🚥 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: 9
🧹 Nitpick comments (11)
app/src/screens/EventRegistrationFormScreen.js (1)
143-147: 💤 Low valueValidate
freshEvent.startAtbefore writing to participating document.Ensure the
startAtfield is defined before writing to Firestore to preventundefinedfrom being stored, which could cause issues in reputation bucketing logic that relies on event timestamps.♻️ Optional enhancement
transaction.set(participatingRef, { eventId: event.id, - eventStartAt: freshEvent.startAt, + ...(freshEvent.startAt && { eventStartAt: freshEvent.startAt }), joinedAt: new Date().toISOString(), });🤖 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/screens/EventRegistrationFormScreen.js` around lines 143 - 147, Before calling transaction.set on participatingRef, verify freshEvent.startAt is defined and valid; if it's undefined/invalid, abort the transaction (throw) or set an explicit fallback (e.g., use event.startAt or current timestamp) instead of writing undefined. Update the logic around transaction.set(participatingRef, {...}) to perform this check for freshEvent.startAt and either throw a descriptive error to halt the transaction or substitute a safe timestamp value so the participating document never stores undefined.app/src/screens/EventDetail.js (2)
512-517: 💤 Low valueValidate
eventData.startAtexists before writing to participating document.Similar to the reminder document, ensure
startAtis defined before writing it to avoid storingundefinedin Firestore, which could break reputation bucketing downstream.♻️ Optional enhancement
transaction.set(userRef, { eventId: eventId, - eventStartAt: eventData.startAt, + ...(eventData.startAt && { eventStartAt: eventData.startAt }), joinedAt: new Date().toISOString(), });🤖 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/screens/EventDetail.js` around lines 512 - 517, The transaction writes eventData.startAt into the participating user document without checking it, which can store undefined; before calling transaction.set for userRef (and when building participantPayload if applicable), check that eventData.startAt is defined and only include eventStartAt in the object when it exists (or explicitly set it to null/default) to avoid writing undefined to Firestore; update the transaction.set(userRef, {...}) call to conditionally add eventStartAt based on eventData.startAt (referencing transaction.set, userRef, participantPayload, eventData.startAt, and eventId).
403-411: 💤 Low valueConsider validating
event.startAtbefore writing to Firestore.While events should always have a
startAtfield, writingundefinedto Firestore when it's missing could cause issues in reputation bucketing logic that depends on this timestamp. A defensive check would prevent data integrity issues.♻️ Optional enhancement
const docRef = await addDoc(collection(db, 'reminders'), { userId: user.uid, eventId: event.id, eventTitle: event.title, - eventStartAt: event.startAt, + ...(event.startAt && { eventStartAt: event.startAt }), remindAt: new Date(new Date(event.startAt).getTime() - 10 * 60000), // 10 mins before notificationId: notifId, createdAt: new Date().toISOString(), });This pattern conditionally includes the field only when it exists.
🤖 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/screens/EventDetail.js` around lines 403 - 411, The code is writing event.startAt and a derived remindAt without validating it; update the block that calls addDoc(collection(db, 'reminders'), ...) to first check event.startAt and handle the missing case: either abort/log/throw and avoid creating the reminder (use process/console logger and return) or build the payload so eventStartAt and remindAt are only included when event.startAt is a valid date (i.e., check typeof event.startAt and !isNaN(new Date(event.startAt))). Ensure the notificationId/other fields (notifId, user.uid, event.id, event.title, createdAt) are still persisted as appropriate and that remindAt calculation only runs when event.startAt is valid.app/src/screens/PaymentScreen.js (1)
198-205: 💤 Low valueValidate
freshEvent.startAtbefore writing to participating document.Ensure the
startAtfield exists before writing it to Firestore to avoid storingundefined, which could break downstream reputation bucketing that depends on event timestamps.♻️ Optional enhancement
transaction.set(participatingRef, { eventId: event.id, - eventStartAt: freshEvent.startAt, + ...(freshEvent.startAt && { eventStartAt: freshEvent.startAt }), joinedAt: new Date().toISOString(), role: 'attendee', ticketId: ticketRef.id, status: 'paid', });🤖 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/screens/PaymentScreen.js` around lines 198 - 205, The participate write is storing freshEvent.startAt directly which can be undefined; before calling transaction.set(participatingRef, ...) validate freshEvent.startAt and only write a safe value (e.g., use a normalized timestamp or null) or omit the eventStartAt field when absent to avoid persisting undefined. Locate the transaction.set call that writes the participating document and change the payload construction so it checks freshEvent.startAt (from freshEvent) and assigns a fallback or skips the key if undefined, ensuring downstream reputation bucketing always sees a defined timestamp or null.app/src/lib/checkInService.js (1)
473-474: ⚡ Quick winWrap event fetch in error handling for robustness.
If the event document doesn't exist or the fetch fails (e.g., network error, permissions),
eventStartAtwill benulland sync will continue. While the fallback chain insyncOfflineCheckInItemhandles this, logging the error would aid debugging and surface potential data issues.♻️ Proposed enhancement
- const eventSnap = await getDoc(doc(db, 'events', eventId)); - const eventStartAt = eventSnap.exists() ? eventSnap.data()?.startAt : null; + let eventStartAt = null; + try { + const eventSnap = await getDoc(doc(db, 'events', eventId)); + eventStartAt = eventSnap.exists() ? eventSnap.data()?.startAt : null; + } catch (err) { + logger.warn('Could not fetch event for offline sync, continuing without event-level startAt:', err); + }🤖 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/lib/checkInService.js` around lines 473 - 474, The event fetch using getDoc(doc(db, 'events', eventId)) should be wrapped in a try/catch so failures (network/permission) are logged rather than silently setting eventStartAt to null; inside the catch log the error (use the module's logger or console) with context including eventId and function name (syncOfflineCheckInItem or the surrounding function) and then set eventStartAt to null so existing fallback behavior remains; update the code around eventSnap/eventStartAt to reflect this error handling while keeping the rest of syncOfflineCheckInItem logic unchanged.cloud-functions/src/reputation.test.ts (1)
94-105: 💤 Low valueTest context missing
eventIdfor idempotency key.The wrapped trigger uses
context.eventIdto build the idempotency key. The test context only providesparamsbut noteventId, resulting in an idempotency key ofparticipating_create_undefined. Consider providingeventIdto more accurately simulate production behavior and enable idempotency testing.🧪 Proposed fix to add eventId
const wrapped = testEnv.wrap(onParticipatingCreate); - await wrapped(snap, { params: { userId: 'u1', eventId: 'e1' } }); + await wrapped(snap, { + params: { userId: 'u1', eventId: 'e1' }, + eventId: 'test-event-id-123' + });🤖 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 `@cloud-functions/src/reputation.test.ts` around lines 94 - 105, The test for onParticipatingCreate is missing context.eventId so the idempotency key becomes "participating_create_undefined"; update the test to pass an eventId in the wrapped invocation (e.g., when calling wrapped(snap, { params: { userId: 'u1', eventId: 'e1' } }) include an eventId in the second argument's context/eventId field) so the code that builds the idempotency key (using context.eventId) is exercised correctly and idempotency behavior can be asserted.cloud-functions/src/reputation.ts (2)
535-544: 💤 Low valueBackfill is additive, not idempotent.
Using
FieldValue.increment()withmerge: truemeans running this backfill multiple times will accumulate values (e.g., running twice doubles all counts). Consider either:
- Adding a
processedBackfillmarker to skip already-processed records, or- Documenting that buckets should be cleared before re-running backfill.
🤖 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 `@cloud-functions/src/reputation.ts` around lines 535 - 544, The backfill currently uses admin.firestore.FieldValue.increment(...) with batch.set(..., { merge: true }) on bucketRef (using bucket.monthKey, bucket.registrations, bucket.attendances, bucket.reminders), which makes the operation non-idempotent; modify the backfill to be safe by adding a processed marker or making writes idempotent: add a processedBackfill flag (e.g., processedBackfill: true or processedAt timestamp) to each bucket document and have the routine skip documents that already have that marker, or compute and write absolute counts instead of using FieldValue.increment so repeated runs won’t accumulate—ensure the logic around batch.set and bucketRef checks this marker before applying increments/updates.
188-240: ⚖️ Poor tradeoffConsider throttling parallel subcollection reads.
The current implementation fires up to 500 concurrent
.get()calls for user reputation buckets within each page. Under high load, this may exceed Firestore rate limits or cause memory pressure. Consider batching the reads (e.g., process 50-100 users at a time within each page) or using a semaphore to limit concurrency.🤖 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 `@cloud-functions/src/reputation.ts` around lines 188 - 240, The code spawns up to 500 concurrent subcollection reads inside paginateQuery by using Promise.all over docs; replace that with a bounded-concurrency approach to avoid Firestore rate limits — either process docs in fixed-size chunks (e.g., 50–100) or use a semaphore/promise limiter (p-limit) to cap concurrent calls to userDoc.ref.collection(REPUTATION_BUCKETS_COLLECTION).get(); keep the same aggregation logic (attendanceCount, registrationCount, remindersSet, points) and the batch.update calls but perform them as each chunk completes, and only call batch.commit() after each chunk (or when batch size approaches limits) to avoid large in-memory batches; update the section around Promise.all(docs.map(...)) to iterate in controlled concurrency instead.cloud-functions/lib/server.js (1)
187-232: ⚖️ Poor tradeoffConsider whether authentication is needed for the certificate endpoint.
The
/api/certificateendpoint is publicly accessible without authentication. While the endpoint validates that the certificate exists and isn't revoked, anyone who knows (or guesses) aneventIdandparticipantIdcan access the certificate.The walkthrough.md mentions "strict authorization" for this endpoint. If the intent is that only the certificate holder should access it, consider adding authentication. However, if the security model relies on the obscurity of
participantIdvalues plus revocation capability, the current approach may be acceptable.🤖 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 `@cloud-functions/lib/server.js` around lines 187 - 232, The /api/certificate handler currently allows public access; update it to enforce authentication and owner-only access by adding an auth check (e.g., middleware like authenticateRequest or inline use of admin.auth().verifyIdToken on the Authorization header) inside the app.get('/api/certificate', ...) flow before fetching the participant document, and then validate that the authenticated user's uid equals participantId or that the user has an admin/organizer role in the event document; if the token is missing/invalid or the user is not authorized, return 401/403 respectively while preserving the existing existence/revocation/storage checks and signed-url generation logic.cloud-functions/src/certificateService.ts (1)
135-137: 💤 Low valueJSDoc comment is misleading.
The docstring says "returns its signed URL" but the function now returns an API-based download URL (
/api/certificate?...), not a signed Storage URL. Consider updating the documentation to reflect the actual behavior.📝 Proposed fix
-/** - * Uploads a generated PDF certificate buffer to Firebase Storage and returns its signed URL. - */ +/** + * Uploads a generated PDF certificate buffer to Firebase Storage and returns an API download URL. + */🤖 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 `@cloud-functions/src/certificateService.ts` around lines 135 - 137, The JSDoc above the certificate upload function incorrectly states it "returns its signed URL"; update that docstring for the function that uploads the PDF buffer (the function with the current comment "Uploads a generated PDF certificate buffer to Firebase Storage and returns its signed URL.") to accurately describe that it uploads the PDF to Firebase Storage and returns the application API download URL (e.g. /api/certificate?...), update the "`@returns`" line to mention the API-based download URL and any query params/tokens returned, and adjust any example usage in the comment to show calling the API endpoint rather than a signed Storage URL.cloud-functions/lib/dailyDigest.js (1)
54-64: ⚡ Quick win
startAttype matches current writes; only legacy/mixed docs could break the count
getTodayEventCountincloud-functions/lib/dailyDigest.jsqueriesevents.startAtusingtoday.toISOString()/tomorrow.toISOString(). Current event creation writesstartAtas an ISO string (app/src/screens/CreateEvent.jsusesstartAt: startDate.toISOString()), so this query is consistent for newly created events.
cloud-functions/src/reputation.tssupportsstartAtas FirestoreTimestamptoo, so if older/legacy events were stored asTimestamp, the daily digest count could undercount them—consider normalizing/migrating to a single type.🤖 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 `@cloud-functions/lib/dailyDigest.js` around lines 54 - 64, getTodayEventCount currently queries events.startAt using ISO strings (today.toISOString()/tomorrow.toISOString()), which will miss legacy documents where startAt is a Firestore Timestamp; update getTodayEventCount to handle both types by querying for Timestamp ranges when documents use Timestamps or by normalizing startAt values before comparison (e.g., convert stored Timestamps to ISO strings or convert ISO strings to Timestamps), and/or perform a dual-check: fetch documents where startAt is between Timestamp(today) and Timestamp(tomorrow) as well as where startAt is between ISO strings, ensuring you reference the getTodayEventCount function and consider compatibility with cloud-functions/src/reputation.ts which expects Timestamp values.
🤖 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/lib/checkInService.js`:
- Around line 407-413: The catch block that handles errors from
getDoc(checkInRef) in syncOfflineCheckIns currently logs to console.error then
returns silently; change it to log the error (include checkInRef/id and error
details) and propagate the failure so the caller can track failed items—either
rethrow the error or return a structured failure result; update references in
this block (checkInRef, checkInSnap, getDoc, and syncOfflineCheckIns) so errors
are recorded in logs and the syncOfflineCheckIns caller can handle/report failed
syncs rather than dropping the item silently.
In `@cloud-functions/eslint.config.mjs`:
- Around line 35-36: Remove the two obsolete ESLint core rule entries
"require-jsdoc" and "valid-jsdoc" from the flat config so the config loads under
ESLint v9+; locate the rules in the cloud-functions eslint config (the entries
currently set to "off") and delete those two lines (or remove those keys from
the exported rules object) so they are no longer present in the configuration.
In `@cloud-functions/lib/onEventCreate.js`:
- Around line 54-65: The current snapshot.forEach uses the pushToken guard to
skip creating the in-app notification; change the logic so the notifications
document is always created (use the same notifRef and batch.set for the
'notifications' collection unconditionally) and only gate push-message sending
on the presence of pushToken; update the block around snapshot.forEach /
pushToken / notifRef / batch.set so batch.set always runs to create the in-app
notification and any push delivery logic (where pushToken is read) remains
conditional.
- Around line 43-48: The query uses startAfter(startAfter) without an orderBy,
which breaks Firestore cursor pagination; update the users query built in
onEventCreate.js to include an explicit ordering (e.g.,
.orderBy(FieldPath.documentId())) before .startAfter(...) and ensure the value
passed to startAfter is the matching document id or snapshot used for ordering
(mirror the pattern used in dailyDigest.js), and import/resolve FieldPath from
the Firestore SDK as needed.
In `@cloud-functions/lib/server.js`:
- Line 77: The ipWhitelist middleware is imported after it's used, causing a
TDZ/ReferenceError at runtime; open cloud-functions/src/server.ts and move the
import/require for the ipWhitelist module (the symbol ipWhitelist or import from
"./middleware/ipWhitelist") to precede the first call to app.use("/api",
ipWhitelist) so the emitted CommonJS require appears before that usage; ensure
the import statement is placed above where app is configured (before any
app.use("/api", ...)) and retain the same imported symbol name (ipWhitelist) so
no other code changes are necessary.
In `@cloud-functions/package.json`:
- Around line 47-55: The package.json devDependencies block lists pinned
versions (eslint, typescript, firebase-tools, firebase-functions-test, jest,
prettier, ts-jest, ts-node, globals); verify each version is resolvable in your
CI/registry by running npm view <pkg>@<version> or npm install in the target
environment for firebase-tools@15.19.0, firebase-functions-test@3.1.0,
jest@30.4.2, prettier@3.0.0, ts-jest@29.4.11, ts-node@10.9.2, globals@17.6.0 (in
addition to eslint@10.4.0 and typescript@6.0.3 already confirmed); if any
package/version is unavailable, update the version string in the cloud-functions
package.json to a published release (or add an alternative registry setting in
CI) and re-run install to ensure the lockfile is consistent.
In `@cloud-functions/src/reputation.test.ts`:
- Around line 22-28: The test helper getMonthStart uses local-time setters
causing timezone mismatch with production getMonthKeyFromDate which uses UTC;
update getMonthStart to construct the month-start in UTC using Date.UTC or the
UTC setters (e.g., create a Date from Date.UTC(year, month - offset, 1, 0, 0, 0,
0) or use setUTCDate/setUTCHours/setUTCMonth) so the returned Date aligns with
getMonthKeyFromDate's getUTCFullYear/getUTCMonth behavior.
In `@cloud-functions/src/reputation.ts`:
- Around line 614-620: getTopContributors is missing the App Check enforcement
that other callables use; add a call to enforceAppCheck(context) near the start
of getTopContributors (after the auth check or in the same initialization block)
so the function rejects requests without valid App Check tokens; reference the
existing enforceAppCheck(context) helper (as used in setRole.ts) and ensure it
is invoked before proceeding with any business logic in getTopContributors.
In `@cloud-functions/src/server.ts`:
- Line 40: The middleware ipWhitelist is used in the app.use('/api',
ipWhitelist) call near the top but is imported later in the file, causing a
runtime ReferenceError; move the import for ipWhitelist up into the top import
block with the other imports so it is defined before app.use is executed, and
remove the now-duplicate mid-file import of ipWhitelist to avoid redundancy.
---
Nitpick comments:
In `@app/src/lib/checkInService.js`:
- Around line 473-474: The event fetch using getDoc(doc(db, 'events', eventId))
should be wrapped in a try/catch so failures (network/permission) are logged
rather than silently setting eventStartAt to null; inside the catch log the
error (use the module's logger or console) with context including eventId and
function name (syncOfflineCheckInItem or the surrounding function) and then set
eventStartAt to null so existing fallback behavior remains; update the code
around eventSnap/eventStartAt to reflect this error handling while keeping the
rest of syncOfflineCheckInItem logic unchanged.
In `@app/src/screens/EventDetail.js`:
- Around line 512-517: The transaction writes eventData.startAt into the
participating user document without checking it, which can store undefined;
before calling transaction.set for userRef (and when building participantPayload
if applicable), check that eventData.startAt is defined and only include
eventStartAt in the object when it exists (or explicitly set it to null/default)
to avoid writing undefined to Firestore; update the transaction.set(userRef,
{...}) call to conditionally add eventStartAt based on eventData.startAt
(referencing transaction.set, userRef, participantPayload, eventData.startAt,
and eventId).
- Around line 403-411: The code is writing event.startAt and a derived remindAt
without validating it; update the block that calls addDoc(collection(db,
'reminders'), ...) to first check event.startAt and handle the missing case:
either abort/log/throw and avoid creating the reminder (use process/console
logger and return) or build the payload so eventStartAt and remindAt are only
included when event.startAt is a valid date (i.e., check typeof event.startAt
and !isNaN(new Date(event.startAt))). Ensure the notificationId/other fields
(notifId, user.uid, event.id, event.title, createdAt) are still persisted as
appropriate and that remindAt calculation only runs when event.startAt is valid.
In `@app/src/screens/EventRegistrationFormScreen.js`:
- Around line 143-147: Before calling transaction.set on participatingRef,
verify freshEvent.startAt is defined and valid; if it's undefined/invalid, abort
the transaction (throw) or set an explicit fallback (e.g., use event.startAt or
current timestamp) instead of writing undefined. Update the logic around
transaction.set(participatingRef, {...}) to perform this check for
freshEvent.startAt and either throw a descriptive error to halt the transaction
or substitute a safe timestamp value so the participating document never stores
undefined.
In `@app/src/screens/PaymentScreen.js`:
- Around line 198-205: The participate write is storing freshEvent.startAt
directly which can be undefined; before calling
transaction.set(participatingRef, ...) validate freshEvent.startAt and only
write a safe value (e.g., use a normalized timestamp or null) or omit the
eventStartAt field when absent to avoid persisting undefined. Locate the
transaction.set call that writes the participating document and change the
payload construction so it checks freshEvent.startAt (from freshEvent) and
assigns a fallback or skips the key if undefined, ensuring downstream reputation
bucketing always sees a defined timestamp or null.
In `@cloud-functions/lib/dailyDigest.js`:
- Around line 54-64: getTodayEventCount currently queries events.startAt using
ISO strings (today.toISOString()/tomorrow.toISOString()), which will miss legacy
documents where startAt is a Firestore Timestamp; update getTodayEventCount to
handle both types by querying for Timestamp ranges when documents use Timestamps
or by normalizing startAt values before comparison (e.g., convert stored
Timestamps to ISO strings or convert ISO strings to Timestamps), and/or perform
a dual-check: fetch documents where startAt is between Timestamp(today) and
Timestamp(tomorrow) as well as where startAt is between ISO strings, ensuring
you reference the getTodayEventCount function and consider compatibility with
cloud-functions/src/reputation.ts which expects Timestamp values.
In `@cloud-functions/lib/server.js`:
- Around line 187-232: The /api/certificate handler currently allows public
access; update it to enforce authentication and owner-only access by adding an
auth check (e.g., middleware like authenticateRequest or inline use of
admin.auth().verifyIdToken on the Authorization header) inside the
app.get('/api/certificate', ...) flow before fetching the participant document,
and then validate that the authenticated user's uid equals participantId or that
the user has an admin/organizer role in the event document; if the token is
missing/invalid or the user is not authorized, return 401/403 respectively while
preserving the existing existence/revocation/storage checks and signed-url
generation logic.
In `@cloud-functions/src/certificateService.ts`:
- Around line 135-137: The JSDoc above the certificate upload function
incorrectly states it "returns its signed URL"; update that docstring for the
function that uploads the PDF buffer (the function with the current comment
"Uploads a generated PDF certificate buffer to Firebase Storage and returns its
signed URL.") to accurately describe that it uploads the PDF to Firebase Storage
and returns the application API download URL (e.g. /api/certificate?...), update
the "`@returns`" line to mention the API-based download URL and any query
params/tokens returned, and adjust any example usage in the comment to show
calling the API endpoint rather than a signed Storage URL.
In `@cloud-functions/src/reputation.test.ts`:
- Around line 94-105: The test for onParticipatingCreate is missing
context.eventId so the idempotency key becomes "participating_create_undefined";
update the test to pass an eventId in the wrapped invocation (e.g., when calling
wrapped(snap, { params: { userId: 'u1', eventId: 'e1' } }) include an eventId in
the second argument's context/eventId field) so the code that builds the
idempotency key (using context.eventId) is exercised correctly and idempotency
behavior can be asserted.
In `@cloud-functions/src/reputation.ts`:
- Around line 535-544: The backfill currently uses
admin.firestore.FieldValue.increment(...) with batch.set(..., { merge: true })
on bucketRef (using bucket.monthKey, bucket.registrations, bucket.attendances,
bucket.reminders), which makes the operation non-idempotent; modify the backfill
to be safe by adding a processed marker or making writes idempotent: add a
processedBackfill flag (e.g., processedBackfill: true or processedAt timestamp)
to each bucket document and have the routine skip documents that already have
that marker, or compute and write absolute counts instead of using
FieldValue.increment so repeated runs won’t accumulate—ensure the logic around
batch.set and bucketRef checks this marker before applying increments/updates.
- Around line 188-240: The code spawns up to 500 concurrent subcollection reads
inside paginateQuery by using Promise.all over docs; replace that with a
bounded-concurrency approach to avoid Firestore rate limits — either process
docs in fixed-size chunks (e.g., 50–100) or use a semaphore/promise limiter
(p-limit) to cap concurrent calls to
userDoc.ref.collection(REPUTATION_BUCKETS_COLLECTION).get(); keep the same
aggregation logic (attendanceCount, registrationCount, remindersSet, points) and
the batch.update calls but perform them as each chunk completes, and only call
batch.commit() after each chunk (or when batch size approaches limits) to avoid
large in-memory batches; update the section around Promise.all(docs.map(...)) to
iterate in controlled concurrency instead.
🪄 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 Plus
Run ID: 93158615-1892-4d03-bf68-211f3302099a
⛔ Files ignored due to path filters (2)
cloud-functions/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
.eslintrc.jsonapp/src/lib/checkInService.jsapp/src/screens/EventDetail.jsapp/src/screens/EventRegistrationFormScreen.jsapp/src/screens/PaymentScreen.jscloud-functions/.eslintrc.jsoncloud-functions/eslint.config.mjscloud-functions/lib/analyzeAttendance.jscloud-functions/lib/certificateService.jscloud-functions/lib/dailyDigest.jscloud-functions/lib/eventNotifications.jscloud-functions/lib/index.jscloud-functions/lib/onEventCreate.jscloud-functions/lib/reputation.jscloud-functions/lib/server.jscloud-functions/lib/setRole.jscloud-functions/package.jsoncloud-functions/src/analyzeAttendance.tscloud-functions/src/certificateService.tscloud-functions/src/dailyDigest.tscloud-functions/src/eventNotifications.tscloud-functions/src/onEventCreate.tscloud-functions/src/postEventFeedback.tscloud-functions/src/reputation.test.tscloud-functions/src/reputation.tscloud-functions/src/server.tsdocs/DATA_MODEL.mddocs/FEATURES.mdeslint.config.mjspackage.jsonwalkthrough.md
💤 Files with no reviewable changes (2)
- .eslintrc.json
- cloud-functions/.eslintrc.json
| let query = db.collection('users') | ||
| .select('pushToken') | ||
| .limit(BATCH_SIZE); | ||
| if (startAfter) { | ||
| query = query.startAfter(startAfter); | ||
| } |
There was a problem hiding this comment.
Missing orderBy breaks cursor-based pagination.
The query uses startAfter(startAfter) without an orderBy clause. Firestore cursor pagination requires explicit ordering for deterministic results; without it, batches may skip or duplicate users across iterations.
Compare with the working pattern in dailyDigest.js which correctly uses .orderBy(FieldPath.documentId()).
🐛 Proposed fix
+const { FieldPath } = require("firebase-admin/firestore");
+
async function processUserBatch(db, eventId, eventTitle, startAfter) {
let query = db.collection('users')
+ .orderBy(FieldPath.documentId())
.select('pushToken')
.limit(BATCH_SIZE);🤖 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 `@cloud-functions/lib/onEventCreate.js` around lines 43 - 48, The query uses
startAfter(startAfter) without an orderBy, which breaks Firestore cursor
pagination; update the users query built in onEventCreate.js to include an
explicit ordering (e.g., .orderBy(FieldPath.documentId())) before
.startAfter(...) and ensure the value passed to startAfter is the matching
document id or snapshot used for ordering (mirror the pattern used in
dailyDigest.js), and import/resolve FieldPath from the Firestore SDK as needed.
| snapshot.forEach(userDoc => { | ||
| const pushToken = userDoc.get('pushToken'); | ||
| if (pushToken) { | ||
| const notifRef = userDoc.ref.collection('notifications').doc(`${eventId}_${userDoc.id}`); | ||
| batch.set(notifRef, { | ||
| title: 'New Event Alert! 📢', | ||
| body: `Check out: "${eventTitle}"`, | ||
| eventId: eventId, | ||
| createdAt: firestore_1.FieldValue.serverTimestamp(), | ||
| read: false | ||
| }); | ||
| } |
There was a problem hiding this comment.
Users without push tokens miss in-app notifications.
The if (pushToken) guard on line 56 gates both push AND in-app notification creation. Users who haven't granted push permissions should still receive in-app notifications.
🐛 Proposed fix - always create in-app notification
snapshot.forEach(userDoc => {
const pushToken = userDoc.get('pushToken');
- if (pushToken) {
- const notifRef = userDoc.ref.collection('notifications').doc(`${eventId}_${userDoc.id}`);
- batch.set(notifRef, {
- title: 'New Event Alert! 📢',
- body: `Check out: "${eventTitle}"`,
- eventId: eventId,
- createdAt: firestore_1.FieldValue.serverTimestamp(),
- read: false
- });
- }
+ const notifRef = userDoc.ref.collection('notifications').doc(`${eventId}_${userDoc.id}`);
+ batch.set(notifRef, {
+ title: 'New Event Alert! 📢',
+ body: `Check out: "${eventTitle}"`,
+ eventId: eventId,
+ createdAt: firestore_1.FieldValue.serverTimestamp(),
+ read: false
+ });
});🤖 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 `@cloud-functions/lib/onEventCreate.js` around lines 54 - 65, The current
snapshot.forEach uses the pushToken guard to skip creating the in-app
notification; change the logic so the notifications document is always created
(use the same notifRef and batch.set for the 'notifications' collection
unconditionally) and only gate push-message sending on the presence of
pushToken; update the block around snapshot.forEach / pushToken / notifRef /
batch.set so batch.set always runs to create the in-app notification and any
push delivery logic (where pushToken is read) remains conditional.
| "eslint": "^10.4.0", | ||
| "firebase-functions-test": "^3.1.0", | ||
| "firebase-tools": "^15.19.0", | ||
| "jest": "^30.4.2", | ||
| "prettier": "^3.0.0", | ||
| "ts-jest": "^29.4.11", | ||
| "ts-node": "^10.9.2", | ||
| "typescript": "^6.0.3" | ||
| "globals": "^17.6.0", | ||
| "jest": "^30.4.2", | ||
| "prettier": "^3.0.0", | ||
| "ts-jest": "^29.4.11", | ||
| "ts-node": "^10.9.2", | ||
| "typescript": "^6.0.3" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the latest published npm versions for eslint, typescript, @eslint/js, and @eslint/eslintrc, and do versions eslint@10.4.0andtypescript@6.0.3 exist?
💡 Result:
As of May 31, 2026, the latest published versions of the requested packages on npm are as follows: * eslint: 10.4.1 [1][2] * typescript: 6.0.3 [3][4] * @eslint/js: 9.39.4 [5] * @eslint/eslintrc: 3.3.5 [1] Regarding the specific versions you asked about: * eslint@10.4.0: This version exists [1][2]. It was published in May 2026 [1][2] and is part of the 10.4.x release series [1][2]. * typescript@6.0.3: This version exists [3][4]. It was officially released in April 2026 [4][6].
Citations:
- 1: https://www.npmjs.com/package/eslint
- 2: https://registry.npmjs.org/eslint
- 3: https://npmx.dev/package/typescript/v/6.0.3
- 4: microsoft/TypeScript@v6.0.2...v6.0.3
- 5: https://github.com/eslint/eslint/releases
- 6: https://www.linkedin.com/posts/teamco_oh-great-masters-of-code-and-those-just-activity-7451225590805716992-654c
Confirm npm availability of pinned devDependencies (eslint/typescript exist; remaining packages to double-check)
eslint@10.4.0 and typescript@6.0.3 are published on npm. Still verify the other pinned devDependencies in cloud-functions/package.json (e.g., firebase-tools@15.19.0, firebase-functions-test@3.1.0, jest@30.4.2, prettier@3.0.0, ts-jest@29.4.11, ts-node@10.9.2, globals@17.6.0) are resolvable in the target registry/CI environment.
🤖 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 `@cloud-functions/package.json` around lines 47 - 55, The package.json
devDependencies block lists pinned versions (eslint, typescript, firebase-tools,
firebase-functions-test, jest, prettier, ts-jest, ts-node, globals); verify each
version is resolvable in your CI/registry by running npm view <pkg>@<version> or
npm install in the target environment for firebase-tools@15.19.0,
firebase-functions-test@3.1.0, jest@30.4.2, prettier@3.0.0, ts-jest@29.4.11,
ts-node@10.9.2, globals@17.6.0 (in addition to eslint@10.4.0 and
typescript@6.0.3 already confirmed); if any package/version is unavailable,
update the version string in the cloud-functions package.json to a published
release (or add an alternative registry setting in CI) and re-run install to
ensure the lockfile is consistent.
…plemented properly ,getMonthKeyFromDate fixed , ipwhitelist declared properly
|
|
@roshankumar0036singh |
|
@shantanushok why 35k diff? |
We wanted to ensure that user reputation accurately reflects recent involvement rather than just accumulating points infinitely over time. To solve this, we built a linear time-decay algorithm over a rolling 12-month window. Here is exactly how the math and architecture work:
The formula is: Multiplier = (12 - months_elapsed) / 12 This creates a smooth, linear degradation of points over time or a year . 0 months ago (Current Month): Points retain 100% of their value. (Attendance = 10 pts) This involved changes being made onto multiple files (almost 21 in the first commit itself). To make all of this possible I had to implement the following components:
onParticipatingCreate / onParticipatingDelete: Increments/decrements the registration counter when a user joins or leaves an event.
Additionally I also addressed the issues which were being flagged by the coderabbitai and copilot , sonarcloud which may have snowballed . |
|
@shantanushok but still it isnt justyfying the diff against your feature there are many unecessary pr related chnages there |
|
@shantanushok remoev the walkthorugh file and package lock and package json file you didn made chnages in them |
2 participants
Description
This PR introduces the Time-Decay Reputation System, and resolves all recent merge conflicts with
upstream/main.Key Changes:
reputation.ts): Replaced flat reputation points with a time-decay model. Recent event attendance awards more points than older attendances (e.g., 6 months old = 50% points).FieldValue.increment()andrunTransaction(). This ensures background backfills do not overwrite or clobber real-time trigger writes.reputation.test.ts) that run against the Firestore emulator to verify decay math, fallback logic, and Firestore trigger correctness.eslint@10and migrated away from deprecated legacy configs toeslint.config.mjs.package-lock.jsonand file conflicts from the latest upstream bumps.Fixes # 334
Type of change
How Has This Been Tested?
The core reputation math and Firestore triggers were verified using the local Firebase Emulator Suite. I also ran local builds and linting to guarantee the ESLint v10 upgrade works smoothly.
npm run test): Verified that a 6-month old event correctly yields a 50% reputation reward, and a 12-month event yields 25%.onParticipatingCreatesecurely increments bucket totals via atomic operations.tsccompiles successfully acrosscloud-functions.Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation