fix: correct ending braces - waitlist ready#144
Open
grishabhatia wants to merge 6 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds server callables ChangesWaitlist feature implementation and security
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs:
Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 3
🤖 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 `@firestore.rules`:
- Around line 27-37: The current allow create/update/delete rules let clients
directly mutate waitlist docs and bypass server-controlled
joinWaitlist/leaveWaitlist flows; change these rules to deny client-initiated
writes and only allow server/callable-managed writes by requiring a server-only
custom claim (e.g., request.auth.token.isServer == true) or a specific admin
claim in the create/update/delete checks instead of allowing request.auth.uid ==
resource.data.userId; update the allow create, allow update, and allow delete
clauses to require that server-only claim (and keep reads as-is) so only the
callable functions (joinWaitlist/leaveWaitlist) can perform mutations.
- Around line 4-47: The rules file is missing rules for actively used
collections (clubs, reminders, feedback, feedbackRequests) which will be denied
by default; add Firestore match blocks for match /clubs/{clubId}, match
/reminders/{reminderId}, match /events/{eventId}/feedback/{feedbackId} and match
/feedbackRequests/{reqId} with access patterns mirroring existing collections:
require request.auth != null for reads, restrict creates/updates/deletes to the
resource owner (compare request.auth.uid to request.resource.data.userId or
resource.data.ownerId) or to admins (request.auth.token.role == 'admin'), and
for feedback subcollection ensure creators' userId matches request.auth.uid and
allow event owner or admin to read/manage; implement these checks in the same
style as the events/waitlist/registrations rules so deployments won't break the
app.
- Around line 43-46: The create rule currently allows any authenticated user to
set arbitrary registration fields including status='confirmed'; change the
create check (the allow create that uses request.resource.data.userId) to also
require request.resource.data.status == 'pending' (or omit status from client
writes) and keep request.resource.data.userId == request.auth.uid so clients can
only create their own pending registrations; additionally add/update rules to
prevent clients from flipping status to 'confirmed' (e.g., in the update rule
require request.resource.data.status == resource.data.status ||
request.auth.token.admin == true or only allow status transitions by a trusted
server identity) so joinWaitlist and seat counts remain trustworthy.
🪄 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: 431bbf1b-adec-4813-b8c7-b2c1c30a32cd
📒 Files selected for processing (3)
app/src/screens/EventDetail.jscloud-functions/src/index.tsfirestore.rules
Comment on lines
+4
to
47
|
|
||
| match /users/{userId} { | ||
| allow create: if request.auth != null && request.auth.uid == userId; | ||
| allow read: if request.auth != null && request.auth.uid == userId; | ||
| allow update: if request.auth.token.admin == true; | ||
| } | ||
| match /clubs/{clubId} { | ||
| allow read: if true; | ||
| allow create: if request.auth != null && request.auth.token.admin == true; | ||
| allow update, delete: if request.auth != null && request.auth.token.admin == true; | ||
| allow read: if request.auth != null; | ||
| allow write: if request.auth != null && request.auth.uid == userId; | ||
| } | ||
|
|
||
| match /events/{eventId} { | ||
| allow read: if true; | ||
| allow create: if request.auth != null && (request.auth.token.admin == true || request.auth.token.club == true); | ||
| allow update, delete: if request.auth != null && (request.auth.token.admin == true || request.auth.uid == resource.data.ownerId); | ||
| } | ||
| match /reminders/{rid} { | ||
| allow create: if request.auth != null && request.auth.uid == request.resource.data.userId; | ||
| allow read, update, delete: if request.auth != null && request.auth.uid == resource.data.userId; | ||
| } | ||
| match /admin/{doc} { | ||
| allow read, write: if request.auth != null && request.auth.token.admin == true; | ||
| allow create: if request.auth != null && | ||
| (request.auth.token.role == 'organizer' || request.auth.token.role == 'admin') && | ||
| request.resource.data.ownerId == request.auth.uid; | ||
| allow update: if request.auth != null && | ||
| (resource.data.ownerId == request.auth.uid || | ||
| request.auth.token.role == 'admin'); | ||
| allow delete: if request.auth != null && | ||
| (resource.data.ownerId == request.auth.uid || | ||
| request.auth.token.role == 'admin'); | ||
|
|
||
| match /waitlist/{waitlistId} { | ||
| allow read: if request.auth != null && | ||
| (request.auth.uid == resource.data.userId || | ||
| get(/databases/$(database)/documents/events/$(eventId)).data.ownerId == request.auth.uid || | ||
| request.auth.token.role == 'admin'); | ||
| allow create: if request.auth != null && | ||
| request.auth.uid == request.resource.data.userId; | ||
| allow update: if request.auth != null && | ||
| (request.auth.uid == resource.data.userId || | ||
| get(/databases/$(database)/documents/events/$(eventId)).data.ownerId == request.auth.uid || | ||
| request.auth.token.role == 'admin') && | ||
| request.resource.data.userId == resource.data.userId; | ||
| allow delete: if request.auth != null && | ||
| (request.auth.uid == resource.data.userId || | ||
| get(/databases/$(database)/documents/events/$(eventId)).data.ownerId == request.auth.uid || | ||
| request.auth.token.role == 'admin'); | ||
| } | ||
| } | ||
| match /feedback/{docId} { | ||
| allow create: if request.auth != null | ||
| && request.resource.data.userId == request.auth.uid; | ||
|
|
||
| allow read: if request.auth != null | ||
| && (resource.data.userId == request.auth.uid | ||
| || request.auth.token.admin == true); | ||
|
|
||
| allow update, delete: if request.auth != null | ||
| && request.auth.token.admin == true; | ||
|
|
||
| match /registrations/{registrationId} { | ||
| allow read: if request.auth != null; | ||
| allow create: if request.auth != null && | ||
| request.auth.uid == request.resource.data.userId; | ||
| allow delete: if request.auth != null && | ||
| request.auth.uid == resource.data.userId; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for name in clubs reminders admin feedback; do
echo "=== ${name} ==="
rg -n -C2 --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
"(collection|doc)\(\s*['\"]${name}['\"]|/${name}(/|['\"])|match /${name})" .
echo
doneRepository: roshankumar0036singh/Uni-Event
Length of output: 283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for name in clubs reminders admin feedback; do
echo "=== Searching for '$name' ==="
rg -n -C2 --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
"${name}" . || true
echo
doneRepository: roshankumar0036singh/Uni-Event
Length of output: 50387
🏁 Script executed:
cat firestore.rulesRepository: roshankumar0036singh/Uni-Event
Length of output: 2185
🏁 Script executed:
rg -t js -t ts "collection.*feedback|feedback.*collection|match /feedback" app/ cloud-functions/ --max-count=20Repository: roshankumar0036singh/Uni-Event
Length of output: 555
Add Firestore rules for actively used collections before deploying.
The ruleset now covers only users, events, waitlist, and registrations. However, multiple collections actively used in the codebase have no rules defined and will be denied by default:
clubs— Read/written by reputation.ts (Cloud Function), MobileAdmin.js, DesktopAdmin.js, ProfileScreen.jsreminders— Read/written by reminders.ts (Cloud Function), EventDetail.js, RemindersScreen.jsfeedback— Written by ReportBugScreen.js and accessed as event subcollectionsfeedbackRequests— Queried by UserFeed.js
Add rules for these collections or migrate data to covered collections. Deploying without these rules will break the app.
🤖 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 4 - 47, The rules file is missing rules for
actively used collections (clubs, reminders, feedback, feedbackRequests) which
will be denied by default; add Firestore match blocks for match /clubs/{clubId},
match /reminders/{reminderId}, match /events/{eventId}/feedback/{feedbackId} and
match /feedbackRequests/{reqId} with access patterns mirroring existing
collections: require request.auth != null for reads, restrict
creates/updates/deletes to the resource owner (compare request.auth.uid to
request.resource.data.userId or resource.data.ownerId) or to admins
(request.auth.token.role == 'admin'), and for feedback subcollection ensure
creators' userId matches request.auth.uid and allow event owner or admin to
read/manage; implement these checks in the same style as the
events/waitlist/registrations rules so deployments won't break the app.
Comment on lines
+27
to
+37
| allow create: if request.auth != null && | ||
| request.auth.uid == request.resource.data.userId; | ||
| allow update: if request.auth != null && | ||
| (request.auth.uid == resource.data.userId || | ||
| get(/databases/$(database)/documents/events/$(eventId)).data.ownerId == request.auth.uid || | ||
| request.auth.token.role == 'admin') && | ||
| request.resource.data.userId == resource.data.userId; | ||
| allow delete: if request.auth != null && | ||
| (request.auth.uid == resource.data.userId || | ||
| get(/databases/$(database)/documents/events/$(eventId)).data.ownerId == request.auth.uid || | ||
| request.auth.token.role == 'admin'); |
Contributor
There was a problem hiding this comment.
Lock down waitlist mutations to the callable functions.
These rules let an authenticated user create, rewrite, and hard-delete their own waitlist documents directly. That bypasses joinWaitlist/leaveWaitlist, so a client can insert duplicate or backdated waiting entries, change status, or delete the record instead of soft-leaving.
Minimal safe rule change
match /waitlist/{waitlistId} {
allow read: if request.auth != null &&
(request.auth.uid == resource.data.userId ||
get(/databases/$(database)/documents/events/$(eventId)).data.ownerId == request.auth.uid ||
request.auth.token.role == 'admin');
- allow create: if request.auth != null &&
- request.auth.uid == request.resource.data.userId;
- allow update: if request.auth != null &&
- (request.auth.uid == resource.data.userId ||
- get(/databases/$(database)/documents/events/$(eventId)).data.ownerId == request.auth.uid ||
- request.auth.token.role == 'admin') &&
- request.resource.data.userId == resource.data.userId;
- allow delete: if request.auth != null &&
- (request.auth.uid == resource.data.userId ||
- get(/databases/$(database)/documents/events/$(eventId)).data.ownerId == request.auth.uid ||
- request.auth.token.role == 'admin');
+ allow create, update, delete: if 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 `@firestore.rules` around lines 27 - 37, The current allow create/update/delete
rules let clients directly mutate waitlist docs and bypass server-controlled
joinWaitlist/leaveWaitlist flows; change these rules to deny client-initiated
writes and only allow server/callable-managed writes by requiring a server-only
custom claim (e.g., request.auth.token.isServer == true) or a specific admin
claim in the create/update/delete checks instead of allowing request.auth.uid ==
resource.data.userId; update the allow create, allow update, and allow delete
clauses to require that server-only claim (and keep reads as-is) so only the
callable functions (joinWaitlist/leaveWaitlist) can perform mutations.
Comment on lines
+43
to
+46
| allow create: if request.auth != null && | ||
| request.auth.uid == request.resource.data.userId; | ||
| allow delete: if request.auth != null && | ||
| request.auth.uid == resource.data.userId; |
Contributor
There was a problem hiding this comment.
Don't let clients mint their own confirmed registrations.
allow create only ties the document to request.auth.uid, so any signed-in user can create a registration for themselves with arbitrary eventId and status values, including confirmed. That makes the seat count used by joinWaitlist untrustworthy and bypasses any approval/payment checks that are supposed to gate registrations.
🤖 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 43 - 46, The create rule currently allows any
authenticated user to set arbitrary registration fields including
status='confirmed'; change the create check (the allow create that uses
request.resource.data.userId) to also require request.resource.data.status ==
'pending' (or omit status from client writes) and keep
request.resource.data.userId == request.auth.uid so clients can only create
their own pending registrations; additionally add/update rules to prevent
clients from flipping status to 'confirmed' (e.g., in the update rule require
request.resource.data.status == resource.data.status || request.auth.token.admin
== true or only allow status transitions by a trusted server identity) so
joinWaitlist and seat counts remain trustworthy.
Author
|
@roshankumar0036singh |
Author
|
@roshankumar0036singh |
Owner
|
@grishabhatia we need the fixes for it run the lint and fix it as i am alreayd woirking to remove all lint erros from code so dont want it to get added from code |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/screens/EventDetail.js (1)
168-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential null reference: Firestore queries use
user.uidwithout null guard.Lines 168 and 172-177 access
user.uidwithout checking ifuseris truthy first. While later code (line 184) does checkif (user), these earlier calls would throw ifuseris null/undefined on initial render.Proposed fix: Add user guard
- getDoc(doc(db, `events/${eventId}/feedback`, user.uid)).then(snap => { - if (snap.exists()) setHasGivenFeedback(true); - }); + if (user) { + getDoc(doc(db, `events/${eventId}/feedback`, user.uid)).then(snap => { + if (snap.exists()) setHasGivenFeedback(true); + }); + } - getDocs( - query( - collection(db, 'reminders'), - where('userId', '==', user.uid), - where('eventId', '==', eventId), - ), - ).then(snap => { - if (!snap.empty) { - setReminderId(snap.docs[0].id); - } - }); + if (user) { + getDocs( + query( + collection(db, 'reminders'), + where('userId', '==', user.uid), + where('eventId', '==', eventId), + ), + ).then(snap => { + if (!snap.empty) { + setReminderId(snap.docs[0].id); + } + }); + }🤖 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 168 - 182, The calls to getDoc(...) and getDocs(...) use user.uid without guarding for a null/undefined user, which can throw on initial render; wrap both calls in a guard that checks user truthiness (e.g. if (user) { ... }) before calling doc(..., user.uid) and querying reminders, or otherwise short-circuit using user?.uid; ensure you still call setHasGivenFeedback and setReminderId only inside that guarded block so getDoc/getDocs are never invoked with a null uid (references: getDoc, getDocs, doc(..., `events/${eventId}/feedback`, user.uid), collection(db, 'reminders'), where('userId','==', user.uid), setHasGivenFeedback, setReminderId).
🧹 Nitpick comments (3)
app/src/screens/EventDetail.js (3)
373-379: 💤 Low valueRedundant require:
Shareis already imported at the top.
Shareis imported fromreact-nativeon line 27, but line 374 re-requires it using CommonJS syntax. This is redundant and mixes module systems unnecessarily.Proposed fix: Use the existing import
} else { - const { Share } = require('react-native'); await Share.share({ message: shareMessage, url: eventUrl, title: event.title, }); }🤖 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 373 - 379, Remove the redundant CommonJS require for Share inside the else branch and use the already-imported Share symbol (imported at top of EventDetail.js) when calling Share.share with shareMessage, eventUrl and event.title; update the else block to call Share.share directly and remove the require('react-native') line.
197-233: ⚖️ Poor tradeoffPerformance: Two sequential queries to calculate waitlist position.
checkWaitlistStatusruns two queries - first to check if the user is on the waitlist, then to get all waiting users for position calculation. This could be optimized into a single query by always fetching all waiting entries and finding the user in the result.🤖 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 197 - 233, checkWaitlistStatus currently issues two queries; instead perform a single ordered query using getDocs(query(collection(db, 'events', eventId, 'waitlist'), where('status','==','waiting'), orderBy('joinedAt','asc'))) to fetch all waiting entries, then find the user's entry (match by doc.data().userId or doc.id) to compute position and call setIsOnWaitlist and setWaitlistPosition accordingly (or clear them if not found); keep the existing try/catch and dependencies and reuse existing symbols getDocs, query, collection, where, orderBy, setIsOnWaitlist, setWaitlistPosition and remove the initial existence check/query.
482-482: 💤 Low valueUnused destructured variable:
request.The
requestvariable is destructured fromCalendarService.useCalendarAuth()but is never used in the component.Proposed fix
- const { request, response, promptAsync } = CalendarService.useCalendarAuth(); + const { response, promptAsync } = CalendarService.useCalendarAuth();🤖 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` at line 482, Remove the unused destructured variable `request` from the call to CalendarService.useCalendarAuth() in EventDetail.js; update the destructuring to only capture the used values (e.g., `response` and `promptAsync`) so the unused `request` identifier is not declared and ESLint/compile warnings are resolved. Ensure the change is made where CalendarService.useCalendarAuth() is invoked so the rest of the component (references to `response` and `promptAsync`) remain unchanged.
🤖 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/screens/EventDetail.js`:
- Around line 211-217: The Firestore query in EventDetail.js uses orderBy
(inside the getDocs query for collection(..., 'waitlist') in the
checkWaitlistStatus/getDocs block) but orderBy is not imported from
'firebase/firestore', causing a ReferenceError; fix this by adding orderBy to
the named imports from 'firebase/firestore' alongside existing imports (so the
symbol orderBy is available wherever getDocs/query/collection/where are used).
- Around line 1515-1871: getStyles is declared after the component's return
(making it unreachable and causing a ReferenceError when getStyles(theme) is
called); move the getStyles function (the const getStyles = theme =>
StyleSheet.create({...}) block) to before the component's return or better yet
hoist it outside the component entirely and/or memoize it (e.g., using useMemo)
so it isn't recreated on every render; ensure any references to getStyles(theme)
still point to the moved function and that StyleSheet.create remains intact.
- Around line 45-47: Remove the unused declarations causing lint errors: delete
the const { width } = Dimensions.get('window'); and the const UniEventLogo =
require('../../assets/UniEvent.png'); lines from EventDetail.js, and also remove
the Dimensions import (from the import list) if it is no longer used anywhere in
the file; verify no other references to width or UniEventLogo remain before
committing.
---
Outside diff comments:
In `@app/src/screens/EventDetail.js`:
- Around line 168-182: The calls to getDoc(...) and getDocs(...) use user.uid
without guarding for a null/undefined user, which can throw on initial render;
wrap both calls in a guard that checks user truthiness (e.g. if (user) { ... })
before calling doc(..., user.uid) and querying reminders, or otherwise
short-circuit using user?.uid; ensure you still call setHasGivenFeedback and
setReminderId only inside that guarded block so getDoc/getDocs are never invoked
with a null uid (references: getDoc, getDocs, doc(...,
`events/${eventId}/feedback`, user.uid), collection(db, 'reminders'),
where('userId','==', user.uid), setHasGivenFeedback, setReminderId).
---
Nitpick comments:
In `@app/src/screens/EventDetail.js`:
- Around line 373-379: Remove the redundant CommonJS require for Share inside
the else branch and use the already-imported Share symbol (imported at top of
EventDetail.js) when calling Share.share with shareMessage, eventUrl and
event.title; update the else block to call Share.share directly and remove the
require('react-native') line.
- Around line 197-233: checkWaitlistStatus currently issues two queries; instead
perform a single ordered query using getDocs(query(collection(db, 'events',
eventId, 'waitlist'), where('status','==','waiting'),
orderBy('joinedAt','asc'))) to fetch all waiting entries, then find the user's
entry (match by doc.data().userId or doc.id) to compute position and call
setIsOnWaitlist and setWaitlistPosition accordingly (or clear them if not
found); keep the existing try/catch and dependencies and reuse existing symbols
getDocs, query, collection, where, orderBy, setIsOnWaitlist, setWaitlistPosition
and remove the initial existence check/query.
- Line 482: Remove the unused destructured variable `request` from the call to
CalendarService.useCalendarAuth() in EventDetail.js; update the destructuring to
only capture the used values (e.g., `response` and `promptAsync`) so the unused
`request` identifier is not declared and ESLint/compile warnings are resolved.
Ensure the change is made where CalendarService.useCalendarAuth() is invoked so
the rest of the component (references to `response` and `promptAsync`) remain
unchanged.
🪄 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: f8071c02-6a88-4c27-9ad6-16e17700292c
📒 Files selected for processing (1)
app/src/screens/EventDetail.js
Comment on lines
+45
to
+47
| const { width } = Dimensions.get('window'); | ||
|
|
||
| const UniEventLogo = require('../../assets/UniEvent.png'); |
Contributor
There was a problem hiding this comment.
Unused variables: width and UniEventLogo.
Both width (from Dimensions.get('window')) and UniEventLogo are defined but never used in the component. Per PR comments, the maintainer requested lint errors be fixed in this PR.
Proposed fix: Remove unused declarations
-const { width } = Dimensions.get('window');
-
-const UniEventLogo = require('../../assets/UniEvent.png');Also remove the Dimensions import from line 22 if no longer needed.
📝 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 { width } = Dimensions.get('window'); | |
| const UniEventLogo = require('../../assets/UniEvent.png'); |
🤖 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 45 - 47, Remove the unused
declarations causing lint errors: delete the const { width } =
Dimensions.get('window'); and the const UniEventLogo =
require('../../assets/UniEvent.png'); lines from EventDetail.js, and also remove
the Dimensions import (from the import list) if it is no longer used anywhere in
the file; verify no other references to width or UniEventLogo remain before
committing.
Comment on lines
+211
to
+217
| const allWaiting = await getDocs( | ||
| query( | ||
| collection(db, 'events', eventId, 'waitlist'), | ||
| where('status', '==', 'waiting'), | ||
| orderBy('joinedAt', 'asc'), | ||
| ), | ||
| ); |
Contributor
There was a problem hiding this comment.
Critical: orderBy is not imported but used on line 215.
The orderBy function is used in the Firestore query but is not included in the imports from firebase/firestore (lines 3-16). This will throw a ReferenceError when checkWaitlistStatus executes.
Proposed fix: Add orderBy to imports
import {
addDoc,
collection,
deleteDoc,
doc,
getDoc,
getDocs,
increment,
onSnapshot,
+ orderBy,
query,
setDoc,
updateDoc,
where,
} from 'firebase/firestore';📝 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 allWaiting = await getDocs( | |
| query( | |
| collection(db, 'events', eventId, 'waitlist'), | |
| where('status', '==', 'waiting'), | |
| orderBy('joinedAt', 'asc'), | |
| ), | |
| ); | |
| import { | |
| addDoc, | |
| collection, | |
| deleteDoc, | |
| doc, | |
| getDoc, | |
| getDocs, | |
| increment, | |
| onSnapshot, | |
| orderBy, | |
| query, | |
| setDoc, | |
| updateDoc, | |
| where, | |
| } from 'firebase/firestore'; |
🤖 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 211 - 217, The Firestore query
in EventDetail.js uses orderBy (inside the getDocs query for collection(...,
'waitlist') in the checkWaitlistStatus/getDocs block) but orderBy is not
imported from 'firebase/firestore', causing a ReferenceError; fix this by adding
orderBy to the named imports from 'firebase/firestore' alongside existing
imports (so the symbol orderBy is available wherever
getDocs/query/collection/where are used).
Comment on lines
+1515
to
+1871
| const getStyles = theme => | ||
| StyleSheet.create({ | ||
| headerImage: { height: 350, width: '100%' }, | ||
| headerGradient: { flex: 1, paddingTop: 40, paddingHorizontal: 20 }, | ||
| headerSafe: { flexDirection: 'row', justifyContent: 'space-between' }, | ||
| backButton: { | ||
| width: 40, | ||
| height: 40, | ||
| borderRadius: 20, | ||
| backgroundColor: 'rgba(0,0,0,0.5)', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| ...theme.shadows.small, | ||
| }, | ||
| bookmarkButton: { | ||
| width: 40, | ||
| height: 40, | ||
| borderRadius: 20, | ||
| backgroundColor: 'rgba(0,0,0,0.5)', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| ...theme.shadows.small, | ||
| }, | ||
| liveBadge: { | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| gap: 6, | ||
| backgroundColor: '#FF3B30', | ||
| paddingHorizontal: 12, | ||
| paddingVertical: 6, | ||
| borderRadius: 20, | ||
| position: 'absolute', | ||
| top: 20, | ||
| left: 20, | ||
| }, | ||
| liveText: { color: '#fff', fontSize: 12, fontWeight: 'bold' }, | ||
|
|
||
| contentSheet: { | ||
| flex: 1, | ||
| marginTop: -40, | ||
| borderTopLeftRadius: 32, | ||
| borderTopRightRadius: 32, | ||
| backgroundColor: theme.colors.background, | ||
| paddingHorizontal: 24, | ||
| paddingTop: 32, | ||
| }, | ||
|
|
||
| headerSection: { | ||
| marginBottom: 20, | ||
| }, | ||
| badgeRow: { | ||
| flexDirection: 'row', | ||
| gap: 8, | ||
| marginBottom: 16, | ||
| }, | ||
| categoryBadge: { | ||
| paddingHorizontal: 12, | ||
| paddingVertical: 6, | ||
| borderRadius: 20, | ||
| }, | ||
| categoryText: { | ||
| fontSize: 12, | ||
| fontWeight: '600', | ||
| }, | ||
| priceBadge: { | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| gap: 4, | ||
| paddingHorizontal: 12, | ||
| paddingVertical: 6, | ||
| borderRadius: 20, | ||
| }, | ||
| priceText: { | ||
| color: '#fff', | ||
| fontSize: 12, | ||
| fontWeight: '700', | ||
| }, | ||
| eventTitle: { | ||
| fontSize: 28, | ||
| fontWeight: '800', | ||
| marginBottom: 16, | ||
| lineHeight: 34, | ||
| }, | ||
| hostButton: { | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| gap: 12, | ||
| paddingVertical: 12, | ||
| }, | ||
| hostAvatar: { | ||
| width: 44, | ||
| height: 44, | ||
| borderRadius: 22, | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| }, | ||
| hostAvatarText: { | ||
| fontSize: 18, | ||
| fontWeight: '700', | ||
| }, | ||
| hostLabel: { | ||
| fontSize: 12, | ||
| }, | ||
| hostName: { | ||
| fontSize: 16, | ||
| fontWeight: '600', | ||
| }, | ||
|
|
||
| quickActionsCard: { | ||
| flexDirection: 'row', | ||
| justifyContent: 'space-around', | ||
| padding: 16, | ||
| borderRadius: 20, | ||
| marginBottom: 20, | ||
| ...theme.shadows.small, | ||
| }, | ||
| quickAction: { | ||
| alignItems: 'center', | ||
| gap: 8, | ||
| }, | ||
| quickActionIcon: { | ||
| width: 48, | ||
| height: 48, | ||
| borderRadius: 24, | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| }, | ||
| quickActionLabel: { | ||
| fontSize: 12, | ||
| fontWeight: '500', | ||
| }, | ||
|
|
||
| detailsCard: { | ||
| borderRadius: 20, | ||
| padding: 20, | ||
| marginBottom: 20, | ||
| ...theme.shadows.small, | ||
| }, | ||
| detailRow: { | ||
| flexDirection: 'row', | ||
| alignItems: 'flex-start', | ||
| gap: 16, | ||
| }, | ||
| detailIconContainer: { | ||
| width: 48, | ||
| height: 48, | ||
| borderRadius: 24, | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| }, | ||
| detailContent: { | ||
| flex: 1, | ||
| }, | ||
| detailLabel: { | ||
| fontSize: 12, | ||
| fontWeight: '500', | ||
| marginBottom: 4, | ||
| }, | ||
| detailValue: { | ||
| fontSize: 16, | ||
| fontWeight: '600', | ||
| marginBottom: 2, | ||
| }, | ||
| detailSubValue: { | ||
| fontSize: 14, | ||
| }, | ||
| detailDivider: { | ||
| height: 1, | ||
| marginVertical: 16, | ||
| }, | ||
|
|
||
| aboutSection: { | ||
| marginBottom: 20, | ||
| }, | ||
| sectionTitle: { | ||
| fontSize: 20, | ||
| fontWeight: '700', | ||
| marginBottom: 12, | ||
| }, | ||
| description: { | ||
| fontSize: 15, | ||
| lineHeight: 24, | ||
| }, | ||
|
|
||
| outlinedButton: { | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| gap: 12, | ||
| paddingVertical: 16, | ||
| paddingHorizontal: 24, | ||
| borderRadius: 14, | ||
| borderWidth: 2, | ||
| marginBottom: 14, | ||
| backgroundColor: theme.colors.surface, | ||
| ...theme.shadows.small, | ||
| }, | ||
| outlinedButtonText: { | ||
| fontSize: 16, | ||
| fontWeight: '700', | ||
| }, | ||
|
|
||
| meetLinkCard: { | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| padding: 14, | ||
| paddingHorizontal: 16, | ||
| borderRadius: 14, | ||
| marginBottom: 20, | ||
| gap: 12, | ||
| ...theme.shadows.default, | ||
| }, | ||
| meetLinkIcon: { | ||
| width: 36, | ||
| height: 36, | ||
| borderRadius: 10, | ||
| backgroundColor: 'rgba(255,255,255,0.2)', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| }, | ||
| meetLinkTitle: { | ||
| color: '#fff', | ||
| fontSize: 14, | ||
| fontWeight: '700', | ||
| marginBottom: 2, | ||
| }, | ||
| meetLinkSubtitle: { | ||
| color: 'rgba(255,255,255,0.85)', | ||
| fontSize: 11, | ||
| }, | ||
|
|
||
| organizerSection: { | ||
| marginBottom: 20, | ||
| }, | ||
| organizerGrid: { | ||
| flexDirection: 'row', | ||
| gap: 10, | ||
| }, | ||
| organizerCard: { | ||
| flex: 1, | ||
| padding: 16, | ||
| paddingVertical: 18, | ||
| borderRadius: 14, | ||
| alignItems: 'center', | ||
| ...theme.shadows.small, | ||
| justifyContent: 'center', | ||
| }, | ||
| organizerIconBg: { | ||
| width: 44, | ||
| height: 44, | ||
| borderRadius: 12, | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| }, | ||
| organizerCardTitle: { | ||
| fontSize: 15, | ||
| fontWeight: '700', | ||
| marginBottom: 4, | ||
| }, | ||
| organizerCardDesc: { | ||
| fontSize: 12, | ||
| textAlign: 'center', | ||
| opacity: 0.7, | ||
| }, | ||
|
|
||
| compactButton: { | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| gap: 8, | ||
| paddingVertical: 10, | ||
| paddingHorizontal: 16, | ||
| borderWidth: 1, | ||
| borderRadius: 12, | ||
| flexGrow: 1, | ||
| minWidth: '45%', | ||
| }, | ||
| compactButtonText: { | ||
| fontSize: 14, | ||
| fontWeight: '600', | ||
| }, | ||
|
|
||
| feedbackCard: { | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| padding: 18, | ||
| borderRadius: 20, | ||
| gap: 12, | ||
| marginBottom: 20, | ||
| ...theme.shadows.default, | ||
| }, | ||
| feedbackText: { | ||
| fontSize: 16, | ||
| fontWeight: '700', | ||
| flex: 1, | ||
| }, | ||
|
|
||
| waitlistButton: { | ||
| backgroundColor: '#f59e0b', | ||
| paddingVertical: 14, | ||
| paddingHorizontal: 32, | ||
| borderRadius: 12, | ||
| ...theme.shadows.default, | ||
| }, | ||
| leaveWaitlistButton: { | ||
| backgroundColor: '#fef3c7', | ||
| paddingVertical: 14, | ||
| paddingHorizontal: 32, | ||
| borderRadius: 12, | ||
| borderWidth: 1, | ||
| borderColor: '#f59e0b', | ||
| }, | ||
| leaveWaitlistText: { | ||
| color: '#92400e', | ||
| fontWeight: 'bold', | ||
| fontSize: 14, | ||
| }, | ||
| buttonText: { | ||
| color: '#fff', | ||
| fontWeight: 'bold', | ||
| fontSize: 16, | ||
| }, | ||
|
|
||
| fabContainer: { | ||
| position: 'absolute', | ||
| bottom: 0, | ||
| left: 0, | ||
| right: 0, | ||
| padding: 20, | ||
| paddingBottom: 30, | ||
| borderTopWidth: 1, | ||
| borderTopColor: theme.colors.border, | ||
| flexDirection: 'row', | ||
| justifyContent: 'space-between', | ||
| alignItems: 'center', | ||
| ...theme.shadows.large, | ||
| }, | ||
| fabSubInfo: { justifyContent: 'center' }, | ||
| fabLabel: { fontSize: 12, color: theme.colors.textSecondary }, | ||
| fabValue: { fontSize: 16, fontWeight: 'bold', color: theme.colors.text }, | ||
| primaryBtn: { | ||
| backgroundColor: theme.colors.primary, | ||
| paddingVertical: 14, | ||
| paddingHorizontal: 32, | ||
| borderRadius: 12, | ||
| ...theme.shadows.default, | ||
| }, | ||
| secondaryBtn: { | ||
| backgroundColor: theme.colors.surface, | ||
| borderWidth: 2, | ||
| borderColor: theme.colors.primary, | ||
| }, | ||
| primaryBtnText: { color: '#fff', fontWeight: 'bold', fontSize: 16 }, | ||
| secondaryBtnText: { color: theme.colors.primary }, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: getStyles is defined after the return statement - unreachable code.
The getStyles function is declared inside the component but placed after the return statement (line 862-1513), making it unreachable. Since line 53 calls getStyles(theme) before this const declaration executes, this will throw a ReferenceError due to the temporal dead zone.
Move getStyles definition before the return statement, or move it outside the component entirely (recommended for performance - avoids recreating the function on every render).
Proposed fix: Move getStyles before the return statement
// ... existing code before return ...
+ const getStyles = theme =>
+ StyleSheet.create({
+ headerImage: { height: 350, width: '100%' },
+ // ... rest of styles ...
+ });
+
if (loading || !event)
return (
<View style={{ flex: 1, justifyContent: 'center', alignItems: 'center' }}>
<ActivityIndicator size="large" color={theme.colors.primary} />
</View>
);
return (
<View style={{ flex: 1, backgroundColor: theme.colors.background }}>
// ... JSX ...
</View>
);
-
- const getStyles = theme =>
- StyleSheet.create({
- headerImage: { height: 350, width: '100%' },
- // ... rest of styles ...
- });
}🤖 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 1515 - 1871, getStyles is
declared after the component's return (making it unreachable and causing a
ReferenceError when getStyles(theme) is called); move the getStyles function
(the const getStyles = theme => StyleSheet.create({...}) block) to before the
component's return or better yet hoist it outside the component entirely and/or
memoize it (e.g., using useMemo) so it isn't recreated on every render; ensure
any references to getStyles(theme) still point to the moved function and that
StyleSheet.create remains intact.
Author
|
@roshankumar0036singh |
Owner
|
@grishabhatia cacn you check what the coderabbit suggestion are about |
Author
|
The CodeRabbit suggestions are mostly about:
Since you mentioned you are already working to remove all lint errors from the codebase, would you prefer to handle these yourself, or should I push fixes for them? Please let me know. |
Owner
|
@grishabhatia plz fix the things in your pr |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/screens/EventDetail.js (2)
169-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard guest users before the feedback/reminder reads.
Lines 169-183 dereference
user.uideven though this screen has signed-out paths elsewhere. Opening the event while logged out will throw before the guest state can render. Move these reads into the existingif (user)branch and clear the derived state whenuserbecomes null.Suggested fix
- getDoc(doc(db, `events/${eventId}/feedback`, user.uid)).then(snap => { - if (snap.exists()) setHasGivenFeedback(true); - }); - - getDocs( - query( - collection(db, 'reminders'), - where('userId', '==', user.uid), - where('eventId', '==', eventId), - ), - ).then(snap => { - if (!snap.empty) { - setReminderId(snap.docs[0].id); - } - }); - if (user) { + getDoc(doc(db, `events/${eventId}/feedback`, user.uid)).then(snap => { + setHasGivenFeedback(snap.exists()); + }); + + getDocs( + query( + collection(db, 'reminders'), + where('userId', '==', user.uid), + where('eventId', '==', eventId), + ), + ).then(snap => { + setReminderId(snap.empty ? null : snap.docs[0].id); + }); + getDoc(doc(db, 'users', user.uid, 'savedEvents', eventId)).then(snap => { setIsBookmarked(snap.exists()); }); + } else { + setHasGivenFeedback(false); + setReminderId(null); + setIsBookmarked(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 `@app/src/screens/EventDetail.js` around lines 169 - 189, The code reads user.uid for feedback/reminder/bookmark without guarding for a null user; move the getDoc(for feedback), getDocs(query remiders), and the savedEvents getDoc calls into the existing if (user) block (so use user.uid only when user is truthy) and when user becomes null clear derived state by calling setHasGivenFeedback(false), setReminderId(null), and setIsBookmarked(false) so the component won’t crash when signed out; update the logic referencing getDoc/getDocs, setHasGivenFeedback, setReminderId, setIsBookmarked, user.uid and eventId accordingly.
482-491:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
responseto the destructuring on line 482.The
useCalendarAuth()hook returns{ request, response, promptAsync, getAccessToken }, but the code only destructuresrequestandpromptAsync. Line 485 attempts to readresponse?.type, which will be undefined and cause a runtime error.Change line 482 to:
const { request, response, promptAsync } = CalendarService.useCalendarAuth();🤖 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 482 - 491, The useCalendarAuth hook's returned `response` is missing from the destructuring, so `response?.type` in the useEffect is undefined; update the destructuring of CalendarService.useCalendarAuth() to include `response` (alongside `request` and `promptAsync`) so the useEffect can read `response` correctly and trigger CalendarService.addToCalendar(access_token, event) as intended.
🤖 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/screens/EventDetail.js`:
- Around line 471-474: The scheduled reminder's notificationId returned by
scheduleEventReminder(event) is never persisted or wired to the UI; capture the
returned notificationId, persist it to Firestore (e.g., with updateDoc on the
same doc you use for RSVP/userProfile or the event RSVP doc referenced by
userProfileRef), and update the local component state/variable that tracks
reminderId (e.g., setReminderId or reminderId) so the bell UI reflects the
scheduled reminder and future taps can cancel it; also handle and log errors
from scheduleEventReminder before writing to Firestore and show a user-friendly
Alert on failure.
---
Outside diff comments:
In `@app/src/screens/EventDetail.js`:
- Around line 169-189: The code reads user.uid for feedback/reminder/bookmark
without guarding for a null user; move the getDoc(for feedback), getDocs(query
remiders), and the savedEvents getDoc calls into the existing if (user) block
(so use user.uid only when user is truthy) and when user becomes null clear
derived state by calling setHasGivenFeedback(false), setReminderId(null), and
setIsBookmarked(false) so the component won’t crash when signed out; update the
logic referencing getDoc/getDocs, setHasGivenFeedback, setReminderId,
setIsBookmarked, user.uid and eventId accordingly.
- Around line 482-491: The useCalendarAuth hook's returned `response` is missing
from the destructuring, so `response?.type` in the useEffect is undefined;
update the destructuring of CalendarService.useCalendarAuth() to include
`response` (alongside `request` and `promptAsync`) so the useEffect can read
`response` correctly and trigger CalendarService.addToCalendar(access_token,
event) as intended.
🪄 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: 6fa88908-fc36-470f-82ce-10ff8057a771
📒 Files selected for processing (1)
app/src/screens/EventDetail.js
Comment on lines
+471
to
+474
| await updateDoc(userProfileRef, { points: increment(10) }); | ||
|
|
||
| await scheduleEventReminder(event); | ||
| Alert.alert( | ||
| 'Registered! 🎉', | ||
| earlyBird | ||
| ? `You earned +${RSVP_POINTS_CHANGE} Points and the 🐦 Early Bird badge for being one of the first to RSVP!` | ||
| : `You earned +${RSVP_POINTS_CHANGE} Points for registering.`, | ||
| ); | ||
| Alert.alert('Success', 'Registered! (+10 Points)'); |
Contributor
There was a problem hiding this comment.
Persist the RSVP reminder you auto-schedule here.
Lines 473-474 create a reminder side effect, but this path never stores the notificationId in Firestore or updates reminderId. The bell UI stays "off", a later tap can create a second reminder, and the auto-created notification cannot be canceled from this screen.
Suggested fix
- await scheduleEventReminder(event);
+ const notifId = await scheduleEventReminder(event);
+ if (notifId) {
+ const reminderRef = await addDoc(collection(db, 'reminders'), {
+ userId: user.uid,
+ eventId: event.id,
+ eventTitle: event.title,
+ remindAt: new Date(new Date(event.startAt).getTime() - 10 * 60000),
+ notificationId: notifId,
+ createdAt: new Date().toISOString(),
+ });
+ setReminderId(reminderRef.id);
+ }🤖 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 471 - 474, The scheduled
reminder's notificationId returned by scheduleEventReminder(event) is never
persisted or wired to the UI; capture the returned notificationId, persist it to
Firestore (e.g., with updateDoc on the same doc you use for RSVP/userProfile or
the event RSVP doc referenced by userProfileRef), and update the local component
state/variable that tracks reminderId (e.g., setReminderId or reminderId) so the
bell UI reflects the scheduled reminder and future taps can cancel it; also
handle and log errors from scheduleEventReminder before writing to Firestore and
show a user-friendly Alert on failure.
Author
|
@roshankumar0036singh |
Owner
|
@grishabhatia theres a conflict here check it |
|
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.
Phase 1: Waitlist System - Join/Leave Functionality
Files Changed
app/src/screens/EventDetail.js- Waitlist UIcloud-functions/src/index.ts- Cloud Functionsfirestore.rules- Security rulesChanges
Related Issue
#106
@roshankumar0036singh This is a clean PR from latest main. Ready for review.
Summary by CodeRabbit
New Features
Security
Improvements