feat(notifications): validate template vars and clean up stale push subscriptions#162
feat(notifications): validate template vars and clean up stale push subscriptions#162ochemegina-ai wants to merge 1 commit into
Conversation
…ubscriptions Add structured TemplateRenderResult to prevent partial email sends when required template variables are missing or rendering fails. Add PushSubscriptionCleanupResult and cleanupPushSubscriptions() to remove expired or repeatedly failing push subscriptions from the store. Thresholds are configurable via PUSH_MAX_FAILURES and PUSH_STALE_DAYS env vars. Add vitest and unit tests covering success and failure paths for both features. Closes DelegoLabs#136 Closes DelegoLabs#137
|
@ochemegina-ai Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughNotification emails now render named templates into HTML/text results with structured render errors, and push subscriptions now live in an in-memory store with failure/age-based cleanup and delivery tracking. Vitest coverage was added for both notification paths. ChangesEmail template rendering
Push subscription cleanup
Sequence Diagram(s)sequenceDiagram
participant sendTemplatedEmail
participant renderTemplate
participant sendEmail
sendTemplatedEmail->>renderTemplate: templateName, vars
renderTemplate-->>sendTemplatedEmail: TemplateRenderResult
alt render error
sendTemplatedEmail-->>sendTemplatedEmail: return structured error
else render success
sendTemplatedEmail->>sendEmail: EmailMessage.body
sendEmail-->>sendTemplatedEmail: resolve or throw
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Warning |
ochemegina-ai
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/backend/notifications/tsconfig.json (1)
13-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
tests/**/*is included in thetscbuild output.Since
buildruns plaintscwithoutDir: ./dist, includingtests/**/*will emit compiled test files into the production bundle. Type-checking tests is desirable, but they shouldn't ship. Consider excluding tests from the build (e.g.exclude: ["tests/**/*"]here, with a separate tsconfig for typecheck/Vitest).🤖 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 `@apps/backend/notifications/tsconfig.json` around lines 13 - 19, The tsconfig for notifications is currently including tests in the main TypeScript build, which causes compiled test files to be emitted into the production bundle. Update the configuration around the include list to keep production sources under build output while removing tests from the emit path, and if tests still need type-checking, point them to a separate config for test/Vitest usage. Use the existing notifications tsconfig include settings and the build setup that runs plain tsc with outDir to locate the change.apps/backend/notifications/email/index.ts (1)
51-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlain-text body is discarded.
renderTemplateproduces bothhtmlandtext, butEmailMessageonly carries a singlebodyandsendTemplatedEmaildropsrendered.text. Once SMTP is wired, a multipart (HTML + text alternative) is generally preferred for deliverability. Consider extendingEmailMessageto carry both so the text part isn't lost here.🤖 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 `@apps/backend/notifications/email/index.ts` around lines 51 - 76, The plain-text alternative is being dropped in sendTemplatedEmail, so update the email flow to preserve both renderTemplate outputs. Extend EmailMessage and the sendEmail/sendTemplatedEmail path to accept both html and text (or equivalent multipart fields), and use the rendered text instead of collapsing everything into a single body in sendTemplatedEmail. Keep the change centered around sendTemplatedEmail, EmailMessage, and sendEmail so the multipart payload can be passed through cleanly.apps/backend/notifications/tests/push-cleanup.test.ts (1)
147-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStaleness "exactly at boundary" test passes only by timing luck.
daysAgo(30)is computed at registration, whilestaleCutoffis computed a few ms later insidecleanupPushSubscriptions, sonew Date(lastActiveAt) < staleCutoffis true purely because of the elapsed wall-clock time between the two calls — not because the boundary itself is inclusive. The "exactly the boundary" intent isn't deterministically tested. Consider freezing time withvi.useFakeTimers()/vi.setSystemTime()(you already importvi) so the boundary semantics are asserted deterministically.🤖 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 `@apps/backend/notifications/tests/push-cleanup.test.ts` around lines 147 - 157, The “exactly at boundary” push cleanup test is timing-dependent because daysAgo(30) and cleanupPushSubscriptions() use slightly different wall-clock instants. Update the push-cleanup test in push-cleanup.test.ts to freeze time with vi.useFakeTimers()/vi.setSystemTime() around addPushSubscription, daysAgo(30), and cleanupPushSubscriptions so the staleness boundary is asserted deterministically rather than by millisecond drift.apps/backend/notifications/push/index.ts (1)
131-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
failedcounter is unreachable with the in-memoryMap.
Map.prototype.deletedoes not throw, so thecatchon lines 141-143 never runs andfailedis always0. The result field is therefore meaningless until the real DB/Redis layer lands, and thefailed: 0assertions in the tests (e.g. lines 122, 190) don't actually exercise a failure path. This is fine as forward-looking scaffolding for the persisted implementation, but consider a brief note so it isn't mistaken for live error tracking.📝 Optional clarifying comment
if (isFailureExceeded || isStale) { + // NOTE: Map.delete never throws; `failed` only becomes reachable once + // this is backed by a DB/Redis layer where deletion can error. try { subscriptions.delete(endpoint); removed += 1; } catch { failed += 1; } }🤖 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 `@apps/backend/notifications/push/index.ts` around lines 131 - 148, The `failed` counter in the subscription cleanup logic is currently unreachable because `subscriptions` is an in-memory `Map` and `Map.prototype.delete` does not throw. In `removeStaleSubscriptions`, either add a brief code comment or adjust the implementation to make it clear this `failed` path is scaffolding for the future persisted store, not live error tracking. Keep the `removed`/`failed` return shape in place for now, but make sure the intent is explicit near the `for (const [endpoint, sub] of subscriptions)` loop and the `subscriptions.delete(endpoint)` call so it is not mistaken for real failure handling.
🤖 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 `@apps/backend/notifications/push/index.ts`:
- Around line 101-110: The cleanup cutoff documentation in recordDeliveryFailure
and the surrounding PUSH_MAX_FAILURES comments is off by one compared to the
actual behavior in cleanupPushSubscriptions, which removes subscriptions when
failureCount >= maxFailures. Update the wording near recordDeliveryFailure and
the threshold description to match the intended inclusive cutoff, using the
existing symbols cleanupPushSubscriptions, recordDeliveryFailure, and
PUSH_MAX_FAILURES so the semantics are unambiguous.
In `@apps/backend/notifications/templates/index.ts`:
- Around line 42-46: The interpolate helper is inserting vars[key] directly into
the HTML template, which allows HTML injection in rendered emails. Update
interpolate in templates/index.ts to escape substituted values before
replacement, while keeping the plain-text derivation path unchanged and
unescaped. Use the existing interpolate function and its callers to ensure only
HTML output is escaped, not the raw values used elsewhere.
- Line 12: The template loader in index.ts relies on readFile(join(__dirname,
`${templateName}.html`)), but the HTML templates are not present in the compiled
output, causing TEMPLATE_NOT_FOUND at runtime. Fix this by ensuring the
templates are copied or bundled alongside the built JavaScript for the
notification templates module, and keep the loading logic in
createNotificationTemplate/getTemplate compatible with the emitted
dist/templates layout.
---
Nitpick comments:
In `@apps/backend/notifications/email/index.ts`:
- Around line 51-76: The plain-text alternative is being dropped in
sendTemplatedEmail, so update the email flow to preserve both renderTemplate
outputs. Extend EmailMessage and the sendEmail/sendTemplatedEmail path to accept
both html and text (or equivalent multipart fields), and use the rendered text
instead of collapsing everything into a single body in sendTemplatedEmail. Keep
the change centered around sendTemplatedEmail, EmailMessage, and sendEmail so
the multipart payload can be passed through cleanly.
In `@apps/backend/notifications/push/index.ts`:
- Around line 131-148: The `failed` counter in the subscription cleanup logic is
currently unreachable because `subscriptions` is an in-memory `Map` and
`Map.prototype.delete` does not throw. In `removeStaleSubscriptions`, either add
a brief code comment or adjust the implementation to make it clear this `failed`
path is scaffolding for the future persisted store, not live error tracking.
Keep the `removed`/`failed` return shape in place for now, but make sure the
intent is explicit near the `for (const [endpoint, sub] of subscriptions)` loop
and the `subscriptions.delete(endpoint)` call so it is not mistaken for real
failure handling.
In `@apps/backend/notifications/tests/push-cleanup.test.ts`:
- Around line 147-157: The “exactly at boundary” push cleanup test is
timing-dependent because daysAgo(30) and cleanupPushSubscriptions() use slightly
different wall-clock instants. Update the push-cleanup test in
push-cleanup.test.ts to freeze time with vi.useFakeTimers()/vi.setSystemTime()
around addPushSubscription, daysAgo(30), and cleanupPushSubscriptions so the
staleness boundary is asserted deterministically rather than by millisecond
drift.
In `@apps/backend/notifications/tsconfig.json`:
- Around line 13-19: The tsconfig for notifications is currently including tests
in the main TypeScript build, which causes compiled test files to be emitted
into the production bundle. Update the configuration around the include list to
keep production sources under build output while removing tests from the emit
path, and if tests still need type-checking, point them to a separate config for
test/Vitest usage. Use the existing notifications tsconfig include settings and
the build setup that runs plain tsc with outDir to locate the change.
🪄 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 Plus
Run ID: 90f5fe71-80ed-4f87-a1c6-c74fcb741e19
📒 Files selected for processing (7)
apps/backend/notifications/email/index.tsapps/backend/notifications/package.jsonapps/backend/notifications/push/index.tsapps/backend/notifications/templates/index.tsapps/backend/notifications/tests/push-cleanup.test.tsapps/backend/notifications/tests/templates.test.tsapps/backend/notifications/tsconfig.json
| /** | ||
| * Increment the failure count for a subscription. | ||
| * The subscription will be eligible for cleanup once the count exceeds the | ||
| * PUSH_MAX_FAILURES threshold. | ||
| */ | ||
| export function recordDeliveryFailure(endpoint: string): void { | ||
| const sub = subscriptions.get(endpoint); | ||
| if (!sub) return; | ||
| sub.failureCount += 1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Doc/threshold semantics mismatch: "exceeds" vs >=.
The comment says cleanup happens "once the count exceeds the PUSH_MAX_FAILURES threshold" (line 9 mirrors this with "after this many … failures"), but cleanupPushSubscriptions uses sub.failureCount >= maxFailures (line 134). With the default of 5, a subscription is removed at exactly 5 failures, not 6. The test on line 116 (failureCount: 5 → removed) confirms the >= behavior is intended, so the wording in lines 9 and 103-104 is the part that's off-by-one. Align the docs (or the operator) to avoid confusion about the actual cutoff.
📝 Suggested doc wording fix
/**
* Increment the failure count for a subscription.
- * The subscription will be eligible for cleanup once the count exceeds the
- * PUSH_MAX_FAILURES threshold.
+ * The subscription becomes eligible for cleanup once the count reaches the
+ * PUSH_MAX_FAILURES threshold (failureCount >= PUSH_MAX_FAILURES).
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Increment the failure count for a subscription. | |
| * The subscription will be eligible for cleanup once the count exceeds the | |
| * PUSH_MAX_FAILURES threshold. | |
| */ | |
| export function recordDeliveryFailure(endpoint: string): void { | |
| const sub = subscriptions.get(endpoint); | |
| if (!sub) return; | |
| sub.failureCount += 1; | |
| } | |
| /** | |
| * Increment the failure count for a subscription. | |
| * The subscription becomes eligible for cleanup once the count reaches the | |
| * PUSH_MAX_FAILURES threshold (failureCount >= PUSH_MAX_FAILURES). | |
| */ | |
| export function recordDeliveryFailure(endpoint: string): void { | |
| const sub = subscriptions.get(endpoint); | |
| if (!sub) return; | |
| sub.failureCount += 1; | |
| } |
🤖 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 `@apps/backend/notifications/push/index.ts` around lines 101 - 110, The cleanup
cutoff documentation in recordDeliveryFailure and the surrounding
PUSH_MAX_FAILURES comments is off by one compared to the actual behavior in
cleanupPushSubscriptions, which removes subscriptions when failureCount >=
maxFailures. Update the wording near recordDeliveryFailure and the threshold
description to match the intended inclusive cutoff, using the existing symbols
cleanupPushSubscriptions, recordDeliveryFailure, and PUSH_MAX_FAILURES so the
semantics are unambiguous.
| import { join, dirname } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| const __dirname = dirname(fileURLToPath(import.meta.url)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e html . apps/backend/notifications
cat apps/backend/notifications/package.json
fd -t f -e json -g '*.json' apps/backend/notifications | xargs -I{} sh -c 'echo "== {} =="; cat {}'Repository: DelegoLabs/Delego
Length of output: 2155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/backend/notifications/templates/index.ts =="
cat -n apps/backend/notifications/templates/index.ts
echo
echo "== asset-copy/build references in apps/backend/notifications =="
rg -n "copy|asset|html|templates" apps/backend/notifications/package.json apps/backend/notifications/tsconfig.json apps/backend/notifications -g '!**/dist/**' -g '!**/node_modules/**'Repository: DelegoLabs/Delego
Length of output: 7927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/backend/notifications/templates/index.ts =="
cat -n apps/backend/notifications/templates/index.ts
echo
echo "== asset-copy/build references in apps/backend/notifications =="
rg -n "copy|asset|html|templates" apps/backend/notifications/package.json apps/backend/notifications/tsconfig.json apps/backend/notifications -g '!**/dist/**' -g '!**/node_modules/**'Repository: DelegoLabs/Delego
Length of output: 7927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root package.json scripts =="
cat package.json | sed -n '1,220p'
echo
echo "== build/asset-copy references across repo =="
rg -n "\"build\"|copy.*html|copy.*assets|assets|templates/|approval-request\.html|notifications" . \
-g '!**/dist/**' -g '!**/node_modules/**' -g '!**/.git/**'Repository: DelegoLabs/Delego
Length of output: 7403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root package.json scripts =="
cat package.json | sed -n '1,220p'
echo
echo "== build/asset-copy references across repo =="
rg -n "\"build\"|copy.*html|copy.*assets|assets|templates/|approval-request\.html|notifications" . \
-g '!**/dist/**' -g '!**/node_modules/**' -g '!**/.git/**'Repository: DelegoLabs/Delego
Length of output: 7403
Copy notification templates during build. tsc emits dist/templates/*.js, but it does not copy templates/*.html, so readFile(join(__dirname, \${templateName}.html`))will fail in a built deployment and returnTEMPLATE_NOT_FOUND`. Add an asset-copy step or bundle the templates with the compiled output.
🤖 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 `@apps/backend/notifications/templates/index.ts` at line 12, The template
loader in index.ts relies on readFile(join(__dirname, `${templateName}.html`)),
but the HTML templates are not present in the compiled output, causing
TEMPLATE_NOT_FOUND at runtime. Fix this by ensuring the templates are copied or
bundled alongside the built JavaScript for the notification templates module,
and keep the loading logic in createNotificationTemplate/getTemplate compatible
with the emitted dist/templates layout.
| function interpolate(template: string, vars: Record<string, string>): string { | ||
| return template.replace(/\{\{(\w+)\}\}/g, (match, key: string) => | ||
| Object.prototype.hasOwnProperty.call(vars, key) ? vars[key] : match | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Variable values are interpolated into HTML without escaping (HTML injection).
interpolate substitutes vars[key] directly into the HTML body. If any of these values (orderId, amount, approvalUrl, etc.) is derived from user-controlled or external data, the rendered email can contain injected markup/attributes (e.g. a crafted approvalUrl breaking out of an href, or arbitrary tags in amount). Escape values before substitution into HTML; the plain-text derivation should use the unescaped value.
🛡️ Proposed fix
+function escapeHtml(value: string): string {
+ return value
+ .replace(/&/g, "&")
+ .replace(/</g, "<")
+ .replace(/>/g, ">")
+ .replace(/"/g, """)
+ .replace(/'/g, "&`#39`;");
+}
+
function interpolate(template: string, vars: Record<string, string>): string {
return template.replace(/\{\{(\w+)\}\}/g, (match, key: string) =>
- Object.prototype.hasOwnProperty.call(vars, key) ? vars[key] : match
+ Object.prototype.hasOwnProperty.call(vars, key) ? escapeHtml(vars[key]) : match
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function interpolate(template: string, vars: Record<string, string>): string { | |
| return template.replace(/\{\{(\w+)\}\}/g, (match, key: string) => | |
| Object.prototype.hasOwnProperty.call(vars, key) ? vars[key] : match | |
| ); | |
| } | |
| function escapeHtml(value: string): string { | |
| return value | |
| .replace(/&/g, "&") | |
| .replace(/</g, "<") | |
| .replace(/>/g, ">") | |
| .replace(/"/g, """) | |
| .replace(/'/g, "&`#39`;"); | |
| } | |
| function interpolate(template: string, vars: Record<string, string>): string { | |
| return template.replace(/\{\{(\w+)\}\}/g, (match, key: string) => | |
| Object.prototype.hasOwnProperty.call(vars, key) ? escapeHtml(vars[key]) : match | |
| ); | |
| } |
🤖 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 `@apps/backend/notifications/templates/index.ts` around lines 42 - 46, The
interpolate helper is inserting vars[key] directly into the HTML template, which
allows HTML injection in rendered emails. Update interpolate in
templates/index.ts to escape substituted values before replacement, while
keeping the plain-text derivation path unchanged and unescaped. Use the existing
interpolate function and its callers to ensure only HTML output is escaped, not
the raw values used elsewhere.
1 participant
Add structured TemplateRenderResult to prevent partial email sends when
required template variables are missing or rendering fails.
Add PushSubscriptionCleanupResult and cleanupPushSubscriptions() to remove
expired or repeatedly failing push subscriptions from the store. Thresholds
are configurable via PUSH_MAX_FAILURES and PUSH_STALE_DAYS env vars.
Add vitest and unit tests covering success and failure paths for both features.
Closes #136
Closes #137
Summary by CodeRabbit