tweak: prevent Gmail from threading data exports#6551
Conversation
Summary of ChangesHello @jaskfla, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where Gmail would thread multiple data export notification emails, making it difficult to distinguish between individual exports. By introducing a unique identifier in the email headers, each export notification will now appear as a separate conversation. Additionally, the changes include refactoring email utility functions for better modularity and maintainability, along with minor code modernizations and performance optimizations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the goal of preventing Gmail from threading data export emails by adding a unique X-Entity-Ref-ID header. The implementation is solid, propagating the necessary changes through the email sending logic and updating type definitions accordingly.
I've also noticed and commented on some other improvements:
- The refactoring to only query for necessary user fields is a great performance optimization.
- The switch to using an options object for
sendResponseAsEmailimproves code clarity. - A bug involving incorrect
awaitusage with synchronousfsmethods inExportSurveyDataHandler.jshas been fixed. I've added suggestions to further improve that piece of logic for better performance and robustness by using asynchronous file system APIs.
Overall, these are good changes that improve the codebase. My review comments focus on taking one of the fixes a step further to align with best practices for asynchronous Node.js applications.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
jaskfla
force-pushed
the
email-refactors
branch
2 times, most recently
from
afe4beb to
09ed23d
Compare
jaskfla
force-pushed
the
email-refactors
branch
from
09ed23d to
717b866
Compare
jaskfla
force-pushed
the
email-refactors
branch
from
2a415e4 to
3894e9c
Compare
jaskfla
force-pushed
the
email-refactors
branch
from
3894e9c to
047cd30
Compare
jaskfla
force-pushed
the
email-refactors
branch
from
047cd30 to
b3216c3
Compare
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
| await sendEmail(user.email, { | ||
| // Prevent threading in Gmail, even if subject is the same. (`emailAfterTimeout` is used for | ||
| // large data exports; grouping separate exports into a single thread is probably undesirable.) | ||
| headers: { 'X-Entity-Ref-ID': crypto.randomUUID() }, |
There was a problem hiding this comment.
[Bugs & Correctness] suggestion
crypto is used as a global without an explicit import. The Web Crypto API (globalThis.crypto) is only available as an unflagged global from Node.js v19+. While .node-version is set to latest, relying on the implicit global is fragile — an explicit import { randomUUID } from 'node:crypto' (and calling randomUUID() directly) makes the intent clear and is compatible with any Node.js version that supports randomUUID (v14.17.0+).
[Tupaia Conventions] suggestion
crypto.randomUUID() uses the global crypto object, which was only added as a Node.js global in v19. For older Node versions this will throw at runtime. Prefer an explicit import: import { randomUUID } from 'node:crypto'. This is consistent with how web-config-server now uses import fs from 'node:fs/promises' (explicit node: protocol imports) in the same PR.
| req: any, | ||
| res: any, | ||
| constructEmailFromResponse: ConstructEmailFromResponseT, | ||
| constructEmailFromResponse: ConstructEmailFromResponseT<typeof req>, |
There was a problem hiding this comment.
[Tupaia Conventions] suggestion
ConstructEmailFromResponseT<typeof req> where req: any — typeof req resolves to any, making the generic parameter meaningless and providing no additional type safety. Since req and res are already any in setupEmailResponse, the generic annotation is misleading. Either type req properly as Request throughout the function, or drop the generic and use ConstructEmailFromResponseT directly (matching its default T = unknown).
| (req: any, res: any, next: () => void) => { | ||
| const { respondWithEmailTimeout } = req.query; | ||
| (req: Request, res: Response, next: NextFunction) => { | ||
| const { respondWithEmailTimeout } = req.query as { respondWithEmailTimeout?: string }; |
There was a problem hiding this comment.
[Bugs & Correctness] suggestion
The type cast req.query as { respondWithEmailTimeout?: string } silently hides the case where the query parameter is supplied multiple times (e.g. ?respondWithEmailTimeout=5&respondWithEmailTimeout=10), which Express parses as string[]. Number.parseInt coerces an array to a string via .toString(), so ['5','10'] becomes '5,10' and parseInt silently returns 5, ignoring the second value without any error. A simple guard like const raw = req.query.respondWithEmailTimeout; if (Array.isArray(raw)) throw new Error(...) would prevent this silent mis-parse.
|
🦸 Review Hero Summary Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
|
jaskfla
force-pushed
the
email-refactors
branch
from
b3216c3 to
961c186
Compare
jaskfla
force-pushed
the
email-refactors
branch
2 times, most recently
from
841a5f8 to
4363212
Compare
| req: any, | ||
| res: any, | ||
| constructEmailFromResponse: ConstructEmailFromResponseT, | ||
| constructEmailFromResponse: ConstructEmailFromResponseT<typeof req>, |
There was a problem hiding this comment.
Generic type parameter bound to wrong variable
Low Severity
The generic T in ConstructEmailFromResponseT<T> types the responseBody parameter, but setupEmailResponse instantiates it as ConstructEmailFromResponseT<typeof req>, which semantically says "the response body has the same type as the request." This is incorrect — the response body and request are unrelated types. Currently harmless because req: any makes typeof req resolve to any, but if req is ever properly typed, this would incorrectly constrain responseBody to the request type.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4363212. Configure here.
jaskfla
force-pushed
the
email-refactors
branch
from
4363212 to
4c40f39
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5a93702. Configure here.
| }; | ||
| }; | ||
| interface EmailAfterTimeoutMailOptions | ||
| extends Pick<MailOptions, 'attachments' | 'subject' | 'templateContext'> {} |
There was a problem hiding this comment.
Email subject became optional weakening type safety
Low Severity
EmailAfterTimeoutMailOptions picks subject from MailOptions, which inherits it from nodemailer's Mail.Options where it's optional (subject?: string). The previous code explicitly required subject: string in the ConstructEmailFromResponseT return type. Future implementations of constructEmailFromResponse can now omit the subject without a TypeScript error, potentially resulting in emails sent without a subject line.
Reviewed by Cursor Bugbot for commit 5a93702. Configure here.
1 participant
🦸 Review Hero