Mail when new Feedback Submitted#153
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe changes add a Firestore trigger that formats newly created feedback documents and sends notification emails through Nodemailer and SMTP. SMTP credentials and feedback addresses are defined as secrets with exported getters. Nodemailer dependencies are added, and the trigger is re-exported from the functions entry point. Sequence Diagram(s)sequenceDiagram
participant Firestore
participant onFeedbackCreated
participant EnvSecrets
participant Nodemailer
participant SMTPService
Firestore->>onFeedbackCreated: feedback document created
onFeedbackCreated->>onFeedbackCreated: format subject and body
onFeedbackCreated->>EnvSecrets: read SMTP and email secrets
EnvSecrets-->>onFeedbackCreated: return configuration
onFeedbackCreated->>Nodemailer: create transporter and send mail
Nodemailer->>SMTPService: relay email
SMTPService-->>onFeedbackCreated: return send result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@functions/src/functions/onFeedbackCreated.ts`:
- Line 63: Guard against invalid SMTP port by validating the value returned from
getSmtpPort(): replace the direct Number(getSmtpPort()) usage (the const port
variable) with parsing and a sanity check (e.g., parseInt or Number, then
isFinite and >0 && <=65535); if invalid, either throw a clear configuration
error or fall back to a safe default port (e.g., 587) before passing it to
nodemailer createTransport/send logic so transporter always receives a valid
numeric port.
- Around line 84-88: The catch block in onFeedbackCreated currently only logs
errors for send failures (`logger.error(...)` referencing feedbackId) which
prevents Firebase from retrying; decide whether notifications are critical and
either rethrow the error (throw error) so Firebase will retry the function, or
if you want best-effort keep the swallow but add actionable monitoring (e.g.,
emit a metric/event or call an alerting helper) and document the choice in
onFeedbackCreated’s comments; implement the chosen approach by modifying the
catch block around the email send call to either rethrow the caught error or
invoke the monitoring/alerting helper and leave a comment explaining the
decision.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b7052d5-57b8-4d53-8855-f9400c2cdf6b
⛔ Files ignored due to path filters (1)
functions/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
functions/package.jsonfunctions/src/env.tsfunctions/src/functions/onFeedbackCreated.tsfunctions/src/index.ts
| } catch (error) { | ||
| logger.error( | ||
| `Failed to send feedback notification for ${feedbackId}: ${String(error)}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Silent failure may cause lost notifications.
When email sending fails, the error is logged but not rethrown. This means Firebase considers the function successful and will not retry. Feedback notifications could be silently lost with only a log entry.
Consider whether this is intentional:
- If notifications are critical: Rethrow the error so Firebase retries the function (with exponential backoff).
- If best-effort is acceptable: Add alerting/monitoring on these error logs so failures are actionable.
Option A: Rethrow to enable retries
} catch (error) {
logger.error(
`Failed to send feedback notification for ${feedbackId}: ${String(error)}`,
);
+ throw error; // Allow Firebase to retry
}Option B: Keep silent failure but document the decision
} catch (error) {
+ // Intentionally not rethrowing: avoid duplicate emails on transient SMTP failures.
+ // Monitor these logs for persistent failures.
logger.error(
`Failed to send feedback notification for ${feedbackId}: ${String(error)}`,
);
}📝 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.
| } catch (error) { | |
| logger.error( | |
| `Failed to send feedback notification for ${feedbackId}: ${String(error)}`, | |
| ); | |
| } | |
| } catch (error) { | |
| logger.error( | |
| `Failed to send feedback notification for ${feedbackId}: ${String(error)}`, | |
| ); | |
| throw error; // Allow Firebase to retry | |
| } |
| } catch (error) { | |
| logger.error( | |
| `Failed to send feedback notification for ${feedbackId}: ${String(error)}`, | |
| ); | |
| } | |
| } catch (error) { | |
| // Intentionally not rethrowing: avoid duplicate emails on transient SMTP failures. | |
| // Monitor these logs for persistent failures. | |
| logger.error( | |
| `Failed to send feedback notification for ${feedbackId}: ${String(error)}`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/functions/onFeedbackCreated.ts` around lines 84 - 88, The catch
block in onFeedbackCreated currently only logs errors for send failures
(`logger.error(...)` referencing feedbackId) which prevents Firebase from
retrying; decide whether notifications are critical and either rethrow the error
(throw error) so Firebase will retry the function, or if you want best-effort
keep the swallow but add actionable monitoring (e.g., emit a metric/event or
call an alerting helper) and document the choice in onFeedbackCreated’s
comments; implement the chosen approach by modifying the catch block around the
email send call to either rethrow the caught error or invoke the
monitoring/alerting helper and leave a comment explaining the decision.
There was a problem hiding this comment.
not a critcial component.
PSchmiedmayer
left a comment
PSchmiedmayer
left a comment
There was a problem hiding this comment.
@PaulGoldschmidt Looks solid; to be followed up once we have the SMPT credentials in place.
|
thanks; will include this if possible in release 4.0.2. |
|
because we can't send transactional emails currently with stanford's mail servers, we might have to postpone this feature for now. |
|
closing this for now, maybe adding this in the future if we have proper mail sending infra. |
|
sending infra is now available! 🎉 |
PSchmiedmayer
left a comment
PSchmiedmayer
left a comment
There was a problem hiding this comment.
Fine with merging this once tested, nice work @PaulGoldschmidt 🚀
|
will merge this now, test in Staging and then pack a new release after checking that everything works soon @PSchmiedmayer 🚀 |
2 participants
Mail when new Feedback Submitted
♻️ Current situation & Problem
@AJohn155 requested that he gets notified upon new feedback submitted. This PR aims to solve this by sending a mail to a given address upon
⚙️ Release Notes
I decided to use Nodemailer instead of the firebase provided firestore send email function which would need additional setup by garrick and offers no real benefit as we still need our own SMTP sender anyways.
📚 Documentation
N/A
✅ Testing
For testing I need SMTP credentials, @PSchmiedmayer could you provide me with some? Also need to add these before merging into GCP IAM as a secret.
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: