feat(notifications): in-app update notifications with release notes#1166
Merged
Conversation
Contributor
|
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:
WalkthroughAdds server-side update checking and caching, a /v1/system/update-status API, DB migration and helpers for notify/dedup state, an update-check Fastify plugin that schedules refreshes and conditionally dispatches notifications, notification orchestration and templates (Discord/Apprise), NotificationService plumbing, and client hook/UI and settings to consume/control notifications. ChangesUpdate-check & notification feature
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Cron Scheduler
participant Plugin as update-check Plugin
participant UpdateCheck as UpdateCheckService
participant GitHub as GitHub API
participant DB as Database
participant NotificationSvc as NotificationService
participant Discord as Discord Webhook
participant Apprise as Apprise
Scheduler->>Plugin: trigger cron handler
Plugin->>UpdateCheck: refresh()
UpdateCheck->>GitHub: fetch latest release
GitHub-->>UpdateCheck: release metadata / status
UpdateCheck->>DB: cache status (lastCheckedAt, latestVersion, metadata, status)
Plugin->>DB: read `notifyOnUpdate`, read `lastNotifiedVersion`
alt updateAvailable && notifyOnUpdate && semver.gt(latest, watermark)
Plugin->>NotificationSvc: sendUpdateAvailableNotification(release)
NotificationSvc->>Discord: send Discord embed (if configured)
NotificationSvc->>Apprise: send Apprise notification (if configured)
Discord-->>NotificationSvc: success/failure
Apprise-->>NotificationSvc: success/failure
NotificationSvc-->>Plugin: aggregated result
Plugin->>DB: setLastNotifiedVersion(latestVersion) after success
end
sequenceDiagram
participant Client as Browser UI
participant Hook as useVersionCheck
participant API as GET /v1/system/update-status
participant UpdateCheck as UpdateCheckService
participant UI as VersionDisplay
Client->>Hook: mount
Hook->>API: fetch cached status
API->>UpdateCheck: getStatus()
UpdateCheck-->>API: cached payload
API-->>Hook: update payload (updateAvailable, latestVersion, metadata)
Hook-->>UI: deliver data
alt updateAvailable
UI->>UI: format date, render notes
UI->>Client: render popover with release details + GitHub link
else no update
UI->>Client: render version label
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/hooks/useVersionCheck.tsx (1)
52-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the per-mount toast gate.
toastShownRefnever resets, so this hook will only ever toast once per component lifetime. After the first update notification, later releases in the same tab are silently skipped even thoughsessionStoragealready dedupes per version.🔧 Proposed fix
-import { useEffect, useRef } from 'react' +import { useEffect } from 'react' @@ export function useVersionCheck(): VersionCheckResult { - const toastShownRef = useRef(false) - const { data: status, @@ if ( versionInfo.updateAvailable && versionInfo.latestVersion && - !toastShownRef.current && notifiedVersion !== versionInfo.latestVersion ) { - toastShownRef.current = true sessionStorage.setItem(VERSION_TOAST_KEY, versionInfo.latestVersion)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/hooks/useVersionCheck.tsx` around lines 52 - 87, The hook currently uses toastShownRef to block further toasts for the component lifetime, causing newer releases in the same tab to be skipped; remove the per-mount gate by deleting the toastShownRef declaration and all uses of toastShownRef.current in the useEffect condition and body, and rely solely on sessionStorage (VERSION_TOAST_KEY) + versionInfo.latestVersion to dedupe toasts; keep the existing sessionStorage.setItem(VERSION_TOAST_KEY, versionInfo.latestVersion) and the rest of the notification logic inside the useEffect so each new version triggers a toast even within the same mounted component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/schemas/system/update-status.schema.ts`:
- Around line 43-45: Update the 500-response schema for the update-status route
so it matches Fastify Sensible's HttpError shape instead of only { message }:
replace or refactor UpdateStatusErrorSchema to reuse the shared ErrorSchema (or
mirror its fields { statusCode, code, error, message }) and ensure the route's
response schema references that ErrorSchema for the 500 status; update any
imports to use ErrorSchema and update the responses map for the route that
references UpdateStatusErrorSchema.
In `@src/services/notifications/templates/discord-embeds.ts`:
- Around line 338-390: The webhook payload currently allows raw release notes
(from createUpdateAvailableEmbed via release.releaseBody) which can trigger
pings; fix by adding an explicit allowed_mentions field to the webhook payload
in update-available.ts (set "allowed_mentions": {"parse": []}) or sanitize
release.releaseBody inside createUpdateAvailableEmbed by escaping or stripping
occurrences of `@everyone`, `@here` and user/role mention patterns (e.g.
/@everyone|@here|<@!?\d+>|<@&\d+>/) before building description; reference
createUpdateAvailableEmbed, release.releaseBody, and the webhook payload in
update-available.ts and ensure the final payload contains allowed_mentions to
suppress mentions.
---
Outside diff comments:
In `@src/client/hooks/useVersionCheck.tsx`:
- Around line 52-87: The hook currently uses toastShownRef to block further
toasts for the component lifetime, causing newer releases in the same tab to be
skipped; remove the per-mount gate by deleting the toastShownRef declaration and
all uses of toastShownRef.current in the useEffect condition and body, and rely
solely on sessionStorage (VERSION_TOAST_KEY) + versionInfo.latestVersion to
dedupe toasts; keep the existing sessionStorage.setItem(VERSION_TOAST_KEY,
versionInfo.latestVersion) and the rest of the notification logic inside the
useEffect so each new version triggers a toast even within the same mounted
component.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bd4b85f0-d75b-47d0-93df-27ac3a214e04
📒 Files selected for processing (19)
migrations/migrations/090_20260430_add_update_notify_columns.tssrc/client/components/ui/version-display.tsxsrc/client/features/notifications/components/general/general-settings-form.tsxsrc/client/hooks/useVersionCheck.tsxsrc/plugins/custom/update-check.tssrc/routes/v1/system/update-status.tssrc/schemas/config/config.schema.tssrc/schemas/system/update-status.schema.tssrc/services/database/methods/config.tssrc/services/database/types/config-methods.tssrc/services/notification.service.tssrc/services/notifications/channels/apprise.service.tssrc/services/notifications/channels/apprise.tssrc/services/notifications/orchestration/index.tssrc/services/notifications/orchestration/update-available.tssrc/services/notifications/templates/apprise-html.tssrc/services/notifications/templates/discord-embeds.tssrc/services/update-check.service.tssrc/types/config.types.ts
7 tasks
bommerts
force-pushed
the
feature/update-notify-v2
branch
from
ab9f0fc to
2a746dc
Compare
Author
|
Force-pushed
Single commit; typecheck + biome clean. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/notifications/templates/apprise-html.ts`:
- Around line 412-418: The current stripMarkdown replacements in apprise-html.ts
are too greedy and eat underscores/asterisks inside words or across lines;
update the emphasis regexes in the .replace chain (the patterns handling **, *,
__, _) to only match markers that are not adjacent to word characters and not
spanning newlines (use lookarounds like (?<!\w) and (?!\w) and forbid \n inside
the capture), and ensure the list-bullet replacement (.replace(/^\s*[-*+]\s+/gm,
'• ')) runs before single-asterisk/single-underscore emphasis so item lines like
"* item" are not consumed by the emphasis regexes.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b6470d14-c267-40d5-9ff7-1483dfed90c0
📒 Files selected for processing (20)
migrations/migrations/090_20260430_add_update_notify_columns.tssrc/client/components/ui/version-display.tsxsrc/client/features/notifications/components/general/general-settings-form.tsxsrc/client/hooks/useVersionCheck.tsxsrc/plugins/custom/update-check.tssrc/routes/v1/system/update-status.tssrc/schemas/config/config.schema.tssrc/schemas/system/update-status.schema.tssrc/services/database/methods/config.tssrc/services/database/types/config-methods.tssrc/services/notification.service.tssrc/services/notifications/channels/apprise.service.tssrc/services/notifications/channels/apprise.tssrc/services/notifications/orchestration/index.tssrc/services/notifications/orchestration/update-available.tssrc/services/notifications/templates/apprise-html.tssrc/services/notifications/templates/discord-embeds.tssrc/services/update-check.service.tssrc/types/config.types.tssrc/types/discord.types.ts
✅ Files skipped from review due to trivial changes (3)
- src/services/notifications/orchestration/index.ts
- src/types/discord.types.ts
- migrations/migrations/090_20260430_add_update_notify_columns.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/services/database/types/config-methods.ts
- src/schemas/config/config.schema.ts
- src/services/notifications/channels/apprise.service.ts
- src/client/features/notifications/components/general/general-settings-form.tsx
- src/services/notifications/templates/discord-embeds.ts
- src/services/update-check.service.ts
- src/services/notifications/orchestration/update-available.ts
- src/plugins/custom/update-check.ts
- src/client/components/ui/version-display.tsx
- src/services/database/methods/config.ts
Adds an opt-in toggle so Pulsarr sends a one-time Discord webhook + Apprise system notification when a new GitHub release is detected. Replaces the old browser-side fetch with a server-side hourly cron + cached backend route, and surfaces release notes inline via a popover next to the version label. Server - New 'UpdateCheckService' (cached, in-flight refresh dedup, semver normalisation, GitHub fetch with timeout/rate-limit handling) - New 'update-check' plugin: hourly cron, baseline-on-enable, single-flight notification dispatch, watermark-advance-after-attempt to prevent retry storms - New GET '/v1/system/update-status' route returning the cached payload - New 'sendUpdateAvailableNotification' on AppriseService + 'sendUpdateAvailable' orchestrator + facade method on NotificationService - New 'createUpdateAvailableNotificationHtml' (Apprise) and 'createUpdateAvailableEmbed' (Discord) templates - dedicated, NOT routed through the approval-card system template - 'notifyOnUpdate' added to Config / ConfigFull / ConfigUpdate schemas. 'lastNotifiedVersion' is internal-only - dedicated DB accessors that bypass ALLOWED_COLUMNS. Re-enabling 'notifyOnUpdate' resets the watermark so users only get notified for releases newer than the moment they enabled the toggle. - Migration 090 adds 'notifyOnUpdate' (boolean default false) and 'lastNotifiedVersion' (nullable string) columns Client - 'useVersionCheck' rewritten to call '/v1/system/update-status'; no more direct GitHub fetches from the browser - 'VersionDisplay' replaces the tooltip with a popover showing release name, truncated body, and a 'View on GitHub' link - New 'Notify me when a Pulsarr update is available' Switch in general notification settings Supersedes jamcalli#1154 and jamcalli#1155. Refs jamcalli#1153.
bommerts
force-pushed
the
feature/update-notify-v2
branch
from
2a746dc to
987912b
Compare
Author
|
Force-pushed The previous regex was too greedy on single-marker emphasis and would have mangled common GitHub release-note content:
Fixed by reordering bullet conversion before single-marker emphasis and making the single-marker patterns boundary-aware ( Verified against 9 cases: snake_case, mixed-case identifiers, bullet lists, mid-sentence emphasis, bold across newlines, and a realistic release-notes excerpt — all pass.
Single commit; typecheck + biome clean. |
Owner
|
I'll have to do some work to get this in cleanly. Some of your assumptions are misaligned with the behavior and I will fix upstream before merging. For example, the Will also need to add a markdown formatter / renderer. Otherwise there is risk that future maintainers could use syntax that's perfectly allowed as per standard md conventions, but causes bugs within the Pulsarr UI / notificaitons. I'll get this looked at sometime this week. Also you should figure out why your tests aren't passing. They all work in CI and within my dev environment. |
Author
|
Thanks @jamcalli — appreciated, and happy to defer. The apprise-not-actually-ready-for-5s detail is news to me; that fully explains the On the tests: CI on Happy to iterate on whatever lands after your rework if useful, and no rush from my end. |
Contributor
|
Actionable comments posted: 0 |
Owner
|
I finally had some time to go through this and make some corrects / adjustments. Let me know your thoughts. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/notifications/templates/apprise-html.ts (1)
307-314: Add a trust-boundary comment (and optional sanitizer) forreleaseBodyHtmlinjection
src/services/update-check.service.tsgeneratesreleaseBodyHtmlby calling GitHub’sPOST https://api.github.com/markdownand returnsresponse.text()with no additional sanitization in this repo.src/services/notifications/templates/apprise-html.tsthen injects${release.releaseBodyHtml}directly into the Apprise HTML.- GitHub’s Markdown-to-HTML output is sanitized (e.g., it strips dangerous/script content), but the template still relies on that guarantee—add a brief comment documenting the trust boundary and consider a lightweight allowlist/sanitization step as defense-in-depth.
🤖 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 `@src/services/notifications/templates/apprise-html.ts` around lines 307 - 314, The template injects release.releaseBodyHtml directly into the HTML (in the notesCard construction in apprise-html.ts), so add a trust-boundary comment next to that interpolation describing the provenance (generated via GitHub POST /markdown and currently returned as response.text()) and the assumption that GitHub sanitizes output; also add an optional defense-in-depth sanitization step before interpolation (e.g., call a small allowlist sanitizer or DOMPurify-like function such as sanitizeHtmlIfNeeded(release.releaseBodyHtml)) and reference that sanitizer in the comment so future reviewers know the mitigation and where to change it.
🤖 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 `@src/plugins/custom/update-check.ts`:
- Around line 40-41: The code advances the watermark by calling
fastify.db.setLastNotifiedVersion(targetVersion) where targetVersion =
status.latestVersion before attempting delivery, which prevents retries if
delivery (the logic that sets sent) fails; move the call to
fastify.db.setLastNotifiedVersion(targetVersion) so it runs only after
successful delivery (i.e., when the variable sent is truthy in the delivery
block), ensuring the watermark is updated only on success and failed
notifications can be retried on the next cron run.
---
Nitpick comments:
In `@src/services/notifications/templates/apprise-html.ts`:
- Around line 307-314: The template injects release.releaseBodyHtml directly
into the HTML (in the notesCard construction in apprise-html.ts), so add a
trust-boundary comment next to that interpolation describing the provenance
(generated via GitHub POST /markdown and currently returned as response.text())
and the assumption that GitHub sanitizes output; also add an optional
defense-in-depth sanitization step before interpolation (e.g., call a small
allowlist sanitizer or DOMPurify-like function such as
sanitizeHtmlIfNeeded(release.releaseBodyHtml)) and reference that sanitizer in
the comment so future reviewers know the mitigation and where to change it.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ee0b7706-3873-4a2d-b55a-8ba70eadf742
📒 Files selected for processing (20)
migrations/migrations/090_20260430_add_update_notify_columns.tssrc/client/components/ui/version-display.tsxsrc/client/features/notifications/components/general/general-settings-form.tsxsrc/client/hooks/useVersionCheck.tsxsrc/plugins/custom/update-check.tssrc/routes/v1/system/update-status.tssrc/schemas/config/config.schema.tssrc/schemas/system/update-status.schema.tssrc/services/database/methods/config.tssrc/services/database/types/config-methods.tssrc/services/notification.service.tssrc/services/notifications/channels/apprise.service.tssrc/services/notifications/channels/apprise.tssrc/services/notifications/channels/discord-dm.tssrc/services/notifications/discord-bot/bot.service.tssrc/services/notifications/orchestration/update-available.tssrc/services/notifications/templates/apprise-html.tssrc/services/notifications/templates/discord-embeds.tssrc/services/update-check.service.tssrc/types/config.types.ts
💤 Files with no reviewable changes (2)
- src/services/database/types/config-methods.ts
- src/routes/v1/system/update-status.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/services/notifications/channels/apprise.service.ts
- src/services/notifications/channels/apprise.ts
- src/client/hooks/useVersionCheck.tsx
- src/services/notifications/templates/discord-embeds.ts
- src/schemas/system/update-status.schema.ts
jamcalli
force-pushed
the
feature/update-notify-v2
branch
from
f2fadd5 to
53afbe8
Compare
Author
|
Thanks @jamcalli — this looks great. The I have a working install running on a server here that I've been using to validate the feature end-to-end; happy to rebuild against |
Owner
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
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 `@src/client/hooks/useVersionCheck.tsx`:
- Around line 41-47: The refetch interval needs to be dynamic: in the
useAppQuery call that uses versionCheckKeys.status() and fetchUpdateStatus,
replace the fixed refetchInterval: FIFTEEN_MINUTES with a function that checks
the returned data (e.g., data?.status) and returns a short cadence (e.g., a few
seconds) while status === 'pending', and returns FIFTEEN_MINUTES once a
non-'pending' response is seen; implement this by changing refetchInterval to a
function (or callback) that inspects the query data and returns the appropriate
milliseconds, ensuring retry remains false and other options (staleTime) are
unchanged.
In `@src/services/notifications/templates/apprise-html.ts`:
- Around line 307-314: Add a short inline comment next to the
release.releaseBodyHtml interpolation in the notesCard template explaining that
the HTML is trusted because it is rendered via GitHub's markdown API
(renderReleaseBody → response.text()) and therefore not escaped here; also note
that if the rendering source changes or is replaced, the code must instead
escape/sanitize release.releaseBodyHtml (e.g., via escapeHtml or a sanitizer)
before injecting into the Apprise HTML.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ec69bb02-e000-4c9e-b883-730b9cc277b9
⛔ Files ignored due to path filters (5)
docs/docs/notifications/apprise.mdis excluded by!docs/**docs/docs/notifications/discord.mdis excluded by!docs/**docs/docs/notifications/general-settings.mdis excluded by!docs/**docs/docs/notifications/native-webhooks.mdis excluded by!docs/**docs/docs/notifications/plex-mobile.mdis excluded by!docs/**
📒 Files selected for processing (22)
migrations/migrations/090_20260430_add_update_notify_columns.tssrc/client/components/ui/version-display.tsxsrc/client/features/notifications/components/general/general-settings-form.tsxsrc/client/hooks/useVersionCheck.tsxsrc/plugins/custom/update-check.tssrc/routes/v1/system/update-status.tssrc/schemas/config/config.schema.tssrc/schemas/system/update-status.schema.tssrc/services/database/methods/config.tssrc/services/database/types/config-methods.tssrc/services/notification.service.tssrc/services/notifications/channels/apprise.service.tssrc/services/notifications/channels/apprise.tssrc/services/notifications/channels/discord-dm.tssrc/services/notifications/discord-bot/bot.service.tssrc/services/notifications/orchestration/index.tssrc/services/notifications/orchestration/update-available.tssrc/services/notifications/templates/apprise-html.tssrc/services/notifications/templates/discord-embeds.tssrc/services/update-check.service.tssrc/types/config.types.tssrc/types/discord.types.ts
Owner
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/database/methods/config.ts (1)
586-609:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the dedupe watermark when
notifyOnUpdateis re-enabled.The PR contract says re-enabling this setting should clear the watermark, but
updateConfig()only writes the new mode andsetLastNotifiedVersion()cannot storenull. As written, a user who already received the notification for the current latest release, turns notifications off, then turns them back on before upgrading will stay suppressed by the stalelastNotifiedVersion.💡 Suggested fix
export async function updateConfig( this: DatabaseService, config: Partial<Config>, ): Promise<boolean> { try { + const current = + config.notifyOnUpdate !== undefined + ? await this.knex('configs').where({ id: 1 }).first('notifyOnUpdate') + : undefined + const updateData: Record<string, unknown> = { updated_at: this.timestamp, } for (const [key, value] of Object.entries(config)) { if (value !== undefined && ALLOWED_COLUMNS.has(key)) { if (JSON_COLUMNS.has(key)) { updateData[key] = value !== undefined && value !== null ? JSON.stringify(value) : null } else { updateData[key] = value } } } + + if ( + config.notifyOnUpdate !== undefined && + config.notifyOnUpdate !== 'none' && + (current?.notifyOnUpdate ?? 'none') === 'none' + ) { + updateData.lastNotifiedVersion = null + } const updated = await this.knex('configs') .where({ id: 1 }) .update(updateData) return updated > 0 @@ export async function setLastNotifiedVersion( this: DatabaseService, - version: string, + version: string | null, ): Promise<boolean> {Also applies to: 643-656
🤖 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 `@src/services/database/methods/config.ts` around lines 586 - 609, When re-enabling notifyOnUpdate you must clear the dedupe watermark; modify updateConfig so if the incoming config contains notifyOnUpdate === true and the current service config (this.config.notifyOnUpdate) is false, add last_notified_version: null into updateData before performing the knex update (bypass setLastNotifiedVersion since it cannot store null). Update the logic inside updateConfig (refer to the updateConfig function and the updateData object) to detect this transition and set updateData['last_notified_version'] = null so the DB row is cleared when notifications are re-enabled.
🧹 Nitpick comments (1)
src/client/components/ui/version-display.tsx (1)
63-68: 💤 Low valueConsider sanitizing
releaseBodyHtmlbefore rendering withdangerouslySetInnerHTML
src/client/components/ui/version-display.tsxinjectsreleaseBodyHtmldirectly; there’s no existing HTML-sanitizer dependency in the project (e.g., DOMPurify isn’t present). On the server,releaseBodyHtmlis produced by forwarding GitHub’s/markdownoutput without any additional local sanitization, so the UI relies on GitHub always emitting safe HTML. Add explicit sanitization (preferably on the server so both notifications and the UI share it) or document the safety assumption.🤖 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 `@src/client/components/ui/version-display.tsx` around lines 63 - 68, The releaseBodyHtml is being injected via dangerouslySetInnerHTML in the VersionDisplay component without sanitization; update the pipeline to sanitize HTML server-side where the GitHub /markdown output is forwarded (the function that fetches/forwards release notes, e.g., getReleaseBodyHtml or fetchReleaseNotes) using a well-known sanitizer (DOMPurify server build, sanitize-html, or similar) and return a sanitized string, then change the VersionDisplay consumer to expect and render sanitized releaseBodyHtml (still using dangerouslySetInnerHTML but only with the sanitized value) or, if you prefer client-side hardening, run the same sanitizer in the component before setting dangerouslySetInnerHTML; ensure tests or a comment document the chosen sanitizer and that releaseBodyHtml passed to VersionDisplay is sanitized.
🤖 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 `@src/services/notifications/orchestration/update-available.ts`:
- Around line 96-102: The db lookup for the primary Discord user
(deps.db.getPrimaryUser()) must be moved inside the try block in sendDiscordDM
so any thrown error is caught and the DM channel degrades to false instead of
bubbling up and aborting sendUpdateAvailable's Promise.all; update sendDiscordDM
to perform await deps.db.getPrimaryUser() inside its try, handle missing or
invalid primaryUser?.discord_id the same way as now, and return false on any
exception so webhook/Apprise paths can still run.
In `@src/services/notifications/templates/apprise-html.ts`:
- Around line 324-330: The plain-text fallback variable textBody currently omits
the release notes; append release.releaseBody (truncated if needed) to textBody
so Apprise-only text targets receive the notes too. Locate the textBody
construction (variable textBody) and after adding Published (or before "View
release"), append a sanitized/truncated version of release.releaseBody (e.g.,
strip HTML, limit length) and then continue with the existing "View release" and
signature lines.
---
Outside diff comments:
In `@src/services/database/methods/config.ts`:
- Around line 586-609: When re-enabling notifyOnUpdate you must clear the dedupe
watermark; modify updateConfig so if the incoming config contains notifyOnUpdate
=== true and the current service config (this.config.notifyOnUpdate) is false,
add last_notified_version: null into updateData before performing the knex
update (bypass setLastNotifiedVersion since it cannot store null). Update the
logic inside updateConfig (refer to the updateConfig function and the updateData
object) to detect this transition and set updateData['last_notified_version'] =
null so the DB row is cleared when notifications are re-enabled.
---
Nitpick comments:
In `@src/client/components/ui/version-display.tsx`:
- Around line 63-68: The releaseBodyHtml is being injected via
dangerouslySetInnerHTML in the VersionDisplay component without sanitization;
update the pipeline to sanitize HTML server-side where the GitHub /markdown
output is forwarded (the function that fetches/forwards release notes, e.g.,
getReleaseBodyHtml or fetchReleaseNotes) using a well-known sanitizer (DOMPurify
server build, sanitize-html, or similar) and return a sanitized string, then
change the VersionDisplay consumer to expect and render sanitized
releaseBodyHtml (still using dangerouslySetInnerHTML but only with the sanitized
value) or, if you prefer client-side hardening, run the same sanitizer in the
component before setting dangerouslySetInnerHTML; ensure tests or a comment
document the chosen sanitizer and that releaseBodyHtml passed to VersionDisplay
is sanitized.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 871f99e0-42e0-48e1-8b0a-a04050700d7b
⛔ Files ignored due to path filters (5)
docs/docs/notifications/apprise.mdis excluded by!docs/**docs/docs/notifications/discord.mdis excluded by!docs/**docs/docs/notifications/general-settings.mdis excluded by!docs/**docs/docs/notifications/native-webhooks.mdis excluded by!docs/**docs/docs/notifications/plex-mobile.mdis excluded by!docs/**
📒 Files selected for processing (22)
migrations/migrations/090_20260430_add_update_notify_columns.tssrc/client/components/ui/version-display.tsxsrc/client/features/notifications/components/general/general-settings-form.tsxsrc/client/hooks/useVersionCheck.tsxsrc/plugins/custom/update-check.tssrc/routes/v1/system/update-status.tssrc/schemas/config/config.schema.tssrc/schemas/system/update-status.schema.tssrc/services/database/methods/config.tssrc/services/database/types/config-methods.tssrc/services/notification.service.tssrc/services/notifications/channels/apprise.service.tssrc/services/notifications/channels/apprise.tssrc/services/notifications/channels/discord-dm.tssrc/services/notifications/discord-bot/bot.service.tssrc/services/notifications/orchestration/index.tssrc/services/notifications/orchestration/update-available.tssrc/services/notifications/templates/apprise-html.tssrc/services/notifications/templates/discord-embeds.tssrc/services/update-check.service.tssrc/types/config.types.tssrc/types/discord.types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/hooks/useVersionCheck.tsx (1)
72-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove the session toast watermark into the delayed callback.
Line 72 records the version as notified before the 3-second timer fires. If this hook unmounts before the timeout runs, the cleanup cancels the toast but the session watermark stays set, so the user won't see the notification again in that session.
Suggested fix
) { - sessionStorage.setItem(VERSION_TOAST_KEY, versionInfo.latestVersion) - const url = versionInfo.releaseUrl const timeoutId = setTimeout(() => { + sessionStorage.setItem(VERSION_TOAST_KEY, versionInfo.latestVersion) toast( `A new version (v${versionInfo.latestVersion}) is available. You're running v${versionInfo.currentVersion}.`, {🤖 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 `@src/client/hooks/useVersionCheck.tsx` around lines 72 - 93, The session watermark (sessionStorage.setItem(VERSION_TOAST_KEY, versionInfo.latestVersion)) is being set immediately instead of only when the toast actually appears, so move that call inside the delayed callback used to show the toast (the setTimeout handler that calls toast) so the watermark is only written if the timeout fires; keep the existing clearTimeout(timeoutId) in the cleanup to prevent writing when the hook unmounts early and reference versionInfo, VERSION_TOAST_KEY, timeoutId, and the toast action when making the change.
♻️ Duplicate comments (1)
src/services/notifications/templates/apprise-html.ts (1)
324-330:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude release notes in the plain-text fallback.
The
textBodycurrently omitsrelease.releaseBody, so Apprise targets that only render plain text lose the release notes entirely. This was flagged in an earlier review and remains unaddressed.🔧 Suggested fix
+ const releaseNotes = release.releaseBody?.trim() + let textBody = 'Pulsarr update available\n\n' textBody += `${displayName}\n` textBody += `Current: v${release.currentVersion}\n` textBody += `Latest: v${release.latestVersion}\n` if (publishedAt) textBody += `Published: ${publishedAt}\n` + if (releaseNotes) textBody += `\nRelease Notes:\n${releaseNotes}\n` textBody += `\nView release: ${release.releaseUrl}\n` textBody += '\n- Pulsarr'🤖 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 `@src/services/notifications/templates/apprise-html.ts` around lines 324 - 330, The plain-text fallback in the Apprise template builds textBody but omits the release notes; update the code that constructs textBody (variable textBody in this file) to include release.releaseBody (if present) between the version lines and the "View release" link so plain-text targets receive the release notes; use the existing publishedAt/displayName/release.currentVersion/release.latestVersion/release.releaseUrl context and ensure you prepend/append appropriate newlines so the release.releaseBody is readable in the plain-text output.
🤖 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 `@src/plugins/custom/update-check.ts`:
- Around line 49-55: The code logs a successful dispatch even when persisting
the watermark fails; after a successful send (when variable sent is true), call
fastify.db.setLastNotifiedVersion(targetVersion) and check its return value — if
it returns false or throws, treat this as an error: log an error via
fastify.log.error (include targetVersion and the failure) and do not log the
'Update-available notification dispatched' info; only log success when
setLastNotifiedVersion succeeds. Ensure you handle exceptions from
setLastNotifiedVersion (try/catch) and reference the sent, targetVersion,
fastify.db.setLastNotifiedVersion and fastify.log symbols.
In `@src/services/database/methods/config.ts`:
- Around line 624-635: The catch in getLastNotifiedVersion is swallowing DB
errors and returning null (which dispatchNotification treats as "no prior
notification"); instead of returning null on error, log the error with
this.log.error as is and re-throw the caught error so callers can skip dispatch;
modify the catch in getLastNotifiedVersion
(DatabaseService.getLastNotifiedVersion) to throw the original error after
logging rather than returning null.
- Around line 643-657: The setLastNotifiedVersion function currently only
accepts a non-null string which prevents clearing the nullable DB column; change
the signature and any related type augmentation to accept version: string |
null, update the update payload in setLastNotifiedVersion to write
lastNotifiedVersion: version (allowing null) and keep updated_at behavior, and
adjust its callers (notably update-check.ts) to call
setLastNotifiedVersion(null) when you need to clear the watermark so the dedupe
state can be reset.
---
Outside diff comments:
In `@src/client/hooks/useVersionCheck.tsx`:
- Around line 72-93: The session watermark
(sessionStorage.setItem(VERSION_TOAST_KEY, versionInfo.latestVersion)) is being
set immediately instead of only when the toast actually appears, so move that
call inside the delayed callback used to show the toast (the setTimeout handler
that calls toast) so the watermark is only written if the timeout fires; keep
the existing clearTimeout(timeoutId) in the cleanup to prevent writing when the
hook unmounts early and reference versionInfo, VERSION_TOAST_KEY, timeoutId, and
the toast action when making the change.
---
Duplicate comments:
In `@src/services/notifications/templates/apprise-html.ts`:
- Around line 324-330: The plain-text fallback in the Apprise template builds
textBody but omits the release notes; update the code that constructs textBody
(variable textBody in this file) to include release.releaseBody (if present)
between the version lines and the "View release" link so plain-text targets
receive the release notes; use the existing
publishedAt/displayName/release.currentVersion/release.latestVersion/release.releaseUrl
context and ensure you prepend/append appropriate newlines so the
release.releaseBody is readable in the plain-text output.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 10b821a3-19ff-4b2e-ab54-c279b64dfac8
⛔ Files ignored due to path filters (5)
docs/docs/notifications/apprise.mdis excluded by!docs/**docs/docs/notifications/discord.mdis excluded by!docs/**docs/docs/notifications/general-settings.mdis excluded by!docs/**docs/docs/notifications/native-webhooks.mdis excluded by!docs/**docs/docs/notifications/plex-mobile.mdis excluded by!docs/**
📒 Files selected for processing (22)
migrations/migrations/090_20260430_add_update_notify_columns.tssrc/client/components/ui/version-display.tsxsrc/client/features/notifications/components/general/general-settings-form.tsxsrc/client/hooks/useVersionCheck.tsxsrc/plugins/custom/update-check.tssrc/routes/v1/system/update-status.tssrc/schemas/config/config.schema.tssrc/schemas/system/update-status.schema.tssrc/services/database/methods/config.tssrc/services/database/types/config-methods.tssrc/services/notification.service.tssrc/services/notifications/channels/apprise.service.tssrc/services/notifications/channels/apprise.tssrc/services/notifications/channels/discord-dm.tssrc/services/notifications/discord-bot/bot.service.tssrc/services/notifications/orchestration/index.tssrc/services/notifications/orchestration/update-available.tssrc/services/notifications/templates/apprise-html.tssrc/services/notifications/templates/discord-embeds.tssrc/services/update-check.service.tssrc/types/config.types.tssrc/types/discord.types.ts
Comment on lines
+624
to
+635
| export async function getLastNotifiedVersion( | ||
| this: DatabaseService, | ||
| ): Promise<string | null> { | ||
| try { | ||
| const row = await this.knex('configs') | ||
| .where({ id: 1 }) | ||
| .first('lastNotifiedVersion') | ||
| return (row?.lastNotifiedVersion as string | null) ?? null | ||
| } catch (error) { | ||
| this.log.error({ error }, 'Error reading lastNotifiedVersion') | ||
| return null | ||
| } |
Contributor
There was a problem hiding this comment.
Don't map a failed watermark read to null.
dispatchNotification() treats null as “no prior notification”, so returning null here on a transient query failure can resend the current latest-release notification. Re-throw after logging so callers skip dispatch instead of losing the dedupe state.
Suggested fix
export async function getLastNotifiedVersion(
this: DatabaseService,
): Promise<string | null> {
try {
const row = await this.knex('configs')
.where({ id: 1 })
.first('lastNotifiedVersion')
return (row?.lastNotifiedVersion as string | null) ?? null
} catch (error) {
this.log.error({ error }, 'Error reading lastNotifiedVersion')
- return null
+ throw error
}
}🤖 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 `@src/services/database/methods/config.ts` around lines 624 - 635, The catch in
getLastNotifiedVersion is swallowing DB errors and returning null (which
dispatchNotification treats as "no prior notification"); instead of returning
null on error, log the error with this.log.error as is and re-throw the caught
error so callers can skip dispatch; modify the catch in getLastNotifiedVersion
(DatabaseService.getLastNotifiedVersion) to throw the original error after
logging rather than returning null.
Comment on lines
+643
to
+657
| export async function setLastNotifiedVersion( | ||
| this: DatabaseService, | ||
| version: string, | ||
| ): Promise<boolean> { | ||
| try { | ||
| const updated = await this.knex('configs').where({ id: 1 }).update({ | ||
| lastNotifiedVersion: version, | ||
| updated_at: this.timestamp, | ||
| }) | ||
| return updated > 0 | ||
| } catch (error) { | ||
| this.log.error({ error, version }, 'Error setting lastNotifiedVersion') | ||
| return false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "notifyOnUpdate|setLastNotifiedVersion|getLastNotifiedVersion" src -C3
fd 'config-methods.ts' src --exec sed -n '1,120p' {}Repository: jamcalli/Pulsarr
Length of output: 12374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all call sites of the watermark helpers
rg -n "setLastNotifiedVersion\s*\(" src -S
rg -n "getLastNotifiedVersion\s*\(" src -S
# 2) Find where notifyOnUpdate is updated / toggle handled server-side
rg -n "notifyOnUpdate" src -S
# 3) Inspect the DB methods + update-check logic around the watermark usage
for f in \
src/services/database/methods/config.ts \
src/plugins/custom/update-check.ts
do
echo "===== $f (relevant excerpt) ====="
# Print around known line anchors
case "$f" in
*config.ts)
sed -n '430,760p' "$f" | nl -ba | sed -n '1,220p'
;;
*update-check.ts)
sed -n '1,140p' "$f" | nl -ba
;;
esac
doneRepository: jamcalli/Pulsarr
Length of output: 2393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "lastNotifiedVersion" src -S
# Inspect the watermark read/write logic
echo "===== src/plugins/custom/update-check.ts ====="
sed -n '1,120p' src/plugins/custom/update-check.ts
# Inspect DB helpers around get/set + updateConfig exclusion list
echo "===== src/services/database/methods/config.ts (relevant) ====="
sed -n '430,520p' src/services/database/methods/config.ts
sed -n '600,690p' src/services/database/methods/config.ts
# Inspect config update path (client) that toggles notifyOnUpdate
echo "===== src/client/features/notifications/components/general/general-settings-form.tsx (relevant) ====="
sed -n '1,220p' src/client/features/notifications/components/general/general-settings-form.tsx
sed -n '220,360p' src/client/features/notifications/components/general/general-settings-form.tsx
# Inspect public DB interface augmentation
echo "===== src/services/database/types/config-methods.ts ====="
sed -n '1,120p' src/services/database/types/config-methods.tsRepository: jamcalli/Pulsarr
Length of output: 22080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "lastNotifiedVersion" . -S
# Find other potential watermark-clearing paths
rg -n "setLastNotifiedVersion\s*\(" . -S
rg -n "getLastNotifiedVersion\s*\(" . -S
# Look for config update handling that reacts specifically to notifyOnUpdate changes
rg -n "notifyOnUpdate.*(none|re-?enable|enable)|\\bnotifyOnUpdate\\b" src -SRepository: jamcalli/Pulsarr
Length of output: 3255
Add a way to clear the update notification watermark
lastNotifiedVersion is internal/nullable at the DB level (migrations/...090_20260430_add_update_notify_columns.ts sets table.string('lastNotifiedVersion').nullable()), but setLastNotifiedVersion (and its type augmentation) only accepts version: string, and the only caller (src/plugins/custom/update-check.ts) only ever advances it after a successful send—there’s no path to clear it when notifyOnUpdate is toggled back on. As a result, the dedupe state persists across disable/re-enable because the update-check plugin always uses storedLastNotified if present.
🤖 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 `@src/services/database/methods/config.ts` around lines 643 - 657, The
setLastNotifiedVersion function currently only accepts a non-null string which
prevents clearing the nullable DB column; change the signature and any related
type augmentation to accept version: string | null, update the update payload in
setLastNotifiedVersion to write lastNotifiedVersion: version (allowing null) and
keep updated_at behavior, and adjust its callers (notably update-check.ts) to
call setLastNotifiedVersion(null) when you need to clear the watermark so the
dedupe state can be reset.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an opt-in toggle so Pulsarr sends a one-time Discord webhook + Apprise system notification when a new GitHub release is detected. Replaces the old browser-side fetch with a server-side hourly cron + cached backend route, and surfaces release notes inline via a popover next to the version label.
This is the notifier slice of #1153 only — no auto-installer.
Server
UpdateCheckService(cached, in-flight refresh dedup, semver normalisation, GitHub fetch with timeout / rate-limit / error handling)update-checkplugin: hourly cron, baseline-on-enable, single-flight notification dispatch, watermark-advance-after-attempt to prevent retry storms when a webhook URL is permanently brokenGET /v1/system/update-statusroute returning the cached payloadsendUpdateAvailableNotificationonAppriseService+sendUpdateAvailableorchestrator + facade method onNotificationServicecreateUpdateAvailableNotificationHtml(Apprise) andcreateUpdateAvailableEmbed(Discord) templates — dedicated, NOT routed through the approval-card system templatenotifyOnUpdateadded toConfig/ConfigFull/ConfigUpdateschemas.lastNotifiedVersionis internal-only — dedicated DB accessors that bypassALLOWED_COLUMNS. Re-enablingnotifyOnUpdateresets the watermark so users only get notified for releases newer than the moment they enabled the toggle.090_20260430_add_update_notify_columns.tsaddsnotifyOnUpdate(boolean default false) andlastNotifiedVersion(nullable string) columnsClient
useVersionCheckrewritten to call/v1/system/update-status; no more direct GitHub fetches from the browserVersionDisplayreplaces the tooltip with a popover showing release name, truncated body, and a "View on GitHub" linkTested
tmdb.serviceandplex-label-sync)tsc --noEmitserver + client clean, biome cleanupdate-checkcron registered inschedulestableNote on
<hr>for Apprise → TelegramWhile testing Apprise → Telegram delivery, found that Telegram's HTML parser rejects
<hr>tags, returningBad Request: can't parse entities: Unsupported start tag "hr". This is a pre-existing issue in the sharedhtmlWrapper(apprise-html.ts:41) that affects all Pulsarr Apprise → Telegram notifications (delete-sync, media, etc.). To keep PR scope tight, this PR addresses it ONLY forcreateUpdateAvailableNotificationHtmlby inlining the wrapper styling without the<hr>. Other notification templates are unaffected — fixing them would be a separate PR.Refs
Summary by CodeRabbit