feat(notifications): add Discord thread ID support#3065
Conversation
Allow Discord webhook notifications to be sent to a specific thread by optionally configuring a thread ID. When set, the thread_id query parameter is appended to the webhook URL as documented in the Discord API. Closes seerr-team#2719
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds Discord webhook thread support: settings gain an optional ChangesDiscord Webhook Thread Support
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/lib/notifications/agents/discord.ts`:
- Around line 318-323: The current construction of webhookUrl by
string-concatenating `?thread_id=...` can produce malformed URLs when
`settings.options.webhookUrl` already has query params; update the logic that
sets `webhookUrl` to create a new URL object from `settings.options.webhookUrl`,
call `url.searchParams.set('thread_id', settings.options.webhookThreadId)` when
`settings.options.webhookThreadId` is present, and then use `url.toString()` for
the value passed to `axios.post`; replace the existing string-concat branch that
assigns `webhookUrl` and ensure any subsequent use (the `axios.post(webhookUrl,
...)` call) uses the safe URL string.
In `@src/components/Settings/Notifications/NotificationsDiscord.tsx`:
- Around line 75-80: The Yup schema for webhookThreadId in
NotificationsDiscord.tsx currently uses .nullable() but treats empty string ""
as invalid; update the validation for webhookThreadId to accept empty strings by
transforming them to null before applying the regex or marking the field not
required. For example, on the webhookThreadId field, add .transform(value =>
(value === "" ? null : value)) (or .notRequired()) prior to
.matches(/^\d{17,19}$/, ...) so empty input is allowed while still validating
numeric IDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 426d3320-acf6-4e63-ae09-a521f3522048
📒 Files selected for processing (4)
server/lib/notifications/agents/discord.tsserver/lib/settings/index.tssrc/components/Settings/Notifications/NotificationsDiscord.tsxsrc/i18n/locale/en.json
…lidation Use the URL API instead of string concatenation to append thread_id as a query parameter, preventing malformed URLs when the webhook URL already contains query params. Also transform empty string to null before Yup regex validation so leaving the Thread ID field blank does not trigger a validation error.
|
Same as a previous PR; this does not address the real issue. Please see #3013 (comment) |
fallenbagel
added
blocked
Yes, the linked issue is wrong. But the feature itself is not wrong -- we may want to add an option on the UI to make it easier to specify a thread ID (for all Discord notifications). |
|
Thanks for the context. To clarify the scope: the Thread ID field is added to the Discord notification agent settings globally, not to a specific notification type, so it applies to all Discord notifications sent through that agent. Happy to update the linked issue if you can point me to a better one. |
As it stands, yes. It doesn't address the originally requested feature. That said, as @gauthier-th mentioned, it could still be a valuable enhancement to improve the UX in the future. So IMO, it would be better to simply remove the linked issue and slightly adjust the description to better reflect the current scope (sending notifications to a thread is already possible, it's just not obvious). |
…idation Yup's .matches() can reject empty strings even when .nullable() is set, because the regex test runs before the null coercion is fully applied. Adding excludeEmptyString: true ensures the optional thread ID field passes validation when left blank.
4 participants
Description
Discord webhooks support posting to a specific thread by passing a
thread_idquery parameter in the webhook URL. While this is already possible by appending the parameter manually to the webhook URL, it's not obvious and easy to get wrong. This PR adds an explicit Thread ID field to the Discord notification agent settings to make thread routing a first-class, discoverable option.How Has This Been Tested?
Ran the dev server on localhost and navigated to Settings → Notifications → Discord. Next, I verified that:
abc) shows the validation error and won't let you saveI did not test against a live Discord webhook.
Screenshots / Logs (if applicable)
None
Checklist:
pnpm buildpnpm i18n:extractAI Disclosure: I used Claude Code to help find the issue and develop the structure of the implementation.
Summary by CodeRabbit