feat: Auto-block users for ghosting events#645
Open
riddhima25bet10005-a11y wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a ChangesGhosting Detection and Shadow-Ban Enforcement
Sequence DiagramsequenceDiagram
rect rgba(135, 206, 235, 0.5)
Note over Scheduler,UsersCollection: Daily ghosting detection
participant Scheduler as Cloud Scheduler
participant detectGhostingUsers
participant EventsCollection as events
participant Participants as events/{id}/participants
participant UsersCollection as users
Scheduler->>detectGhostingUsers: onRun (every 24h UTC)
detectGhostingUsers->>EventsCollection: query endAt in [now-24h, now]
EventsCollection-->>detectGhostingUsers: recent event docs
detectGhostingUsers->>Participants: query status == 'rsvp' per event
Participants-->>detectGhostingUsers: ghosting participants
detectGhostingUsers->>UsersCollection: batch.update missedRsvpsCount / isShadowBanned
end
rect rgba(255, 165, 0, 0.5)
Note over App,UsersCollection: Enforcement at registration time
participant App
participant FirestoreRules as Firestore Rules
App->>FirestoreRules: create participants/{participantId}
FirestoreRules->>UsersCollection: get users/{userId}.isShadowBanned
UsersCollection-->>FirestoreRules: isShadowBanned: true
FirestoreRules-->>App: deny write
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
cloud-functions/src/events/attendance.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. cloud-functions/src/events/index.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
cloud-functions/lib/inactiveUsers.js-80-84 (1)
80-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap periodic batch commits in the same error-handling path.
The commit at Line 81 is outside the try/catch used for final commit, so commit failures there won’t include the standardized error log context.
Proposed fix
- for (const userDoc of usersSnapshot.docs) { + try { + for (const userDoc of usersSnapshot.docs) { const userData = userDoc.data(); @@ - if (operationCount === 500) { - await batch.commit(); - batch = db.batch(); - operationCount = 0; - } - } - try { + if (operationCount === 500) { + await batch.commit(); + batch = db.batch(); + operationCount = 0; + } + } if (operationCount > 0) { await batch.commit(); } console.log('Inactive users scan completed.'); }Also applies to: 86-95
🤖 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/inactiveUsers.js` around lines 80 - 84, The periodic batch commit at line 81 (when operationCount === 500) is not wrapped in the same try/catch error-handling as the final commit, so failures there will lack standardized error logging. Wrap the await batch.commit() call and batch reset (batch = db.batch()) in a try/catch block that uses the same error handling pattern used for the final commit operation. Apply this same fix to all periodic batch commit operations in the function, including the locations mentioned in the "Also applies to: 86-95" section, ensuring all batch.commit() calls use consistent error handling with proper logging context.cloud-functions/lib/attendanceStreak.js-84-87 (1)
84-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove or redact debug logging of user attendance timestamps.
Logging
lastAttendanceAtexposes user activity timing in function logs. Consider removing these debug statements or redacting sensitive fields before production deployment.🔒 Suggested fix
- console.log('previous currentStreak:', currentStreak); - console.log('lastAttendanceAt:', lastAttendanceAt); - console.log('newStreak:', newStreak); - console.log('newLongestStreak:', newLongestStreak); + console.log(`Streak updated for user: newStreak=${newStreak}, newLongest=${newLongestStreak}`);🤖 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/attendanceStreak.js` around lines 84 - 87, The console.log statements at lines 84-87 expose sensitive user activity information by logging lastAttendanceAt which contains user activity timing data that should not be exposed in production logs. Remove these debug console.log statements entirely, or if debug logging is needed for development, redact or exclude the lastAttendanceAt field from the logged output to prevent exposing user timing information in the function logs.cloud-functions/src/sendBulkEmails.ts-182-191 (1)
182-191:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAudit log status does not reflect actual outcome.
finalStatusis unconditionally set to'completed'even when all emails fail. This reduces the audit log's usefulness for monitoring and debugging.Proposed fix to derive status from outcome
- let finalStatus = 'completed'; + let finalStatus: string; + if (failureCount === 0) { + finalStatus = 'completed'; + } else if (successCount === 0) { + finalStatus = 'failed'; + } else { + finalStatus = 'partial_failure'; + } // Update audit log with terminal status🤖 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/sendBulkEmails.ts` around lines 182 - 191, The `finalStatus` variable is unconditionally set to 'completed' regardless of the actual outcome of the email sending operation. Instead of hardcoding the status, derive it from the actual results using the `successCount` and `failureCount` values that are already available. Update the logic before the audit log update to conditionally set `finalStatus` based on whether emails succeeded, failed, or had partial failures, so the audit log accurately reflects the outcome of the operation.cloud-functions/lib/backfillEventAnalytics.js-56-64 (1)
56-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty string
startAfterIdwill cause an error.If
data.startAfterIdis an empty string, it passes thetypeofcheck butdb.collection('events').doc('')throws because Firestore document IDs cannot be empty. Either trim and reject empty strings, or the error at line 60-61 will surface but with a less clear message.🛡️ Suggested fix
-const startAfterId = typeof data?.startAfterId === 'string' ? data.startAfterId : null; +const startAfterId = typeof data?.startAfterId === 'string' && data.startAfterId.trim() + ? data.startAfterId.trim() + : null;🤖 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/backfillEventAnalytics.js` around lines 56 - 64, The validation for startAfterId at line 56 checks that it is a string type, but does not validate that it is not an empty string. This allows empty strings to pass through to the db.collection('events').doc() call at line 59, which throws an error because Firestore does not allow empty document IDs. Update the condition that extracts startAfterId to also check that the string is not empty after trimming (or check its length), ensuring that only valid non-empty document IDs proceed to the startSnap retrieval logic.cloud-functions/lib/postEventFeedback.js-57-67 (1)
57-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNo timeout on external HTTP call - function could hang indefinitely.
The
fetchcall to EmailJS has no timeout configured. If the EmailJS API becomes unresponsive, this could cause the Cloud Function to hang until it times out (default 60s for scheduled functions), blocking processing of other events.Proposed fix using AbortController
async function sendEmail(name, email, eventTitle, eventId) { const payload = { /* ... */ }; try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); const res = await fetch('https://api.emailjs.com/api/v1.0/email/send', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(payload), + signal: controller.signal, }); + clearTimeout(timeoutId); return res.ok; }🤖 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/postEventFeedback.js` around lines 57 - 67, The fetch call to the EmailJS API endpoint in the try block has no timeout configured, which could cause the Cloud Function to hang indefinitely if the external service becomes unresponsive. Add an AbortController with a reasonable timeout value (such as 5-10 seconds) to the fetch request. Create the AbortController before the fetch call, set up a timeout that calls abort() on the controller if the request exceeds the timeout duration, and pass the AbortController signal to the fetch options. Make sure to clear the timeout in both success and error paths to prevent memory leaks.cloud-functions/lib/logger.js-60-87 (1)
60-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix inconsistent context field mapping in
logError.On Line 86,
context?.contextis used, whilelogEntrystores request payload fromcontext?.input(Line 55). This mismatch causes structured error context to be silently lost for callers that pass{ input: ... }.Suggested patch
- context: context?.context ?? null, + context: context?.input ?? null, + output: context?.output ?? null,🤖 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/logger.js` around lines 60 - 87, In the logError function, the context field being logged on line 86 is using context?.context, but this is inconsistent with the logEntry structure which stores the request payload from context?.input on line 55. Change the context field assignment in the error logging object to use context?.input instead of context?.context to maintain consistency and prevent structured error context from being lost for callers that pass input data.
🧹 Nitpick comments (6)
cloud-functions/lib/reputation.test.js (1)
2-17: ⚡ Quick winRemove arithmetic-only tests that don’t execute the function under test.
Lines 2–17 validate local math, not
calculatePoints, so they can pass even when the exported implementation regresses. Keep the function-based tests only.Suggested cleanup
'use strict'; -describe('calculateReputation logic', () => { - test('calculates reputation points correctly', () => { - const attendanceCount = 2; - const registrationCount = 3; - const remindersSet = 1; - const points = attendanceCount * 10 + registrationCount * 2 + remindersSet; - expect(points).toBe(27); - }); -}); -test('calculates zero points correctly', () => { - const attendanceCount = 0; - const registrationCount = 0; - const remindersSet = 0; - const points = attendanceCount * 10 + registrationCount * 2 + remindersSet; - expect(points).toBe(0); -}); const { calculatePoints } = require('./reputation'); describe('calculatePoints', () => {🤖 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/reputation.test.js` around lines 2 - 17, Remove the describe block and the standalone test that validate arithmetic calculations directly (the 'calculates reputation points correctly' test and the 'calculates zero points correctly' test). These tests perform inline math calculations instead of actually calling the calculateReputation function being tested, so they provide no validation that the exported implementation is correct. Keep only the tests that directly invoke the actual calculateReputation function.app/src/screens/ClubProfileScreen.js (1)
324-332: ⚡ Quick winAvoid redundant writes when the selected swatch is tapped again.
On Line 365, pressing the currently selected color still calls
setDoc. Add an early return inhandleColorChangewhen the selected value is unchanged to avoid unnecessary writes and listener churn.Proposed diff
const handleColorChange = async color => { if (!isOwner || !clubId) return; if (!clubThemeColors.includes(color)) return; + if (club?.themeColor === color) return; try { await setDoc(doc(db, 'publicUsers', clubId), { themeColor: color }, { merge: true }); } catch (e) { console.error('Failed to save color', e); } };Also applies to: 365-365
🤖 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/ClubProfileScreen.js` around lines 324 - 332, The handleColorChange function performs a Firestore write operation every time a color swatch is tapped, even when the selected color hasn't actually changed. Add an early return check in handleColorChange that compares the incoming color parameter with the current selected color value, and return early if they are the same. This prevents unnecessary writes to Firestore and avoids listener churn when users tap on the already-selected color swatch.cloud-functions/lib/computeShowUpRatios.js (1)
79-87: ⚡ Quick winInefficient re-scan of documents to count events per category.
The code re-iterates
pastEvents.docsfor each category (line 83-84) making this O(categories × events). Since you're already iterating over all docs in lines 58-71, count events per category there instead.♻️ Suggested optimization
const categoryBuckets = new Map(); +const categoryEventCounts = new Map(); let overallTotalRsvps = 0; let overallTotalAttendees = 0; for (const doc of pastEvents.docs) { const data = doc.data(); const rsvps = data.participantCount || 0; const checkedIn = data.stats?.totalCheckedIn || 0; const category = data.category || 'General'; if (rsvps === 0) continue; const bucket = categoryBuckets.get(category) || { totalRsvps: 0, totalAttendees: 0 }; bucket.totalRsvps += rsvps; bucket.totalAttendees += checkedIn; categoryBuckets.set(category, bucket); + categoryEventCounts.set(category, (categoryEventCounts.get(category) || 0) + 1); overallTotalRsvps += rsvps; overallTotalAttendees += checkedIn; } // ... for (const [category, counts] of categoryBuckets) { const ratio = counts.totalRsvps > 0 ? counts.totalAttendees / counts.totalRsvps : 0; categoryRatios[category] = { ratio: Math.round(ratio * 100) / 100, - eventCount: pastEvents.docs.filter(d => (d.data().category || 'General') === category) - .length, + eventCount: categoryEventCounts.get(category) || 0, totalRsvps: counts.totalRsvps, totalAttendees: counts.totalAttendees, }; }🤖 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/computeShowUpRatios.js` around lines 79 - 87, The categoryRatios computation re-iterates through all pastEvents.docs for each category when building the eventCount property, creating O(categories × events) complexity. Instead, while iterating through pastEvents.docs in the existing loop (around lines 58-71 where categoryBuckets is being populated), also track the event count per category in a parallel data structure (e.g., categoryEventCounts). Then in the categoryBuckets loop starting at line 79, replace the filter operation on pastEvents.docs with a direct lookup into the pre-computed categoryEventCounts map to retrieve the event count for each category.cloud-functions/lib/branchReport.js (1)
258-290: ⚖️ Poor tradeoffO(events) subcollection reads may be slow for large datasets.
The loop reads the
checkInssubcollection for every event (line 282), and potentiallyparticipantsas well (line 274). For a large event dataset (e.g., 1000+ events), this could result in thousands of Firestore reads and significant latency.Since this is an admin-only, infrequent report, it may be acceptable. If latency becomes problematic, consider:
- Adding pagination/date limits to the query
- Pre-aggregating attendance counts per branch on events (similar to
branchCounts)- Running as an async job with progress tracking
🤖 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/branchReport.js` around lines 258 - 290, The loop iterating through eventsSnap.docs performs potentially expensive subcollection reads for each event: reading 'participants' collection at the else branch condition and reading 'checkIns' collection in all iterations. For large event datasets, this results in O(events) subcollection reads causing performance issues. To fix this, implement one of the suggested approaches: add pagination or date range filters to the eventsSnap query at the beginning of the function to limit the number of events processed, pre-aggregate attendance counts per branch directly on the event documents (similar to the existing branchCounts pattern), or restructure the function to run as an async background job with progress tracking if this is an infrequent admin report. Reference the queries that fetch eventsSnap at the start of the function and the subcollection reads in the loop when implementing these optimizations.cloud-functions/lib/postEventFeedback.js (2)
54-54: ⚡ Quick winHardcoded production URL in email template.
The feedback link URL is hardcoded to
https://unievent-ez2w.onrender.com. This makes it difficult to test in staging/development environments and creates maintenance burden if the domain changes.Consider using an environment variable like
process.env.APP_BASE_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/lib/postEventFeedback.js` at line 54, The feedback_link property in the email template is using a hardcoded production URL that will not work for staging or development environments and creates maintenance issues if the domain changes. Replace the hardcoded URL string in the feedback_link property with an environment variable reference such as process.env.APP_BASE_URL, ensuring the environment variable is properly defined and accessible in the function context where this email template is constructed.
92-100: ⚖️ Poor tradeoffSequential email sending limits throughput for events with many participants.
Emails are sent one-by-one in a
for...ofloop. For events with hundreds of participants, this could take a very long time and risk Cloud Function timeout. Consider batching withPromise.all(with concurrency limits) or using a queue-based approach.🤖 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/postEventFeedback.js` around lines 92 - 100, The notifyParticipants function sends emails sequentially using a for...of loop with await, which causes poor throughput for events with many participants and risks Cloud Function timeouts. Replace the sequential loop with batched email sending using Promise.all with a concurrency limit, or implement a queue-based approach. Collect the sendEmail promises and resolve them in controlled batches (e.g., 5-10 concurrent emails at a time) rather than awaiting each one individually.
🤖 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 `@cloud-functions/lib/auditLog.js`:
- Around line 83-91: The logEntry object being constructed does not capture the
full document identity for subcollection operations. When auditLogSubcollection
fires on a nested path like events/{docId}/participants/{subdocId}, the current
logEntry only records the parent collectionId and the parent documentId
(context.params.docId), but omits the subcollectionId and subdocId from the
audit trail. Enhance the logEntry object to include additional fields that
capture the subcollectionId and the subdocument ID (subdocId) extracted from
context.params, so that the audit log can properly identify which subdocument
was modified instead of just the parent document.
- Around line 99-104: The Firestore trigger patterns in exports.auditLog and
exports.auditLogSubcollection use wildcards that match every document in
Firestore, causing unnecessary function invocations on all write operations.
Replace the broad wildcard patterns {collectionId}/{docId} and
{collectionId}/{docId}/{subcollectionId}/{subdocId} with specific collection
names that actually require auditing, such as users, projects, or other
business-critical collections. This way the auditLogHandler function will only
be triggered for writes to collections that genuinely need audit logging,
eliminating unnecessary invocations and reducing costs and latency.
In `@cloud-functions/lib/clubReputation.js`:
- Around line 168-190: The Promise.all construct in the snapshot.docs.map block
executes all document processing concurrently, causing multiple async callbacks
to simultaneously modify the shared batch object and opCount variable without
synchronization. Since Firestore WriteBatch is not thread-safe and opCount
increments are not atomic, this creates a race condition that can lose batch
operations or cause incorrect operation counts. Replace the concurrent
Promise.all approach with sequential processing: convert the array map and
Promise.all pattern to a sequential loop (such as a for...of loop) that
processes each userDoc one at a time, ensuring batch.update() calls and opCount
increments happen atomically and in order.
In `@cloud-functions/lib/computeShowUpRatios.js`:
- Around line 44-50: The query in computeShowUpRatios.js that filters the events
collection on multiple fields (endAt with two range conditions and
participantCount > 0) requires a composite index in Firestore. Add a composite
index to firestore.indexes.json that indexes the events collection on endAt in
ascending order and participantCount in ascending order, as these two fields are
being used together with inequality filters in the where clauses.
In `@cloud-functions/lib/dedicatedStudentCertificate.js`:
- Around line 151-165: The two separate userRef.update() calls that remove the
pending_delivery certificate and add the delivered certificate are not atomic,
creating a window where the certificate data could be lost if the function fails
between updates. Combine both operations into a single userRef.update() call by
passing an object that simultaneously removes the old certificate entry via
FieldValue.arrayRemove() and adds the new one via FieldValue.arrayUnion() in the
same update operation to ensure atomicity.
In `@cloud-functions/lib/feedbackSentimentAnalysis.js`:
- Around line 122-127: The query filtering for sentiment == null in the
collectionGroup query will not match feedback documents where the sentiment
field is absent because Firestore only matches null when the field is explicitly
set to that value. To fix this, either initialize the sentiment field to null
when creating feedback documents (in the feedback document creation logic that
currently omits this field), or modify the query to retrieve all documents with
a non-null feedback field and then filter at the application level to find those
with a missing or null sentiment field. The first approach is preferred as it
maintains consistency in the data model.
In `@cloud-functions/lib/inactiveUsers.js`:
- Around line 40-45: Add validation to the getLastActiveDate function to guard
against invalid date values. After creating the date object (either from
toDate() or new Date(lastActive)), check if the resulting date is valid using
isNaN(date.getTime()). If the date is invalid, return null or a sentinel value
and ensure the calling code at lines 62-63 skips processing malformed records
and logs a warning instead of treating invalid dates as active dates, which
would incorrectly flip isInactive and clear inactiveSince.
In `@cloud-functions/lib/onFeedbackSubmit.js`:
- Around line 146-151: The ownership verification in the feedbackRequests
document check is incomplete. When safeRequestId is provided, the code currently
only validates ownership if the document exists, but silently allows the request
to continue if the document doesn't exist. Modify the condition to ensure that
when safeRequestId is provided, the document must exist in the database and the
userId must match context.params.userId. You should throw a permission-denied
HttpsError if either the document doesn't exist or the userId doesn't match,
rather than only throwing when the document exists but has a mismatched owner.
- Around line 131-132: The eventId and userId variables are being URL-encoded at
their assignment (lines 131-132) but the documents stored in Firestore (in
checkInService.js and events/index.js) use raw, unencoded IDs. When these
encoded IDs are passed to verifyAttendedUser and verifyAbsentUser functions for
Firestore lookups (lines 153, 156), they fail to match the stored documents if
the IDs contain special characters like +, #, or /. Remove the
encodeURIComponent calls from the eventId and userId assignments and use the raw
context.params values directly instead, ensuring the query keys match the actual
document IDs stored in Firestore.
In `@cloud-functions/lib/permanentCleanup.js`:
- Around line 61-77: The batch operations can exceed Firestore's 500-write limit
because each event in the loop can generate up to 2 operations (owner eventCount
decrement via batch.set and event deletion via batch.delete), potentially
creating 1000 operations from a 500-document query. Modify the loop to check if
operationCount would exceed the 500 limit before adding new operations, and
commit the batch preemptively when approaching the threshold. After each
batch.commit() call, reinitialize batch and operationCount for the next batch.
Additionally, change totalDeleted tracking to increment by the number of events
deleted (eventsSnapshot.docs.length or count actual deletions) rather than
operationCount, since operationCount represents batch operations not actual
event deletions.
In `@cloud-functions/lib/postEventFeedback.js`:
- Around line 107-110: New events created via CreateEvent.js are missing the
feedbackRequestSent field, which prevents the Firestore query in
postEventFeedback.js (lines 107-110) from matching them since the in operator
doesn't match missing fields. In CreateEvent.js around line 351, add
feedbackRequestSent: false to the event document object in the transaction.set()
call, placing it alongside other initialization fields like participantCount,
branchCounts, and yearCounts so that newly created events will be included in
the feedback scheduling query.
In `@cloud-functions/lib/utils/emailSender.js`:
- Around line 25-30: The catch blocks in the emailSender.js file are returning a
resolved object with success: false instead of throwing the error, which breaks
promise-based error handling in calling code like rateLimiter.js that expects
.catch() handlers to catch failures. Replace the return statements in the catch
blocks (at lines 25-30, 54-56, and 60-63) with throw statements to properly
reject the promise. For each catch block, throw the error directly (or wrapped
if needed) instead of returning { success: false, error: ... } so that callers
using .catch() can properly handle failures.
In `@cloud-functions/lib/utils/emailTemplateRenderer.js`:
- Around line 126-133: The escapeHtml function at lines 126-133 assumes the
input parameter unsafe is always a string, but template placeholder values can
be numeric or boolean types, causing replaceAll to fail with "replaceAll is not
a function". Convert the unsafe parameter to a string at the start of the
escapeHtml function before calling any replaceAll methods on it. Use a type
conversion method like String() or toString() to ensure the parameter is always
treated as a string regardless of its input type.
In `@cloud-functions/lib/utils/push.js`:
- Around line 21-29: The error handling in the chunk sending loop silently
catches and logs exceptions without propagating failure information. When a
chunk fails to send in the expo.sendPushNotificationsAsync call, the error is
logged but the function continues processing as if successful, meaning lines
31-35 may return a success response even though some or all chunks failed to
send. Implement proper error tracking by either collecting failed chunks
separately, re-throwing critical errors, or returning error state information to
the caller so that partial or complete failures are not reported as success.
Ensure that the calling code at lines 31-35 can distinguish between successful
and failed operations based on the return value or thrown errors.
In `@cloud-functions/lib/utils/rateLimiter.js`:
- Around line 154-177: The alert notification block (lines 154-177) sends an
email on every rate-limit violation without deduplication, risking unbounded
outbound traffic, and does not await the sendEmail call, making delivery
non-deterministic in serverless environments. Add a deduplication check using a
persisted lastDailyLimitAlertDay marker in the transaction data to ensure alerts
are sent at most once per user per day, and either await the sendEmail promise
or enqueue the email to a durable job queue instead of fire-and-forget to
guarantee delivery completes before the function terminates.
In `@cloud-functions/src/events/attendance.ts`:
- Around line 51-84: The batch variable is declared as const and is not reset
after being committed at 500 operations, causing subsequent batch.update() calls
to operate on a stale batch. Change the const declaration to let on the initial
batch assignment so the variable can be reassigned, and after the await
batch.commit() call within the operationCount check, reinitialize batch to a
fresh db.batch() so the next iteration has a new, usable batch to operate on.
- Around line 19-23: The query in attendance.ts comparing the endAt field
against ISO strings will only match string-stored values and silently miss
events stored as Timestamps. The evidence from postEventFeedback.ts (line 38)
shows defensive handling of both Timestamp and string types, indicating
inconsistent storage patterns. Either normalize endAt storage to a single
consistent type across all event creation and storage, or update the comparison
logic in the query at lines 19-23 to handle both Timestamp and string types
using the same conversion pattern seen in postEventFeedback.ts (checking if
endAt has a toDate method and converting accordingly).
In `@firestore.rules`:
- Around line 301-304: The shadow-ban enforcement is bypassable because users
can modify their own isShadowBanned field in the /users/{uid} document. In the
user update rules (around lines 191-194), add a validation check to prevent
users from writing to the isShadowBanned field on their own user documents. This
ensures that only administrators or privileged operations can modify the
isShadowBanned flag, preventing shadow-banned users from unblocking themselves
and bypassing the check at line 303.
---
Minor comments:
In `@cloud-functions/lib/attendanceStreak.js`:
- Around line 84-87: The console.log statements at lines 84-87 expose sensitive
user activity information by logging lastAttendanceAt which contains user
activity timing data that should not be exposed in production logs. Remove these
debug console.log statements entirely, or if debug logging is needed for
development, redact or exclude the lastAttendanceAt field from the logged output
to prevent exposing user timing information in the function logs.
In `@cloud-functions/lib/backfillEventAnalytics.js`:
- Around line 56-64: The validation for startAfterId at line 56 checks that it
is a string type, but does not validate that it is not an empty string. This
allows empty strings to pass through to the db.collection('events').doc() call
at line 59, which throws an error because Firestore does not allow empty
document IDs. Update the condition that extracts startAfterId to also check that
the string is not empty after trimming (or check its length), ensuring that only
valid non-empty document IDs proceed to the startSnap retrieval logic.
In `@cloud-functions/lib/inactiveUsers.js`:
- Around line 80-84: The periodic batch commit at line 81 (when operationCount
=== 500) is not wrapped in the same try/catch error-handling as the final
commit, so failures there will lack standardized error logging. Wrap the await
batch.commit() call and batch reset (batch = db.batch()) in a try/catch block
that uses the same error handling pattern used for the final commit operation.
Apply this same fix to all periodic batch commit operations in the function,
including the locations mentioned in the "Also applies to: 86-95" section,
ensuring all batch.commit() calls use consistent error handling with proper
logging context.
In `@cloud-functions/lib/logger.js`:
- Around line 60-87: In the logError function, the context field being logged on
line 86 is using context?.context, but this is inconsistent with the logEntry
structure which stores the request payload from context?.input on line 55.
Change the context field assignment in the error logging object to use
context?.input instead of context?.context to maintain consistency and prevent
structured error context from being lost for callers that pass input data.
In `@cloud-functions/lib/postEventFeedback.js`:
- Around line 57-67: The fetch call to the EmailJS API endpoint in the try block
has no timeout configured, which could cause the Cloud Function to hang
indefinitely if the external service becomes unresponsive. Add an
AbortController with a reasonable timeout value (such as 5-10 seconds) to the
fetch request. Create the AbortController before the fetch call, set up a
timeout that calls abort() on the controller if the request exceeds the timeout
duration, and pass the AbortController signal to the fetch options. Make sure to
clear the timeout in both success and error paths to prevent memory leaks.
In `@cloud-functions/src/sendBulkEmails.ts`:
- Around line 182-191: The `finalStatus` variable is unconditionally set to
'completed' regardless of the actual outcome of the email sending operation.
Instead of hardcoding the status, derive it from the actual results using the
`successCount` and `failureCount` values that are already available. Update the
logic before the audit log update to conditionally set `finalStatus` based on
whether emails succeeded, failed, or had partial failures, so the audit log
accurately reflects the outcome of the operation.
---
Nitpick comments:
In `@app/src/screens/ClubProfileScreen.js`:
- Around line 324-332: The handleColorChange function performs a Firestore write
operation every time a color swatch is tapped, even when the selected color
hasn't actually changed. Add an early return check in handleColorChange that
compares the incoming color parameter with the current selected color value, and
return early if they are the same. This prevents unnecessary writes to Firestore
and avoids listener churn when users tap on the already-selected color swatch.
In `@cloud-functions/lib/branchReport.js`:
- Around line 258-290: The loop iterating through eventsSnap.docs performs
potentially expensive subcollection reads for each event: reading 'participants'
collection at the else branch condition and reading 'checkIns' collection in all
iterations. For large event datasets, this results in O(events) subcollection
reads causing performance issues. To fix this, implement one of the suggested
approaches: add pagination or date range filters to the eventsSnap query at the
beginning of the function to limit the number of events processed, pre-aggregate
attendance counts per branch directly on the event documents (similar to the
existing branchCounts pattern), or restructure the function to run as an async
background job with progress tracking if this is an infrequent admin report.
Reference the queries that fetch eventsSnap at the start of the function and the
subcollection reads in the loop when implementing these optimizations.
In `@cloud-functions/lib/computeShowUpRatios.js`:
- Around line 79-87: The categoryRatios computation re-iterates through all
pastEvents.docs for each category when building the eventCount property,
creating O(categories × events) complexity. Instead, while iterating through
pastEvents.docs in the existing loop (around lines 58-71 where categoryBuckets
is being populated), also track the event count per category in a parallel data
structure (e.g., categoryEventCounts). Then in the categoryBuckets loop starting
at line 79, replace the filter operation on pastEvents.docs with a direct lookup
into the pre-computed categoryEventCounts map to retrieve the event count for
each category.
In `@cloud-functions/lib/postEventFeedback.js`:
- Line 54: The feedback_link property in the email template is using a hardcoded
production URL that will not work for staging or development environments and
creates maintenance issues if the domain changes. Replace the hardcoded URL
string in the feedback_link property with an environment variable reference such
as process.env.APP_BASE_URL, ensuring the environment variable is properly
defined and accessible in the function context where this email template is
constructed.
- Around line 92-100: The notifyParticipants function sends emails sequentially
using a for...of loop with await, which causes poor throughput for events with
many participants and risks Cloud Function timeouts. Replace the sequential loop
with batched email sending using Promise.all with a concurrency limit, or
implement a queue-based approach. Collect the sendEmail promises and resolve
them in controlled batches (e.g., 5-10 concurrent emails at a time) rather than
awaiting each one individually.
In `@cloud-functions/lib/reputation.test.js`:
- Around line 2-17: Remove the describe block and the standalone test that
validate arithmetic calculations directly (the 'calculates reputation points
correctly' test and the 'calculates zero points correctly' test). These tests
perform inline math calculations instead of actually calling the
calculateReputation function being tested, so they provide no validation that
the exported implementation is correct. Keep only the tests that directly invoke
the actual calculateReputation function.
🪄 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: 8a23f4ba-aa4b-478a-86f4-c219d7dfd816
📒 Files selected for processing (41)
app/src/lib/theme.jsapp/src/screens/ClubProfileScreen.jsapp/src/screens/UserFeed.jsapp/src/screens/__tests__/EventRegistrationFormScreen.test.jscloud-functions/lib/attendanceStreak.jscloud-functions/lib/auditLog.jscloud-functions/lib/backfillEventAnalytics.jscloud-functions/lib/branchReport.jscloud-functions/lib/clubReputation.jscloud-functions/lib/computeShowUpRatios.jscloud-functions/lib/dedicatedStudentCertificate.jscloud-functions/lib/events/index.jscloud-functions/lib/feedbackSentimentAnalysis.jscloud-functions/lib/inactiveUsers.jscloud-functions/lib/logger.jscloud-functions/lib/middleware/appCheck.jscloud-functions/lib/middleware/ipWhitelist.jscloud-functions/lib/middleware/rateLimiter.jscloud-functions/lib/onEventDelete.jscloud-functions/lib/onFeedbackSubmit.jscloud-functions/lib/permanentCleanup.jscloud-functions/lib/postEventFeedback.jscloud-functions/lib/reputation.jscloud-functions/lib/reputation.test.jscloud-functions/lib/sendBulkEmails.jscloud-functions/lib/server.jscloud-functions/lib/utils/emailSender.jscloud-functions/lib/utils/emailTemplateRenderer.jscloud-functions/lib/utils/push.jscloud-functions/lib/utils/rateLimiter.jscloud-functions/lib/validation/schemas.jscloud-functions/lib/validation/validate.jscloud-functions/src/events/attendance.tscloud-functions/src/events/index.tscloud-functions/src/reputation.tscloud-functions/src/sendBulkEmails.tscloud-functions/src/utils/push.tscomments.txtfirestore.indexes.jsonfirestore.rulespr-comments-json.txt
Comment on lines
+83
to
+91
| const logEntry = { | ||
| timestamp: firestore_1.FieldValue.serverTimestamp(), | ||
| userId, | ||
| action, | ||
| collection, | ||
| documentId: context.params.docId, | ||
| before: beforeData || null, | ||
| after: afterData || null, | ||
| }; |
Contributor
There was a problem hiding this comment.
Subcollection trigger loses document identity.
When auditLogSubcollection fires on a path like events/{docId}/participants/{subdocId}, the handler only logs collectionId ('events') and docId, but the actual modified document is identified by subcollectionId and subdocId. These are not recorded, making it impossible to identify which subdocument was changed.
Proposed fix to capture full path
const logEntry = {
timestamp: firestore_1.FieldValue.serverTimestamp(),
userId,
action,
collection,
documentId: context.params.docId,
+ subcollection: context.params.subcollectionId || null,
+ subdocumentId: context.params.subdocId || null,
before: beforeData || null,
after: afterData || null,
};🤖 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/auditLog.js` around lines 83 - 91, The logEntry object
being constructed does not capture the full document identity for subcollection
operations. When auditLogSubcollection fires on a nested path like
events/{docId}/participants/{subdocId}, the current logEntry only records the
parent collectionId and the parent documentId (context.params.docId), but omits
the subcollectionId and subdocId from the audit trail. Enhance the logEntry
object to include additional fields that capture the subcollectionId and the
subdocument ID (subdocId) extracted from context.params, so that the audit log
can properly identify which subdocument was modified instead of just the parent
document.
Comment on lines
+99
to
+104
| exports.auditLog = functions.firestore | ||
| .document('{collectionId}/{docId}') | ||
| .onWrite(auditLogHandler); | ||
| exports.auditLogSubcollection = functions.firestore | ||
| .document('{collectionId}/{docId}/{subcollectionId}/{subdocId}') | ||
| .onWrite(auditLogHandler); |
Contributor
There was a problem hiding this comment.
Wildcard triggers fire on every Firestore write, causing significant cost and performance impact.
The patterns {collectionId}/{docId} and {collectionId}/{docId}/{subcollectionId}/{subdocId} match all documents in Firestore. Every single write operation (including rate_limits, email_audit_logs, user activity, etc.) will invoke these functions, resulting in:
- Massive function invocation costs - proportional to total write volume
- Added latency on every operation
- Audit log storage explosion - one audit record per write
Consider scoping to specific collections that require auditing:
Proposed fix: scope to specific collections
-exports.auditLog = functions.firestore
- .document('{collectionId}/{docId}')
- .onWrite(auditLogHandler);
-exports.auditLogSubcollection = functions.firestore
- .document('{collectionId}/{docId}/{subcollectionId}/{subdocId}')
- .onWrite(auditLogHandler);
+// Define separate triggers for collections that require auditing
+const AUDITED_COLLECTIONS = ['users', 'events', 'clubs'];
+
+exports.auditLogUsers = functions.firestore
+ .document('users/{docId}')
+ .onWrite(auditLogHandler);
+exports.auditLogEvents = functions.firestore
+ .document('events/{docId}')
+ .onWrite(auditLogHandler);
+exports.auditLogClubs = functions.firestore
+ .document('clubs/{docId}')
+ .onWrite(auditLogHandler);🤖 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/auditLog.js` around lines 99 - 104, The Firestore trigger
patterns in exports.auditLog and exports.auditLogSubcollection use wildcards
that match every document in Firestore, causing unnecessary function invocations
on all write operations. Replace the broad wildcard patterns
{collectionId}/{docId} and {collectionId}/{docId}/{subcollectionId}/{subdocId}
with specific collection names that actually require auditing, such as users,
projects, or other business-critical collections. This way the auditLogHandler
function will only be triggered for writes to collections that genuinely need
audit logging, eliminating unnecessary invocations and reducing costs and
latency.
Comment on lines
+44
to
+50
| const pastEvents = await db | ||
| .collection('events') | ||
| .where('endAt', '<', new Date().toISOString()) | ||
| .where('endAt', '>', twelveMonthsAgo.toISOString()) | ||
| .where('participantCount', '>', 0) | ||
| .select('category', 'participantCount', 'stats.totalCheckedIn') | ||
| .get(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if firestore.indexes.json includes the required composite index for computeShowUpRatios
fd -t f 'firestore.indexes.json' --exec cat {} \; 2>/dev/null | jq '.indexes[] | select(.fields[].fieldPath == "endAt" and .fields[].fieldPath == "participantCount")' 2>/dev/null || echo "Index file not found or no matching index"Repository: roshankumar0036singh/Uni-Event
Length of output: 56
🏁 Script executed:
# First, find if firestore.indexes.json exists anywhere in the repo
fd -t f 'firestore.indexes.json'Repository: roshankumar0036singh/Uni-Event
Length of output: 97
🌐 Web query:
Firestore composite index requirements multiple fields inequality filters
💡 Result:
When using inequality or range filters on multiple fields in Cloud Firestore, you must define a composite index to support the query [1][2]. Unlike simple equality filters, Firestore does not automatically create composite indexes for these combinations, so you must create them manually [3]. Key requirements and considerations include: 1. Mandatory Composite Index: Any query involving range or inequality operators (e.g., <, <=, >, >=) on multiple fields requires a corresponding composite index [1][3]. 2. Index Ordering and Selectivity: The order of fields in your query's orderBy clause significantly impacts performance [1][4]. Firestore uses the leftmost fields of the index to satisfy equality constraints and the range/inequality constraint on the first field of the orderBy clause [1][2]. To optimize performance and cost, specify your orderBy clause such that the fields with the highest selectivity (those that match the smallest number of documents) appear first [1][5]. 3. Constraints and Limits: - Firestore limits the number of range or inequality fields in a single query to 10 to prevent excessively expensive operations [1][6]. - Queries that apply range or inequality filters on document fields while only using equality constraints on the document ID (name) are not supported [1][2]. 4. Query Execution: Firestore uses the leftmost fields of the composite index to scan for a subset of documents. Remaining fields are then used to filter the results; while these remaining fields do not reduce the number of index entries scanned, they remove unmatched documents from the final set returned to the client [1][2]. For optimal performance, always analyze the selectivity of your filters and configure your composite index and orderBy clauses accordingly [4][5].
Citations:
- 1: https://cloud.google.com/firestore/docs/query-data/multiple-range-fields
- 2: https://firebase.google.com/docs/firestore/query-data/multiple-range-fields
- 3: https://cloud.google.com/firestore/docs/concepts/index-overview
- 4: https://firebase.google.com/docs/firestore/query-data/multiple-range-optimize-indexes
- 5: https://docs.cloud.google.com/firestore/native/docs/query-data/multiple-range-optimize-indexes
- 6: https://cloud.google.com/firestore/native/docs/query-data/multiple-range-fields
🏁 Script executed:
# Check the actual query in computeShowUpRatios.js to confirm the fields
cat -n cloud-functions/lib/computeShowUpRatios.js | head -60Repository: roshankumar0036singh/Uni-Event
Length of output: 3028
🏁 Script executed:
# Read the actual contents of firestore.indexes.json
cat firestore.indexes.jsonRepository: roshankumar0036singh/Uni-Event
Length of output: 2948
Composite index required for this query.
The query combines range filters on endAt (two conditions) with a filter on participantCount > 0. Firestore requires a composite index when filtering on multiple fields with at least one inequality. Without this index, the query will fail at runtime.
Add the following composite index to firestore.indexes.json:
Required composite index
{
"collectionGroup": "events",
"queryScope": "COLLECTION",
"fields": [
{ "fieldPath": "endAt", "order": "ASCENDING" },
{ "fieldPath": "participantCount", "order": "ASCENDING" }
]
}🤖 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/computeShowUpRatios.js` around lines 44 - 50, The query
in computeShowUpRatios.js that filters the events collection on multiple fields
(endAt with two range conditions and participantCount > 0) requires a composite
index in Firestore. Add a composite index to firestore.indexes.json that indexes
the events collection on endAt in ascending order and participantCount in
ascending order, as these two fields are being used together with inequality
filters in the where clauses.
Comment on lines
+151
to
+165
| await userRef.update({ | ||
| certificates: firestore_1.FieldValue.arrayRemove({ | ||
| type: 'dedicated_student', | ||
| status: 'pending_delivery', | ||
| claimedAt: now.toDate().toISOString(), | ||
| }), | ||
| }); | ||
| await userRef.update({ | ||
| certificates: firestore_1.FieldValue.arrayUnion({ | ||
| type: 'dedicated_student', | ||
| status: 'delivered', | ||
| awardedAt: now.toDate().toISOString(), | ||
| certificateUrl: signedUrl, | ||
| }), | ||
| }); |
Contributor
There was a problem hiding this comment.
Non-atomic certificate status update risks inconsistent state.
The two sequential userRef.update() calls (lines 151-157 and 158-165) are not atomic. If the function crashes or times out between them, the pending_delivery entry is removed but delivered is never added, leaving the user's certificate in limbo with no record.
Combine into a single update or use a transaction.
🔧 Proposed fix using a single update
- //add certificate on user doc
- await userRef.update({
- certificates: firestore_1.FieldValue.arrayRemove({
- type: 'dedicated_student',
- status: 'pending_delivery',
- claimedAt: now.toDate().toISOString(),
- }),
- });
- await userRef.update({
- certificates: firestore_1.FieldValue.arrayUnion({
- type: 'dedicated_student',
- status: 'delivered',
- awardedAt: now.toDate().toISOString(),
- certificateUrl: signedUrl,
- }),
- });
+ //add certificate on user doc - single atomic update
+ await db.runTransaction(async (tx) => {
+ const snap = await tx.get(userRef);
+ const data = snap.data() || {};
+ const certs = (data.certificates || []).filter(
+ (c) => !(c.type === 'dedicated_student' && c.status === 'pending_delivery')
+ );
+ certs.push({
+ type: 'dedicated_student',
+ status: 'delivered',
+ awardedAt: now.toDate().toISOString(),
+ certificateUrl: signedUrl,
+ });
+ tx.update(userRef, { certificates: certs });
+ });🤖 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/dedicatedStudentCertificate.js` around lines 151 - 165,
The two separate userRef.update() calls that remove the pending_delivery
certificate and add the delivered certificate are not atomic, creating a window
where the certificate data could be lost if the function fails between updates.
Combine both operations into a single userRef.update() call by passing an object
that simultaneously removes the old certificate entry via
FieldValue.arrayRemove() and adds the new one via FieldValue.arrayUnion() in the
same update operation to ensure atomicity.
Comment on lines
+21
to
+29
| for (const chunk of chunks) { | ||
| try { | ||
| const ticketChunk = await expo.sendPushNotificationsAsync(chunk); | ||
| tickets.push(...ticketChunk); | ||
| } | ||
| catch (error) { | ||
| console.error('Error sending push notification chunk:', error); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Chunk transport failures can be silently reported as success.
At Line 26, chunk send exceptions are only logged. If one/all chunks fail before tickets exist, Line 31 sees no ticket errors and the function can return as if successful.
Suggested fix
const chunks = expo.chunkPushNotifications(messages);
const tickets = [];
+ const chunkErrors = [];
@@
catch (error) {
console.error('Error sending push notification chunk:', error);
+ chunkErrors.push(error);
}
}
+ if (chunkErrors.length > 0) {
+ throw new Error(`Failed to send ${chunkErrors.length} push notification chunk(s)`);
+ }Also applies to: 31-35
🤖 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/utils/push.js` around lines 21 - 29, The error handling
in the chunk sending loop silently catches and logs exceptions without
propagating failure information. When a chunk fails to send in the
expo.sendPushNotificationsAsync call, the error is logged but the function
continues processing as if successful, meaning lines 31-35 may return a success
response even though some or all chunks failed to send. Implement proper error
tracking by either collecting failed chunks separately, re-throwing critical
errors, or returning error state information to the caller so that partial or
complete failures are not reported as success. Ensure that the calling code at
lines 31-35 can distinguish between successful and failed operations based on
the return value or thrown errors.
Comment on lines
+154
to
+177
| if (txResult.shouldAlert) { | ||
| const { logEntry } = require('../logger'); | ||
| logEntry('rate-limiter', 'Suspicious activity: Daily event creation limit exceeded', { | ||
| userId: txResult.alertData.userId, | ||
| input: { eventCountDay: txResult.alertData.eventCountDay }, | ||
| }); | ||
| const { sendEmail } = require('./emailSender'); | ||
| sendEmail({ | ||
| to: process.env.ADMIN_EMAIL || 'admin@uni-event.com', | ||
| subject: 'Alert: Event Creation Rate Limit Triggered', | ||
| templateName: 'universal_email_template', | ||
| templateData: { | ||
| subject: 'Alert: Event Creation Rate Limit Triggered', | ||
| to_name: 'Admin', | ||
| message: `User ${txResult.alertData.userId} has triggered the daily event creation rate limit (Max 5 per day). Please review this account for suspicious activity.`, | ||
| cert_display: 'none', | ||
| event_title: 'Abuse Detection', | ||
| date: new Date().toLocaleDateString(), | ||
| download_btn_display: 'none', | ||
| browse_btn_display: 'none', | ||
| event_link: '#', | ||
| }, | ||
| }).catch((err) => console.error('Failed to send admin alert email:', err)); | ||
| } |
Contributor
There was a problem hiding this comment.
Dedupe/throttle admin alerts and make alert delivery deterministic.
On Lines 154-177, every over-limit attempt can send a fresh admin email, so a single abusive client can generate unbounded outbound traffic/cost. Also, sendEmail(...) is not awaited, so alert delivery is best-effort and can be dropped before completion in serverless lifecycles.
Please gate alerts (e.g., once per user/day via a persisted lastDailyLimitAlertDay marker in the same transaction path) and await the send (or enqueue a durable job) for deterministic behavior.
🤖 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/utils/rateLimiter.js` around lines 154 - 177, The alert
notification block (lines 154-177) sends an email on every rate-limit violation
without deduplication, risking unbounded outbound traffic, and does not await
the sendEmail call, making delivery non-deterministic in serverless
environments. Add a deduplication check using a persisted lastDailyLimitAlertDay
marker in the transaction data to ensure alerts are sent at most once per user
per day, and either await the sendEmail promise or enqueue the email to a
durable job queue instead of fire-and-forget to guarantee delivery completes
before the function terminates.
Comment on lines
+19
to
+23
| const recentEventsSnapshot = await db | ||
| .collection('events') | ||
| .where('endAt', '<', now.toISOString()) | ||
| .where('endAt', '>=', yesterday.toISOString()) | ||
| .get(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how endAt is written in event creation code
rg -n --type=ts --type=js -C3 'endAt' | head -80Repository: roshankumar0036singh/Uni-Event
Length of output: 5473
🏁 Script executed:
fd -t f '\.ts$|\.js$' cloud-functions | head -20Repository: roshankumar0036singh/Uni-Event
Length of output: 913
🏁 Script executed:
rg -n 'collection.*events' cloud-functions --type=ts --type=js -A5 | head -100Repository: roshankumar0036singh/Uni-Event
Length of output: 7223
🏁 Script executed:
rg -n 'endAt' cloud-functions --type=ts --type=js -B3 -A3 | head -150Repository: roshankumar0036singh/Uni-Event
Length of output: 2506
🏁 Script executed:
rg -n 'endAt' cloud-functions/src/events/index.ts -B5 -A5Repository: roshankumar0036singh/Uni-Event
Length of output: 56
🏁 Script executed:
cat -n cloud-functions/src/events/index.ts | head -100Repository: roshankumar0036singh/Uni-Event
Length of output: 4393
🏁 Script executed:
rg -n 'collection.*events.*set\|doc.*set\|\.add\|\.update' cloud-functions/src/events/ --type=ts -B3 -A3Repository: roshankumar0036singh/Uni-Event
Length of output: 56
🏁 Script executed:
rg -n 'admin.firestore\|\.doc\(' cloud-functions/src --type=ts | grep -i 'event' | head -30Repository: roshankumar0036singh/Uni-Event
Length of output: 56
🏁 Script executed:
rg -n 'endAt.*=' cloud-functions/src --type=ts -B2 -A2 | head -100Repository: roshankumar0036singh/Uni-Event
Length of output: 460
🏁 Script executed:
find . -name "*.ts" -path "*/api/*" -o -name "*.ts" -path "*/routes/*" | head -20Repository: roshankumar0036singh/Uni-Event
Length of output: 56
🏁 Script executed:
rg -n 'admin\.firestore\(\)\.collection.*events' cloud-functions --type=ts -B2 -A10 | head -150Repository: roshankumar0036singh/Uni-Event
Length of output: 1624
🏁 Script executed:
rg -n 'endAt.*:' app/src --type=ts --type=js -B2 -A2 | head -100Repository: roshankumar0036singh/Uni-Event
Length of output: 1330
🏁 Script executed:
rg -n 'events.*POST\|POST.*events' app --type=ts --type=js -B3 -A10 | head -80Repository: roshankumar0036singh/Uni-Event
Length of output: 56
🏁 Script executed:
find app -name "*api*" -o -name "*service*" -o -name "*request*" | grep -i event | head -10Repository: roshankumar0036singh/Uni-Event
Length of output: 56
🏁 Script executed:
rg -n 'migration\|migrate' cloud-functions --type=ts -B2 -A5 | head -60Repository: roshankumar0036singh/Uni-Event
Length of output: 56
🏁 Script executed:
cat -n cloud-functions/migrations/*.tsRepository: roshankumar0036singh/Uni-Event
Length of output: 6840
🏁 Script executed:
rg -n 'toDate\|Timestamp' cloud-functions/src --type=ts | grep -i 'end' | head -30Repository: roshankumar0036singh/Uni-Event
Length of output: 56
🏁 Script executed:
rg -n 'functions\.(https|pubsub|firestore|database)' cloud-functions/src/events/attendance.ts -B5 -A10Repository: roshankumar0036singh/Uni-Event
Length of output: 643
🏁 Script executed:
cat -n cloud-functions/src/events/attendance.tsRepository: roshankumar0036singh/Uni-Event
Length of output: 3566
🏁 Script executed:
rg -n 'endAt.*toISOString\|toISOString.*endAt' app --type=ts --type=js -B3 -A3Repository: roshankumar0036singh/Uni-Event
Length of output: 56
Confirm data type consistency for endAt field comparisons.
The query compares endAt against ISO strings (now.toISOString(), yesterday.toISOString()), but the codebase shows mixed storage patterns. Evidence from postEventFeedback.ts (line 38) includes defensive code that handles both Timestamp and string types (event.endAt?.toDate ? event.endAt.toDate() : new Date(event.endAt)), suggesting the field is stored inconsistently. ISO string comparison will only match string-stored values and will silently miss Timestamp-stored events. Either normalize endAt storage to a single type across all events, or use consistent comparison logic that handles both Timestamp and string types.
🤖 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/events/attendance.ts` around lines 19 - 23, The query in
attendance.ts comparing the endAt field against ISO strings will only match
string-stored values and silently miss events stored as Timestamps. The evidence
from postEventFeedback.ts (line 38) shows defensive handling of both Timestamp
and string types, indicating inconsistent storage patterns. Either normalize
endAt storage to a single consistent type across all event creation and storage,
or update the comparison logic in the query at lines 19-23 to handle both
Timestamp and string types using the same conversion pattern seen in
postEventFeedback.ts (checking if endAt has a toDate method and converting
accordingly).
Comment on lines
+51
to
+84
| const batch = db.batch(); | ||
| let operationCount = 0; | ||
|
|
||
| // Fetch users and increment their global missed RSVPs count | ||
| for (const userId of Object.keys(ghostCounts)) { | ||
| const userRef = db.collection('users').doc(userId); | ||
| const userSnap = await userRef.get(); | ||
|
|
||
| if (!userSnap.exists) continue; | ||
|
|
||
| const userData = userSnap.data() || {}; | ||
|
|
||
| // Skip users who are already shadowbanned | ||
| if (userData.isShadowBanned) continue; | ||
|
|
||
| const missedRsvpsCount = (userData.missedRsvpsCount || 0) + ghostCounts[userId]; | ||
| const isShadowBanned = missedRsvpsCount >= GHOST_THRESHOLD; | ||
|
|
||
| batch.update(userRef, { | ||
| missedRsvpsCount, | ||
| ...(isShadowBanned && { isShadowBanned: true }), | ||
| }); | ||
|
|
||
| operationCount++; | ||
|
|
||
| if (operationCount === 500) { | ||
| await batch.commit(); | ||
| operationCount = 0; | ||
| } | ||
| } | ||
|
|
||
| if (operationCount > 0) { | ||
| await batch.commit(); | ||
| } |
Contributor
There was a problem hiding this comment.
Batch not reset after commit—operations beyond 500 will fail.
After committing at 500 operations (line 77), operationCount is reset but batch still references the already-committed batch. Any subsequent batch.update() calls will operate on a stale batch, causing failures or silent data loss.
🐛 Proposed fix
const batch = db.batch();
+ let batch = db.batch();
let operationCount = 0;
// Fetch users and increment their global missed RSVPs count
for (const userId of Object.keys(ghostCounts)) {
const userRef = db.collection('users').doc(userId);
const userSnap = await userRef.get();
if (!userSnap.exists) continue;
const userData = userSnap.data() || {};
// Skip users who are already shadowbanned
if (userData.isShadowBanned) continue;
const missedRsvpsCount = (userData.missedRsvpsCount || 0) + ghostCounts[userId];
const isShadowBanned = missedRsvpsCount >= GHOST_THRESHOLD;
batch.update(userRef, {
missedRsvpsCount,
...(isShadowBanned && { isShadowBanned: true }),
});
operationCount++;
if (operationCount === 500) {
await batch.commit();
+ batch = db.batch();
operationCount = 0;
}
}Also change const batch to let batch on line 51.
📝 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
| const batch = db.batch(); | |
| let operationCount = 0; | |
| // Fetch users and increment their global missed RSVPs count | |
| for (const userId of Object.keys(ghostCounts)) { | |
| const userRef = db.collection('users').doc(userId); | |
| const userSnap = await userRef.get(); | |
| if (!userSnap.exists) continue; | |
| const userData = userSnap.data() || {}; | |
| // Skip users who are already shadowbanned | |
| if (userData.isShadowBanned) continue; | |
| const missedRsvpsCount = (userData.missedRsvpsCount || 0) + ghostCounts[userId]; | |
| const isShadowBanned = missedRsvpsCount >= GHOST_THRESHOLD; | |
| batch.update(userRef, { | |
| missedRsvpsCount, | |
| ...(isShadowBanned && { isShadowBanned: true }), | |
| }); | |
| operationCount++; | |
| if (operationCount === 500) { | |
| await batch.commit(); | |
| operationCount = 0; | |
| } | |
| } | |
| if (operationCount > 0) { | |
| await batch.commit(); | |
| } | |
| const batch = db.batch(); | |
| let operationCount = 0; | |
| // Fetch users and increment their global missed RSVPs count | |
| for (const userId of Object.keys(ghostCounts)) { | |
| const userRef = db.collection('users').doc(userId); | |
| const userSnap = await userRef.get(); | |
| if (!userSnap.exists) continue; | |
| const userData = userSnap.data() || {}; | |
| // Skip users who are already shadowbanned | |
| if (userData.isShadowBanned) continue; | |
| const missedRsvpsCount = (userData.missedRsvpsCount || 0) + ghostCounts[userId]; | |
| const isShadowBanned = missedRsvpsCount >= GHOST_THRESHOLD; | |
| batch.update(userRef, { | |
| missedRsvpsCount, | |
| ...(isShadowBanned && { isShadowBanned: true }), | |
| }); | |
| operationCount++; | |
| if (operationCount === 500) { | |
| await batch.commit(); | |
| batch = db.batch(); | |
| operationCount = 0; | |
| } | |
| } | |
| if (operationCount > 0) { | |
| 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 `@cloud-functions/src/events/attendance.ts` around lines 51 - 84, The batch
variable is declared as const and is not reset after being committed at 500
operations, causing subsequent batch.update() calls to operate on a stale batch.
Change the const declaration to let on the initial batch assignment so the
variable can be reassigned, and after the await batch.commit() call within the
operationCount check, reinitialize batch to a fresh db.batch() so the next
iteration has a new, usable batch to operate on.
Comment on lines
301
to
304
| allow create: if request.auth != null && | ||
| request.auth.uid == participantId && | ||
| !getUserData(request.auth.uid).get('isShadowBanned', false) && | ||
| validateAttendanceStatus(request.resource.data) && |
Contributor
There was a problem hiding this comment.
Shadow-ban enforcement is bypassable because isShadowBanned is user-writable.
Line 303 trusts /users/{uid}.isShadowBanned, but Lines 191-194 currently allow users to update their own /users/{uid} doc without blocking this field. A shadow-banned user can unset the flag and create participants again.
Proposed diff
- (request.auth.uid == userId && !request.resource.data.diff(resource.data).affectedKeys().hasAny(['role', 'points', 'reputation', 'attendanceCount', 'pushToken']) && validateRateLimitUpdate(userId))
+ (request.auth.uid == userId && !request.resource.data.diff(resource.data).affectedKeys().hasAny([
+ 'role',
+ 'points',
+ 'reputation',
+ 'attendanceCount',
+ 'pushToken',
+ 'isShadowBanned',
+ 'missedRsvpsCount'
+ ]) && validateRateLimitUpdate(userId))🤖 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 `@firestore.rules` around lines 301 - 304, The shadow-ban enforcement is
bypassable because users can modify their own isShadowBanned field in the
/users/{uid} document. In the user update rules (around lines 191-194), add a
validation check to prevent users from writing to the isShadowBanned field on
their own user documents. This ensures that only administrators or privileged
operations can modify the isShadowBanned flag, preventing shadow-banned users
from unblocking themselves and bypassing the check at line 303.
Contributor
Author
|
Hi @roshankumar0036singh, just a quick note regarding the CodeRabbit review! It seems the bot left suggestions on a number of pre-existing files (like |
Owner
|
clean the branch history |
riddhima25bet10005-a11y
force-pushed
the
feature/issue-572-ghosting
branch
from
3562546 to
23131de
Compare
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
firestore.rules (1)
301-304:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winShadow-ban check is bypassable due to writable user ban fields.
Line 303 trusts
/users/{uid}.isShadowBanned, but Line 193 still lets users update their own user doc without forbiddingisShadowBannedandmissedRsvpsCount. A shadow-banned user can unset the flag and RSVP again.Suggested minimal rules fix
- (request.auth.uid == userId && !request.resource.data.diff(resource.data).affectedKeys().hasAny(['role', 'points', 'reputation', 'attendanceCount', 'pushToken']) && validateRateLimitUpdate(userId)) + (request.auth.uid == userId && !request.resource.data.diff(resource.data).affectedKeys().hasAny([ + 'role', + 'points', + 'reputation', + 'attendanceCount', + 'pushToken', + 'isShadowBanned', + 'missedRsvpsCount' + ]) && validateRateLimitUpdate(userId))🤖 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 `@firestore.rules` around lines 301 - 304, The shadow-ban check that reads getUserData(request.auth.uid).get('isShadowBanned', false) on line 303 can be bypassed because the user document write rules around line 193 do not prevent users from updating the isShadowBanned and missedRsvpsCount fields in their own documents. To fix this, modify the user document creation and update rules to explicitly forbid authenticated users from modifying these critical fields in their own user documents, ensuring that only backend logic or admin operations can change shadow-ban status.
🤖 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.
Duplicate comments:
In `@firestore.rules`:
- Around line 301-304: The shadow-ban check that reads
getUserData(request.auth.uid).get('isShadowBanned', false) on line 303 can be
bypassed because the user document write rules around line 193 do not prevent
users from updating the isShadowBanned and missedRsvpsCount fields in their own
documents. To fix this, modify the user document creation and update rules to
explicitly forbid authenticated users from modifying these critical fields in
their own user documents, ensuring that only backend logic or admin operations
can change shadow-ban status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c6d69e5-1901-4d55-bc1a-d88ab6fdfad9
📒 Files selected for processing (3)
cloud-functions/src/events/attendance.tscloud-functions/src/events/index.tsfirestore.rules
🚧 Files skipped from review as they are similar to previous changes (1)
- cloud-functions/src/events/attendance.ts
Contributor
Author
|
@roshankumar0036singh |
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
2 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.
Fixes #572
Description
This PR introduces an automated system to shadowban users who frequently RSVP to events but fail to attend ("ghosting").
Changes Made:
attendance.ts): Added a scheduled function (detectGhostingUsers) that runs daily. It queries events that ended in the last 24 hours, finds all users with anrsvpstatus (did not check in), and increments theirmissedRsvpsCount. If the count reaches the threshold of 3,isShadowBannedis set totrueon their profile.firestore.rules): Modified theparticipantssubcollection rules to explicitly reject the creation of new RSVPs if the authenticated user hasisShadowBanned: true.GSSoC '26
This PR follows the contribution guidelines for GirlScript Summer of Code 2026.
Summary by CodeRabbit
Release Notes