feat(notifications): enable multiple notification agent instances#1608
feat(notifications): enable multiple notification agent instances#1608Schrottfresser wants to merge 71 commits into
Conversation
Schrottfresser
force-pushed
the
notifications-rework
branch
2 times, most recently
from
9a25506 to
c2fd0cc
Compare
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Schrottfresser
force-pushed
the
notifications-rework
branch
from
7649231 to
ba68fc0
Compare
github-actions
Bot
added
merge conflict
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Schrottfresser
force-pushed
the
notifications-rework
branch
from
ba68fc0 to
04210c6
Compare
|
I extracted the settings interfaces from server/lib/settings/index.ts to server/interfaces/settings.ts to be able to include these interfaces in the client. I also find it more clean to have these interfaces in a separate file. |
Why? You don't need to move these to use them in the client? |
When trying to import interfaces or types from the server/lib/settings/index.ts i got an error because of the |
Schrottfresser
force-pushed
the
notifications-rework
branch
from
b4aaaf3 to
73aba35
Compare
|
This PR is feature complete now. I want to extract the table component in future which will not influence any features. |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Schrottfresser
force-pushed
the
notifications-rework
branch
from
cdec1ca to
0433a60
Compare
github-actions
Bot
added
merge conflict
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Schrottfresser
force-pushed
the
notifications-rework
branch
from
6656010 to
a520350
Compare
github-actions
Bot
removed
the
merge conflict
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Schrottfresser
force-pushed
the
notifications-rework
branch
from
a520350 to
9d1ba87
Compare
github-actions
Bot
removed
the
merge conflict
Hi @daddario11 thanks for checking the conflicts. I hadn't enough time in the last few weeks to check the status and will merge in the next few days once again. |
@Schrottfresser looking forward to it, have been hoping for this feature for a while! |
|
Hi @daddario11, haven't had the chance to look through the code, but I was wondering if this PR adds the ability to set up 2 webhooks (send one notification to my server and the other to my friend's server). And from your screenshot I see that it's setting up an email server, I hope that's not necessary for webhooks, right? Also, in the notification title or body, can the instance name or server name or even the URL of the Seerr be specified? An use case would be if I get 2 notifications: one from the Seerr hosted on my server and the other from the Seerr hosted on my friend's server. This way I can distinguish from which server they came. |
|
Hey @denisgabriel5, just to clarify I'm not the one who opened this PR, that's @Schrottfresser. But based on looking at the code I can say: yes you can set up multiple ntfy instances for different servers, no email is not required, and |
Even right now in the current versions and old versions we had support for setting server name. |
thank you for the correction @fallenbagel |
github-actions
Bot
added
blocked:template
and removed
merge conflict
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (2)
src/components/Settings/SettingsNotifications/NotificationModal/PushoverModal.tsx (1)
59-90:⚠️ Potential issue | 🟠 Major
.matches()will reject the empty disabled state, and.when({ then })trips Biome.Two issues in
NotificationsPushoverSchema:
- When the instance is disabled with no token,
accessToken/userTokendefault to'';.matches(/^[a-z\d]{30}$/i, ...)is then evaluated against the empty string and fails, blocking save. AddexcludeEmptyString: trueto both.matches()calls..when('enabled', { is, then, otherwise })triggers Biome'slint/suspicious/noThenProperty—use the callback form instead (already raised in DiscordModal/NtfyModal/EmailModal reviews).🔧 Suggested fix (item 1)
.matches( /^[a-z\d]{30}$/i, - intl.formatMessage(messages.pushoverValidationAccessTokenRequired) + { + message: intl.formatMessage(messages.pushoverValidationAccessTokenRequired), + excludeEmptyString: true, + } ),(same for
userToken)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/PushoverModal.tsx` around lines 59 - 90, NotificationsPushoverSchema currently fails validation for disabled instances because .matches(...) runs against empty strings and the object .when('enabled', { then }) form triggers the linter; update both accessToken and userToken validators to (1) use the callback form of when — e.g. .when('enabled', (enabled, schema) => enabled ? schema.nullable().required(intl.formatMessage(...)) : schema.nullable()) — and (2) add excludeEmptyString: true to their .matches(...) calls so the empty '' value is excluded from the regex check.server/lib/notifications/index.ts (1)
204-227:⚠️ Potential issue | 🔴 CriticalGuard missing-instance mutations before touching
activeAgents.If
instanceIdis unknown,unregisterAgent()will executesplice(-1, 1)and drop the last registered agent, whilereregisterAgent()writes tothis.activeAgents[-1]without replacing anything. Return early wheninstanceIndex === -1and log a warning instead.🛠️ Suggested fix
public unregisterAgent = (instanceId: number) => { const instanceIndex = this.activeAgents.findIndex( (instance) => instance.id === instanceId ); + if (instanceIndex === -1) { + logger.warn( + `Tried to unregister unknown notification agent instance id ${instanceId}`, + { label: 'Notifications' } + ); + return; + } + this.activeAgents.splice(instanceIndex, 1); logger.info( `Unregistered notification agent instance with id ${instanceId}`, { label: 'Notifications' } ); @@ public reregisterAgent = (agent: NotificationAgent, instanceId: number) => { const instanceIndex = this.activeAgents.findIndex( (instance) => instance.id === instanceId ); + if (instanceIndex === -1) { + logger.warn( + `Tried to reregister unknown notification agent instance id ${instanceId}`, + { label: 'Notifications' } + ); + return; + } + this.activeAgents[instanceIndex] = agent;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/notifications/index.ts` around lines 204 - 227, The methods unregisterAgent and reregisterAgent mutate activeAgents without checking that the computed instanceIndex is valid; if findIndex returns -1 they will incorrectly remove or overwrite the last element or write to [-1]. Fix both functions by checking if instanceIndex === -1 and return early (no mutation) while calling logger.warn (include the instanceId and label 'Notifications') to indicate the unknown id, otherwise proceed with splice or assignment as currently implemented.
🟡 Minor comments (13)
src/components/RequestModal/TvRequestModal.tsx-538-548 (1)
538-548:⚠️ Potential issue | 🟡 Minor
disabledcheck is inconsistent withtoggleAllSeasonsgating whenremaining === 0.The
disabledcondition usesquota?.tv.remainingas a truthy check, so whenremaining === 0(limit set, nothing left) the toggle renders as enabled, yettoggleAllSeasonsstill short-circuits via(quota?.tv.remaining ?? 0) < unrequestedSeasons.length. Result: clickable-looking control that does nothing.Align the predicate with
toggleAllSeasons(and tighten theunrequestedSeasons.length > 0edge while you're at it).🔧 Proposed fix
<ToggleSwitch isToggled={isAllSeasons()} onToggle={() => toggleAllSeasons()} disabled={ !!( - quota?.tv.remaining && - quota.tv.limit && - quota.tv.remaining < unrequestedSeasons.length + quota?.tv.limit && + (quota?.tv.remaining ?? 0) < unrequestedSeasons.length ) } />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RequestModal/TvRequestModal.tsx` around lines 538 - 548, The ToggleSwitch renders enabled when quota.tv.remaining === 0 but toggleAllSeasons short-circuits in that case, so update the disabled predicate to match the gating logic used by toggleAllSeasons: compute remaining as (quota?.tv.remaining ?? 0) and disable the control when remaining < unrequestedSeasons.length or when unrequestedSeasons.length === 0; reference ToggleSwitch props, isAllSeasons(), toggleAllSeasons, quota.tv.remaining, quota.tv.limit, and unrequestedSeasons.length to locate and replace the current disabled expression.server/interfaces/settings.ts-108-157 (1)
108-157:⚠️ Potential issue | 🟡 MinorDuplicate
ProxySettingsinterface declaration.
ProxySettingsis exported twice with identical shape (lines 108–117 and 148–157). TypeScript's declaration merging will accept this, but it's dead code that will silently cause issues if one definition drifts. Remove the duplicate.♻️ Proposed cleanup
export interface MainSettings { ... youtubeUrl: string; } -export interface ProxySettings { - enabled: boolean; - hostname: string; - port: number; - useSsl: boolean; - user: string; - password: string; - bypassFilter: string; - bypassLocalAddresses: boolean; -} - export interface DnsCacheSettings {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/interfaces/settings.ts` around lines 108 - 157, The file declares the ProxySettings interface twice; remove the duplicate declaration to avoid accidental drift and confusion. Locate the two exported ProxySettings interfaces (the ones with properties enabled, hostname, port, useSsl, user, password, bypassFilter, bypassLocalAddresses) and delete one of them so only a single export of ProxySettings remains; ensure any imports/uses still reference ProxySettings unchanged and run type checks to confirm no regressions.server/entity/User.ts-211-214 (1)
211-214:⚠️ Potential issue | 🟡 MinorAdd validation before using default email instance to prevent silent failures.
retrieveDefaultNotificationInstanceSettingsreturnsagentTemplates.email(with emptysmtpHostandemailFrom) when no admin-configured default email instance exists. BothgeneratePasswordandresetPasswordpass this fallback template directly toPreparedEmail, which hands empty SMTP settings tonodemailer.createTransport. Attempts to send fail later with a generic "Failed to send..." log rather than a clear signal that email is not configured.The codebase already uses a better pattern in
usersettings.ts(checkinginstances.some()for a valid default before proceeding). Apply the same validation here.🛡️ Suggested guard
- const defaultEmailInstance = retrieveDefaultNotificationInstanceSettings( - NotificationAgentKey.EMAIL - ) as NotificationAgentEmail; - const email = new PreparedEmail(defaultEmailInstance); + const defaultEmailInstance = retrieveDefaultNotificationInstanceSettings( + NotificationAgentKey.EMAIL + ) as NotificationAgentEmail | undefined; + if (!defaultEmailInstance?.options?.smtpHost) { + logger.warn( + 'No default email agent configured; skipping password email', + { label: 'User Management', email: this.email } + ); + return; + } + const email = new PreparedEmail(defaultEmailInstance);Apply the same fix to
resetPassword(lines 252–255).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/entity/User.ts` around lines 211 - 214, The default email template returned by retrieveDefaultNotificationInstanceSettings(NotificationAgentKey.EMAIL) can be an empty fallback and must be validated before creating PreparedEmail; update generatePassword and resetPassword to check the returned defaultEmailInstance for required SMTP settings (e.g., smtpHost and emailFrom or a simple isValid check similar to usersettings.ts' instances.some()) and if invalid, log a clear error and abort the email send flow instead of constructing PreparedEmail (which passes empty settings to nodemailer.createTransport); apply the same guard in both generatePassword and resetPassword and ensure the error message clearly states email is not configured so callers can handle the failure early.server/routes/settings/notification.ts-117-141 (1)
117-141:⚠️ Potential issue | 🟡 MinorUse 401 (or
next({ status: 401 })) whenreq.useris missing.
req.userbeing absent indicates an authentication problem, not a server fault. Returning500(Line 120) misleads clients/monitoring. Prefer401. The trailing message in Line 138 also includes rawreq.body.agentin the response — typically harmless, but worth noting if input sanitization is a concern.🔧 Suggested fix
if (!req.user) { return next({ - status: 500, + status: 401, message: 'User information is missing from the request.', }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/settings/notification.ts` around lines 117 - 141, Update the error handling in the notificationRoutes.post('/test' ...) handler so that when req.user is missing you call next with status 401 (authentication error) instead of 500; locate the req.user check and change the error status to 401 and an appropriate message. Also review the createAccordingNotificationAgent and sendTestNotification branches: keep the existing 500 for a missing/invalid agent returned by createAccordingNotificationAgent, and consider sanitizing or avoiding echoing raw req.body.agent in the final next({ status: 500, message: ... }) error to prevent reflecting unsanitized input.src/components/Settings/SettingsNotifications/NotificationModal/GotifyModal.tsx-99-103 (1)
99-103:⚠️ Potential issue | 🟡 MinorInconsistent optional chaining on
data/data.options.Lines 99–101 access
data.options.url/token/prioritydirectly, but Line 102 usesdata?.options.locale. The optional chain ondataonly is misleading — ifdataever isundefined, lines 99–101 already crash, and Line 102 will still throw on.locale(the chain stops atdata?.options, which isundefined, then.localeis fine — but the inconsistency suggests intent is unclear). Either drop the?.(sincedatais asserted non-optional by the prop type) or extend it consistently:♻️ Proposed cleanup
- url: data.options.url, - token: data.options.token, - priority: data.options.priority, - locale: data?.options.locale ?? 'en', + url: data.options.url, + token: data.options.token, + priority: data.options.priority, + locale: data.options.locale ?? 'en',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/GotifyModal.tsx` around lines 99 - 103, The object initialisation in GotifyModal.tsx mixes direct access to data.options (url/token/priority) with an optional chain for locale (data?.options.locale), which is inconsistent and misleading; update the code to access locale the same way as the other fields — e.g., use data.options.locale ?? 'en' — or, if data/data.options may actually be undefined, add an explicit guard (throw or early return) before constructing the object so all accesses can safely use data.options; refer to the GotifyModal component and the object literal that sets url, token, priority, locale to locate the change.server/routes/settings/notification.ts-212-230 (1)
212-230:⚠️ Potential issue | 🟡 MinorUse 204 No Content for successful DELETE.
Line 229 returns
200with an empty body, but conventional REST (and the project's other handlers — see Line 134) uses204for empty success responses. Also, as noted on the create handler, ifsettings.save()fails aftersplice/unregisterAgent, the in-memory state has already diverged from disk; consider persisting first.🔧 Suggested fix
- res.status(200).send(); + res.status(204).send();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/settings/notification.ts` around lines 212 - 230, Change the DELETE handler to return 204 No Content and avoid mutating disk/in-memory order: compute a new instances array (e.g. newInstances = instances.filter(i => i.id !== Number(req.params.id))), assign it to settings.notification.instances, call await settings.save() and only after successful save call notificationManager.unregisterAgent(Number(req.params.id)); finally respond with res.status(204).send(); keep using notificationRoutes.delete, getSettings, settings.save, notificationManager.unregisterAgent and avoid using instances.splice before save.src/components/Settings/SettingsNotifications/NotificationModal/TelegramModal.tsx-355-370 (1)
355-370:⚠️ Potential issue | 🟡 Minor
errorprop onNotificationTypeSelectorwill never display a message here.The schema on Lines 59–92 doesn't define a validation rule for
types, soerrors.typesis alwaysundefined— the user will never see the "must select at least one notification type" feedback thatPushbulletModalandGotifyModalsurface. For consistency, use the same conditional as the other modals (and add avalidationTypesmessage to this file's localmessages):♻️ Suggested fix
<NotificationTypeSelector currentTypes={values.enabled && values.types ? values.types : 0} onUpdate={(newTypes) => { setFieldValue('types', newTypes); setFieldTouched('types'); if (newTypes) { setFieldValue('enabled', true); } }} error={ - errors.types && touched.types - ? (errors.types as string) + values.enabled && !values.types && touched.types + ? intl.formatMessage(messages.validationTypes) : undefined } />Add to
messages:validationTypes: 'You must select at least one notification type',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/TelegramModal.tsx` around lines 355 - 370, The NotificationTypeSelector's error prop never shows because this file's validation schema lacks a rule for "types" and there's no local messages.validationTypes; update the validation schema in this file to include a rule for "types" (matching the other modals) and add a messages.validationTypes entry with the text "You must select at least one notification type", then change the NotificationTypeSelector error conditional to mirror PushbulletModal/GotifyModal (use errors.types && touched.types ? messages.validationTypes or errors.types : undefined) so the validation message displays correctly; reference NotificationTypeSelector, errors, touched, messages, and the file's validation schema to locate the changes.src/components/Settings/SettingsNotifications/NotificationModal/DiscordModal.tsx-191-225 (1)
191-225:⚠️ Potential issue | 🟡 MinorWrong
htmlForon the Webhook URL label (a11y).Line 192 uses
htmlFor="name"for the Discord Webhook URL label, but the corresponding field's id iswebhookUrl(line 213). Clicking the label currently focuses the (already-rendered above) instance Name input instead of the Webhook URL input.🔧 Suggested fix
- <label htmlFor="name" className="text-label"> + <label htmlFor="webhookUrl" className="text-label"> {intl.formatMessage(messages.discordWebhookUrl)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/DiscordModal.tsx` around lines 191 - 225, The label for the Discord Webhook URL uses htmlFor="name" but the input Field has id="webhookUrl", causing the label to focus the wrong control; update the label's htmlFor attribute in the DiscordModal component so it matches the Field id (change htmlFor from "name" to "webhookUrl") to restore correct accessible label association for the Field with id "webhookUrl".src/components/Settings/SettingsNotifications/NotificationModal/PushoverModal.tsx-53-57 (1)
53-57:⚠️ Potential issue | 🟡 MinorSounds list never refreshes for the form's live token; also missing URL-encoding.
The SWR key is bound to the initial
data.options.accessToken, not the live Formikvalues.accessToken. A user pasting/changing the application token in the form won't see the sounds list populate until they save and reopen the modal. Additionally, the token is interpolated into a query string withoutencodeURIComponent, which will break for any token containing reserved characters.To make this reactive, lift the SWR call inside the Formik render-prop (or a child component) so it can read
values.accessToken:{({ values, ... }) => { const { data: soundsData } = useSWR<PushoverSound[]>( values.accessToken ? `/api/v1/settings/notifications/pushover/sounds?token=${encodeURIComponent(values.accessToken)}` : null ); ... }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/PushoverModal.tsx` around lines 53 - 57, The SWR call in PushoverModal is currently using the static data.options.accessToken so the sounds list doesn't update as the user edits the form and the token isn't URL-encoded; move the useSWR<PushoverSound[]> call inside the Formik render-prop (or a child component that reads Formik's values) so it uses values.accessToken, and build the key using encodeURIComponent(values.accessToken) when present (or null otherwise) so the sounds endpoint refreshes live and handles reserved characters.server/lib/settings/index.ts-416-421 (2)
416-421:⚠️ Potential issue | 🟡 Minor
userEmailRequiredderivation ignores whether the email instance is enabled.
enablePushRegistrationandemailEnabledcorrectly requireinstance.enabled, butuserEmailRequiredonly checksinstance.default && agent === EMAIL && options.userEmailRequired. A disabled default email instance that still hasuserEmailRequired: truewill gate user-email collection on the public settings even though email is off, blocking signups/etc. for no reason.🔧 Suggested fix
userEmailRequired: this.notification.instances.some( (instance) => instance.default && instance.agent === NotificationAgentKey.EMAIL && + instance.enabled && instance.options.userEmailRequired ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/settings/index.ts` around lines 416 - 421, The computation of userEmailRequired on the public settings reads notification.instances but misses checking instance.enabled, so a disabled default EMAIL instance with options.userEmailRequired could incorrectly require emails; update the logic that sets userEmailRequired to also require instance.enabled (same pattern used by enablePushRegistration/emailEnabled) when iterating notification.instances for NotificationAgentKey.EMAIL and options.userEmailRequired so disabled instances do not gate user-email collection.
416-421:⚠️ Potential issue | 🟡 MinorNarrow the type of
instancewhen accessinguserEmailRequiredon the loosely-typedoptionsfield.
instance.optionsis typed asRecord<string, unknown>on the baseNotificationAgentConfig. Although the code checksinstance.agent === NotificationAgentKey.EMAIL, TypeScript does not narrow the type based on this runtime check alone. Add a type guard or cast toNotificationAgentEmailto ensure type safety when accessinginstance.options.userEmailRequired:instance.agent === NotificationAgentKey.EMAIL && (instance as NotificationAgentEmail).options.userEmailRequiredOr use a type predicate to create a proper type guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/settings/index.ts` around lines 416 - 421, The code reads instance.options.userEmailRequired on a loosely typed NotificationAgentConfig; TypeScript won't narrow options based on the runtime agent check, so add a proper narrowing before accessing userEmailRequired: either cast the instance to NotificationAgentEmail or implement a type predicate that returns instance is NotificationAgentEmail when instance.agent === NotificationAgentKey.EMAIL, then use that narrowed type when reading options.userEmailRequired (references: instance, userEmailRequired, NotificationAgentKey.EMAIL, NotificationAgentEmail, NotificationAgentConfig, options).src/components/Settings/SettingsNotifications/NotificationModal/index.tsx-66-103 (1)
66-103:⚠️ Potential issue | 🟡 Minor
afterSave()runs on failure, closing the modal even when save/create fails.Both
onSaveandonCreateinvokeafterSave()fromfinally, so a failed POST still triggers the parent close/refresh flow alongside the error toast. The user loses their unsaved form data and has to start over. Consider only callingafterSave()from thetryblock (success path) and letting the catch surface the toast without dismissing the modal.🔧 Suggested fix
const onSave = async (submitData: NotificationAgentConfig) => { try { await axios.post( `/api/v1/settings/notification/${submitData.id}`, submitData ); addToast(intl.formatMessage(messages.toastSaveSuccess), { appearance: 'success', autoDismiss: true, }); + afterSave(); } catch { addToast(intl.formatMessage(messages.toastSaveFail), { appearance: 'error', autoDismiss: true, }); - } finally { - afterSave(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/index.tsx` around lines 66 - 103, The modal is being closed regardless of success because afterSave() is called in the finally block of both onSave and onCreate; change the flow so afterSave() is only invoked on the successful path: remove afterSave() from the finally blocks in onSave and onCreate and instead call afterSave() immediately after the success toast inside each try block (after the axios.post and addToast success call), leaving the catch block to only show the error toast so the modal stays open on failure.src/components/Settings/SettingsNotifications/NotificationModal/NtfyModal.tsx-64-73 (1)
64-73:⚠️ Potential issue | 🟡 MinorWrong validation message wired to the
topicschema.Line 73 calls
.defined(intl.formatMessage(messages.ntfyValidationUrl))on thetopicfield, but the message should bemessages.ntfyValidationTopic. As-is, a missing topic surfaces "You must provide a valid URL" to users.🔧 Suggested fix
- .defined(intl.formatMessage(messages.ntfyValidationUrl)), + .defined(intl.formatMessage(messages.ntfyValidationTopic)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/NtfyModal.tsx` around lines 64 - 73, The Yup validation for the topic field (the topic schema in NtfyModal.tsx) is using the wrong message: replace the .defined(intl.formatMessage(messages.ntfyValidationUrl)) call on the topic schema with .defined(intl.formatMessage(messages.ntfyValidationTopic)) so the topic field surfaces the correct "topic" validation message while keeping the existing .when('enabled') conditional (.nullable()/.required(...)) logic intact.
🧹 Nitpick comments (9)
server/routes/auth.test.ts (1)
90-122: Scope thenotification.instancesmutation in atry/finally.If any of the assertions between Line 92 and Line 121 throws, the pushed instance leaks into subsequent tests in this file (and the same applies to the
localLoginanduser.emailmutations). This matches the established convention for settings mutations in the test suite — capture prior state, mutate, exercise insidetry, restore insidefinally.♻️ Suggested pattern
it('includes userEmailRequired warning when email is required but invalid', async () => { const settings = getSettings(); - settings.notification.instances.push({ - default: true, - agent: NotificationAgentKey.EMAIL, - options: { - userEmailRequired: true, - }, - } as NotificationAgentEmail); - - // Change the user's email to something invalid - const userRepo = getRepository(User); - const user = await userRepo.findOneOrFail({ - where: { email: '[email protected]' }, - }); - user.email = 'not-an-email'; - await userRepo.save(user); - ... - settings.notification.instances = []; + const priorInstances = [...settings.notification.instances]; + settings.notification.instances.push({ + default: true, + agent: NotificationAgentKey.EMAIL, + options: { userEmailRequired: true }, + } as NotificationAgentEmail); + try { + // ... existing test body ... + } finally { + settings.notification.instances = priorInstances; + } });Based on learnings, settings mutations in this test suite should be scoped to the narrowest possible window via
try/finallyrather than ad-hoc cleanup at the end of the test body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.test.ts` around lines 90 - 122, The test mutates global state (settings.notification.instances, settings.main.localLogin and the User.email) and should restore prior values in a try/finally to avoid leaking into other tests: capture originals (e.g., const origInstances = settings.notification.instances.slice(), const origLocalLogin = settings.main.localLogin, const origEmail = user.email), perform the push, user.email change and login/assertions inside a try block, and in finally restore settings.notification.instances = origInstances, settings.main.localLogin = origLocalLogin and reset user.email then await userRepo.save(user) to persist the reverted email; reference getSettings(), settings.notification.instances, settings.main.localLogin, userRepo and User to locate the changes.server/routes/settings/notification.ts (1)
33-43:findFirstFreeNotificationInstanceIdhas correct semantics but is O(n²).The loop logic is correct (returns the smallest non-negative integer not present, or
instances.lengthif[0..n-1]are all taken). For typical instance counts this is fine, but a Set-based lookup avoids the nestedfind:♻️ Optional optimization
const findFirstFreeNotificationInstanceId = () => { const instances = getSettings().notification.instances; - - for (let i = 0; i < instances.length; ++i) { - if (!instances.find((instance) => instance.id === i)) { - return i; - } - } - - return instances.length; + const usedIds = new Set(instances.map((i) => i.id)); + for (let i = 0; i <= instances.length; ++i) { + if (!usedIds.has(i)) return i; + } + return instances.length; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/settings/notification.ts` around lines 33 - 43, findFirstFreeNotificationInstanceId is correct but does O(n²) work by calling instances.find inside the loop; to fix, build a Set of existing ids from getSettings().notification.instances (or map ids into a boolean/indexed map) and then iterate i from 0 to instances.length checking set.has(i) to return the first missing id (or instances.length if none), updating the function findFirstFreeNotificationInstanceId to use the Set-based lookup for O(n) complexity.src/components/Settings/SettingsNotifications/NotificationModal/GotifyModal.tsx (1)
105-161: De-duplicate test/submit payload construction.The mapping from
valuesinto aNotificationAgentGotifypayload is duplicated betweenonSubmit(Lines 105–121) andonSecondary(Lines 145–161). A smallbuildPayload()helper inside the render scope keeps the two flows in sync as the schema evolves. Same suggestion applies toPushbulletModal/TelegramModal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/GotifyModal.tsx` around lines 105 - 161, The payload mapping from form values to the NotificationAgentGotify shape is duplicated in the onSubmit handler and the Modal secondary onSecondary callback (which calls onTest); extract a small helper function (e.g., buildGotifyPayload(values): NotificationAgentGotify) inside the render scope and call it from both onSubmit and onSecondary so they share the same logic; ensure the helper maps enabled, types, name, id, agent, default, embedPoster: true and options: { url: values.url, token: values.token, priority: Number(values.priority), locale: values.locale } and use that helper in place of the inline object in onSubmit and the onSecondary/onTest invocation.src/components/Settings/SettingsNotifications/NotificationModal/PushbulletModal.tsx (1)
60-122: De-duplicate the test/submit payload construction.The mapping from Formik
valuestoNotificationAgentPushbulletis duplicated betweenonSubmit(Lines 72-83) andonSecondary(Lines 110-121). Extracting a small helper inside the render function avoids drift if the payload shape evolves (e.g., when adding more options later).♻️ Suggested refactor
{({ errors, touched, isSubmitting, values, isValid, setFieldValue, setFieldTouched, handleSubmit, }) => { + const buildPayload = (): NotificationAgentPushbullet => ({ + enabled: values.enabled, + types: values.types, + name: values.name, + id: values.id, + agent: values.agent, + default: values.default, + embedPoster: true, + options: { + accessToken: values.accessToken, + channelTag: values.channelTag, + }, + });Then both
onSubmitandonSecondarycan callbuildPayload().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/PushbulletModal.tsx` around lines 60 - 122, The payload mapping from Formik values to the NotificationAgentPushbullet object is duplicated in the onSubmit handler and the onSecondary test call; create a small helper function (e.g., buildPayload or buildPushbulletPayload) inside the render scope that accepts the current Formik values (or reads values closure) and returns the object shape used for onSave/onTest (fields: enabled, types, name, id, agent, default, embedPoster: true, options: { accessToken, channelTag }). Replace the inline objects in onSubmit (which calls onSave) and onSecondary (which calls onTest) to call this helper to avoid duplication and keep the payload construction centralized.src/components/Settings/SettingsNotifications/NotificationModal/EmailModal.tsx (1)
448-460:rows="10"should be a number for the underlying<textarea>.React's intrinsic
<textarea>rowsprop is typed asnumber. Passing the string"10"works at runtime due to HTML coercion but will surface as a TS error ifSensitiveInput's prop typing is tightened. Preferrows={10}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/EmailModal.tsx` around lines 448 - 460, The rows prop for the PGP private key textarea is passed as a string ("10") which should be a number; update the SensitiveInput usage for the pgpPrivateKey field (the component instance with id/name "pgpPrivateKey") to pass rows as a numeric literal (rows={10}) so it matches the intrinsic textarea typing and avoids future TypeScript errors.src/components/Settings/SettingsNotifications/NotificationModal/NtfyModal.tsx (1)
362-379: Redundant truthy guard onvalues.types.
values.enabled && values.types ? values.types || 0 : 0evaluatesvalues.types || 0only aftervalues.typeswas already truthy in the ternary condition, so the|| 0is unreachable. Simplify tovalues.enabled && values.types ? values.types : 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/NtfyModal.tsx` around lines 362 - 379, The currentTypes prop on NotificationTypeSelector uses a redundant guard: replace the expression `values.enabled && values.types ? values.types || 0 : 0` with the simplified `values.enabled && values.types ? values.types : 0` (i.e., update the currentTypes prop in NtfyModal to directly return values.types when enabled, otherwise 0) so the unreachable `|| 0` is removed while preserving the same fallback behavior.src/components/Settings/SettingsNotifications/NotificationModal/index.tsx (2)
105-136: Test toast flow assumesaddToastinvokes its callback synchronously.The toast id is captured via the third-arg callback and immediately referenced post-
await.react-toast-notificationsv2 does invoke the callback synchronously, so this works today, but it's brittle—any future async-id implementation would leak the "sending…" toast (it'sautoDismiss: false). A safer pattern is to callremoveToast(toastId)only when defined and consider also clearing ononCloseto prevent dangling toasts if the modal unmounts mid-request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/index.tsx` around lines 105 - 136, The onTest function captures the toast id via addToast's callback which assumes synchronous invocation; make it robust by storing the toast id in a local state/ref or the synchronous return (instead of relying on the third-arg callback), ensure removal happens in a finally block so removeToast(toastId) is attempted regardless of success/failure, and attach an onClose/onUnmount handler to clear the pending "sending…" toast if the modal unmounts mid-request; update references to toastId, addToast, removeToast and the onTest flow to implement these changes.
229-238: WebPush case usesNotificationAgentConfigwhile every other agent uses a provider-specific cast.All other notification agents cast to their own interface (
NotificationAgentDiscord,NotificationAgentEmail, etc.), but WebPush casts to the baseNotificationAgentConfig. WebPushModalProps accepts the broaderNotificationAgentConfigtype, which differs from the pattern. For consistency, consider exporting aNotificationAgentWebPushinterface and casting to it like the other agents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/SettingsNotifications/NotificationModal/index.tsx` around lines 229 - 238, The WebPush branch is casting data to the broad NotificationAgentConfig instead of a provider-specific type; define and export a NotificationAgentWebPush interface (matching WebPushModalProps shape) and use it consistently like the others: update WebPushModalProps to reference NotificationAgentWebPush if needed and change the cast in the NotificationAgentKey.WEBPUSH case to cast data as NotificationAgentWebPush before passing to <WebPushModal /> so it mirrors other agents (e.g., NotificationAgentDiscord/Email) and keeps typing consistent.server/lib/settings/index.ts (1)
109-211:typesfield is optional but pattern inconsistent across agent templates.The
typesfield onNotificationAgentConfigis optional (types?: number), so omitting it fromwebpushtemplates is not a TypeScript error. However, most other agents (discord,slack,telegram,pushbullet,pushover,webhook) explicitly settypes: 0, while onlywebpushomit it. For consistency, either addtypes: 0towebpush, or document why these agents intentionally differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/settings/index.ts` around lines 109 - 211, The agent templates are inconsistent: the optional NotificationAgentConfig.types field is set to 0 for most agents but omitted for the email and webpush objects; update the email and webpush config objects to include types: 0 for consistency (the objects referenced are the email block using NotificationAgentKey.EMAIL and the webpush block using NotificationAgentKey.WEBPUSH) so all agent templates follow the same pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@seerr-api.yml`:
- Around line 1413-1415: The NotificationInstance schema currently includes an
id that is being reused for create/test/update request bodies, making id appear
client-writable; change the API models so the response schema
(NotificationInstance) retains id as readOnly and create a separate request
schema (e.g., NotificationInstanceCreate and NotificationInstanceUpdate) that
omits id, then update the operation request/response refs for endpoints like
/settings/notification/{instanceId} and the related create/test/update paths to
use the new request schemas; alternatively, if you prefer a minimal change, mark
the id property in NotificationInstance as readOnly to prevent it appearing
writable in generated SDKs and ensure request bodies reference the request-only
schema where applicable.
- Around line 1506-1517: Add the missing options.customHeaders property to the
webhook template schema so generated clients include the header list; update the
"options" object (used by the webhook template for /settings/notification) to
include customHeaders: type array with items being an object containing at least
"name" (string) and "value" (string) properties (and allow empty arrays/null as
appropriate), matching the UI/settings types that consume options.customHeaders.
In `@server/lib/overseerrMerge.ts`:
- Around line 143-148: The check against the new path
settings.notification.agentTemplates.email.options.senderName never sees legacy
"Overseerr" because Settings().load(undefined, true) skips migrations; fix by
either loading with migrations (call Settings().load(undefined, false) or remove
the raw=true) or, if you must skip migrations, detect and migrate from the
legacy key settings.notifications.agents.email.options.senderName whenever the
new path equals the default "Seerr" — copy the legacy value into
settings.notification.agentTemplates.email.options.senderName (and persist/save
the Settings) so the rebrand replacement actually runs.
In `@server/routes/settings/notification.ts`:
- Around line 87-210: Both POST '/' and POST '/:id' handlers currently persist
the incoming request without enforcing that only one instance per agent type has
default: true; when request.default === true, iterate
settings.notification.instances and set default = false for any instance with
the same agent before assigning instances[notificationInstanceIndex] = request
and saving. Update the logic in the routes that create/update agents (the
anonymous async handlers for notificationRoutes.post('/') and
notificationRoutes.post('/:id')) to perform this normalization prior to
settings.save(), and ensure the same normalization runs whether registering
(notificationManager.registerAgent) or reregistering
(notificationManager.reregisterAgent) so
retrieveDefaultNotificationInstanceSettings will see a single default per agent
type.
- Around line 87-115: The code registers the notification agent with
notificationManager.registerAgent before persisting the new instance via
settings.save, risking in-memory/disk divergence on save failure; update the
POST handler (the notificationRoutes.post function) to persist the new instance
into settings.instances and await settings.save() first, then call
notificationManager.registerAgent(createAccordingNotificationAgent(...)) only
after save succeeds, and for symmetry update the PUT (/:id) and DELETE (/:id)
handlers similarly—or alternatively, if you must register first, wrap
registerAgent in try/catch and call notificationManager.unregisterAgent (or roll
back the registration) when settings.save() throws so in-memory state is
consistent with persisted settings.
- Around line 157-210: The POST /:id handler currently allows creating instances
with arbitrary caller-supplied IDs and duplicates code for agent creation;
update the notificationRoutes.post handler to validate and sanitize
req.params.id (convert to Number, ensure !isNaN and id >= 0), then if the id is
outside the valid existing-range reject with 404 (or require clients to use POST
/ to create new instances) instead of creating a new entry at that id; also
hoist the createAccordingNotificationAgent(...) call and its null-check so you
only create the agent once and then choose between
notificationManager.registerAgent(...) or
notificationManager.reregisterAgent(...), and ensure you use
findFirstFreeNotificationInstanceId (or
settings.notification.findFirstFreeNotificationInstanceId) when creating a
genuinely new instance rather than trusting the caller-supplied id.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/EmailModal.tsx`:
- Around line 123-126: The PGP private key regex validator inside EmailModal's
validation schema (.matches(...)) is rejecting empty strings produced by Formik;
update that .matches call on the PGP private key field in the EmailModal
component to pass the option excludeEmptyString: true so empty string values are
treated as “no value” and validation is skipped when pgpPassword is empty,
preserving the conditional "no PGP" workflow.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/WebhookModal.tsx`:
- Around line 129-206: The schema's custom tests for webhookUrl and jsonPayload
run even when the webhook is disabled; update the 'valid-url' and
'validate-json' test functions inside NotificationsWebhookSchema to first check
this.parent.enabled and immediately return true if enabled is false, so the URL
and JSON validation only run when enabled is true; ensure you reference the
existing test names ('valid-url' for webhookUrl and 'validate-json' for
jsonPayload) and rely on this.parent.enabled to guard the checks.
---
Minor comments:
In `@server/entity/User.ts`:
- Around line 211-214: The default email template returned by
retrieveDefaultNotificationInstanceSettings(NotificationAgentKey.EMAIL) can be
an empty fallback and must be validated before creating PreparedEmail; update
generatePassword and resetPassword to check the returned defaultEmailInstance
for required SMTP settings (e.g., smtpHost and emailFrom or a simple isValid
check similar to usersettings.ts' instances.some()) and if invalid, log a clear
error and abort the email send flow instead of constructing PreparedEmail (which
passes empty settings to nodemailer.createTransport); apply the same guard in
both generatePassword and resetPassword and ensure the error message clearly
states email is not configured so callers can handle the failure early.
In `@server/interfaces/settings.ts`:
- Around line 108-157: The file declares the ProxySettings interface twice;
remove the duplicate declaration to avoid accidental drift and confusion. Locate
the two exported ProxySettings interfaces (the ones with properties enabled,
hostname, port, useSsl, user, password, bypassFilter, bypassLocalAddresses) and
delete one of them so only a single export of ProxySettings remains; ensure any
imports/uses still reference ProxySettings unchanged and run type checks to
confirm no regressions.
In `@server/lib/settings/index.ts`:
- Around line 416-421: The computation of userEmailRequired on the public
settings reads notification.instances but misses checking instance.enabled, so a
disabled default EMAIL instance with options.userEmailRequired could incorrectly
require emails; update the logic that sets userEmailRequired to also require
instance.enabled (same pattern used by enablePushRegistration/emailEnabled) when
iterating notification.instances for NotificationAgentKey.EMAIL and
options.userEmailRequired so disabled instances do not gate user-email
collection.
- Around line 416-421: The code reads instance.options.userEmailRequired on a
loosely typed NotificationAgentConfig; TypeScript won't narrow options based on
the runtime agent check, so add a proper narrowing before accessing
userEmailRequired: either cast the instance to NotificationAgentEmail or
implement a type predicate that returns instance is NotificationAgentEmail when
instance.agent === NotificationAgentKey.EMAIL, then use that narrowed type when
reading options.userEmailRequired (references: instance, userEmailRequired,
NotificationAgentKey.EMAIL, NotificationAgentEmail, NotificationAgentConfig,
options).
In `@server/routes/settings/notification.ts`:
- Around line 117-141: Update the error handling in the
notificationRoutes.post('/test' ...) handler so that when req.user is missing
you call next with status 401 (authentication error) instead of 500; locate the
req.user check and change the error status to 401 and an appropriate message.
Also review the createAccordingNotificationAgent and sendTestNotification
branches: keep the existing 500 for a missing/invalid agent returned by
createAccordingNotificationAgent, and consider sanitizing or avoiding echoing
raw req.body.agent in the final next({ status: 500, message: ... }) error to
prevent reflecting unsanitized input.
- Around line 212-230: Change the DELETE handler to return 204 No Content and
avoid mutating disk/in-memory order: compute a new instances array (e.g.
newInstances = instances.filter(i => i.id !== Number(req.params.id))), assign it
to settings.notification.instances, call await settings.save() and only after
successful save call notificationManager.unregisterAgent(Number(req.params.id));
finally respond with res.status(204).send(); keep using
notificationRoutes.delete, getSettings, settings.save,
notificationManager.unregisterAgent and avoid using instances.splice before
save.
In `@src/components/RequestModal/TvRequestModal.tsx`:
- Around line 538-548: The ToggleSwitch renders enabled when quota.tv.remaining
=== 0 but toggleAllSeasons short-circuits in that case, so update the disabled
predicate to match the gating logic used by toggleAllSeasons: compute remaining
as (quota?.tv.remaining ?? 0) and disable the control when remaining <
unrequestedSeasons.length or when unrequestedSeasons.length === 0; reference
ToggleSwitch props, isAllSeasons(), toggleAllSeasons, quota.tv.remaining,
quota.tv.limit, and unrequestedSeasons.length to locate and replace the current
disabled expression.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/DiscordModal.tsx`:
- Around line 191-225: The label for the Discord Webhook URL uses htmlFor="name"
but the input Field has id="webhookUrl", causing the label to focus the wrong
control; update the label's htmlFor attribute in the DiscordModal component so
it matches the Field id (change htmlFor from "name" to "webhookUrl") to restore
correct accessible label association for the Field with id "webhookUrl".
In
`@src/components/Settings/SettingsNotifications/NotificationModal/GotifyModal.tsx`:
- Around line 99-103: The object initialisation in GotifyModal.tsx mixes direct
access to data.options (url/token/priority) with an optional chain for locale
(data?.options.locale), which is inconsistent and misleading; update the code to
access locale the same way as the other fields — e.g., use data.options.locale
?? 'en' — or, if data/data.options may actually be undefined, add an explicit
guard (throw or early return) before constructing the object so all accesses can
safely use data.options; refer to the GotifyModal component and the object
literal that sets url, token, priority, locale to locate the change.
In `@src/components/Settings/SettingsNotifications/NotificationModal/index.tsx`:
- Around line 66-103: The modal is being closed regardless of success because
afterSave() is called in the finally block of both onSave and onCreate; change
the flow so afterSave() is only invoked on the successful path: remove
afterSave() from the finally blocks in onSave and onCreate and instead call
afterSave() immediately after the success toast inside each try block (after the
axios.post and addToast success call), leaving the catch block to only show the
error toast so the modal stays open on failure.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/NtfyModal.tsx`:
- Around line 64-73: The Yup validation for the topic field (the topic schema in
NtfyModal.tsx) is using the wrong message: replace the
.defined(intl.formatMessage(messages.ntfyValidationUrl)) call on the topic
schema with .defined(intl.formatMessage(messages.ntfyValidationTopic)) so the
topic field surfaces the correct "topic" validation message while keeping the
existing .when('enabled') conditional (.nullable()/.required(...)) logic intact.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/PushoverModal.tsx`:
- Around line 53-57: The SWR call in PushoverModal is currently using the static
data.options.accessToken so the sounds list doesn't update as the user edits the
form and the token isn't URL-encoded; move the useSWR<PushoverSound[]> call
inside the Formik render-prop (or a child component that reads Formik's values)
so it uses values.accessToken, and build the key using
encodeURIComponent(values.accessToken) when present (or null otherwise) so the
sounds endpoint refreshes live and handles reserved characters.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/TelegramModal.tsx`:
- Around line 355-370: The NotificationTypeSelector's error prop never shows
because this file's validation schema lacks a rule for "types" and there's no
local messages.validationTypes; update the validation schema in this file to
include a rule for "types" (matching the other modals) and add a
messages.validationTypes entry with the text "You must select at least one
notification type", then change the NotificationTypeSelector error conditional
to mirror PushbulletModal/GotifyModal (use errors.types && touched.types ?
messages.validationTypes or errors.types : undefined) so the validation message
displays correctly; reference NotificationTypeSelector, errors, touched,
messages, and the file's validation schema to locate the changes.
---
Duplicate comments:
In `@server/lib/notifications/index.ts`:
- Around line 204-227: The methods unregisterAgent and reregisterAgent mutate
activeAgents without checking that the computed instanceIndex is valid; if
findIndex returns -1 they will incorrectly remove or overwrite the last element
or write to [-1]. Fix both functions by checking if instanceIndex === -1 and
return early (no mutation) while calling logger.warn (include the instanceId and
label 'Notifications') to indicate the unknown id, otherwise proceed with splice
or assignment as currently implemented.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/PushoverModal.tsx`:
- Around line 59-90: NotificationsPushoverSchema currently fails validation for
disabled instances because .matches(...) runs against empty strings and the
object .when('enabled', { then }) form triggers the linter; update both
accessToken and userToken validators to (1) use the callback form of when — e.g.
.when('enabled', (enabled, schema) => enabled ?
schema.nullable().required(intl.formatMessage(...)) : schema.nullable()) — and
(2) add excludeEmptyString: true to their .matches(...) calls so the empty ''
value is excluded from the regex check.
---
Nitpick comments:
In `@server/lib/settings/index.ts`:
- Around line 109-211: The agent templates are inconsistent: the optional
NotificationAgentConfig.types field is set to 0 for most agents but omitted for
the email and webpush objects; update the email and webpush config objects to
include types: 0 for consistency (the objects referenced are the email block
using NotificationAgentKey.EMAIL and the webpush block using
NotificationAgentKey.WEBPUSH) so all agent templates follow the same pattern.
In `@server/routes/auth.test.ts`:
- Around line 90-122: The test mutates global state
(settings.notification.instances, settings.main.localLogin and the User.email)
and should restore prior values in a try/finally to avoid leaking into other
tests: capture originals (e.g., const origInstances =
settings.notification.instances.slice(), const origLocalLogin =
settings.main.localLogin, const origEmail = user.email), perform the push,
user.email change and login/assertions inside a try block, and in finally
restore settings.notification.instances = origInstances,
settings.main.localLogin = origLocalLogin and reset user.email then await
userRepo.save(user) to persist the reverted email; reference getSettings(),
settings.notification.instances, settings.main.localLogin, userRepo and User to
locate the changes.
In `@server/routes/settings/notification.ts`:
- Around line 33-43: findFirstFreeNotificationInstanceId is correct but does
O(n²) work by calling instances.find inside the loop; to fix, build a Set of
existing ids from getSettings().notification.instances (or map ids into a
boolean/indexed map) and then iterate i from 0 to instances.length checking
set.has(i) to return the first missing id (or instances.length if none),
updating the function findFirstFreeNotificationInstanceId to use the Set-based
lookup for O(n) complexity.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/EmailModal.tsx`:
- Around line 448-460: The rows prop for the PGP private key textarea is passed
as a string ("10") which should be a number; update the SensitiveInput usage for
the pgpPrivateKey field (the component instance with id/name "pgpPrivateKey") to
pass rows as a numeric literal (rows={10}) so it matches the intrinsic textarea
typing and avoids future TypeScript errors.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/GotifyModal.tsx`:
- Around line 105-161: The payload mapping from form values to the
NotificationAgentGotify shape is duplicated in the onSubmit handler and the
Modal secondary onSecondary callback (which calls onTest); extract a small
helper function (e.g., buildGotifyPayload(values): NotificationAgentGotify)
inside the render scope and call it from both onSubmit and onSecondary so they
share the same logic; ensure the helper maps enabled, types, name, id, agent,
default, embedPoster: true and options: { url: values.url, token: values.token,
priority: Number(values.priority), locale: values.locale } and use that helper
in place of the inline object in onSubmit and the onSecondary/onTest invocation.
In `@src/components/Settings/SettingsNotifications/NotificationModal/index.tsx`:
- Around line 105-136: The onTest function captures the toast id via addToast's
callback which assumes synchronous invocation; make it robust by storing the
toast id in a local state/ref or the synchronous return (instead of relying on
the third-arg callback), ensure removal happens in a finally block so
removeToast(toastId) is attempted regardless of success/failure, and attach an
onClose/onUnmount handler to clear the pending "sending…" toast if the modal
unmounts mid-request; update references to toastId, addToast, removeToast and
the onTest flow to implement these changes.
- Around line 229-238: The WebPush branch is casting data to the broad
NotificationAgentConfig instead of a provider-specific type; define and export a
NotificationAgentWebPush interface (matching WebPushModalProps shape) and use it
consistently like the others: update WebPushModalProps to reference
NotificationAgentWebPush if needed and change the cast in the
NotificationAgentKey.WEBPUSH case to cast data as NotificationAgentWebPush
before passing to <WebPushModal /> so it mirrors other agents (e.g.,
NotificationAgentDiscord/Email) and keeps typing consistent.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/NtfyModal.tsx`:
- Around line 362-379: The currentTypes prop on NotificationTypeSelector uses a
redundant guard: replace the expression `values.enabled && values.types ?
values.types || 0 : 0` with the simplified `values.enabled && values.types ?
values.types : 0` (i.e., update the currentTypes prop in NtfyModal to directly
return values.types when enabled, otherwise 0) so the unreachable `|| 0` is
removed while preserving the same fallback behavior.
In
`@src/components/Settings/SettingsNotifications/NotificationModal/PushbulletModal.tsx`:
- Around line 60-122: The payload mapping from Formik values to the
NotificationAgentPushbullet object is duplicated in the onSubmit handler and the
onSecondary test call; create a small helper function (e.g., buildPayload or
buildPushbulletPayload) inside the render scope that accepts the current Formik
values (or reads values closure) and returns the object shape used for
onSave/onTest (fields: enabled, types, name, id, agent, default, embedPoster:
true, options: { accessToken, channelTag }). Replace the inline objects in
onSubmit (which calls onSave) and onSecondary (which calls onTest) to call this
helper to avoid duplication and keep the payload construction centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if ( | ||
| settings.notification.agentTemplates.email.options.senderName === | ||
| 'Overseerr' | ||
| ) { | ||
| settings.notification.agentTemplates.email.options.senderName = 'Seerr'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether anything migrates the legacy senderName path into the new agentTemplates path,
# and check how Settings.load() handles the legacy notifications.agents shape.
rg -nP -C3 "notifications\.agents\.email|agentTemplates\.email|senderName"
rg -nP -C5 "mergeSettings|class Settings\b|public (?:async )?load\b"Repository: seerr-team/seerr
Length of output: 50372
🏁 Script executed:
# First, get the full overseerrMerge.ts file to understand context
wc -l server/lib/overseerrMerge.tsRepository: seerr-team/seerr
Length of output: 93
🏁 Script executed:
# Get the full overseerrMerge.ts file
cat -n server/lib/overseerrMerge.tsRepository: seerr-team/seerr
Length of output: 50372
🏁 Script executed:
# Look for Settings class and load method
fd -t f "settings" "server/lib/settings/" | head -10Repository: seerr-team/seerr
Length of output: 124
🏁 Script executed:
# Search for migration or merge logic between old and new paths
rg -n "notifications\.agents\." server/lib/ --max-count 20Repository: seerr-team/seerr
Length of output: 484
🏁 Script executed:
# Search specifically in settings-related files for migration
rg -n "agentTemplates|notification\." server/lib/settings/ -A 2 -B 2Repository: seerr-team/seerr
Length of output: 1345
🏁 Script executed:
# Find the Settings class definition and load method
head -300 server/lib/settings/index.ts | tail -100Repository: seerr-team/seerr
Length of output: 2516
🏁 Script executed:
# Search for Settings class and async load method
rg -n "class Settings|async load" server/lib/settings/index.ts -A 10 | head -80Repository: seerr-team/seerr
Length of output: 635
🏁 Script executed:
# Look at migrations directory to see all migrations
fd -t f . server/lib/settings/migrations/ | sortRepository: seerr-team/seerr
Length of output: 541
🏁 Script executed:
# Check if there's a migration that handles notifications.agents → agentTemplates
rg -n "notifications.agents|agentTemplates" server/lib/settings/migrations/ -B 2 -A 5Repository: seerr-team/seerr
Length of output: 917
🏁 Script executed:
# Get the full Settings.load method to see what raw parameter does
sed -n '499,600p' server/lib/settings/index.tsRepository: seerr-team/seerr
Length of output: 2302
🏁 Script executed:
# Look for where migrations are applied
rg -n "migration" server/lib/settings/index.ts -i | head -20Repository: seerr-team/seerr
Length of output: 480
🏁 Script executed:
# Check how the settings are parsed from JSON - look for JSON parsing
rg -n "JSON.parse|readFileSync" server/lib/settings/index.ts -A 3 -B 1 | head -40Repository: seerr-team/seerr
Length of output: 418
🏁 Script executed:
# Look for any notification/agent structure migration
rg -n "notifications|agentTemplates" server/lib/settings/migrations/ -lRepository: seerr-team/seerr
Length of output: 114
🏁 Script executed:
# Find and examine the migrator file
cat server/lib/settings/migrator.tsRepository: seerr-team/seerr
Length of output: 3293
🏁 Script executed:
# Also check if there's a migration for notification structure changes
rg -n "notification" server/lib/settings/migrations/ -lRepository: seerr-team/seerr
Length of output: 114
🏁 Script executed:
# Verify that load with raw=true skips migrations by checking the exact flow
sed -n '516,530p' server/lib/settings/index.tsRepository: seerr-team/seerr
Length of output: 502
🏁 Script executed:
# Double-check what happens when raw=true - confirm it doesn't call runMigrations
rg -n "raw" server/lib/settings/index.ts -B 2 -A 2Repository: seerr-team/seerr
Length of output: 640
🏁 Script executed:
# Search all migrations to confirm none handle notifications.agents → agentTemplates conversion
cat server/lib/settings/migrations/*.ts | grep -i "notification\|agent" || echo "No notification migrations found"Repository: seerr-team/seerr
Length of output: 219
🏁 Script executed:
# Verify the default value for senderName in new structure
rg -n "senderName.*Seerr" server/lib/settings/index.tsRepository: seerr-team/seerr
Length of output: 99
Legacy senderName is never migrated to the new path because migrations are skipped.
The code calls Settings().load(undefined, true) with raw = true, which bypasses all migrations (line 516: if (data && !raw) evaluates to false). Additionally, there is no migration that converts settings.notifications.agents.email.options.senderName (old Overseerr path) to settings.notification.agentTemplates.email.options.senderName (new path). Since the new path defaults to 'Seerr' (per server/lib/settings/index.ts:124), this check will never match 'Overseerr' for real Overseerr installations, making the rebrand replacement a no-op.
Either apply migrations before the check, or read from the legacy path if the new path contains the default value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/lib/overseerrMerge.ts` around lines 143 - 148, The check against the
new path settings.notification.agentTemplates.email.options.senderName never
sees legacy "Overseerr" because Settings().load(undefined, true) skips
migrations; fix by either loading with migrations (call
Settings().load(undefined, false) or remove the raw=true) or, if you must skip
migrations, detect and migrate from the legacy key
settings.notifications.agents.email.options.senderName whenever the new path
equals the default "Seerr" — copy the legacy value into
settings.notification.agentTemplates.email.options.senderName (and persist/save
the Settings) so the rebrand replacement actually runs.
| notificationRoutes.post('/', async (req, res, next) => { | ||
| const settings = getSettings(); | ||
| const instances = settings.notification.instances; | ||
|
|
||
| const notificationInstanceId = findFirstFreeNotificationInstanceId(); | ||
|
|
||
| const request = req.body; | ||
| request.id = notificationInstanceId; | ||
|
|
||
| const notificationAgent = createAccordingNotificationAgent( | ||
| request, | ||
| notificationInstanceId | ||
| ); | ||
|
|
||
| if (!notificationAgent) { | ||
| return next({ | ||
| status: 500, | ||
| message: 'A valid agent is missing from the request.', | ||
| }); | ||
| } | ||
|
|
||
| notificationManager.registerAgent(notificationAgent); | ||
|
|
||
| const notificationInstanceIndex = instances.length; | ||
| instances[notificationInstanceIndex] = request; | ||
| await settings.save(); | ||
|
|
||
| res.status(200).json(instances[notificationInstanceIndex]); | ||
| }); |
There was a problem hiding this comment.
Agent registered before persistence — partial-failure leaves inconsistent state.
If notificationManager.registerAgent(notificationAgent) succeeds but settings.save() throws on Line 112, the manager has a live agent that isn't reflected in persisted settings. On the next process restart the registration is lost, but until then the in-memory state diverges from disk. Consider persisting first and registering only after a successful save (or rolling back the registration in a catch).
🛡️ Suggested ordering
notificationManager.registerAgent(notificationAgent);
const notificationInstanceIndex = instances.length;
- instances[notificationInstanceIndex] = request;
- await settings.save();
+ instances[notificationInstanceIndex] = request;
+ try {
+ await settings.save();
+ } catch (e) {
+ notificationManager.unregisterAgent(notificationInstanceId);
+ instances.splice(notificationInstanceIndex, 1);
+ return next({ status: 500, message: 'Failed to persist notification instance.' });
+ }The same concern applies to the POST /:id and DELETE /:id handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/settings/notification.ts` around lines 87 - 115, The code
registers the notification agent with notificationManager.registerAgent before
persisting the new instance via settings.save, risking in-memory/disk divergence
on save failure; update the POST handler (the notificationRoutes.post function)
to persist the new instance into settings.instances and await settings.save()
first, then call
notificationManager.registerAgent(createAccordingNotificationAgent(...)) only
after save succeeds, and for symmetry update the PUT (/:id) and DELETE (/:id)
handlers similarly—or alternatively, if you must register first, wrap
registerAgent in try/catch and call notificationManager.unregisterAgent (or roll
back the registration) when settings.save() throws so in-memory state is
consistent with persisted settings.
… default toggle call
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const defaultInstance = retrieveDefaultNotificationInstanceSettings( | ||
| request.agent | ||
| ); | ||
| defaultInstance.default = false; |
| const defaultInstance = retrieveDefaultNotificationInstanceSettings( | ||
| request.agent | ||
| ); | ||
| defaultInstance.default = false; |
|
Hello @Schrottfresser, Sorry for the delay, but this PR cannot be reviewed as-is due to the number of modifications in a single PR. We need to discuss how to split this into multiple PRs, using a dedicated branch, similar to what we did in #3109. |
Hi! Thanks for checking. |
|
For now, we are working on using a template framework for notifications, which will allow users to customize notification payloads. The work has already started here. Once this is done, I think we can continue with the work you started to avoid having too much ongoing work on the notification system at the same time. |
|
@M0NsTeRRR To start with splitting up this PR I created another one for extracting the settings interfaces: #3216. |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
7 participants
Description
This PR will enable Jellyseerr to have multiple instances of notification agents. Therefore the entire notifications frontend was reworked and all notifications endpoints and the data structure was reworked.
For email password resetting and such features a default agent can be selected for each agent type.
Screenshots / Logs (if applicable)
How Has This Been Tested?
This new functionality has been successfully tested by me with my home server and Jellyfin instance. There was no testing done with Plex although this PR is not supposed to change anything related to it.
Not every notification type has been tested because the logic wasn't changed for any. Registration and de registration of instances was tested as well as UI behavior (for every type) and the new default instance logic.
Anyone interested in this feature (especially with a Plex instance) is very welcome to checkout this branch and test it out!
Checklist:
pnpm buildpnpm i18n:extractDo I need to do a migration for this and move the old notification instances in the new logic and setting them as default respectively?
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes