feat: add automated email template testing and preview (#337)#427
Merged
roshankumar0036singh merged 15 commits intoJun 1, 2026
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 43 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds template discovery/rendering and preview, a typed sendEmail utility and tests, unified certificate email sending (Buffer|URL) with per-participant outcomes and event-summary persistence, server auth and rate-limit middleware plus template-preview endpoints, batched push processing with failure tracking, reputation nullish-coalescing fixes, templates, and build/lint config updates. ChangesCloud Functions Platform Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds developer tooling for previewing EmailJS HTML templates and introduces utilities for rendering/sending templated emails, alongside several backend runtime refactors (rate limiting, push delivery, digest paging, certificate sending).
Changes:
- Add a filesystem-based email template renderer with sample-data defaults and Jest tests.
- Add developer-facing Express endpoints to list and preview templates with query-param overrides.
- Update email/certificate/push/digest backend logic (mostly in
cloud-functions/lib/**) and adjust TypeScript config for Node/Jest types.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| cloud-functions/tsconfig.json | Adds Node/Jest typings and excludes test files from compilation. |
| cloud-functions/src/utils/emailTemplateRenderer.ts | Implements template discovery, sample data, and placeholder replacement. |
| cloud-functions/src/utils/emailTemplateRenderer.test.ts | Adds Jest tests for renderer behavior against real HTML templates. |
| cloud-functions/src/utils/emailSender.ts | Adds EmailJS send/dry-run helper built on template rendering. |
| cloud-functions/src/server.ts | Adds /email-preview and /email-preview/:templateName developer preview endpoints. |
| cloud-functions/lib/server.js | Adds rate limiting middleware and compiles in email preview endpoints. |
| cloud-functions/lib/** | Various compiled JS changes (push refactor, digest paging, certificate handling, exports). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cloud-functions/lib/server.js (1)
319-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
digestOptInin the manual digest route too.
cloud-functions/lib/dailyDigest.jsnow skips users withdigestOptIn === false, but this handler still creates notifications and push messages for every user. Admin-triggered digests sent through/api/sendDailyDigestwill ignore opt-out preferences unless this path is aligned with the shared implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/lib/server.js` around lines 319 - 345, The manual digest loop currently creates in-app notifications and push messages for every user; update the usersSnapshot.forEach handler to respect the user's digestOptIn by skipping users where userData.digestOptIn === false before creating notifRef/batch.set or pushing to messages; in other words, add a guard using userData.digestOptIn (treat undefined as opted-in) at the top of the forEach callback so that notifRef, batch.set, and messages.push only run for users who have not explicitly opted out.
🧹 Nitpick comments (1)
cloud-functions/lib/certificateService.js (1)
266-289: ⚖️ Poor tradeoffPer-participant pipeline runs fully sequentially.
Each participant is processed one at a time through PDF generation → storage upload → Firestore write → email send. For events with many participants this serial loop can be slow and risks exceeding the HTTP request/function timeout. Consider processing in bounded-concurrency batches (e.g.
p-limitor chunkedPromise.allSettled) to improve throughput while protecting Storage/Resend rate limits.🤖 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/certificateService.js` around lines 266 - 289, The current for-loop processes participants sequentially (calling processParticipant and handleExistingCertificateParticipant), causing slow execution and timeout risk; refactor the participant processing to run with bounded concurrency (e.g., use a p-limit instance or chunk participants and run Promise.allSettled per chunk) so that calls to processParticipant and handleExistingCertificateParticipant execute in parallel up to a safe limit, collect outcomes into the same results array, and then preserve the existing success check that updates events (certificatesSent / certificatesSentAt) only when all results are successful; ensure concurrency limits respect Storage/Resend rate limits and maintain idempotency semantics used in the current loop.
🤖 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/certificateService.js`:
- Around line 109-114: The uploadPdfAndGetUrl function currently requests an
unbounded long-lived signed URL (expires: '2499-12-31'); update the source in
cloud-functions/src/certificateService.ts to generate short-lived V4 signed URLs
instead: call file.getSignedUrl with { version: 'v4', action: 'read', expires:
<boundedExpiry> } where <boundedExpiry> is at most 7 days from now (preferably
much shorter, e.g. 15–60 minutes) or use a Date object/epoch seconds
accordingly; alternatively, change downstream logic to serve the PDF via
authenticated access and remove public signed URLs so participant PII isn’t
exposed. Ensure you modify the uploadPdfAndGetUrl implementation and any callers
that expect the long-lived URL to instead refresh or request short-lived URLs on
demand.
In `@cloud-functions/lib/dailyDigest.js`:
- Around line 106-124: When sendPushNotifications(pageMessages) fails inside the
loop, don't swallow the error — either rethrow it or track failures and return a
non-success result; modify the loop in the daily digest routine that calls
sendPushNotifications so that on catch you increment a failedPushCount (or set
success = false) and include that in the final return (e.g., return { success:
false, count, processed: processedCount, failedPushes: failedPushCount }), or
simply rethrow the caught error after logging so the caller sees the failure;
update the catch block that currently calls functions.logger.error(...) to
implement one of these behaviors referencing sendPushNotifications,
pageMessages, processedCount, count and usersSnapshot.
In `@cloud-functions/lib/eventNotifications.js`:
- Around line 41-49: getUpcomingEvents currently computes a tight 10–11 minute
ISO range from Date.now(), which can miss events if the scheduler drifts; update
the startRange/endRange computation in getUpcomingEvents to either round
Date.now() to minute boundaries (e.g., floor to the previous minute for start
and ceil for end) or expand the window by a small buffer (e.g., start = now +
9m30s, end = now + 11m30s) and keep the existing dedupe logic using the
notified10Min field; modify the startRange/endRange variables and the query that
uses them accordingly so the query fetches the slightly wider/rounded interval.
In `@cloud-functions/lib/reputation.js`:
- Around line 73-76: The code uses || to derive attendanceCount,
registrationCount, and remindersSet which treats 0 as missing; update the
fallbacks to use nullish coalescing (??) or explicit undefined checks when
reading from userData.reputation (e.g., compute attendanceCount,
registrationCount, remindersSet by checking userData.reputation?.attendanceCount
?? userData.attendanceCount ?? 0) so that legitimate zero values are preserved
before calling calculatePoints; adjust the three variable assignments
accordingly to ensure calculatePoints receives the correct numeric counters.
In `@cloud-functions/lib/server.js`:
- Around line 230-291: The preview route app.get('/email-preview/:templateName')
currently injects unescaped templateName, varsJson, err.message and rendered
html (from renderTemplate) directly into the response, enabling reflected XSS;
fix by HTML-escaping templateName and any strings produced from req.query
(overrides), varsJson and err.message before interpolation, and stop inlining
raw rendered html — instead serve it inside a sandboxed iframe (use <iframe
sandbox="allow-same-origin"> with the template placed via a sanitized srcdoc or
a server-side sanitization step) or run the rendered html through a strict
sanitizer before embedding; ensure renderTemplate and getSampleData outputs are
treated as untrusted and escaped/sanitized prior to insertion.
In `@cloud-functions/src/server.ts`:
- Around line 222-288: The preview endpoint is vulnerable to reflected XSS
because templateName, query overrides (used via renderTemplate and varsJson),
and err.message are injected into the HTML without escaping; fix by
HTML-escaping or context-encoding all user-controlled values before injecting
into the wrapper and inside renderTemplate: ensure renderTemplate performs
context-aware escaping for text nodes vs attribute/URL contexts (escape
templateName used in <title> and <h2>, escape overrides when building varsJson
or render a safe JSON viewer, and sanitize err.message before sending the 404
body), and additionally add an environment check in the
/email-preview/:templateName route to disable or protect this endpoint in
production (e.g., require an admin flag or NODE_ENV !== 'production').
In `@cloud-functions/src/utils/emailSender.ts`:
- Around line 94-99: Add an AbortController-based timeout around the EmailJS
fetch call: create an AbortController, set a timer (e.g., const timeout =
setTimeout(() => controller.abort(), <ms>)) and pass controller.signal into the
fetch options for the POST to 'https://api.emailjs.com/api/v1.0/email/send' (the
same call that uses payload and returns response), then clearTimeout(timeout) in
a finally block so the timer is always cleared; apply the same pattern to other
EmailJS fetch call sites in this module.
- Around line 83-99: sendEmail currently renders renderedHtml from
docs/EmailJs_Template using templateName but sends only template_id
(process.env.EMAILJS_TEMPLATE_ID or "template_general") to EmailJS, causing
dry-run HTML to diverge and EmailJS rejection if you try to POST raw HTML; fix
by aligning the local dry-run rendering with the actual template_id used in the
live send: either derive template_id from templateName (map templateName ->
template_id) or load the local template that corresponds to
process.env.EMAILJS_TEMPLATE_ID so the renderedHtml matches what EmailJS will
render, and do not attempt to include raw HTML in the POST body (keep using
template_id + template_params for the fetch to
https://api.emailjs.com/api/v1.0/email/send).
- Around line 83-92: The current payload object (payload with service_id:
serviceId, template_id: templateId, user_id: publicKey, template_params: {...})
only sends the public `user_id` and will fail for EmailJS accounts that require
private-key authorization; update the code that builds `payload` to
conditionally include `accessToken` (e.g., from a configured secret like
EMAILJS_ACCESS_TOKEN) when present or when account is set to strict/private-key
mode, and confirm the account has “Allow EmailJS API for non-browser
applications” enabled if you intend to use only `user_id`.
In `@cloud-functions/src/utils/emailTemplateRenderer.ts`:
- Line 8: TEMPLATES_DIR currently points at a repo-root docs folder which is not
packaged into the Firebase Functions deploy, causing getAvailableTemplates() and
renderTemplate() to fail at runtime; fix by moving or copying EmailJs_Template
into the functions bundle and updating the constant: place the templates under
the cloud-functions package (e.g., cloud-functions/templates), update const
TEMPLATES_DIR in emailTemplateRenderer.ts to path.resolve(__dirname,
'./templates') (or the correct relative path inside the deployed bundle), and
ensure your build/CI or firebase.json deploy settings include that templates
directory so the files are present at runtime.
---
Outside diff comments:
In `@cloud-functions/lib/server.js`:
- Around line 319-345: The manual digest loop currently creates in-app
notifications and push messages for every user; update the usersSnapshot.forEach
handler to respect the user's digestOptIn by skipping users where
userData.digestOptIn === false before creating notifRef/batch.set or pushing to
messages; in other words, add a guard using userData.digestOptIn (treat
undefined as opted-in) at the top of the forEach callback so that notifRef,
batch.set, and messages.push only run for users who have not explicitly opted
out.
---
Nitpick comments:
In `@cloud-functions/lib/certificateService.js`:
- Around line 266-289: The current for-loop processes participants sequentially
(calling processParticipant and handleExistingCertificateParticipant), causing
slow execution and timeout risk; refactor the participant processing to run with
bounded concurrency (e.g., use a p-limit instance or chunk participants and run
Promise.allSettled per chunk) so that calls to processParticipant and
handleExistingCertificateParticipant execute in parallel up to a safe limit,
collect outcomes into the same results array, and then preserve the existing
success check that updates events (certificatesSent / certificatesSentAt) only
when all results are successful; ensure concurrency limits respect
Storage/Resend rate limits and maintain idempotency semantics used in the
current loop.
🪄 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: c4404b27-7810-4204-9957-c18f9e82da28
📒 Files selected for processing (18)
cloud-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/server.tscloud-functions/src/utils/emailSender.tscloud-functions/src/utils/emailTemplateRenderer.test.tscloud-functions/src/utils/emailTemplateRenderer.tscloud-functions/tsconfig.json
💤 Files with no reviewable changes (6)
- cloud-functions/lib/utils/distance.js
- cloud-functions/lib/setRole.js
- cloud-functions/lib/analyzeAttendance.js
- cloud-functions/lib/onEventCreate.js
- cloud-functions/lib/lib/participants.js
- cloud-functions/lib/utils/fraudScore.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cloud-functions/src/utils/emailTemplateRenderer.ts (1)
102-115:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse context-aware sanitization for URL and CSS placeholders.
escapeHtml()is only safe for text/attribute escaping. These templates also inject placeholders intohref="{{cert_url}}",href="{{event_link}}", andstyle="display: {{...}}", so values likejavascript:...orblock;position:fixed;...still pass through as active URL/CSS content. This needs per-field allowlists/validators rather than one generic escaper.🤖 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/utils/emailTemplateRenderer.ts` around lines 102 - 115, The current generic escapeHtml used in the replacement callback doesn't prevent dangerous URL/CSS values; update the placeholder handling in the replacement function (the regex replace that references mergedData and escapeHtml) to perform context-aware sanitization: for URL placeholders like cert_url and event_link validate/allowlist schemes (e.g., https:, mailto:) and whitelist host patterns or absolute/relative path formats, rejecting or neutralizing anything else; for style/display placeholders only allow a small set of safe values (e.g., "block","inline","none","inline-block") or map booleans to safe styles; continue to use escapeHtml for plain text placeholders but route known attribute or style keys through their respective validators before returning the replacement.
🤖 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/src/certificateService.ts`:
- Around line 120-121: The code currently persists an expiring GCS V4 signed URL
(produced by uploadPdfAndGetUrl()) into participant.certificateUrl and then
reuses it in handleExistingCertificateParticipant(), causing expired links to be
sent; change persistCertificateUrl() to store the durable storage object path
(e.g., "certificates/${eventId}/${participantId}.pdf" or the value of file.name)
instead of the signed URL, keep uploadPdfAndGetUrl() to return the uploaded
file/object reference, and update send/resend logic to call file.getSignedUrl({
action: 'read', expires: ... , version: 'v4' }) at send time to generate a fresh
signed URL instead of reading participant.certificateUrl directly.
In `@cloud-functions/src/dailyDigest.ts`:
- Around line 106-108: The current return uses success: false when failedPushes
> 0 which signals a retryable failure even though notifications were already
persisted; change the return to indicate partial success instead (for example
return { success: true, partial: true, count, processed: processedCount,
failedPushes } or at minimum success: true with failedPushes included) so
callers won't retry and create duplicates; keep the failedPushes and
processedCount fields but flip the success/partial semantics in the failing
branch (without adding idempotency logic).
In `@cloud-functions/src/utils/emailSender.test.ts`:
- Around line 7-20: The test suite replaces the global.fetch with mockFetch and
never restores it; save the original global.fetch before assigning mockFetch
(e.g., const originalFetch = global.fetch) and restore it in the afterAll block
(use global.fetch = originalFetch) so other tests are not affected; update the
file's top-level setup where mockFetch and global.fetch are assigned and modify
the afterAll cleanup to restore both process.env and global.fetch, referencing
mockFetch, global.fetch, beforeEach, and afterAll to locate the code.
In `@cloud-functions/src/utils/emailTemplateRenderer.ts`:
- Around line 98-100: renderTemplate currently always merges SAMPLE_DATA
(getSampleData(templateName)), causing live sendEmail flows to silently include
preview values; make sample-data fallback opt-in by adding an options parameter
(e.g., { useSampleData?: boolean } default false) to renderTemplate and only
merge getSampleData(templateName) when useSampleData is true; update any preview
endpoints to call renderTemplate(..., { useSampleData: true }) and leave
sendEmail() and its callers unchanged so live sends fail on missing fields;
adjust function signatures for renderTemplate and any internal callers (and
add/modify tests) to reflect the new option.
---
Duplicate comments:
In `@cloud-functions/src/utils/emailTemplateRenderer.ts`:
- Around line 102-115: The current generic escapeHtml used in the replacement
callback doesn't prevent dangerous URL/CSS values; update the placeholder
handling in the replacement function (the regex replace that references
mergedData and escapeHtml) to perform context-aware sanitization: for URL
placeholders like cert_url and event_link validate/allowlist schemes (e.g.,
https:, mailto:) and whitelist host patterns or absolute/relative path formats,
rejecting or neutralizing anything else; for style/display placeholders only
allow a small set of safe values (e.g., "block","inline","none","inline-block")
or map booleans to safe styles; continue to use escapeHtml for plain text
placeholders but route known attribute or style keys through their respective
validators before returning the replacement.
🪄 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: 3c7f23c9-d3c7-4a35-9bad-215feda30de8
📒 Files selected for processing (20)
app/src/screens/LocationHeatmapScreen.jsapp/src/screens/UserFeed.jscloud-functions/lib/certificateService.jscloud-functions/lib/dailyDigest.jscloud-functions/lib/eventNotifications.jscloud-functions/lib/reputation.jscloud-functions/lib/server.jscloud-functions/src/certificateService.tscloud-functions/src/dailyDigest.tscloud-functions/src/eventNotifications.tscloud-functions/src/reputation.tscloud-functions/src/server.tscloud-functions/src/utils/emailSender.test.tscloud-functions/src/utils/emailSender.tscloud-functions/src/utils/emailTemplateRenderer.test.tscloud-functions/src/utils/emailTemplateRenderer.tscloud-functions/templates/certificate_email_template.htmlcloud-functions/templates/feedback_email_template.htmlcloud-functions/templates/universal_email_template.htmlcloud-functions/tsconfig.json
✅ Files skipped from review due to trivial changes (2)
- cloud-functions/templates/feedback_email_template.html
- app/src/screens/LocationHeatmapScreen.js
🚧 Files skipped from review as they are similar to previous changes (6)
- cloud-functions/lib/eventNotifications.js
- cloud-functions/lib/dailyDigest.js
- cloud-functions/src/utils/emailTemplateRenderer.test.ts
- cloud-functions/lib/reputation.js
- cloud-functions/src/utils/emailSender.ts
- cloud-functions/lib/server.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cloud-functions/src/utils/emailTemplateRenderer.test.ts (1)
68-128: ⚡ Quick winAdd an escaping regression test.
These cases prove placeholder replacement, but they do not assert that unsafe input is encoded. A future change that weakens
escapeHtml()would still pass this suite.Suggested test
describe('renderTemplate()', () => { + it('escapes HTML in injected values', () => { + const html = renderTemplate('feedback_email_template', { + to_name: '<script>alert(1)</script>', + }); + + expect(html).toContain('<script>alert(1)</script>'); + expect(html).not.toContain('<script>alert(1)</script>'); + }); + it('should replace placeholders with provided data', () => { const html = renderTemplate('feedback_email_template', { to_name: 'TestUser',🤖 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/utils/emailTemplateRenderer.test.ts` around lines 68 - 128, Add an escaping regression test that ensures renderTemplate/escapeHtml still HTML-encodes unsafe input: call renderTemplate (e.g., with 'feedback_email_template') passing a malicious value like "<script>alert(1)</script>" or strings with "&" for a field such as to_name, then assert the returned html contains the escaped form (e.g., "<script>alert(1)</script>" and "&") and does NOT contain the raw "<", ">", or "&" for that variable; also keep the existing expectNoPlaceholders check to ensure placeholders are replaced.
🤖 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.
Nitpick comments:
In `@cloud-functions/src/utils/emailTemplateRenderer.test.ts`:
- Around line 68-128: Add an escaping regression test that ensures
renderTemplate/escapeHtml still HTML-encodes unsafe input: call renderTemplate
(e.g., with 'feedback_email_template') passing a malicious value like
"<script>alert(1)</script>" or strings with "&" for a field such as to_name,
then assert the returned html contains the escaped form (e.g.,
"<script>alert(1)</script>" and "&") and does NOT contain the
raw "<", ">", or "&" for that variable; also keep the existing
expectNoPlaceholders check to ensure placeholders are replaced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 920dab10-16cf-46e0-a786-26624111c59d
📒 Files selected for processing (19)
cloud-functions/lib/analyzeAttendance.jscloud-functions/lib/certificateService.jscloud-functions/lib/dailyDigest.jscloud-functions/lib/eventNotifications.jscloud-functions/lib/lib/participants.jscloud-functions/lib/reminders.jscloud-functions/lib/reputation.jscloud-functions/lib/server.jscloud-functions/src/certificateService.tscloud-functions/src/dailyDigest.tscloud-functions/src/server.tscloud-functions/src/utils/emailSender.test.tscloud-functions/src/utils/emailTemplateRenderer.test.tscloud-functions/src/utils/emailTemplateRenderer.tscloud-functions/templates/certificate_email_template.htmlcloud-functions/templates/feedback_email_template.htmlcloud-functions/templates/universal_email_template.htmlcloud-functions/tsconfig.jsonpackage.json
🚧 Files skipped from review as they are similar to previous changes (12)
- cloud-functions/tsconfig.json
- cloud-functions/templates/universal_email_template.html
- cloud-functions/templates/feedback_email_template.html
- cloud-functions/lib/reminders.js
- cloud-functions/src/dailyDigest.ts
- cloud-functions/templates/certificate_email_template.html
- cloud-functions/src/utils/emailSender.test.ts
- cloud-functions/src/certificateService.ts
- cloud-functions/lib/reputation.js
- cloud-functions/lib/eventNotifications.js
- cloud-functions/lib/dailyDigest.js
- cloud-functions/lib/certificateService.js
Contributor
Author
|
@roshankumar0036singh Could you please review this |
Owner
|
certificatesSent only marks true if 100% succeed — partial success is silently lost fix them to handle it |
Owner
|
The old expo instance was removed, but Expo is still imported. why though ? |
Owner
|
reputation.js has leftover merge conflict artifacts |
Owner
|
Signed URL expiry is now 7 days — certificates become inaccessible |
Owner
|
emailSender.ts uses EmailJS but certificateService.ts uses Resend |
Owner
|
dailyDigest.ts exports sendDailyDigest as an onCall function but server.ts duplicates the logic as an Express route |
Owner
|
tsconfig.json excludes *.test.ts from compilation |
Owner
|
niversal_email_template.html missing newline at end of file |
Owner
|
@Shriraj888 fix everything |
Contributor
Author
|
@roshankumar0036singh Thanks for the review. I've addressed all the comments and pushed the changes. Could you please review it again when you have a moment? |
Owner
|
conflcit |
…email-template-testing-and-preview # Conflicts: # app/src/screens/UserFeed.js # cloud-functions/src/certificateService.ts # cloud-functions/src/server.ts # cloud-functions/tsconfig.json
Contributor
Author
|
@roshankumar0036singh I've resolved the conflicts and pushed the updated changes. Whenever you get a chance, I'd appreciate a final review. Thanks! |
Owner
|
@Shriraj888 just fix this and everything is good tsconfig.build.json excludes tests but tsconfig.json still includes them |
Owner
|
@Shriraj888 solve the conflcit carefully and also there are 5 comment left to reolsve |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cloud-functions/src/certificateService.ts (1)
120-135:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStop storing a durable certificate link as a signed URL.
This still writes a bearer URL into
participant.certificateUrl, and the resend path reuses that stored value later. That makes certificate access live as long as the stored URL does, instead of being minted per delivery. Persist the storage object path here and generate a fresh short-lived signed URL when emailing/resending.🤖 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 120 - 135, The code currently persists a long-lived signed URL into participant.certificateUrl; instead, change persistCertificateUrl (and callers) to save the storage object path (e.g., the same storagePath used in uploadPdfAndGetUrl) into participant.certificatePath (or overwrite certificateUrl with the object path) and the timestamp, and remove storing the signedUrl; update uploadPdfAndGetUrl to return the storagePath (or ensure caller has it) and leave signed URL generation to the email/resend flow (generate a short-lived signed URL from the saved storage path when sending or resending certificates). Ensure references to participant.certificateUrl in resend/email logic are replaced to fetch the stored object path and call file.getSignedUrl at send time.
🧹 Nitpick comments (2)
cloud-functions/package.json (2)
4-4: ⚡ Quick win
tsc -p tsconfig.build.jsonis safe; remove redundant@types/dotenv
cloud-functions/tsconfig.build.jsonexists, so thebuildscript should not fail due to a missing file.@types/dotenvindependenciesis redundant withdotenvv17 (it ships its own TypeScript types); remove it (or move it todevDependenciesif you truly need it).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/package.json` at line 4, The build script "build": "tsc -p tsconfig.build.json" is valid because tsconfig.build.json exists, so leave that script unchanged; remove the redundant "`@types/dotenv`" entry from package.json dependencies (or move it to devDependencies if you need it only for type-checking) since dotenv v17 includes its own types—update the "dependencies" section to delete "`@types/dotenv`" and run npm install to update lockfile.
25-25: 💤 Low valueRemove misplaced
@types/dotenv;dotenv@^17.2.3already ships its own types."`@types/dotenv`": "^6.1.1",
@types/dotenvis deprecated and unnecessary here becausedotenv@^17.2.3includes TypeScript declarations. Drop it entirely, or (if you must keep it) move it todevDependenciesto avoid redundant/misplaced typing packages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/package.json` at line 25, The package.json currently lists "`@types/dotenv`": "^6.1.1" as a dependency but dotenv@^17.2.3 already provides its own TypeScript types; remove the "`@types/dotenv`" entry from dependencies (or if you insist on keeping it, move it into devDependencies) so you don't ship deprecated/redundant typing packages; update package.json accordingly and run a quick npm install to ensure lockfile updates.
🤖 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/src/certificateService.ts`:
- Around line 365-374: The update currently sets certificatesSentAt to null on
partial/failing retries; change the payload so certificatesSentAt is only
written when allSucceeded is true (i.e., include certificatesSentAt:
FieldValue.serverTimestamp() when allSucceeded), and omit certificatesSentAt
from the update when not allSucceeded so the previous successful timestamp is
preserved; still update certificatesSent (set to allSucceeded),
certificatesLastAttemptAt (always set FieldValue.serverTimestamp()), and
certificateSummary as before. Reference: the
admin.firestore().collection('events').doc(eventId).update({...}) call and the
variables allSucceeded, summary, certificatesSentAt, certificatesLastAttemptAt,
certificatesSent.
- Around line 8-10: The getResendClient function uses a default placeholder API
key which hides misconfiguration; change it to read process.env.RESEND_API_KEY,
and if it's missing/empty throw a clear startup error (e.g., new
Error("RESEND_API_KEY is required")) before constructing new Resend(API_KEY) so
the application fails fast; update the Resend construction in getResendClient to
use the validated API key.
In `@cloud-functions/src/utils/push.ts`:
- Around line 15-16: The function sendPushNotifications currently returns early
with undefined when messages.length === 0, while the normal path returns the
tickets array; change the no-op branch in sendPushNotifications to return an
empty array of tickets (e.g., return []) so callers always receive the same
shape; also apply the same fix to the similar early-return at the other no-op
branch referenced around line 36 to consistently return an empty tickets array
instead of undefined.
---
Duplicate comments:
In `@cloud-functions/src/certificateService.ts`:
- Around line 120-135: The code currently persists a long-lived signed URL into
participant.certificateUrl; instead, change persistCertificateUrl (and callers)
to save the storage object path (e.g., the same storagePath used in
uploadPdfAndGetUrl) into participant.certificatePath (or overwrite
certificateUrl with the object path) and the timestamp, and remove storing the
signedUrl; update uploadPdfAndGetUrl to return the storagePath (or ensure caller
has it) and leave signed URL generation to the email/resend flow (generate a
short-lived signed URL from the saved storage path when sending or resending
certificates). Ensure references to participant.certificateUrl in resend/email
logic are replaced to fetch the stored object path and call file.getSignedUrl at
send time.
---
Nitpick comments:
In `@cloud-functions/package.json`:
- Line 4: The build script "build": "tsc -p tsconfig.build.json" is valid
because tsconfig.build.json exists, so leave that script unchanged; remove the
redundant "`@types/dotenv`" entry from package.json dependencies (or move it to
devDependencies if you need it only for type-checking) since dotenv v17 includes
its own types—update the "dependencies" section to delete "`@types/dotenv`" and
run npm install to update lockfile.
- Line 25: The package.json currently lists "`@types/dotenv`": "^6.1.1" as a
dependency but dotenv@^17.2.3 already provides its own TypeScript types; remove
the "`@types/dotenv`" entry from dependencies (or if you insist on keeping it,
move it into devDependencies) so you don't ship deprecated/redundant typing
packages; update package.json accordingly and run a quick npm install to ensure
lockfile updates.
🪄 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: f668bee3-2001-425e-880e-1895545334c5
📒 Files selected for processing (23)
app/src/screens/UserFeed.jscloud-functions/lib/analyzeAttendance.jscloud-functions/lib/certificateService.jscloud-functions/lib/dailyDigest.jscloud-functions/lib/index.jscloud-functions/lib/reminders.jscloud-functions/lib/reputation.jscloud-functions/lib/server.jscloud-functions/package.jsoncloud-functions/src/certificateService.tscloud-functions/src/dailyDigest.tscloud-functions/src/reputation.tscloud-functions/src/server.tscloud-functions/src/utils/emailSender.test.tscloud-functions/src/utils/emailSender.tscloud-functions/src/utils/emailTemplateRenderer.test.tscloud-functions/src/utils/push.tscloud-functions/templates/feedback_email_template.htmlcloud-functions/templates/universal_email_template.htmlcloud-functions/tsconfig.build.jsoncloud-functions/tsconfig.jsonpackage.jsonsonar-project.properties
✅ Files skipped from review due to trivial changes (1)
- cloud-functions/tsconfig.build.json
🚧 Files skipped from review as they are similar to previous changes (9)
- app/src/screens/UserFeed.js
- cloud-functions/templates/universal_email_template.html
- cloud-functions/src/reputation.ts
- cloud-functions/templates/feedback_email_template.html
- cloud-functions/src/dailyDigest.ts
- cloud-functions/lib/reminders.js
- package.json
- cloud-functions/src/utils/emailTemplateRenderer.test.ts
- cloud-functions/lib/analyzeAttendance.js
roshankumar0036singh
previously approved these changes
May 31, 2026
Owner
|
@Shriraj888 handle the coderabbit suggestion |
|
Contributor
Author
|
@roshankumar0036singh I've addressed the CodeRabbit suggestions and updated the changes |
roshankumar0036singh
approved these changes
Jun 1, 2026
roshankumar0036singh
merged commit d53ed7e
into
roshankumar0036singh:main
6 checks passed
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
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
This change introduces automated email template testing and preview capabilities to the cloud-functions service in the UniEvent project.
Key modifications include:
Template Rendering Engine
Developed
emailTemplateRenderer.tsto:docs/EmailJs_Template/{{variable}}syntaxPreview Endpoints
Added the following routes in
server.ts:GET /email-previewLists all available email templates.
GET /email-preview/:templateNameRenders the selected template directly in the browser using fallback sample data and supports query parameter overrides.
Example:
Dry-Run Email Sender
Implemented
emailSender.tsto wrap email dispatching logic and support a{ dryRun: true }option. When enabled, the service returns the compiled HTML content instead of making external email API calls.Jest Typing & Build Configuration
Updated
tsconfig.jsonto:Dependencies
No additional runtime dependencies were required. Changes rely on the existing TypeScript and Jest tooling already present in the project.
Fixes #337
Type of change
How Has This Been Tested?
The modifications have been verified through compiler checks and Jest unit tests.
Unit Tests
Created
emailTemplateRenderer.test.tscontaining 13 unit tests covering:Test command:
npm testResult:
Build Verification
Verified successful TypeScript compilation and build generation.
Build command:
Results:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests