Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a newsletter subscription toggle to account email settings, client helpers for checking/unsubscribing, Mailjet-backed subscribe/check/unsubscribe server methods, parallelized fetch of subscription status, and a new localization key across six locale files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as EmailNotifications
participant Action as ClientAction
participant Server as serverMiscApi
participant Mailjet
User->>UI: toggle newsletter checkbox
UI->>Action: updateNewsletter / checkNewsletterSubscription
Action->>Server: subscribeToNewsletter / unsubscribeFromNewsletter / isNewsletterSubscribed
Server->>Mailjet: Mailjet REST API call (managecontact / getcontactslists)
Mailjet-->>Server: response
Server-->>Action: result (success/failure, status)
Action-->>UI: update state / show spinner or error
UI->>User: reflect subscription state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 4
🤖 Fix all issues with AI agents
In
`@front_end/src/app/`(main)/accounts/settings/notifications/components/email_notifications.tsx:
- Around line 33-46: The newsletter handler currently swallows API errors so
users get no feedback; wrap the logic inside handleNewsletterChange (the async
callback passed into useServerAction) in a try/catch and on failure call your UI
toast/error helper (e.g., toast.error or showErrorToast) to surface the Mailjet
error, only calling setNewsletterSubscribed(checked) on success; alternatively,
if useServerAction supports an onFinished/onError callback, pass a handler there
to show the error toast and keep the checkbox state unchanged — update the
references to handleNewsletterChange and the useServerAction invocation
accordingly.
In `@front_end/src/app/`(main)/accounts/settings/notifications/page.tsx:
- Around line 16-19: The Mailjet network error can throw inside Promise.all and
crash the notifications page; change the call so
serverMiscApi.isNewsletterSubscribed(currentUser.email) cannot throw—either
replace Promise.all with separate awaits and wrap the isNewsletterSubscribed
call in try/catch (defaulting to false on any exception) or use
Promise.allSettled and treat a rejected result for isNewsletterSubscribed as
false; update the logic around ServerPostsApi.getAllSubscriptions() and the
isNewsletterSubscribed handling so the posts fetch result still renders even if
the Mailjet call fails.
In `@front_end/src/app/`(main)/actions.ts:
- Around line 20-26: The server actions unsubscribeFromNewsletter,
checkNewsletterSubscription (and subscribeToNewsletter) currently accept a
client-supplied email which allows one user to act on another's account; change
them to ignore the passed email and instead fetch the authenticated user's email
server-side via ServerProfileApi.getMyProfile(), then call
serverMiscApi.unsubscribeFromNewsletter / serverMiscApi.isNewsletterSubscribed /
serverMiscApi.subscribeToNewsletter with that server-side email; ensure you
remove or ignore the function parameter and handle missing profile/email errors
appropriately.
In `@front_end/src/services/api/misc/misc.server.ts`:
- Around line 24-60: The isNewsletterSubscribed function assumes Mailjet returns
data.Data and calls data.Data.some(...) which will throw if Data is missing or
not an array; change the response handling to validate the shape before using
it: after parsing JSON (data) check that data && Array.isArray(data.Data) and
only then run the .some(...) predicate (using newsletterListId from parseInt as
before); if Data is missing or not an array, return false (optionally log the
unexpected response for debugging) so the function fails gracefully instead of
throwing.
🧹 Nitpick comments (2)
front_end/src/services/api/misc/misc.server.ts (2)
24-134: Extract repeated Mailjet auth boilerplate into a shared helper.The API key retrieval, validation, and Base64 auth encoding are duplicated across
isNewsletterSubscribed,unsubscribeFromNewsletter, andsubscribeToNewsletter. This makes maintenance harder and increases the chance of inconsistencies (e.g.,isNewsletterSubscribedsilently returnsfalseon missing config while the others throw).Consider extracting a private helper like
getMailjetConfig()that returns{ auth, listId }or throws.Example helper
private getMailjetConfig(): { auth: string; listId: string } { const apiKey = process.env.MAILJET_API_KEY; const secretKey = process.env.MAILJET_SECRET_KEY; const listId = process.env.MAILJET_NEWSLETTER_ID; if (!apiKey || !secretKey || !listId) { throw new Error( "Mailjet configuration is missing. Please set MAILJET_API_KEY, MAILJET_SECRET_KEY, and MAILJET_NEWSLETTER_ID environment variables." ); } const auth = Buffer.from(`${apiKey}:${secretKey}`).toString("base64"); return { auth, listId }; }
36-41: No timeout on external Mailjet API fetch calls.All three Mailjet fetch calls lack timeouts. If Mailjet is slow or unresponsive, this will block the server-side render (for the page) or server action indefinitely. Consider adding an
AbortSignal.timeout()or similar mechanism.Example
const response = await fetch(url, { method: "GET", headers: { Authorization: `Basic ${auth}`, }, + signal: AbortSignal.timeout(10_000), });
| const handleNewsletterChange = useCallback( | ||
| async (checked: boolean) => { | ||
| if (checked) { | ||
| await subscribeToNewsletter(user.email); | ||
| } else { | ||
| await unsubscribeFromNewsletter(user.email); | ||
| } | ||
| setNewsletterSubscribed(checked); | ||
| }, | ||
| [user.email] | ||
| ); | ||
| const [updateNewsletter, isNewsletterPending] = useServerAction( | ||
| handleNewsletterChange | ||
| ); |
There was a problem hiding this comment.
No error feedback to user when newsletter subscribe/unsubscribe fails.
If the Mailjet API call fails, useServerAction catches the error and logs it to the console, but the user sees only the spinner disappear with no indication that the action failed. The checkbox state correctly stays unchanged (since setNewsletterSubscribed won't execute), but a silent failure is a poor UX.
Consider adding an onFinished callback to useServerAction or a try/catch within handleNewsletterChange to show an error toast on failure.
🤖 Prompt for AI Agents
In
`@front_end/src/app/`(main)/accounts/settings/notifications/components/email_notifications.tsx
around lines 33 - 46, The newsletter handler currently swallows API errors so
users get no feedback; wrap the logic inside handleNewsletterChange (the async
callback passed into useServerAction) in a try/catch and on failure call your UI
toast/error helper (e.g., toast.error or showErrorToast) to surface the Mailjet
error, only calling setNewsletterSubscribed(checked) on success; alternatively,
if useServerAction supports an onFinished/onError callback, pass a handler there
to show the error toast and keep the checkbox state unchanged — update the
references to handleNewsletterChange and the useServerAction invocation
accordingly.
front_end/src/app/(main)/actions.ts
Outdated
| export async function unsubscribeFromNewsletter(email: string) { | ||
| return await serverMiscApi.unsubscribeFromNewsletter(email); | ||
| } | ||
|
|
||
| export async function checkNewsletterSubscription(email: string) { | ||
| return await serverMiscApi.isNewsletterSubscribed(email); | ||
| } |
There was a problem hiding this comment.
Security: Server actions accept arbitrary email, enabling unsubscribing other users.
These server actions take email as a client-supplied parameter. Since "use server" functions are callable by any authenticated (or unauthenticated) client, a malicious user can call unsubscribeFromNewsletter with another user's email to unsubscribe them from the newsletter.
Instead, fetch the authenticated user's email server-side (e.g., via ServerProfileApi.getMyProfile()) and ignore the client-supplied email. This also applies to the pre-existing subscribeToNewsletter.
Proposed fix
+import ServerProfileApi from "@/services/api/profile/profile.server";
+
-export async function unsubscribeFromNewsletter(email: string) {
- return await serverMiscApi.unsubscribeFromNewsletter(email);
+export async function unsubscribeFromNewsletter() {
+ const user = await ServerProfileApi.getMyProfile();
+ if (!user) throw new Error("Not authenticated");
+ return await serverMiscApi.unsubscribeFromNewsletter(user.email);
}
-export async function checkNewsletterSubscription(email: string) {
- return await serverMiscApi.isNewsletterSubscribed(email);
+export async function checkNewsletterSubscription() {
+ const user = await ServerProfileApi.getMyProfile();
+ if (!user) throw new Error("Not authenticated");
+ return await serverMiscApi.isNewsletterSubscribed(user.email);
}Apply the same pattern to subscribeToNewsletter as well.
🤖 Prompt for AI Agents
In `@front_end/src/app/`(main)/actions.ts around lines 20 - 26, The server actions
unsubscribeFromNewsletter, checkNewsletterSubscription (and
subscribeToNewsletter) currently accept a client-supplied email which allows one
user to act on another's account; change them to ignore the passed email and
instead fetch the authenticated user's email server-side via
ServerProfileApi.getMyProfile(), then call
serverMiscApi.unsubscribeFromNewsletter / serverMiscApi.isNewsletterSubscribed /
serverMiscApi.subscribeToNewsletter with that server-side email; ensure you
remove or ignore the function parameter and handle missing profile/email errors
appropriately.
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@front_end/src/app/`(main)/actions.ts:
- Around line 17-19: The console.log in subscribeToNewsletter leaks raw email
PII; remove that logging and instead call
serverMiscApi.subscribeToNewsletter(email) directly (or if you need a log, log a
non-identifying event such as "newsletter subscription attempt" without the
email or log a hashed/anonymized value). Update the function
subscribeToNewsletter to eliminate the raw email output and use a non-PII log or
anonymization strategy if telemetry is required.
| export async function subscribeToNewsletter(email: string) { | ||
| console.log("subscribeToNewsletter", email); | ||
| return await serverMiscApi.subscribeToNewsletter(email); |
There was a problem hiding this comment.
Remove raw email logging (PII).
Server logs should not record user emails unless explicitly required and sanitized.
🩹 Suggested fix
export async function subscribeToNewsletter(email: string) {
- console.log("subscribeToNewsletter", email);
return await serverMiscApi.subscribeToNewsletter(email);
}📝 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.
| export async function subscribeToNewsletter(email: string) { | |
| console.log("subscribeToNewsletter", email); | |
| return await serverMiscApi.subscribeToNewsletter(email); | |
| export async function subscribeToNewsletter(email: string) { | |
| return await serverMiscApi.subscribeToNewsletter(email); | |
| } |
🤖 Prompt for AI Agents
In `@front_end/src/app/`(main)/actions.ts around lines 17 - 19, The console.log in
subscribeToNewsletter leaks raw email PII; remove that logging and instead call
serverMiscApi.subscribeToNewsletter(email) directly (or if you need a log, log a
non-identifying event such as "newsletter subscription attempt" without the
email or log a hashed/anonymized value). Update the function
subscribeToNewsletter to eliminate the raw email output and use a non-PII log or
anonymization strategy if telemetry is required.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@front_end/src/services/api/misc/misc.server.ts`:
- Around line 36-63: The isNewsletterSubscribed function currently only handles
HTTP error responses but lets fetch-level network errors propagate; wrap the
fetch and subsequent response processing in a try-catch around the existing
logic (including the fetch(url, { method: "GET", headers: { Authorization:
`Basic ${auth}` } }), response handling, and JSON parsing) and on any thrown
error catch it and return false so network failures behave like HTTP errors and
do not bubble up to callers.
- Around line 93-99: The error logging in unsubscribeFromNewsletter (and
likewise in subscribeToNewsletter) may print Mailjet errorText containing user
emails; update both functions to redact email addresses before logging by
replacing any email-like patterns in errorText with a redaction token (e.g.,
"[REDACTED_EMAIL]") and then log the sanitized message; locate the
response/errorText handling in unsubscribeFromNewsletter and the analogous block
in subscribeToNewsletter and apply the same redaction step prior to calling
console.error and throwing the Error.
| const response = await fetch(url, { | ||
| method: "GET", | ||
| headers: { | ||
| Authorization: `Basic ${auth}`, | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| if (response.status === 404) { | ||
| return false; | ||
| } | ||
| console.error( | ||
| `Failed to check newsletter subscription: ${response.status} ${response.statusText}` | ||
| ); | ||
| return false; | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| const newsletterListId = parseInt(listId, 10); | ||
|
|
||
| return ( | ||
| Array.isArray(data?.Data) && | ||
| data.Data.some( | ||
| (list: { ListID: number; IsUnsub: boolean }) => | ||
| list.ListID === newsletterListId && !list.IsUnsub | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd front_end && find . -name "misc.server.ts" -type f | head -5Repository: Metaculus/metaculus
Length of output: 101
🏁 Script executed:
cat -n front_end/src/services/api/misc/misc.server.ts | head -80Repository: Metaculus/metaculus
Length of output: 3104
🏁 Script executed:
rg "isNewsletterSubscribed" front_end/src --type ts --type tsx -B 2 -A 2Repository: Metaculus/metaculus
Length of output: 90
🏁 Script executed:
grep -n "unsubscribeFromNewsletter" front_end/src/services/api/misc/misc.server.tsRepository: Metaculus/metaculus
Length of output: 116
🏁 Script executed:
rg "isNewsletterSubscribed" front_end/src -B 2 -A 2Repository: Metaculus/metaculus
Length of output: 3578
🏁 Script executed:
cat -n front_end/src/services/api/misc/misc.server.ts | sed -n '65,110p'Repository: Metaculus/metaculus
Length of output: 1991
Add error handling for fetch-level failures in isNewsletterSubscribed.
Network errors (DNS failures, timeouts) will throw and propagate to callers, while HTTP errors are gracefully handled and return false. Wrap the fetch in a try-catch block to return false on network errors, ensuring consistent behavior across all error scenarios.
🔧 Suggested fix
+ try {
const response = await fetch(url, {
method: "GET",
headers: {
Authorization: `Basic ${auth}`,
},
});
if (!response.ok) {
if (response.status === 404) {
return false;
}
console.error(
`Failed to check newsletter subscription: ${response.status} ${response.statusText}`
);
return false;
}
const data = await response.json();
const newsletterListId = parseInt(listId, 10);
return (
Array.isArray(data?.Data) &&
data.Data.some(
(list: { ListID: number; IsUnsub: boolean }) =>
list.ListID === newsletterListId && !list.IsUnsub
)
);
+ } catch (err) {
+ console.error("Failed to check newsletter subscription", err);
+ return false;
+ }📝 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.
| const response = await fetch(url, { | |
| method: "GET", | |
| headers: { | |
| Authorization: `Basic ${auth}`, | |
| }, | |
| }); | |
| if (!response.ok) { | |
| if (response.status === 404) { | |
| return false; | |
| } | |
| console.error( | |
| `Failed to check newsletter subscription: ${response.status} ${response.statusText}` | |
| ); | |
| return false; | |
| } | |
| const data = await response.json(); | |
| const newsletterListId = parseInt(listId, 10); | |
| return ( | |
| Array.isArray(data?.Data) && | |
| data.Data.some( | |
| (list: { ListID: number; IsUnsub: boolean }) => | |
| list.ListID === newsletterListId && !list.IsUnsub | |
| ) | |
| ); | |
| } | |
| try { | |
| const response = await fetch(url, { | |
| method: "GET", | |
| headers: { | |
| Authorization: `Basic ${auth}`, | |
| }, | |
| }); | |
| if (!response.ok) { | |
| if (response.status === 404) { | |
| return false; | |
| } | |
| console.error( | |
| `Failed to check newsletter subscription: ${response.status} ${response.statusText}` | |
| ); | |
| return false; | |
| } | |
| const data = await response.json(); | |
| const newsletterListId = parseInt(listId, 10); | |
| return ( | |
| Array.isArray(data?.Data) && | |
| data.Data.some( | |
| (list: { ListID: number; IsUnsub: boolean }) => | |
| list.ListID === newsletterListId && !list.IsUnsub | |
| ) | |
| ); | |
| } catch (err) { | |
| console.error("Failed to check newsletter subscription", err); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@front_end/src/services/api/misc/misc.server.ts` around lines 36 - 63, The
isNewsletterSubscribed function currently only handles HTTP error responses but
lets fetch-level network errors propagate; wrap the fetch and subsequent
response processing in a try-catch around the existing logic (including the
fetch(url, { method: "GET", headers: { Authorization: `Basic ${auth}` } }),
response handling, and JSON parsing) and on any thrown error catch it and return
false so network failures behave like HTTP errors and do not bubble up to
callers.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/services/api/misc/misc.server.ts (1)
74-103:⚠️ Potential issue | 🔴 CriticalCritical security issue:
subscribeToNewsletteraccepts arbitrary email without authentication.The method correctly integrates with Mailjet and redacts emails in error logs. However, a significant vulnerability exists:
subscribeToNewsletter(lines 17–19 inactions.ts) accepts anyunsubscribeFromNewsletter(lines 26–29) correctly requires authentication and fetches the user's email server-side.In the authenticated settings context (
email_notifications.tsx, line 36), this creates an abuse vector where an authenticated user could modify the client call to subscribe arbitrary email addresses to the newsletter. The explicit design comment (lines 21–25) acknowledges allowing unauthenticated subscriptions for public signup flows, but this does not require an arbitrary email parameter in authenticated contexts.The fix aligns
subscribeToNewsletterwith the secureunsubscribeFromNewsletterpattern:Suggested fix in
actions.ts-export async function subscribeToNewsletter(email: string) { - return await serverMiscApi.subscribeToNewsletter(email); +export async function subscribeToNewsletter() { + const user = await ServerProfileApi.getMyProfile(); + if (!user) throw new Error("Not authenticated"); + return await serverMiscApi.subscribeToNewsletter(user.email); }And update the call site in
email_notifications.tsx:- await subscribeToNewsletter(user.email); + await subscribeToNewsletter();
2 participants
Summary by CodeRabbit
New Features
Localization