feat: weekly attendance streak (fe + be)#448
Closed
sheeeuWu wants to merge 3 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements an attendance streak system rewarding consistent weekly event participation. Users see their current streak and 4-week milestone progress in a new dedicated screen. A Cloud Function computes streaks on check-in, and users reaching a 4-week streak receive a digital certificate. ChangesEvent Streaks Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
⚔️ Resolve merge conflicts
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 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: 5
🧹 Nitpick comments (5)
app/src/screens/StreakScreen.js (1)
46-49: ⚡ Quick winClarify progress calculation logic.
The ternary on line 48 is correct but potentially confusing. The logic handles the case where
currentStreak % 4 === 0would return 0, but you want to display 4/4 when a milestone is reached. Consider adding a brief comment or renaming to make the intent clearer.♻️ Proposed refactor with clarifying comment
- // progress toward next milestone (every 4 weeks) + // Progress toward next milestone (every 4 weeks). + // At milestone (currentStreak % 4 === 0), show 4/4 instead of 0/4. const nextMilestone = Math.ceil((currentStreak + 1) / 4) * 4; const progress = currentStreak % 4 === 0 && currentStreak > 0 ? 4 : currentStreak % 4; const progressPercent = (progress / 4) * 100;🤖 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/StreakScreen.js` around lines 46 - 49, The progress calculation is correct but unclear: when currentStreak % 4 === 0 we want to show a full 4/4 progress rather than 0. Update the logic around the progress variable (used with nextMilestone and progressPercent) to make intent explicit—either rename progress to something like progressInCycle or progressDisplay and add a short inline comment explaining "treat cycle-complete (mod 0) as 4/4", or replace the ternary with a small helper function getProgressInCycle(currentStreak) that returns 4 when currentStreak % 4 === 0 && currentStreak > 0 otherwise currentStreak % 4; ensure progressPercent uses that value.app/src/screens/ProfileScreen.js (1)
69-73: 💤 Low valueRefactor icon rendering logic for better maintainability.
The hardcoded check for
'lightning-bolt-outline'creates tight coupling. If additional MaterialCommunityIcons are needed in the future, this pattern becomes unmaintainable. Consider passing aniconFamilyprop or consolidating on a single icon library.♻️ Proposed refactor to accept iconFamily prop
const MenuItem = ({ icon, + iconFamily = 'Ionicons', label, description, onPress, theme, styles, width = '50%', showChevron = true, rightElement, }) => ( <TouchableOpacity onPress={onPress} style={[styles.bentoMenuItem, { width }]}> <View style={styles.bentoTop}> <View style={[ styles.menuIconContainer, { backgroundColor: theme.colors.primary + '20', }, ]} > - {icon === 'lightning-bolt-outline' ? ( - <MaterialCommunityIcons name={icon} size={20} color={theme.colors.primary} /> - ) : ( - <Ionicons name={icon} size={20} color={theme.colors.primary} /> - )} + {iconFamily === 'MaterialCommunityIcons' ? ( + <MaterialCommunityIcons name={icon} size={20} color={theme.colors.primary} /> + ) : iconFamily === 'MaterialIcons' ? ( + <MaterialIcons name={icon} size={20} color={theme.colors.primary} /> + ) : ( + <Ionicons name={icon} size={20} color={theme.colors.primary} /> + )} </View>Then update the Streak menu item:
<MenuItem icon="lightning-bolt-outline" + iconFamily="MaterialCommunityIcons" label="Streak"🤖 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/ProfileScreen.js` around lines 69 - 73, The icon rendering currently checks icon === 'lightning-bolt-outline' and conditionally renders MaterialCommunityIcons vs Ionicons; refactor the component (e.g., ProfileScreen / the Streak menu item) to accept an explicit iconFamily prop (or derive a family from a lookup) and render based on that prop instead of a hardcoded icon string; update the render logic to choose the correct component (MaterialCommunityIcons, Ionicons, etc.) via a small map or switch and provide a sensible default/fallback for unknown families so future icons can be added without modifying this conditional.cloud-functions/src/index.ts (1)
20-20: 💤 Low valueAdd semicolon for consistency with other export statements.
Line 20 is missing a semicolon, while all other export statements (lines 6-19) end with semicolons.
✨ Proposed fix
-export * from './attendanceStreak' +export * from './attendanceStreak';🤖 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/index.ts` at line 20, The export statement "export * from './attendanceStreak'" in index.ts is missing a trailing semicolon; update that line to match the other export lines (lines 6-19) by adding a semicolon at the end so it reads export * from './attendanceStreak'; ensuring consistent export formatting across the file.cloud-functions/src/attendanceStreak.ts (1)
70-77: ⚡ Quick winReuse existing Firestore instance and consider transaction-based certificate check.
Lines 71-72 create a new Firestore instance (
db2) and re-read the user document outside the transaction. This is wasteful and introduces a potential race condition where another check-in could modify the streak between the transaction commit and this read.♻️ Proposed refactor
Option 1: Reuse the existing
dbinstance and accept the minor race (the duplicate check inawardDedicatedStudentCertificatemitigates most harm)://dedicated student certificate award - const db2 = admin.firestore(); - const updatedSnap = await db2.collection("users").doc(userId).get(); + const updatedSnap = await db.collection("users").doc(userId).get(); const updatedStreak: number = updatedSnap.data()?.currentStreak || 0;Option 2 (better): Check the streak inside the transaction and return a flag:
+ let shouldAwardCertificate = false; + await db.runTransaction(async (tx) => { ... tx.update(userRef, { currentStreak: newStreak, longestStreak: newLongestStreak, lastAttendanceAt: now, }); + + if (newStreak >= 4) { + shouldAwardCertificate = true; + } console.log("previous currentStreak:", currentStreak); ... }); //dedicated student certificate award - const db2 = admin.firestore(); - const updatedSnap = await db2.collection("users").doc(userId).get(); - const updatedStreak: number = updatedSnap.data()?.currentStreak || 0; - - if (updatedStreak >= 4) { + if (shouldAwardCertificate) { await awardDedicatedStudentCertificate(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 `@cloud-functions/src/attendanceStreak.ts` around lines 70 - 77, Replace the extra Firestore instance and post-transaction read (db2 and updatedSnap) by performing the dedicated-student check as part of the same transaction (or reuse the existing db if you accept the minor race); specifically, inside the transaction that updates the user's streak (the code using db and runTransaction), read the user doc there and compute updatedStreak and a boolean flag, return that flag from the transaction scope, and only call awardDedicatedStudentCertificate(userId) if the transaction returned true — this removes the separate db2 read and prevents the race condition between commit and the certificate check.cloud-functions/src/certificateService.ts (1)
7-9: ⚡ Quick winConsider reusing Resend client instance for better performance.
The
getResendClient()helper creates a newResendinstance on every call (lines 146, 172). While not a critical issue, instantiating clients repeatedly can add unnecessary overhead. Consider creating a module-level singleton instance instead.♻️ Proposed refactor to singleton pattern
+let resendClient: Resend | null = null; + function getResendClient() { - return new Resend(process.env.RESEND_API_KEY || "re_test_placeholder"); + if (!resendClient) { + resendClient = new Resend(process.env.RESEND_API_KEY || "re_test_placeholder"); + } + return resendClient; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/certificateService.ts` around lines 7 - 9, The getResendClient() helper creates a new Resend(...) on every call which adds overhead; change it to a module-level singleton by instantiating Resend once (using process.env.RESEND_API_KEY || "re_test_placeholder") and have getResendClient() return that single instance (or remove getResendClient and export the singleton directly). Update all callers that currently call getResendClient() to use the shared instance so the Resend client is only constructed once.
🤖 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/StreakScreen.js`:
- Around line 208-212: The certificate date rendering in StreakScreen uses new
Date(cert.awardedAt) which will produce "Invalid Date" if awardedAt is missing;
update the JSX that renders the date to first check cert?.awardedAt (or test
Boolean(cert.awardedAt)) and, if present, format it with new
Date(...).toLocaleDateString('en-US', {year:'numeric', month:'long',
day:'numeric'}), otherwise render a safe fallback string like '—' or 'Date
unavailable' so the UI never shows "Invalid Date".
In `@cloud-functions/src/attendanceStreak.ts`:
- Around line 44-53: The boundary check wrongly treats diffMs == WEEK_MS * 2 as
a missed week; update the consecutive-week branch by changing the condition in
the else-if that references diffMs and WEEK_MS (the branch that sets newStreak =
currentStreak + 1) from a strict < to a <= so that diffMs == WEEK_MS * 2 is
counted as consecutive; keep the earlier diffMs < WEEK_MS check and the
newStreak/currentStreak logic unchanged.
- Around line 11-16: The code currently reads userId from
event.data?.data()?.userId which is fragile; change the extraction to use the
path parameter event.params.userId instead (replace the assignment to the userId
variable where it is set and remove the fallback to event.data?.data()?.userId),
and update the early-return log to reflect missing event.params.userId if
absent; ensure all subsequent uses of userId in attendanceStreak.ts refer to
this parameter-derived variable.
In `@cloud-functions/src/dedicatedStudentCertificate.ts`:
- Around line 121-128: The certificates array is storing awardedAt as an ISO
string; change it to store a Firestore Timestamp so queries stay consistent and
indexable: in the update call on db.collection("users").doc(userId").update(...)
where FieldValue.arrayUnion(...) is used to push the certificate object (type:
"dedicated_student", awardedAt: ... , certificateUrl: signedUrl), replace
now.toDate().toISOString() with the Firestore Timestamp instance (e.g., now or
Timestamp.now()) so the awardedAt field is saved as a Timestamp like other
fields such as lastAttendanceAt.
- Around line 113-118: The signed URL for the certificate is set to expire at
"2499-12-31", effectively permanent; update the call to file.getSignedUrl (and
the signedUrl assignment) to use a reasonable expiry (e.g., generate an expires
value programmatically for 5–10 years from now or read a config value) so the
URL can be invalidated later, or implement a URL refresh mechanism that issues
short-lived signed URLs on-demand; ensure you change the literal expires string
to a computed/managed expiry and document the chosen duration.
---
Nitpick comments:
In `@app/src/screens/ProfileScreen.js`:
- Around line 69-73: The icon rendering currently checks icon ===
'lightning-bolt-outline' and conditionally renders MaterialCommunityIcons vs
Ionicons; refactor the component (e.g., ProfileScreen / the Streak menu item) to
accept an explicit iconFamily prop (or derive a family from a lookup) and render
based on that prop instead of a hardcoded icon string; update the render logic
to choose the correct component (MaterialCommunityIcons, Ionicons, etc.) via a
small map or switch and provide a sensible default/fallback for unknown families
so future icons can be added without modifying this conditional.
In `@app/src/screens/StreakScreen.js`:
- Around line 46-49: The progress calculation is correct but unclear: when
currentStreak % 4 === 0 we want to show a full 4/4 progress rather than 0.
Update the logic around the progress variable (used with nextMilestone and
progressPercent) to make intent explicit—either rename progress to something
like progressInCycle or progressDisplay and add a short inline comment
explaining "treat cycle-complete (mod 0) as 4/4", or replace the ternary with a
small helper function getProgressInCycle(currentStreak) that returns 4 when
currentStreak % 4 === 0 && currentStreak > 0 otherwise currentStreak % 4; ensure
progressPercent uses that value.
In `@cloud-functions/src/attendanceStreak.ts`:
- Around line 70-77: Replace the extra Firestore instance and post-transaction
read (db2 and updatedSnap) by performing the dedicated-student check as part of
the same transaction (or reuse the existing db if you accept the minor race);
specifically, inside the transaction that updates the user's streak (the code
using db and runTransaction), read the user doc there and compute updatedStreak
and a boolean flag, return that flag from the transaction scope, and only call
awardDedicatedStudentCertificate(userId) if the transaction returned true — this
removes the separate db2 read and prevents the race condition between commit and
the certificate check.
In `@cloud-functions/src/certificateService.ts`:
- Around line 7-9: The getResendClient() helper creates a new Resend(...) on
every call which adds overhead; change it to a module-level singleton by
instantiating Resend once (using process.env.RESEND_API_KEY ||
"re_test_placeholder") and have getResendClient() return that single instance
(or remove getResendClient and export the singleton directly). Update all
callers that currently call getResendClient() to use the shared instance so the
Resend client is only constructed once.
In `@cloud-functions/src/index.ts`:
- Line 20: The export statement "export * from './attendanceStreak'" in index.ts
is missing a trailing semicolon; update that line to match the other export
lines (lines 6-19) by adding a semicolon at the end so it reads export * from
'./attendanceStreak'; ensuring consistent export formatting across the file.
🪄 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: 0f60cb11-6524-4796-a21b-5d54c0c0b391
⛔ Files ignored due to path filters (16)
app/package-lock.jsonis excluded by!**/package-lock.jsoncloud-functions/lib/analyzeAttendance.js.mapis excluded by!**/*.mapcloud-functions/lib/certificateService.js.mapis excluded by!**/*.mapcloud-functions/lib/dailyDigest.js.mapis excluded by!**/*.mapcloud-functions/lib/eventNotifications.js.mapis excluded by!**/*.mapcloud-functions/lib/inactiveUsers.js.mapis excluded by!**/*.mapcloud-functions/lib/index.js.mapis excluded by!**/*.mapcloud-functions/lib/lib/participants.js.mapis excluded by!**/*.mapcloud-functions/lib/onEventCreate.js.mapis excluded by!**/*.mapcloud-functions/lib/reminders.js.mapis excluded by!**/*.mapcloud-functions/lib/reputation.js.mapis excluded by!**/*.mapcloud-functions/lib/server.js.mapis excluded by!**/*.mapcloud-functions/lib/setRole.js.mapis excluded by!**/*.mapcloud-functions/lib/utils/distance.js.mapis excluded by!**/*.mapcloud-functions/lib/utils/fraudScore.js.mapis excluded by!**/*.mapcloud-functions/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.gitignoreapp/App.jsapp/package.jsonapp/src/lib/AuthContext.jsapp/src/screens/ProfileScreen.jsapp/src/screens/StreakScreen.jscloud-functions/lib/analyzeAttendance.jscloud-functions/lib/certificateService.jscloud-functions/lib/dailyDigest.jscloud-functions/lib/eventNotifications.jscloud-functions/lib/index.jscloud-functions/lib/lib/participants.jscloud-functions/lib/onEventCreate.jscloud-functions/lib/reminders.jscloud-functions/lib/reputation.jscloud-functions/lib/server.jscloud-functions/lib/setRole.jscloud-functions/lib/utils/distance.jscloud-functions/lib/utils/fraudScore.jscloud-functions/src/attendanceStreak.tscloud-functions/src/auditLog.tscloud-functions/src/certificateService.tscloud-functions/src/dedicatedStudentCertificate.tscloud-functions/src/index.ts
💤 Files with no reviewable changes (13)
- cloud-functions/lib/onEventCreate.js
- cloud-functions/lib/reminders.js
- cloud-functions/lib/setRole.js
- cloud-functions/lib/analyzeAttendance.js
- cloud-functions/lib/utils/fraudScore.js
- cloud-functions/lib/index.js
- cloud-functions/lib/eventNotifications.js
- cloud-functions/lib/dailyDigest.js
- cloud-functions/lib/utils/distance.js
- cloud-functions/lib/server.js
- cloud-functions/lib/lib/participants.js
- cloud-functions/lib/certificateService.js
- cloud-functions/lib/reputation.js
Comment on lines
+208
to
+212
| {new Date(cert.awardedAt).toLocaleDateString('en-US', { | ||
| year: 'numeric', | ||
| month: 'long', | ||
| day: 'numeric', | ||
| })} |
Contributor
There was a problem hiding this comment.
Add safety check for missing awardedAt field.
If a certificate record lacks the awardedAt field, new Date(cert.awardedAt) will create an invalid date, displaying "Invalid Date" to users. Add optional chaining or a fallback to handle missing data gracefully.
🛡️ Proposed fix
<Text
style={[styles.certDate, { color: theme.colors.textSecondary }]}
>
- Awarded{' '}
- {new Date(cert.awardedAt).toLocaleDateString('en-US', {
- year: 'numeric',
- month: 'long',
- day: 'numeric',
- })}
+ {cert.awardedAt
+ ? `Awarded ${new Date(cert.awardedAt).toLocaleDateString('en-US', {
+ year: 'numeric',
+ month: 'long',
+ day: 'numeric',
+ })}`
+ : 'Awarded (date unknown)'}
</Text>🤖 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/StreakScreen.js` around lines 208 - 212, The certificate date
rendering in StreakScreen uses new Date(cert.awardedAt) which will produce
"Invalid Date" if awardedAt is missing; update the JSX that renders the date to
first check cert?.awardedAt (or test Boolean(cert.awardedAt)) and, if present,
format it with new Date(...).toLocaleDateString('en-US', {year:'numeric',
month:'long', day:'numeric'}), otherwise render a safe fallback string like '—'
or 'Date unavailable' so the UI never shows "Invalid Date".
Comment on lines
+11
to
+16
| const userId = event.data?.data()?.userId; | ||
|
|
||
| if (!userId) { | ||
| console.error("No userId in checkIn document"); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Extract userId from path parameters, not document data.
The function is triggered on path "events/{eventId}/checkIns/{userId}", so userId is available in event.params.userId. Reading it from the document data (event.data?.data()?.userId) is fragile and could fail if the document structure changes or if userId is missing from the data.
🔧 Proposed fix
- const userId = event.data?.data()?.userId;
+ const userId = event.params.userId;
if (!userId) {
- console.error("No userId in checkIn document");
+ console.error("No userId in path parameters");
return;
}📝 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 userId = event.data?.data()?.userId; | |
| if (!userId) { | |
| console.error("No userId in checkIn document"); | |
| return; | |
| } | |
| const userId = event.params.userId; | |
| if (!userId) { | |
| console.error("No userId in path parameters"); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/attendanceStreak.ts` around lines 11 - 16, The code
currently reads userId from event.data?.data()?.userId which is fragile; change
the extraction to use the path parameter event.params.userId instead (replace
the assignment to the userId variable where it is set and remove the fallback to
event.data?.data()?.userId), and update the early-return log to reflect missing
event.params.userId if absent; ensure all subsequent uses of userId in
attendanceStreak.ts refer to this parameter-derived variable.
Comment on lines
+44
to
+53
| if (diffMs < WEEK_MS) { | ||
| //already attended within current 7-day window | ||
| return; | ||
| } else if (diffMs < WEEK_MS * 2) { | ||
| //attended during the next consecutive week | ||
| newStreak = currentStreak + 1; | ||
| } else { | ||
| //missed a week -> streak resets to 1 | ||
| newStreak = 1; | ||
| } |
Contributor
There was a problem hiding this comment.
Fix boundary condition in consecutive week detection.
Using strict inequality (<) at line 47 causes a missed-week reset when attendance is exactly 14 days apart. A user attending precisely one week apart (e.g., every Monday at the same time) will eventually hit exactly WEEK_MS * 2 milliseconds, incorrectly resetting their streak instead of incrementing it.
🔧 Proposed fix
if (diffMs < WEEK_MS) {
//already attended within current 7-day window
return;
- } else if (diffMs < WEEK_MS * 2) {
+ } else if (diffMs <= WEEK_MS * 2) {
//attended during the next consecutive week
newStreak = currentStreak + 1;
} else {📝 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
| if (diffMs < WEEK_MS) { | |
| //already attended within current 7-day window | |
| return; | |
| } else if (diffMs < WEEK_MS * 2) { | |
| //attended during the next consecutive week | |
| newStreak = currentStreak + 1; | |
| } else { | |
| //missed a week -> streak resets to 1 | |
| newStreak = 1; | |
| } | |
| if (diffMs < WEEK_MS) { | |
| //already attended within current 7-day window | |
| return; | |
| } else if (diffMs <= WEEK_MS * 2) { | |
| //attended during the next consecutive week | |
| newStreak = currentStreak + 1; | |
| } else { | |
| //missed a week -> streak resets to 1 | |
| newStreak = 1; | |
| } |
🤖 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/attendanceStreak.ts` around lines 44 - 53, The boundary
check wrongly treats diffMs == WEEK_MS * 2 as a missed week; update the
consecutive-week branch by changing the condition in the else-if that references
diffMs and WEEK_MS (the branch that sets newStreak = currentStreak + 1) from a
strict < to a <= so that diffMs == WEEK_MS * 2 is counted as consecutive; keep
the earlier diffMs < WEEK_MS check and the newStreak/currentStreak logic
unchanged.
Comment on lines
+113
to
+118
| const [url] = await file.getSignedUrl({ | ||
| action: "read", | ||
| expires: "2499-12-31", | ||
| }); | ||
| signedUrl = url; | ||
| } |
Contributor
There was a problem hiding this comment.
Signed URL expiry date is excessively long, creating a permanent access link.
The expiry date "2499-12-31" (almost 500 years from now) effectively makes the certificate URL permanent. If a certificate needs to be revoked or if a user's privacy settings change, there's no way to invalidate the URL. Consider a shorter expiry (e.g., 5-10 years) or implement a URL-refresh mechanism.
🔒 Proposed fix with reasonable expiry
const [url] = await file.getSignedUrl({
action: "read",
- expires: "2499-12-31",
+ expires: Date.now() + 10 * 365 * 24 * 60 * 60 * 1000, // 10 years
});📝 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 [url] = await file.getSignedUrl({ | |
| action: "read", | |
| expires: "2499-12-31", | |
| }); | |
| signedUrl = url; | |
| } | |
| const [url] = await file.getSignedUrl({ | |
| action: "read", | |
| expires: Date.now() + 10 * 365 * 24 * 60 * 60 * 1000, // 10 years | |
| }); | |
| signedUrl = url; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/dedicatedStudentCertificate.ts` around lines 113 - 118,
The signed URL for the certificate is set to expire at "2499-12-31", effectively
permanent; update the call to file.getSignedUrl (and the signedUrl assignment)
to use a reasonable expiry (e.g., generate an expires value programmatically for
5–10 years from now or read a config value) so the URL can be invalidated later,
or implement a URL refresh mechanism that issues short-lived signed URLs
on-demand; ensure you change the literal expires string to a computed/managed
expiry and document the chosen duration.
Comment on lines
+121
to
+128
| const now = Timestamp.fromDate(new Date()); | ||
| await db.collection("users").doc(userId).update({ | ||
| certificates: FieldValue.arrayUnion({ | ||
| type: "dedicated_student", | ||
| awardedAt: now.toDate().toISOString(), | ||
| certificateUrl: signedUrl, | ||
| }), | ||
| }); |
Contributor
There was a problem hiding this comment.
Store awardedAt as Firestore Timestamp for consistency and queryability.
Line 125 converts the timestamp to an ISO string (now.toDate().toISOString()), losing Firestore's native Timestamp type. This makes date-based queries less efficient and inconsistent with other timestamp fields like lastAttendanceAt in the user document.
📅 Proposed fix
const now = Timestamp.fromDate(new Date());
await db.collection("users").doc(userId).update({
certificates: FieldValue.arrayUnion({
type: "dedicated_student",
- awardedAt: now.toDate().toISOString(),
+ awardedAt: now,
certificateUrl: signedUrl,
}),
});🤖 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/dedicatedStudentCertificate.ts` around lines 121 - 128,
The certificates array is storing awardedAt as an ISO string; change it to store
a Firestore Timestamp so queries stay consistent and indexable: in the update
call on db.collection("users").doc(userId").update(...) where
FieldValue.arrayUnion(...) is used to push the certificate object (type:
"dedicated_student", awardedAt: ... , certificateUrl: signedUrl), replace
now.toDate().toISOString() with the Firestore Timestamp instance (e.g., now or
Timestamp.now()) so the awardedAt field is saved as a Timestamp like other
fields such as lastAttendanceAt.
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.
Description
Implements weekly attendance streak tracking for students. Every time a student checks in to an event, their streak is updated. Reaching a 4-week consecutive streak awards them a "Dedicated Student" certificate delivered via email.
Fixes #57
Type of change
How Has This Been Tested?
All 4 streak scenarios were verified locally using the Firebase emulator suite. To reproduce:
Checklist:
Summary by CodeRabbit
New Features
Updates