Repository navigation
Conversation
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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:
Walkthrough
Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: replacing the webhook KeyMap system with LiquidJS templating. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit writes Liquid with care,
Templates bloom in webhook air.
JSON hops from text to flight,
URLs render clean and bright.
🐰✨
Comment @coderabbitai help to get the list of available commands.
![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
🧹 Nitpick comments (3)
server/lib/notifications/templateEngine.ts (2)
28-39: Numeric comparison helpers assume numeric inputs without validation.The
lt,gt,lte,gtehelpers cast inputs tonumberbut don't validate. Comparing non-numeric values (strings, objects) producesNaNcomparisons that always returnfalse, which could be confusing in templates.🔧 Optional: Add type checking or documentation
Either add runtime validation:
Handlebars.registerHelper('lt', (a: unknown, b: unknown) => { + if (typeof a !== 'number' || typeof b !== 'number') return false; return (a as number) < (b as number); });Or document that these helpers are intended for numeric comparisons only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/notifications/templateEngine.ts` around lines 28 - 39, The numeric comparison Handlebars helpers (lt, gt, lte, gte) currently cast inputs to number without validation, causing NaN comparisons to always return false; update each helper (Handlebars.registerHelper('lt'|'gt'|'lte'|'gte')) to coerce inputs safely (e.g., use Number(...) or parseFloat(...)) and check isNaN on both operands, then either return false or throw/log a descriptive error when values are not numeric, otherwise perform the numeric comparison; ensure the change is applied to all four helper registrations for consistent behavior and consider adding a short comment documenting that these helpers expect numeric inputs.
98-192: Consider exposing additional payload fields to templates.The
NotificationPayloadinterface includespendingRequestsCountandisAdminfields that aren't mapped to the template context. These could be useful for conditional template content (e.g., showing pending count in admin notifications).➕ Optional: Add missing context variables
comment: payload.comment ?? null, + + // Additional payload fields + pending_requests_count: payload.pendingRequestsCount ?? 0, + is_admin: payload.isAdmin ?? false, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/notifications/templateEngine.ts` around lines 98 - 192, The buildContext method is missing mappings for NotificationPayload.pendingRequestsCount and NotificationPayload.isAdmin; update the static buildContext function to add context keys (e.g., pendingRequestsCount and isAdmin or notifyuser_isAdmin) populated from payload.pendingRequestsCount (number) and payload.isAdmin (boolean) so templates can use them, and ensure the values are included alongside the other fields in the context object returned by buildContext.src/components/Settings/Notifications/NotificationsWebhook/index.tsx (1)
137-158: Validation allows syntactically invalid templates to be saved.The dual-mode validation correctly detects Handlebars syntax, but templates with structural issues (unclosed blocks, mismatched braces, invalid JSON structure after rendering) will only fail at webhook send time with errors logged server-side. Users won't see validation feedback until they test.
This is acknowledged in the past review discussion—full frontend Handlebars validation would significantly increase bundle size. The current approach is a reasonable trade-off given that constraint, but consider adding a note in the UI or documentation that template syntax errors will only appear in server logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Settings/Notifications/NotificationsWebhook/index.tsx` around lines 137 - 158, The validator for 'validate-json-or-template' in NotificationsWebhook/index.tsx currently accepts any string containing Handlebars markers even if the template has structural errors; update the UI to inform users that Handlebars/template syntax errors are only validated server-side by (a) adding or updating an intl message (used via intl.formatMessage) that clearly states "Template syntax errors (unclosed blocks, mismatched braces) are only reported by the server" and (b) rendering that message near the payload input or validation hint in the NotificationsWebhook component so users see it at save time; keep the existing validator logic unchanged but surface the doc/notice via the same component that uses validate-json-or-template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/lib/notifications/agents/webhook.ts`:
- Around line 37-44: The current JSON cleanup on renderedString (in
server/lib/notifications/agents/webhook.ts) uses regexes that can corrupt string
values; change the flow to first attempt a strict JSON.parse of renderedString
and only run the destructive regex cleanup as a recovery path if parsing fails,
then re-attempt JSON.parse and throw/log a clear error if it still fails; locate
the code that performs the replacements on renderedString and wrap them behind
the "parse first, cleanup on failure, parse again" logic so valid payloads are
never modified by the regexes.
In `@server/lib/notifications/templateEngine.ts`:
- Around line 76-80: The 'json' Handlebars helper (registered via
Handlebars.registerHelper('json', ...)) uses JSON.stringify directly and can
throw on circular references because full entities (media, request, issue,
comment) are in the template context; update the helper to safely stringify by
wrapping JSON.stringify in a try-catch and returning a safe fallback (e.g.,
"[Circular]" or "{}") on error, or replace the implementation to use a
circular-safe stringifier (e.g., fast-safe-stringify or a custom replacer that
tracks seen objects) so templates never crash when encountering bidirectional
ORM relations.
---
Nitpick comments:
In `@server/lib/notifications/templateEngine.ts`:
- Around line 28-39: The numeric comparison Handlebars helpers (lt, gt, lte,
gte) currently cast inputs to number without validation, causing NaN comparisons
to always return false; update each helper
(Handlebars.registerHelper('lt'|'gt'|'lte'|'gte')) to coerce inputs safely
(e.g., use Number(...) or parseFloat(...)) and check isNaN on both operands,
then either return false or throw/log a descriptive error when values are not
numeric, otherwise perform the numeric comparison; ensure the change is applied
to all four helper registrations for consistent behavior and consider adding a
short comment documenting that these helpers expect numeric inputs.
- Around line 98-192: The buildContext method is missing mappings for
NotificationPayload.pendingRequestsCount and NotificationPayload.isAdmin; update
the static buildContext function to add context keys (e.g., pendingRequestsCount
and isAdmin or notifyuser_isAdmin) populated from payload.pendingRequestsCount
(number) and payload.isAdmin (boolean) so templates can use them, and ensure the
values are included alongside the other fields in the context object returned by
buildContext.
In `@src/components/Settings/Notifications/NotificationsWebhook/index.tsx`:
- Around line 137-158: The validator for 'validate-json-or-template' in
NotificationsWebhook/index.tsx currently accepts any string containing
Handlebars markers even if the template has structural errors; update the UI to
inform users that Handlebars/template syntax errors are only validated
server-side by (a) adding or updating an intl message (used via
intl.formatMessage) that clearly states "Template syntax errors (unclosed
blocks, mismatched braces) are only reported by the server" and (b) rendering
that message near the payload input or validation hint in the
NotificationsWebhook component so users see it at save time; keep the existing
validator logic unchanged but surface the doc/notice via the same component that
uses validate-json-or-template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf513910-243d-4b52-b2c1-891a296bd5bd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.jsonserver/lib/notifications/agents/webhook.tsserver/lib/notifications/templateEngine.tssrc/components/Settings/Notifications/NotificationsWebhook/index.tsx
There was a problem hiding this comment.
Pull request overview
This PR refactors the webhook notification agent’s variable substitution from a custom KeyMap system to a centralized Handlebars-based templating engine, enabling richer template features in webhook URLs and JSON payloads.
Changes:
- Added a new
TemplateEngine(Handlebars + custom helpers) to render notification templates. - Updated the webhook agent to render payloads/URLs via
TemplateEngineand added post-render JSON cleanup/fallback parsing. - Relaxed frontend webhook payload validation to allow Handlebars syntax while still validating strict JSON when no template syntax is detected.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/components/Settings/Notifications/NotificationsWebhook/index.tsx |
Adjusts Yup validation to permit Handlebars syntax in the JSON payload editor. |
server/lib/notifications/templateEngine.ts |
Introduces Handlebars rendering + helper registration and context building for notifications. |
server/lib/notifications/agents/webhook.ts |
Replaces KeyMap substitution with TemplateEngine rendering for payload and URL. |
package.json |
Adds handlebars dependency. |
pnpm-lock.yaml |
Lockfile updates for handlebars and related dependency graph changes. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
M0NsTeRRR
left a comment
M0NsTeRRR
left a comment
There was a problem hiding this comment.
Hello,
Sorry for the delay here’s some feedback before I test it! :)
4644d24 to
7dda3e7
Compare
github-actions
Bot
added
blocked:template
and removed
merge conflict
mmenanno
changed the title
M0NsTeRRR
left a comment
M0NsTeRRR
left a comment
There was a problem hiding this comment.
Hey,
An other review before testing out your PR let me know if something isn't clear or does not make sense.
|
Hey guys, Any timeline for merging and releasing this PR? |
No ETA. If this is already reviewed, then it will be included in 3.4.0 and you can find it in the roadmap issue that links to the milestones. |
This PR recently became active again, during the seerr transition this PR was waiting on a review for a bit. I'm currently working on the feedback I got on the PR yesterday and should have that up soon. So there is active effort on this currently to get it over the finish line |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/settings/notifications.ts (1)
302-309: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMark
options.jsonPayloadas required
WebhookSettingsonly declaresjsonPayloadas a string, so/webhookand/webhook/testcan still hitBuffer.from(req.body.options.jsonPayload)with missing input and return a 500. Add the field torequiredor guard before encoding.🤖 Prompt for 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. In `@server/routes/settings/notifications.ts` around lines 302 - 309, The webhook settings update path is allowing settings.notifications.agents.webhook to encode req.body.options.jsonPayload without guaranteeing it is present. Update the /webhook and /webhook/test request validation to require options.jsonPayload, or add a guard in the webhook handler before Buffer.from is called so missing input is rejected with a client error instead of causing a 500. Use the webhook settings update logic and the webhook/test route handlers to locate the fix.Source: Learnings
🧹 Nitpick comments (1)
server/lib/notifications/agents/webhook.ts (1)
63-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFail-closed error handling for URL/payload rendering looks correct.
This addresses the earlier reviewer concern about not proceeding after a template render failure ("I don't think we should move forward if there's a problem"), and the request to catch/log template errors clearly.
The two try/catch blocks (URL render, payload build) are structurally identical (render → catch → log with same fields → return false). Consider extracting a small helper to reduce duplication, e.g.:
+function renderOrFail<T>( + fn: () => T, + errorLabel: string, + type: Notification, + payload: NotificationPayload +): { ok: true; value: T } | { ok: false } { + try { + return { ok: true, value: fn() }; + } catch (error) { + logger.error(errorLabel, { + label: 'Notifications', + type: Notification[type], + subject: payload.subject, + errorMessage: error instanceof Error ? error.message : 'Unknown error', + }); + return { ok: false }; + } +}Optional, purely a DRY cleanup.
🤖 Prompt for 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. In `@server/lib/notifications/agents/webhook.ts` around lines 63 - 93, The webhook template error handling in webhook.ts already fail-closes correctly, but the duplicated try/catch blocks around TemplateEngine.render and this.buildPayload should be refactored into a small shared helper. Extract the common “render/catch/log/return false” flow into a reusable function or method, and keep the existing logger.error fields (label, type, subject, errorMessage) unchanged so the webhook URL and payload paths use the same centralized logic.
🤖 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 `@docs/using-seerr/notifications/webhook.md`:
- Around line 50-52: Add a language identifier to the fenced code blocks in the
webhook markdown examples so markdownlint no longer reports MD040. Update the
affected code fences in the notification webhook docs to use a specific language
(for example, liquid) while keeping the existing content unchanged; the relevant
snippets are the templated blocks containing media_tmdbid and the media_status
conditional.
---
Outside diff comments:
In `@server/routes/settings/notifications.ts`:
- Around line 302-309: The webhook settings update path is allowing
settings.notifications.agents.webhook to encode req.body.options.jsonPayload
without guaranteeing it is present. Update the /webhook and /webhook/test
request validation to require options.jsonPayload, or add a guard in the webhook
handler before Buffer.from is called so missing input is rejected with a client
error instead of causing a 500. Use the webhook settings update logic and the
webhook/test route handlers to locate the fix.
---
Nitpick comments:
In `@server/lib/notifications/agents/webhook.ts`:
- Around line 63-93: The webhook template error handling in webhook.ts already
fail-closes correctly, but the duplicated try/catch blocks around
TemplateEngine.render and this.buildPayload should be refactored into a small
shared helper. Extract the common “render/catch/log/return false” flow into a
reusable function or method, and keep the existing logger.error fields (label,
type, subject, errorMessage) unchanged so the webhook URL and payload paths use
the same centralized logic.
🪄 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: 05a64d18-3391-4525-86a9-bf8426532472
📒 Files selected for processing (6)
docs/using-seerr/notifications/webhook.mdserver/lib/notifications/agents/webhook.tsserver/lib/notifications/templateEngine.test.tsserver/lib/settings/index.tsserver/lib/settings/migrations/0009_migrate_webhook_payload_encoding.tsserver/routes/settings/notifications.ts
✅ Files skipped from review due to trivial changes (1)
- server/lib/settings/migrations/0009_migrate_webhook_payload_encoding.ts
| @@ -0,0 +1,41 @@ | |||
| import type { AllSettings } from '@server/lib/settings'; | |||
There was a problem hiding this comment.
- What is the purpose of this migration? Your AI seems to have hallucinated again. Please reread what was done here. Is this migration needed?
- Please read
CONTRIBUTING.mdthis is not the way to generate db migration file if you need it.
There was a problem hiding this comment.
The migration unwraps a storage-format change in the save route, not anything about the removed frontend validation: on develop, POST /webhook stores Buffer.from(JSON.stringify(req.body.options.jsonPayload)), while this PR stores the raw text. Without it, existing installs decode to a quoted JSON string literal and the agent would post a string body instead of the rendered object.
On the second point this is a settings.json migration following the existing server/lib/settings/migrations/0001–0008 pattern (auto-run by migrator.ts), not a TypeORM db migration, so the CONTRIBUTING.md migration:generate flow doesn’t apply. I’ve expanded the file’s header comment to make this explicit.
There was a problem hiding this comment.
The migration unwraps a storage-format change in the save route, not anything about the removed frontend validation: on
develop,POST /webhookstoresBuffer.from(JSON.stringify(req.body.options.jsonPayload)), while this PR stores the raw text. Without it, existing installs decode to a quoted JSON string literal and the agent would post a string body instead of the rendered object.
Wouldn't raw text make settings.json way too long. Why aren't we storing like how we did before?
There was a problem hiding this comment.
Sorry, "raw text" was poor wording on my part. It’s still base64 in settings.json, same as before. The only difference is that the PR drops the extra JSON.stringify layer:
develop:base64(JSON.stringify(template)), which is why the agent had to doJSON.parse(JSON.parse(...))- this PR:
base64(template)
So the stored value actually gets slightly shorter: the default payload is 2700 chars versus 2936 before, because the escaped quotes and \ns are gone. The migration only exists to remove that extra layer from existing installs.
That said, if you’d rather not have a migration, I can keep the old base64(JSON.stringify(...)) format and unwrap it when reading. That leaves the storage format untouched and lets me delete 0009 entirely. Happy to go either way.
|
Hey @M0NsTeRRR maybe you are just busy but thought I'd mention this is read for review in case the last notification didn't come through. I noticed the pr still has the |
|
Hey, yes, we’re currently focusing on bug fixes, but your PR will continue to be reviewed once we’re done with these. :) |
|
@M0NsTeRRR Any idea when this might get picked up for review? |
fallenbagel
changed the title
|
As you can see an other maintainer added a comment #2039 (comment) and he is waiting for an answer from @mmenanno |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Replace the bespoke KeyMap substitution in the webhook agent with a shared
LiquidJS-based TemplateEngine, laying groundwork for templating across all
notification agents.
- Add TemplateEngine (server/lib/notifications/templateEngine.ts) wrapping
LiquidJS. Liquid provides conditionals, comparisons, and filters (default,
upcase, truncate, json, url_encode, …) natively, so no custom helpers are
needed. Liquid does not HTML-escape output, so apostrophes etc. render
verbatim. The built-in json filter is overridden to be circular-safe and to
log rather than drop data silently.
- buildContext exposes the same scalar variables as the previous KeyMap
(including discordIds, media_imdbid, jellyfinMediaId, plexRatingKey and
requestedBy_jellyfinUserId) plus the full media/request/issue/comment/extra
values for conditionals.
- webhook agent renders the stored template to a string and lets JSON.parse
validate the result; no regex payload cleanup. customHeaders support is
retained.
- Relax server-side and client-side payload validation: the payload is now a
Liquid template that need not be valid JSON until rendered. The default
payload uses {% if %}…{% else %}null{% endif %} blocks so optional sections
stay null when absent (matching prior behavior), and emits free-text values
(titles, overviews, display names, comments) through the json filter so
characters like quotes and newlines cannot break the surrounding JSON.
- URL variable substitution renders raw; use the url_encode filter where
encoding is needed.
- Add unit tests for TemplateEngine (escaping, optional-section null handling,
circular-safe json filter, and buildContext variable mapping).
…ad storage Addresses review feedback on the LiquidJS webhook refactor: - Fail closed when a webhook URL or payload template fails to render, logging a clear error instead of POSTing to an unrendered URL. - Store the payload as a raw base64 template (drop the JSON.stringify/parse envelope) and add migration 0009 to re-encode existing settings. - Fix the server-side default payload to the LiquidJS template so fresh installs render valid JSON. - Document LiquidJS template syntax and link the NotificationPayload interface; mark the flat variables as deprecated. - Add a test covering undefined template variables.
Adds a language identifier to the two bare fenced code blocks in the webhook notification guide to satisfy markdownlint MD040, and registers liquid as an additional Prism language so Docusaurus highlights them without emitting an unknown-language warning.
- Make TemplateEngine.render async via parseAndRender, awaited in the already-async WebhookAgent.send - Test WebhookAgent.send outcomes (rendered payload posted, render/parse failures, URL template failure, post failure) with mocked axios instead of exercising LiquidJS engine behavior - Trim templateEngine tests to our own code: buildContext mapping and the circular-safe json filter - Clarify the 0009 settings migration comment with the storage-format change that makes it necessary - Link the intro-to-Liquid tutorial from the webhook docs
73387c6 to
4483ec8
Compare
github-actions
Bot
removed
the
merge conflict
Description
This PR refactors the webhook notification agent to use a shared LiquidJS-based templating engine instead of the bespoke
KeyMap+parseKeyssubstitution system, providing a foundation for standardized templating across all notification agents.This replaces the original Handlebars approach from earlier in this PR, following the maintainer discussion (review thread): Liquid is a better fit for an end-user template engine because it ships the logic/filters users need out of the box, so we don't have to implement and maintain custom helpers.
This is part of the larger effort to enable custom templates across notification agents; it lays the groundwork for extending template support to all agents in future PRs. No specific issue to link.
Key Changes
TemplateEngine(server/lib/notifications/templateEngine.ts) wrapping a shared LiquidJS engine.==,>, …), and filters (default,upcase,downcase,capitalize,truncate,json,url_encode) natively. This also resolves several review points for free: numeric comparison coercion,defaulton empty strings, and no HTML-escaping (apostrophes/quotes render verbatim).jsonfilter — the built-injsonfilter is overridden so circular TypeORM relations (e.g.Media↔Issue) can't crash rendering.buildContextexposes the same scalar variables as the oldKeyMap(includingdiscordIds,media_imdbid,jellyfinMediaId,plexRatingKey,requestedBy_jellyfinUserId) plus the fullmedia/request/issue/comment/extravalues for conditionals.JSON.parse; the previous regex "cleanup" of the rendered payload was removed.JSON.parsechecks on save/test were removed. Templates are validated by using the Test button.customHeaderssupport retained.TemplateEngine(escaping, optional-sectionnullhandling, circular-safejson, and variable mapping).New Templating Capabilities
Webhook payloads and URLs can now use Liquid:
{% if media %}…{% else %}…{% endif %},{% if media_status == "AVAILABLE" %}…{% endif %}{% if pendingRequestsCount > 10 %}…{% endif %}{{ notifyuser_username | capitalize }},{{ media_type | upcase }},{{ subject | truncate: 50 }},{{ subject | json }},{{ value | url_encode }}{{ notifyuser_username | default: "Unknown User" }}The default payload now uses
{% if %}…{% else %}null{% endif %}so optional sections (media/request/issue/comment) render asnullwhen absent (matching prior behavior) while staying valid JSON, and emits free-text values (titles, overviews, display names, comments) through thejsonfilter so quotes/newlines can't break the surrounding JSON.Compatibility Notes
{{ variable }}substitution continues to work unchanged."{{media}}","{{request}}", …) should be rewritten as{% if %}blocks (the Reset to Default button produces an up-to-date example). Filter names follow Liquid (upcase/downcaserather thanupper/lower).url_encodefilter where encoding is needed.Review Feedback Addressed
notifyUser→ requester fallback sonotifyuser_*maps tonotifyUseronly, matching the previousKeyMap(M0NsTeRRR).url_encodeis available for URL templating.How Has This Been Tested?
Tested locally with
pnpm devagainst a webhook.site endpoint, plus automated unit tests.Manual:
nullotherwise.Automated:
pnpm test server/lib/notifications/templateEngine.test.ts(escaping, optional-sectionnullhandling, circular-safejson, andbuildContextvariable mapping).pnpm build,pnpm typecheck, andpnpm lintall pass.Screenshots / Logs (if applicable)
N/A — backend refactor (frontend default payload/validation updated, UI unchanged).
Checklist:
pnpm buildpnpm i18n:extract— no new translation keys addedSummary by CodeRabbit
New Features
Bug Fixes
Documentation