feat: overhaul notification system with multiple configs, webhook/apprise support, and admin UI#1084
Merged
Zibbp merged 22 commits intoZibbp:mainfrom Feb 26, 2026
Conversation
Add database schema for named notification configurations supporting multiple providers (webhook, apprise), four event triggers with customizable message templates, and apprise-specific fields (urls, title, type, tag, format).
…oviders BREAKING CHANGE: Remove config.json-based notification settings in favor of database-stored notification configurations. - Rewrite notification service with full CRUD operations and dispatch methods for each event type (video success, live success, error, live) - Add webhook and Apprise provider support with template rendering - Add HTTP handlers with request validation (struct tags + custom rules) - Wire notification service into server, worker, tasks, and live service - Update all trigger call sites to use new service methods - Remove old Notification struct and defaults from config
Add dedicated notifications admin page at /admin/notifications with DataTable listing, create/edit drawer, delete confirmation, and test modal. Template variables are shown inline per trigger type. - Add React Query hooks for notification CRUD and test operations - Add navbar link and i18n keys (en/de/uk) - Remove old inline notification section from settings page - Remove legacy Notification interface and NotificationType from config hooks
Add length limits to prevent unbounded string storage: 255 for name, 2048 for URL, 4096 for templates and apprise_urls, 255 for apprise_tag.
- Return generic error messages to clients, log details server-side - Return 404 instead of 500 for not-found notification entities - Add nil pointer guards in getVariableMap for channel/vod/queue - Leave unknown template variables untouched instead of rendering <nil> - Wrap SendLive goroutine in recover to prevent process crash - Reuse shared http.Client instead of creating one per request - Drain response body before close for HTTP connection reuse - Return error from SendTestNotification so handler can report failure - Validate Apprise notifications require apprise_urls or apprise_tag
- Show server error messages in notification toast instead of generic text - Add client-side validation requiring apprise_urls or apprise_tag - Remove orphaned i18n keys from old notification settings UI (en/de/uk)
Browsers reject Access-Control-Allow-Origin: * when credentials are sent. Use AllowOriginFunc to reflect the request origin instead, which is required for cookie-based session auth with a separate frontend origin.
…artup Automatically migrate old per-trigger webhook notification settings from config.json into the new database-backed system. Triggers sharing the same webhook URL are grouped into a single notification config. The migration is idempotent — it only runs if no notification configs exist in the database yet.
Localize all user-facing strings in the notifications admin page using next-intl useTranslations(). Adds AdminNotificationsPage translation keys for page title, table columns, drawer form labels, validation messages, modal text, and toast notifications in English, German, and Ukrainian.
|
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 a Notification entity (schema, generated Ent code), a DB-backed notification Service (CRUD, templating, send/migrate), HTTP CRUD+test endpoints, worker/task wiring, admin frontend pages/hooks/i18n, and removes the legacy in-settings notification UI and test hooks. Changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
ent/schema/notification.go (1)
40-42: RemoveOptional()from enum fields sinceDefault()already handles optional-on-create semantics without the added risk of nullable columns.
apprise_type(line 40) andapprise_format(line 42) combineOptional()(nullable column) withDefault(...)(application default). SinceDefault()already makes the field not required on create,Optional()only introduces an unnecessary nullable column. If a NULL somehow reaches the database through direct SQL, Ent scans it as an invalid enum value, risking runtime panics in validation code.Using
Default()alone (withoutOptional()) generates aNOT NULLcolumn with a SQL DEFAULT, which achieves the same behavior—optional on create, sensible default—while preventing database-level NULLs entirely.♻️ Proposed fix
- field.Enum("apprise_type").Values("info", "success", "warning", "failure").Default("info").Optional().Comment("Apprise notification type."), + field.Enum("apprise_type").Values("info", "success", "warning", "failure").Default("info").Comment("Apprise notification type."), ... - field.Enum("apprise_format").Values("text", "html", "markdown").Default("text").Optional().Comment("Apprise message format."), + field.Enum("apprise_format").Values("text", "html", "markdown").Default("text").Comment("Apprise message format."),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ent/schema/notification.go` around lines 40 - 42, Remove the .Optional() call from the enum field declarations in the Notification schema so the columns are NOT NULL with a SQL DEFAULT; specifically edit the field.Enum("apprise_type") and field.Enum("apprise_format") declarations in ent/schema/notification.go to keep .Default("...")/.Default("text") but drop .Optional(), leaving apprise_tag unchanged; then regenerate/migrate the Ent schema so the DB columns are created NOT NULL with defaults.internal/live/live.go (1)
445-452: Good use ofrecover()in the notification goroutine.The panic-safe wrapper correctly prevents notification failures from crashing the live-check loop. Two minor observations:
The captured
ctxmay already be cancelled by the time the goroutine runs (ifCheckreturns quickly). For a truly fire-and-forget notification, consider usingcontext.WithoutCancel(ctx)(Go 1.21+) orcontext.Background()with a timeout to decouple the notification lifetime from the caller's context.A nil guard on
s.NotificationServicebefore launching the goroutine would prevent a confusing panic log if the service is ever unset (e.g., in tests):Suggested improvement
+ if s.NotificationService != nil { go func() { defer func() { if r := recover(); r != nil { log.Error().Interface("panic", r).Msg("panic in SendLive notification") } }() - s.NotificationService.SendLive(ctx, lwc.Edges.Channel, vod, vod.Edges.Queue, stream.GameName) + s.NotificationService.SendLive(context.WithoutCancel(ctx), lwc.Edges.Channel, vod, vod.Edges.Queue, stream.GameName) }() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/live/live.go` around lines 445 - 452, The goroutine calling s.NotificationService.SendLive may run with a canceled ctx and can panic if s.NotificationService is nil; before launching the goroutine check that s.NotificationService != nil, and create a decoupled context for the fire-and-forget call (use context.WithoutCancel(ctx) on Go 1.21+ or context.Background() with a short timeout) and pass that new ctx into SendLive(lwc.Edges.Channel, vod, vod.Edges.Queue, stream.GameName) so the notification is independent of the caller's context and avoids nil deref panics.internal/server/server.go (1)
202-202:NewHandlernow takes 19 positional parameters.This constructor already had many parameters and this PR adds one more. Consider grouping dependencies into an options/config struct to improve readability and reduce the risk of accidentally swapping arguments of the same type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/server.go` at line 202, The call to transportHttp.NewHandler now passes 19 positional services which is error-prone; refactor NewHandler and its call site to accept a single options/config struct (e.g., type HandlerOptions or NewHandlerConfig) that groups these dependencies (Database, AuthService, ChannelService, VodService, QueueService, ArchiveService, AdminService, UserService, LiveService, PlaybackService, MetricsService, PlaylistService, TaskService, ChapterService, CategoryService, BlockedVodService, NotificationService, PlatformTwitch, RiverUIServer). Update the NewHandler signature to take this struct and modify the instantiation in server.go to construct and pass the struct (preserving field names) to avoid positional argument ordering issues. Ensure all references to NewHandler across the codebase are updated to the new struct-based call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ent/notification.go`:
- Around line 17-60: The Notification Ent entity has json:"...omitempty" tags on
boolean fields (Enabled, TriggerVideoSuccess, TriggerLiveSuccess, TriggerError,
TriggerIsLive) causing false values to be omitted from JSON responses; update
the schema or response path so booleans are always present: either remove
`omitempty` for those fields in the generated Notification struct/schema
generation (so json tags become e.g. `json:"enabled"`) or stop returning Ent
entities directly and add a DTO/response struct that maps Notification ->
NotificationDTO with explicit bool fields (Enabled, TriggerVideoSuccess,
TriggerLiveSuccess, TriggerError, TriggerIsLive) to ensure false is serialized
as `false`; adjust any handlers that return Notification (e.g., functions
building API responses) to return the DTO instead.
In `@frontend/app/admin/notifications/page.tsx`:
- Around line 312-316: Update every JSX anchor element that uses target="_blank"
in the Notifications page component (the <a href="..." target="_blank">
instances inside the Notifications page JSX, e.g., the anchor rendering
{t("descriptionWikiLink")}) to include rel="noopener noreferrer"; locate the
anchor tags in the component (including the other occurrence around the second
block) and add rel="noopener noreferrer" to each to prevent reverse-tabnabbing.
In `@frontend/messages/de.json`:
- Around line 951-955: The confirm text in the deleteModal object
("deleteModal"."confirmText") is phrased as a question but missing the trailing
question mark; update the value of deleteModal.confirmText to include the "?" at
the end so it reads as a proper confirmation question.
In `@internal/notification/notification.go`:
- Around line 489-528: getVariableMap currently omits keys for nil
channel/vod/queue entities which leaves template placeholders unchanged;
initialize the variables map with all expected template keys (e.g., channel_id,
channel_ext_id, channel_display_name, vod_id, vod_ext_id, vod_platform,
vod_type, vod_title, vod_duration, vod_views, vod_resolution, vod_streamed_at,
vod_created_at, queue_id, queue_created_at and failed_task/category) set to
empty strings (or appropriate zero values) up front, then overwrite those keys
when channelItem, vodItem or qItem are non-nil so renderTemplate receives
empty-string defaults instead of missing keys.
In `@internal/transport/http/handler.go`:
- Around line 111-119: The CORS config currently unconditionally allows all
origins with credentials which is unsafe; update the
middleware.CORSWithConfig(middleware.CORSConfig{...}) call so AllowOriginFunc
validates the incoming origin against a configured allowlist (e.g., read from an
environment/config var) and only returns true when the origin exactly matches an
entry; keep AllowCredentials true only when using a strict allowlist, and ensure
any hardcoded frontend origin (the local proxy at http://127.0.0.1:3000) is
moved into that allowlist or gated by a dev flag so production never returns
true for arbitrary origins (update symbols: AllowOriginFunc,
middleware.CORSConfig, AllowCredentials to use the allowlist).
In `@internal/transport/http/notification.go`:
- Around line 67-91: The validation currently treats whitespace-only strings as
non-empty; update validateNotificationRequest to use strings.TrimSpace when
checking templates and apprise values: call strings.TrimSpace on
videoSuccessTemplate, liveSuccessTemplate, errorTemplate, isLiveTemplate,
appriseUrls and appriseTag (or trim them into local variables) and then perform
the existing empty checks and the notifType == "apprise" check against the
trimmed values so whitespace-only inputs are rejected.
---
Nitpick comments:
In `@ent/schema/notification.go`:
- Around line 40-42: Remove the .Optional() call from the enum field
declarations in the Notification schema so the columns are NOT NULL with a SQL
DEFAULT; specifically edit the field.Enum("apprise_type") and
field.Enum("apprise_format") declarations in ent/schema/notification.go to keep
.Default("...")/.Default("text") but drop .Optional(), leaving apprise_tag
unchanged; then regenerate/migrate the Ent schema so the DB columns are created
NOT NULL with defaults.
In `@internal/live/live.go`:
- Around line 445-452: The goroutine calling s.NotificationService.SendLive may
run with a canceled ctx and can panic if s.NotificationService is nil; before
launching the goroutine check that s.NotificationService != nil, and create a
decoupled context for the fire-and-forget call (use context.WithoutCancel(ctx)
on Go 1.21+ or context.Background() with a short timeout) and pass that new ctx
into SendLive(lwc.Edges.Channel, vod, vod.Edges.Queue, stream.GameName) so the
notification is independent of the caller's context and avoids nil deref panics.
In `@internal/server/server.go`:
- Line 202: The call to transportHttp.NewHandler now passes 19 positional
services which is error-prone; refactor NewHandler and its call site to accept a
single options/config struct (e.g., type HandlerOptions or NewHandlerConfig)
that groups these dependencies (Database, AuthService, ChannelService,
VodService, QueueService, ArchiveService, AdminService, UserService,
LiveService, PlaybackService, MetricsService, PlaylistService, TaskService,
ChapterService, CategoryService, BlockedVodService, NotificationService,
PlatformTwitch, RiverUIServer). Update the NewHandler signature to take this
struct and modify the instantiation in server.go to construct and pass the
struct (preserving field names) to avoid positional argument ordering issues.
Ensure all references to NewHandler across the codebase are updated to the new
struct-based call.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
ent/client.goent/ent.goent/hook/hook.goent/migrate/schema.goent/mutation.goent/notification.goent/notification/notification.goent/notification/where.goent/notification_create.goent/notification_delete.goent/notification_query.goent/notification_update.goent/predicate/predicate.goent/runtime.goent/schema/notification.goent/tx.gofrontend/app/admin/notifications/page.tsxfrontend/app/admin/settings/page.tsxfrontend/app/hooks/useConfig.tsfrontend/app/hooks/useNotification.tsfrontend/app/layout/Navbar.tsxfrontend/messages/de.jsonfrontend/messages/en.jsonfrontend/messages/uk.jsoninternal/config/config.gointernal/live/live.gointernal/notification/notification.gointernal/server/server.gointernal/tasks/shared.gointernal/tasks/shared/shared.gointernal/tasks/worker/worker.gointernal/transport/http/handler.gointernal/transport/http/notification.gointernal/worker/worker.go
Comment on lines
+111
to
119
| // AllowOriginFunc reflects the request origin so credentials work (browsers | ||
| // reject Access-Control-Allow-Origin: * with credentials). | ||
| h.Server.Use(middleware.CORSWithConfig(middleware.CORSConfig{ | ||
| AllowOriginFunc: func(origin string) (bool, error) { | ||
| return true, nil | ||
| }, | ||
| AllowMethods: []string{http.MethodGet, http.MethodHead, http.MethodPut, http.MethodPatch, http.MethodPost, http.MethodDelete}, | ||
| AllowCredentials: true, | ||
| })) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for existing CORS configuration, environment variables, and frontend URLs
rg -i 'cors|frontend.*url|allowed.*origin|CORS_ORIGIN|frontend' --type=go -g '!vendor/**' -nRepository: Zibbp/ganymede
Length of output: 1094
🏁 Script executed:
# Check for environment variable configuration patterns
rg -n 'os\.Getenv|envConfig|config\.' --type=go internal/transport/http/handler.go | head -20Repository: Zibbp/ganymede
Length of output: 194
🏁 Script executed:
# Look for any existing CORS configuration elsewhere in the codebase
rg -n 'CORSWithConfig|AllowOriginFunc' --type=go -g '!vendor/**'Repository: Zibbp/ganymede
Length of output: 370
🏁 Script executed:
# Check the config structure to understand what environment variables are available
fd -t f 'config|env' --type=go | head -20Repository: Zibbp/ganymede
Length of output: 227
🏁 Script executed:
# Look at the env config file to understand available configuration
cat -n internal/config/env.go | head -100Repository: Zibbp/ganymede
Length of output: 4273
🏁 Script executed:
# Check if there's any logic checking for external frontend
rg -n -A5 -B5 'FRONTEND_HOST|external.*frontend' --type-list
rg -n -A5 -B5 'FRONTEND_HOST|external.*frontend'Repository: Zibbp/ganymede
Length of output: 1987
🏁 Script executed:
# Look at the full handler.go file around the CORS setup
cat -n internal/transport/http/handler.go | sed -n '100,200p'Repository: Zibbp/ganymede
Length of output: 3835
Security: Unconditionally allowing all origins with credentials enables cross-origin request attacks.
AllowOriginFunc returning true for every origin means any third-party website can make credentialed (cookie-carrying) requests to this API. Combined with AllowCredentials: true, this permits session-based attacks from arbitrary domains.
The comment suggests this is conditional ("if frontend is external"), but the code unconditionally allows all origins regardless of the actual origin sent by the client. Additionally, the frontend is hardcoded to a local proxy at http://127.0.0.1:3000 (line 186).
Restrict origins to only those that should access this API. If supporting a separate frontend, add a configuration variable:
🔒 Proposed fix
+ allowedOrigin := envConfig.FrontendURL // Add to EnvConfig
h.Server.Use(middleware.CORSWithConfig(middleware.CORSConfig{
- AllowOriginFunc: func(origin string) (bool, error) {
- return true, nil
- },
+ AllowOriginFunc: func(origin string) (bool, error) {
+ return origin == allowedOrigin, nil
+ },
AllowMethods: []string{http.MethodGet, http.MethodHead, http.MethodPut, http.MethodPatch, http.MethodPost, http.MethodDelete},
AllowCredentials: true,
}))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/transport/http/handler.go` around lines 111 - 119, The CORS config
currently unconditionally allows all origins with credentials which is unsafe;
update the middleware.CORSWithConfig(middleware.CORSConfig{...}) call so
AllowOriginFunc validates the incoming origin against a configured allowlist
(e.g., read from an environment/config var) and only returns true when the
origin exactly matches an entry; keep AllowCredentials true only when using a
strict allowlist, and ensure any hardcoded frontend origin (the local proxy at
http://127.0.0.1:3000) is moved into that allowlist or gated by a dev flag so
production never returns true for arbitrary origins (update symbols:
AllowOriginFunc, middleware.CORSConfig, AllowCredentials to use the allowlist).
Contributor
Author
There was a problem hiding this comment.
This is a pre-existing issue, not introduced by this PR. The original CORS config used AllowOrigins: []string{"*"} with AllowCredentials: true, which is equally permissive (and actually broken in most browsers since Access-Control-Allow-Origin: * is rejected when credentials are enabled).
Our change to AllowOriginFunc was a targeted fix to make the dev server actually work — the old wildcard * was silently failing with credentialed requests. We intentionally kept the same permissiveness level as before rather than expanding the scope of this PR.
There was a problem hiding this comment.
@fllppi, thank you for the clarification! You're absolutely right that the wildcard * with AllowCredentials: true is rejected by browsers, so the original config was effectively non-functional for credentialed requests. Your change to AllowOriginFunc does make it work by reflecting the request origin.
I understand the decision to keep the same permissiveness level and not expand the PR's scope—that's a reasonable approach for this notification overhaul.
That said, the underlying security concern remains: allowing any origin with credentials is an insecure configuration and can result in cross-site request forgery. Would you like to open a follow-up issue to track hardening the CORS policy with an explicit allowlist (perhaps gated by a FRONTEND_URL env var for production)? That way it's documented for future work without blocking this PR.
- Add NotificationResponse DTO to avoid ent omitempty on bool fields - Add strings.TrimSpace() to template and apprise validation - Initialize all getVariableMap keys with empty-string defaults - Remove unnecessary Optional() from apprise enum schema fields - Add nil check on NotificationService in live.go goroutine - Use context.WithoutCancel for detached notification goroutine - Add rel="noopener noreferrer" to target="_blank" links
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/transport/http/notification.go (1)
129-156: Validation logic withstrings.TrimSpacelooks correct — addresses the prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transport/http/notification.go` around lines 129 - 156, The validation in validateNotificationRequest is correct as written; no code changes are required—leave the function (validateNotificationRequest) unchanged and remove the duplicate review comment/marker from the PR to avoid confusion.frontend/app/admin/notifications/page.tsx (1)
312-318:rel="noopener noreferrer"correctly applied to both external links.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/admin/notifications/page.tsx` around lines 312 - 318, The external anchor in the JSX inside the Text component already includes rel="noopener noreferrer" and target="_blank", so no code change is required; leave the <Text> block and its <a href="https://github.com/Zibbp/ganymede/wiki/Notifications" target="_blank" rel="noopener noreferrer"> (reference the anchor element around t("descriptionWikiLink") inside the Text component) as-is to ensure both security attributes are present.internal/notification/notification.go (1)
486-544: All variable map keys initialized to empty-string defaults — prior review addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 486 - 544, The variables map currently initializes all keys to empty-string defaults; change these defaults to match the actual field types instead of always using "", e.g. keep string keys as "" but set numeric fields (like "vod_views", "vod_duration") to 0 and time fields (like "vod_streamed_at", "vod_created_at", "queue_created_at") to time.Time{} (import time if needed), while leaving "failed_task" and "category" as strings; update the initial map in getVariableMap so each key's default matches the underlying ent.* field type and preserve the existing overwrites from channelItem, vodItem, and qItem.
🧹 Nitpick comments (2)
internal/transport/http/notification.go (1)
28-67:CreateNotificationRequestandUpdateNotificationRequestare identical — consider deduplication.Both structs share the same 17 fields and validate tags with no difference. If they're intentionally kept separate for future divergence, a comment would help; otherwise, a type alias removes the redundancy.
♻️ Proposed simplification
-// UpdateNotificationRequest is the request body for updating a notification. -type UpdateNotificationRequest struct { - Name string `json:"name" validate:"required"` - Enabled bool `json:"enabled"` - Type string `json:"type" validate:"required,oneof=webhook apprise"` - URL string `json:"url" validate:"required,http_url"` - TriggerVideoSuccess bool `json:"trigger_video_success"` - TriggerLiveSuccess bool `json:"trigger_live_success"` - TriggerError bool `json:"trigger_error"` - TriggerIsLive bool `json:"trigger_is_live"` - VideoSuccessTemplate string `json:"video_success_template"` - LiveSuccessTemplate string `json:"live_success_template"` - ErrorTemplate string `json:"error_template"` - IsLiveTemplate string `json:"is_live_template"` - AppriseUrls string `json:"apprise_urls"` - AppriseTitle string `json:"apprise_title"` - AppriseType string `json:"apprise_type" validate:"omitempty,oneof=info success warning failure"` - AppriseTag string `json:"apprise_tag"` - AppriseFormat string `json:"apprise_format" validate:"omitempty,oneof=text html markdown"` -} +// UpdateNotificationRequest is the request body for updating a notification. +// It shares the same shape as CreateNotificationRequest. +type UpdateNotificationRequest = CreateNotificationRequest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transport/http/notification.go` around lines 28 - 67, CreateNotificationRequest and UpdateNotificationRequest are identical; remove duplication by making UpdateNotificationRequest a type alias or by extracting a single shared struct (e.g., NotificationRequest) and use it for both request types. Update usages of UpdateNotificationRequest to reference the new alias/type and keep validation tags on the single definition (preserve fields like AppriseType and AppriseFormat). If you intend to keep them separate for future divergence, add a brief comment above UpdateNotificationRequest explaining that it's intentionally duplicated.internal/notification/notification.go (1)
353-362:send,sendWebhook, andsendAppriseWithTitleignore the caller's context.The
Send*dispatch methods acceptctxbut it's never threaded through to the outbound HTTP requests. ForSendTestNotificationin particular (called from an HTTP handler with a live request context), a client disconnect leaves the notification HTTP request running for up to the 15 shttpClienttimeout. Consider accepting and forwarding a context withhttp.NewRequestWithContext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 353 - 362, The send path (functions send, sendWebhook, sendAppriseWithTitle and callers like SendTestNotification) currently ignores caller context causing outbound HTTP calls to keep running after the request is cancelled; update these functions to accept a context.Context parameter, propagate that ctx through the call chain, and create outbound requests with http.NewRequestWithContext(ctx, ...) (or use req = req.WithContext(ctx)) so the HTTP client respects cancellations and deadlines; change signatures (e.g., send(ctx context.Context, n *ent.Notification, ...), sendWebhook(ctx context.Context, ...), sendAppriseWithTitle(ctx context.Context, ...)) and update all call sites accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/admin/notifications/page.tsx`:
- Around line 199-221: The edit handler leaves prior validation errors on the
form because form.setValues doesn't clear errors; update handleOpenEdit to first
clear the form's validation state (call the form's clear/reset-errors API, e.g.,
form.clearErrors()) before calling form.setValues and opening the drawer
(functions: handleOpenEdit, form.setValues, form.clearErrors,
setEditingNotification, openDrawer).
In `@internal/notification/notification.go`:
- Around line 525-535: The time fields inserted into the template variables
(e.g., vod_streamed_at, vod_created_at, queue_created_at) are currently assigned
raw time.Time values which, when rendered with fmt.Sprintf("%v"), include the
monotonic clock suffix; update getVariableMap (and the analogous
getTestVariableMap) to pre-format these time.Time fields to a stable,
user-friendly string (for example via t.Format(time.RFC3339)) before assigning
to variables — handle zero times (t.IsZero()) by assigning an empty string or
nil-equivalent string to avoid printing zero-values. Ensure you update the
assignments for vodItem.StreamedAt, vodItem.CreatedAt and queue.CreatedAt in
getVariableMap and their equivalents in getTestVariableMap.
---
Duplicate comments:
In `@frontend/app/admin/notifications/page.tsx`:
- Around line 312-318: The external anchor in the JSX inside the Text component
already includes rel="noopener noreferrer" and target="_blank", so no code
change is required; leave the <Text> block and its <a
href="https://github.com/Zibbp/ganymede/wiki/Notifications" target="_blank"
rel="noopener noreferrer"> (reference the anchor element around
t("descriptionWikiLink") inside the Text component) as-is to ensure both
security attributes are present.
In `@internal/notification/notification.go`:
- Around line 486-544: The variables map currently initializes all keys to
empty-string defaults; change these defaults to match the actual field types
instead of always using "", e.g. keep string keys as "" but set numeric fields
(like "vod_views", "vod_duration") to 0 and time fields (like "vod_streamed_at",
"vod_created_at", "queue_created_at") to time.Time{} (import time if needed),
while leaving "failed_task" and "category" as strings; update the initial map in
getVariableMap so each key's default matches the underlying ent.* field type and
preserve the existing overwrites from channelItem, vodItem, and qItem.
In `@internal/transport/http/notification.go`:
- Around line 129-156: The validation in validateNotificationRequest is correct
as written; no code changes are required—leave the function
(validateNotificationRequest) unchanged and remove the duplicate review
comment/marker from the PR to avoid confusion.
---
Nitpick comments:
In `@internal/notification/notification.go`:
- Around line 353-362: The send path (functions send, sendWebhook,
sendAppriseWithTitle and callers like SendTestNotification) currently ignores
caller context causing outbound HTTP calls to keep running after the request is
cancelled; update these functions to accept a context.Context parameter,
propagate that ctx through the call chain, and create outbound requests with
http.NewRequestWithContext(ctx, ...) (or use req = req.WithContext(ctx)) so the
HTTP client respects cancellations and deadlines; change signatures (e.g.,
send(ctx context.Context, n *ent.Notification, ...), sendWebhook(ctx
context.Context, ...), sendAppriseWithTitle(ctx context.Context, ...)) and
update all call sites accordingly.
In `@internal/transport/http/notification.go`:
- Around line 28-67: CreateNotificationRequest and UpdateNotificationRequest are
identical; remove duplication by making UpdateNotificationRequest a type alias
or by extracting a single shared struct (e.g., NotificationRequest) and use it
for both request types. Update usages of UpdateNotificationRequest to reference
the new alias/type and keep validation tags on the single definition (preserve
fields like AppriseType and AppriseFormat). If you intend to keep them separate
for future divergence, add a brief comment above UpdateNotificationRequest
explaining that it's intentionally duplicated.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ent/migrate/schema.goent/mutation.goent/notification/where.goent/notification_create.goent/notification_update.goent/schema/notification.gofrontend/app/admin/notifications/page.tsxinternal/live/live.gointernal/notification/notification.gointernal/transport/http/notification.go
…ext, deduplicate request types - Clear form validation errors in handleOpenEdit before populating values - Format time.Time fields to RFC3339 in template variable maps (avoids monotonic clock suffix); use type-correct defaults (0 for numeric fields) - Propagate context.Context through send/sendWebhook/sendAppriseWithTitle and use http.NewRequestWithContext so outbound calls respect cancellation - Extract shared NotificationRequest struct with type aliases for CreateNotificationRequest and UpdateNotificationRequest
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/transport/http/notification.go (2)
116-142: Consider accepting the struct directly to reduce the 11-parameter signature.
validateNotificationRequesttakes 11 individual arguments that are all already fields onNotificationRequest. Passing the struct would simplify the signature and make call sites less error-prone.Proposed refactor
-func validateNotificationRequest(notifType string, triggerVideoSuccess, triggerLiveSuccess, triggerError, triggerIsLive bool, videoSuccessTemplate, liveSuccessTemplate, errorTemplate, isLiveTemplate, appriseUrls, appriseTag string) error { +func validateNotificationRequest(req NotificationRequest) error { // At least one trigger must be enabled - if !triggerVideoSuccess && !triggerLiveSuccess && !triggerError && !triggerIsLive { + if !req.TriggerVideoSuccess && !req.TriggerLiveSuccess && !req.TriggerError && !req.TriggerIsLive { return fmt.Errorf("at least one trigger must be enabled") } - if triggerVideoSuccess && strings.TrimSpace(videoSuccessTemplate) == "" { + if req.TriggerVideoSuccess && strings.TrimSpace(req.VideoSuccessTemplate) == "" { return fmt.Errorf("video success template is required when video success trigger is enabled") } // ... same pattern for other fieldsCall sites simplify to:
if err := validateNotificationRequest(req); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transport/http/notification.go` around lines 116 - 142, The function validateNotificationRequest currently accepts 11 separate parameters; change its signature to accept the NotificationRequest struct (e.g., func validateNotificationRequest(req NotificationRequest) error or a pointer) and update the body to reference req.TriggerVideoSuccess, req.VideoSuccessTemplate, req.Type (or whatever field names are used) instead of the individual args; then update all call sites to pass the NotificationRequest instance (e.g., validateNotificationRequest(req)) so the logic and error messages remain identical but the API is simpler and less error-prone.
195-213: Duplicateent.Notificationconstruction in Create and Update handlers.The struct literal at lines 195–213 is nearly identical to lines 246–264. Consider extracting a helper like
notificationFromRequest(req NotificationRequest) *ent.Notificationto keep both handlers DRY.Also applies to: 246-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transport/http/notification.go` around lines 195 - 213, The Create and Update handlers both build an almost identical ent.Notification struct; extract a helper function notificationFromRequest(req NotificationRequest) *ent.Notification (or similar) that maps fields from the request to an ent.Notification and replace the duplicated struct literals in the Create and Update handlers with calls to that helper; ensure the helper sets Name, Enabled, Type (entNotification.Type(req.Type)), URL, all Trigger*, Template fields, IsLiveTemplate, AppriseUrls/Title/Type/Tag/Format (using entNotification.AppriseType/AppriseFormat where used) so behavior remains identical.internal/notification/notification.go (2)
556-577: Test variable map omitsfailed_task/categorythatSendTestNotificationlater overwrites — consider a note.
getTestVariableMapsetsfailed_taskandcategoryto"", andSendTestNotificationoverwrites them only for the"error"and"is_live"cases. This is fine, but a brief inline comment at lines 575-576 (e.g.,// overwritten by SendTestNotification for relevant event types) would improve readability for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 556 - 577, getTestVariableMap initializes failed_task and category as empty strings but SendTestNotification mutates those fields for "error" and "is_live" events; add a concise inline note either next to the "failed_task" and "category" entries in getTestVariableMap or immediately above getTestVariableMap stating "// overwritten by SendTestNotification for relevant event types" to make the behavior explicit for future readers (reference: getTestVariableMap and SendTestNotification).
466-481: Template replacement can cascade if a variable value itself contains{{placeholder}}syntax.
renderTemplatecollects all matches from the original template, then mutatestmplwithstrings.ReplaceAllin a loop. If an earlier variable's value happens to contain{{another_var}}(e.g., a VOD title like"Exploring {{category}} games"), a subsequent replacement pass will substitute it too, producing unexpected output.The risk is low for typical notification data but could surprise users with creative VOD titles. A safer approach is to build the result in a single left-to-right pass using
ReplaceAllStringFunc.Single-pass alternative
func renderTemplate(tmpl string, variableMap map[string]interface{}) string { - res := templateVariableRegex.FindAllStringSubmatch(tmpl, -1) - for _, match := range res { - variableName := strings.TrimSpace(match[1]) - variableValue, ok := variableMap[variableName] - if !ok || variableValue == nil { - continue - } - variableValueString := fmt.Sprintf("%v", variableValue) - tmpl = strings.ReplaceAll(tmpl, match[0], variableValueString) - } - return tmpl + return templateVariableRegex.ReplaceAllStringFunc(tmpl, func(match string) string { + inner := templateVariableRegex.FindStringSubmatch(match) + if inner == nil { + return match + } + variableName := strings.TrimSpace(inner[1]) + variableValue, ok := variableMap[variableName] + if !ok || variableValue == nil { + return match + } + return fmt.Sprintf("%v", variableValue) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 466 - 481, renderTemplate currently mutates tmpl while iterating matches from templateVariableRegex, allowing a variable's replacement text that contains {{...}} to be re-processed (cascading replacement); change renderTemplate to perform a single left-to-right pass without mutating the input during iteration (e.g., use templateVariableRegex.ReplaceAllStringFunc or build the output with a strings.Builder using the regex match indices) so each placeholder is replaced exactly once, resolving variable names via variableMap (and leaving unknown or nil entries untouched) while referencing the existing function renderTemplate and templateVariableRegex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/admin/notifications/page.tsx`:
- Around line 380-406: The ActionIcon buttons for edit/test/delete lack
accessible labels; update the three ActionIcon elements (the ones that call
handleOpenEdit(n), setTestingNotification(n)+openTestModal(), and
setDeletingNotification(n)+openDeleteModal()) to include an aria-label (and
optional title) using localized strings (e.g., t('notifications.edit'),
t('notifications.test'), t('notifications.delete')) so screen readers get
meaningful names; ensure the labels match your i18n keys and add imports/usages
of the translator if needed.
In `@internal/notification/notification.go`:
- Around line 117-228: The log in MigrateFromLegacyConfig currently emits the
full webhook URL (variable url) which may contain secrets; update the logging
call that currently uses log.Info().Str("name", name).Str("url", url").Msg(...)
so it redacts or truncates the URL before logging (e.g., mask the path/token
portion or log only the host and first/last N chars), ensure the redaction logic
is applied consistently where the migrated notification is created and in the
final summary log (len(groups) remains unchanged), and keep the original log
fields name and count but replace the raw url with a safe redactedUrl value.
---
Nitpick comments:
In `@internal/notification/notification.go`:
- Around line 556-577: getTestVariableMap initializes failed_task and category
as empty strings but SendTestNotification mutates those fields for "error" and
"is_live" events; add a concise inline note either next to the "failed_task" and
"category" entries in getTestVariableMap or immediately above getTestVariableMap
stating "// overwritten by SendTestNotification for relevant event types" to
make the behavior explicit for future readers (reference: getTestVariableMap and
SendTestNotification).
- Around line 466-481: renderTemplate currently mutates tmpl while iterating
matches from templateVariableRegex, allowing a variable's replacement text that
contains {{...}} to be re-processed (cascading replacement); change
renderTemplate to perform a single left-to-right pass without mutating the input
during iteration (e.g., use templateVariableRegex.ReplaceAllStringFunc or build
the output with a strings.Builder using the regex match indices) so each
placeholder is replaced exactly once, resolving variable names via variableMap
(and leaving unknown or nil entries untouched) while referencing the existing
function renderTemplate and templateVariableRegex.
In `@internal/transport/http/notification.go`:
- Around line 116-142: The function validateNotificationRequest currently
accepts 11 separate parameters; change its signature to accept the
NotificationRequest struct (e.g., func validateNotificationRequest(req
NotificationRequest) error or a pointer) and update the body to reference
req.TriggerVideoSuccess, req.VideoSuccessTemplate, req.Type (or whatever field
names are used) instead of the individual args; then update all call sites to
pass the NotificationRequest instance (e.g., validateNotificationRequest(req))
so the logic and error messages remain identical but the API is simpler and less
error-prone.
- Around line 195-213: The Create and Update handlers both build an almost
identical ent.Notification struct; extract a helper function
notificationFromRequest(req NotificationRequest) *ent.Notification (or similar)
that maps fields from the request to an ent.Notification and replace the
duplicated struct literals in the Create and Update handlers with calls to that
helper; ensure the helper sets Name, Enabled, Type
(entNotification.Type(req.Type)), URL, all Trigger*, Template fields,
IsLiveTemplate, AppriseUrls/Title/Type/Tag/Format (using
entNotification.AppriseType/AppriseFormat where used) so behavior remains
identical.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/admin/notifications/page.tsxinternal/notification/notification.gointernal/transport/http/notification.go
- Add aria-label and title to ActionIcon buttons with i18n (en/de/uk) - Redact webhook URLs in migration logs to avoid leaking secrets - Fix renderTemplate cascading replacement by using ReplaceAllStringFunc for a single left-to-right pass instead of mutating during iteration - Simplify validateNotificationRequest to accept NotificationRequest struct - Extract notificationFromRequest helper to deduplicate Create/Update - Add clarifying comment on getTestVariableMap overwritten fields
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/messages/de.json (1)
956-961:⚠️ Potential issue | 🟡 MinorMissing
?in delete confirmation text.
"confirmText"reads as a question but lacks the trailing?, unlike all other confirmation strings in the file.✏️ Proposed fix
- "confirmText": "Möchten Sie die Benachrichtigung wirklich löschen", + "confirmText": "Möchten Sie die Benachrichtigung wirklich löschen?",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/messages/de.json` around lines 956 - 961, The "confirmText" string inside the deleteModal JSON is missing a trailing question mark; update the value of deleteModal.confirmText to include a trailing "?" so it reads as a proper question (e.g., append "?" to the existing text) to match other confirmation strings in the file.
🧹 Nitpick comments (1)
internal/notification/notification.go (1)
484-498: Consider extracting the variable name directly from the match string instead of re-running the regex.
ReplaceAllStringFuncalready guaranteesmatchis a complete{{...}}token, so callingFindStringSubmatch(match)re-executes the compiled regex needlessly. The variable name can be extracted with a direct slice:♻️ Proposed simplification
func renderTemplate(tmpl string, variableMap map[string]interface{}) string { return templateVariableRegex.ReplaceAllStringFunc(tmpl, func(match string) string { - // Extract the variable name from the {{...}} match - inner := templateVariableRegex.FindStringSubmatch(match) - if len(inner) < 2 { - return match - } - variableName := strings.TrimSpace(inner[1]) + // match is guaranteed to be "{{...}}" — strip the delimiters directly + variableName := strings.TrimSpace(match[2 : len(match)-2]) variableValue, ok := variableMap[variableName] if !ok || variableValue == nil { return match } return fmt.Sprintf("%v", variableValue) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 484 - 498, The renderTemplate function re-runs templateVariableRegex inside ReplaceAllStringFunc (via templateVariableRegex.FindStringSubmatch(match)) which is wasteful; instead extract the variable name directly from the match string provided to the closure (strip the leading "{{" and trailing "}}", then strings.TrimSpace the result) and use that key to look up variableMap exactly as currently done (preserve the nil/missing behavior and fmt.Sprintf conversion). Update the closure in renderTemplate to parse the variable name by slicing the match, trimming whitespace, and avoid calling templateVariableRegex.FindStringSubmatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/messages/en.json`:
- Around line 956-961: The delete confirmation string is missing a trailing
question mark; update the "confirmText" value inside the "deleteModal" object so
it reads as a proper question (add "?" at the end of the string) to match other
confirmation prompts, e.g., modify the "deleteModal"."confirmText" entry
accordingly.
In `@frontend/messages/uk.json`:
- Around line 954-959: The confirmText under the deleteModal object is missing a
trailing question mark; update the "confirmText" value in the deleteModal (key:
"deleteModal" -> "confirmText") to include a final "?" so it reads as a proper
question (e.g., append "?" to the existing string) to match other confirmation
prompts.
---
Duplicate comments:
In `@frontend/messages/de.json`:
- Around line 956-961: The "confirmText" string inside the deleteModal JSON is
missing a trailing question mark; update the value of deleteModal.confirmText to
include a trailing "?" so it reads as a proper question (e.g., append "?" to the
existing text) to match other confirmation strings in the file.
---
Nitpick comments:
In `@internal/notification/notification.go`:
- Around line 484-498: The renderTemplate function re-runs templateVariableRegex
inside ReplaceAllStringFunc (via
templateVariableRegex.FindStringSubmatch(match)) which is wasteful; instead
extract the variable name directly from the match string provided to the closure
(strip the leading "{{" and trailing "}}", then strings.TrimSpace the result)
and use that key to look up variableMap exactly as currently done (preserve the
nil/missing behavior and fmt.Sprintf conversion). Update the closure in
renderTemplate to parse the variable name by slicing the match, trimming
whitespace, and avoid calling templateVariableRegex.FindStringSubmatch.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app/admin/notifications/page.tsxfrontend/messages/de.jsonfrontend/messages/en.jsonfrontend/messages/uk.jsoninternal/notification/notification.gointernal/transport/http/notification.go
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/admin/notifications/page.tsx
Strip {{}} delimiters via string slicing instead of re-running
templateVariableRegex.FindStringSubmatch on the already-matched string.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/notification/notification.go (2)
339-361: Use constants for test event types to prevent drift.
This avoids typos and keeps the accepted event list in one place.♻️ Suggested refactor
+const ( + testEventVideoSuccess = "video_success" + testEventLiveSuccess = "live_success" + testEventError = "error" + testEventIsLive = "is_live" +) + // SendTestNotification sends a test notification using the config's own templates with dummy data. func (s *Service) SendTestNotification(ctx context.Context, n *ent.Notification, eventType string) error { @@ var tmpl string switch eventType { - case "video_success": + case testEventVideoSuccess: tmpl = n.VideoSuccessTemplate - case "live_success": + case testEventLiveSuccess: tmpl = n.LiveSuccessTemplate - case "error": + case testEventError: variableMap["failed_task"] = "video_download" tmpl = n.ErrorTemplate - case "is_live": + case testEventIsLive: variableMap["category"] = "Demo Game" tmpl = n.IsLiveTemplate default: return fmt.Errorf("unknown test notification event type: %s", eventType) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 339 - 361, Replace string literals for test event types in SendTestNotification with shared constants to avoid drift: define constants (e.g., TestEventVideoSuccess, TestEventLiveSuccess, TestEventError, TestEventIsLive) and use those constant names in the switch inside SendTestNotification (replace "video_success", "live_success", "error", "is_live"). Ensure the constants are declared at package scope (so the accepted event list is centralized) and update any other callers/tests that use the literal strings to use the new constants.
251-337: Consider extracting the common query/send loop.
SendVideoArchiveSuccess,SendLiveArchiveSuccess,SendError, andSendLiveshare the same structure. A small helper would reduce duplication and keep logging/behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 251 - 337, All four methods (SendVideoArchiveSuccess, SendLiveArchiveSuccess, SendError, SendLive) duplicate the same query/render/send loop; extract a helper on Service (e.g., sendNotificationsFor) that accepts the entNotification predicate(s) or a filter builder, the template selector (one of VideoSuccessTemplate, LiveSuccessTemplate, ErrorTemplate, IsLiveTemplate) and the precomputed variableMap, performs the Notification.Query().Where(...).All(ctx), iterates notifications, calls renderTemplate(template, variableMap) and s.send(ctx, n, body, variableMap), and centralizes the error logging using n.ID and n.Name; then replace each original method to build the appropriate variableMap and call the helper with the correct predicate (entNotification.TriggerVideoSuccessEQ(true), TriggerLiveSuccessEQ(true), TriggerErrorEQ(true), TriggerIsLiveEQ(true)) and template field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/notification/notification.go`:
- Around line 131-242: Wrap the whole migration in a DB transaction inside
MigrateFromLegacyConfig so the pre-check and all Notification.Create calls are
atomic: start a transaction (ent Tx) and use tx.Notification.Query().Count(ctx)
for the existence check and replace every s.Store.Client.Notification.Create()
with tx.Notification.Create(), ensuring you call tx.Commit() on success and
tx.Rollback()/defer rollback on error to avoid partial writes; keep the existing
redactURL and logging behavior but return tx.Commit() result instead of nil.
---
Nitpick comments:
In `@internal/notification/notification.go`:
- Around line 339-361: Replace string literals for test event types in
SendTestNotification with shared constants to avoid drift: define constants
(e.g., TestEventVideoSuccess, TestEventLiveSuccess, TestEventError,
TestEventIsLive) and use those constant names in the switch inside
SendTestNotification (replace "video_success", "live_success", "error",
"is_live"). Ensure the constants are declared at package scope (so the accepted
event list is centralized) and update any other callers/tests that use the
literal strings to use the new constants.
- Around line 251-337: All four methods (SendVideoArchiveSuccess,
SendLiveArchiveSuccess, SendError, SendLive) duplicate the same
query/render/send loop; extract a helper on Service (e.g., sendNotificationsFor)
that accepts the entNotification predicate(s) or a filter builder, the template
selector (one of VideoSuccessTemplate, LiveSuccessTemplate, ErrorTemplate,
IsLiveTemplate) and the precomputed variableMap, performs the
Notification.Query().Where(...).All(ctx), iterates notifications, calls
renderTemplate(template, variableMap) and s.send(ctx, n, body, variableMap), and
centralizes the error logging using n.ID and n.Name; then replace each original
method to build the appropriate variableMap and call the helper with the correct
predicate (entNotification.TriggerVideoSuccessEQ(true),
TriggerLiveSuccessEQ(true), TriggerErrorEQ(true), TriggerIsLiveEQ(true)) and
template field.
Wrap the legacy notification migration process in a transaction to ensure atomicity, preventing orphaned rows on failure. Adjusted database queries to use the transaction context and added rollback logic for error handling. This change improves the reliability of the migration process.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai resume |
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 `@internal/notification/notification.go`:
- Around line 446-493: In sendAppriseWithTitle, AppriseUrls and AppriseTag are
assigned raw from n and therefore templates like {{variable}} never get
rendered; update the code that sets payload.URLs and payload.Tag to call
renderTemplate(n.AppriseUrls, variableMap) and renderTemplate(n.AppriseTag,
variableMap) (guarding on non-empty strings and variableMap != nil if you
prefer) so the URL and tag templates are expanded before marshalling the
appriseRequestBody.
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/notification/notification.go (1)
446-494:⚠️ Potential issue | 🟠 MajorRender Apprise URL/tag templates before sending.
Line 453 and Line 462 still pass rawAppriseUrls/AppriseTag, so{{variable}}placeholders won’t render.🧩 Suggested fix
if n.AppriseUrls != "" { - payload.URLs = n.AppriseUrls + if variableMap != nil { + payload.URLs = renderTemplate(n.AppriseUrls, variableMap) + } else { + payload.URLs = n.AppriseUrls + } } if n.AppriseTitle != "" { payload.Title = renderTemplate(n.AppriseTitle, variableMap) } @@ if n.AppriseTag != "" { - payload.Tag = n.AppriseTag + if variableMap != nil { + payload.Tag = renderTemplate(n.AppriseTag, variableMap) + } else { + payload.Tag = n.AppriseTag + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 446 - 494, In sendAppriseWithTitle, AppriseUrls and AppriseTag are assigned raw from n.AppriseUrls and n.AppriseTag so any {{variable}} placeholders never get rendered; before setting payload.URLs and payload.Tag call renderTemplate on n.AppriseUrls and n.AppriseTag with the provided variableMap (similar to how AppriseTitle is handled) and assign the rendered strings to payload.URLs and payload.Tag so the templates are expanded prior to marshalling and sending.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/notification/notification.go`:
- Around line 446-494: In sendAppriseWithTitle, AppriseUrls and AppriseTag are
assigned raw from n.AppriseUrls and n.AppriseTag so any {{variable}}
placeholders never get rendered; before setting payload.URLs and payload.Tag
call renderTemplate on n.AppriseUrls and n.AppriseTag with the provided
variableMap (similar to how AppriseTitle is handled) and assign the rendered
strings to payload.URLs and payload.Tag so the templates are expanded prior to
marshalling and sending.
- Apply renderTemplate to AppriseUrls, AppriseTitle, and AppriseTag in
sendAppriseWithTitle so {{variable}} placeholders are expanded
- Escape {{channel_display_name}} in i18n placeholder strings with ICU
single quotes to prevent next-intl MALFORMED_ARGUMENT errors (en/de/uk)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/notification/notification.go (2)
80-87: Optional: capture the builder return value forSetAppriseType/SetAppriseFormat.Functionally safe due to pointer-receiver semantics (Ent's
Set*methods mutate the underlyingmutationin place), but discarding the return value deviates from the idiomatic fluent/chained Ent builder pattern and may trigger linter warnings.♻️ Proposed refactor (applies to both
CreateNotificationandUpdateNotification)if n.AppriseType != "" { - builder.SetAppriseType(entNotification.AppriseType(n.AppriseType)) + builder = builder.SetAppriseType(entNotification.AppriseType(n.AppriseType)) } if n.AppriseFormat != "" { - builder.SetAppriseFormat(entNotification.AppriseFormat(n.AppriseFormat)) + builder = builder.SetAppriseFormat(entNotification.AppriseFormat(n.AppriseFormat)) }Also applies to: 121-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 80 - 87, The code calls builder.SetAppriseType and builder.SetAppriseFormat but discards their return values; to follow Ent's fluent builder pattern and avoid linter warnings, capture and reassign the returned builder (e.g., builder = builder.SetAppriseType(...)) before calling builder.Save(ctx); apply the same change in both CreateNotification and UpdateNotification where SetAppriseType/SetAppriseFormat are used so subsequent chained calls operate on the returned builder.
177-187: Nit:urlparameter/variable shadows thenet/urlpackage import.The
addTriggerclosure parameter (line 177) and thefor url, g := range groupsloop variable (line 220) both shadow the imported"net/url"package withinMigrateFromLegacyConfig. No functional impact now, but any future call tourl.Parse(...)inside the function would fail to compile without a clear explanation. Renaming towebhookURLremoves the ambiguity.♻️ Proposed rename
- addTrigger := func(url string, label string, apply func(g *triggerGroup)) { - if url == "" { + addTrigger := func(webhookURL string, label string, apply func(g *triggerGroup)) { + if webhookURL == "" { return } - g, ok := groups[url] + g, ok := groups[webhookURL] if !ok { g = &triggerGroup{name: label} - groups[url] = g + groups[webhookURL] = g } apply(g) }- for url, g := range groups { + for webhookURL, g := range groups { // Build a descriptive name name := "Migrated Webhook" if len(groups) > 1 { name = "Migrated: " + g.name } builder := tx.Notification.Create(). SetName(name). SetEnabled(true). SetType(entNotification.TypeWebhook). - SetURL(url). + SetURL(webhookURL). ... - if _, err := builder.Save(ctx); err != nil { + if _, err := builder.Save(ctx); err != nil { _ = tx.Rollback() - return fmt.Errorf("error creating migrated notification for %s: %w", redactURL(url), err) + return fmt.Errorf("error creating migrated notification for %s: %w", redactURL(webhookURL), err) } - log.Info().Str("name", name).Str("url", redactURL(url)).Msg("migrated legacy notification to database") + log.Info().Str("name", name).Str("url", redactURL(webhookURL)).Msg("migrated legacy notification to database") }Also applies to: 220-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 177 - 187, The closure addTrigger currently takes a parameter named url which shadows the imported net/url package (and the for loop variable for url, g := range groups also shadows it); rename the parameter in addTrigger to webhookURL (and update its internal uses) and likewise rename the loop variable to webhookURL (or webhook, webhookKey) so any future calls to url.Parse or other net/url references resolve to the package; update all references to those variables within MigrateFromLegacyConfig (including where addTrigger is called and where groups are iterated) to use the new names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/messages/de.json`:
- Around line 888-995: The AdminNotificationsPage translations use formal
"Sie/Ihrer" while the rest of the file uses informal "du/dein"; update all
strings under "AdminNotificationsPage" to informal German to match the project
convention — specifically change occurrences in keys such as description,
appriseSettingsDescription, urlDescriptionApprise, eventTriggersDescription,
eventTriggersLabel, deleteModal.confirmText and similar nested keys (e.g.,
"Möchten Sie..." → "Möchtest du...", "Besuchen Sie das" → "Besuche das", "Ihrer"
→ "deiner", "Wählen Sie" → "Wähle"). Ensure pronouns, possessives and verb
conjugations are consistently converted to the informal form across the entire
AdminNotificationsPage block.
---
Nitpick comments:
In `@internal/notification/notification.go`:
- Around line 80-87: The code calls builder.SetAppriseType and
builder.SetAppriseFormat but discards their return values; to follow Ent's
fluent builder pattern and avoid linter warnings, capture and reassign the
returned builder (e.g., builder = builder.SetAppriseType(...)) before calling
builder.Save(ctx); apply the same change in both CreateNotification and
UpdateNotification where SetAppriseType/SetAppriseFormat are used so subsequent
chained calls operate on the returned builder.
- Around line 177-187: The closure addTrigger currently takes a parameter named
url which shadows the imported net/url package (and the for loop variable for
url, g := range groups also shadows it); rename the parameter in addTrigger to
webhookURL (and update its internal uses) and likewise rename the loop variable
to webhookURL (or webhook, webhookKey) so any future calls to url.Parse or other
net/url references resolve to the package; update all references to those
variables within MigrateFromLegacyConfig (including where addTrigger is called
and where groups are iterated) to use the new names.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/messages/de.jsonfrontend/messages/en.jsonfrontend/messages/uk.jsoninternal/notification/notification.go
the logic will remove anything that isn't in the config struct so it was being removed before the legacy migration function could run
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 `@internal/notification/notification.go`:
- Around line 140-217: MigrateFromLegacyConfig currently dereferences the legacy
pointer (e.g. legacy.VideoSuccessEnabled) without checking for nil; add an early
guard at the start of MigrateFromLegacyConfig that returns nil if legacy == nil
(i.e. skip migration) before any transaction is opened or any legacy fields are
accessed so the function safely no-ops instead of panicking.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/config/config.gointernal/notification/notification.gointernal/notification/notification_test.go
Owner
|
I fixed an issue in the config with the legacy logic. The config logic will remove any items that don't exist so it was removing |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
internal/notification/notification.go (2)
230-235: Migrated config name is non-deterministic when multiple triggers share a URL.
g.nameis set from whichever trigger's label was processed first byaddTriggerfor that URL. Because Go map iteration order is random, re-running migration (or having the code path hit a different order) could assign a misleading label — e.g.,"Migrated: Error"to a group that fires for video, live, and error events.✨ Proposed fix: derive the name from all active triggers
- name := "Migrated Webhook" - if len(groups) > 1 { - name = "Migrated: " + g.name - } + var parts []string + if g.triggerVideoSuccess { parts = append(parts, "Video") } + if g.triggerLiveSuccess { parts = append(parts, "Live") } + if g.triggerError { parts = append(parts, "Error") } + if g.triggerIsLive { parts = append(parts, "IsLive") } + name := "Migrated: " + strings.Join(parts, "/") + if len(parts) == 0 { name = "Migrated Webhook" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 230 - 235, The current loop over "for url, g := range groups" sets the migrated config name from g.name which is non-deterministic because map iteration order varies; change the naming logic in that loop (and any related addTrigger handling) to derive the name deterministically from all active triggers in the group: collect the set of trigger types/labels present on g (e.g., from g.triggers or the slice/map field that stores triggers), sort those labels lexicographically, then build the descriptive name (e.g., "Migrated: "+strings.Join(sortedLabels, ", ") when multiple triggers exist, or use a single label when only one) so the resulting name is stable across runs. Ensure you reference and use the group structure (g) and its trigger collection rather than g.name alone.
230-235: Migrated config name may not reflect all active triggers when multiple triggers share a URL.
g.nameis set from the firstaddTriggercall for a given URL — and because Go map iteration is unordered, the "first" trigger varies across runs (or between migrations). A record could be named"Migrated: Error"even though it fires for video, live, and error events.Consider building the name from all active triggers in the group after iteration:
✨ Proposed fix
- name := "Migrated Webhook" - if len(groups) > 1 { - name = "Migrated: " + g.name - } + var parts []string + if g.triggerVideoSuccess { parts = append(parts, "Video") } + if g.triggerLiveSuccess { parts = append(parts, "Live") } + if g.triggerError { parts = append(parts, "Error") } + if g.triggerIsLive { parts = append(parts, "IsLive") } + name := "Migrated: " + strings.Join(parts, "/") + if len(parts) == 0 { name = "Migrated Webhook" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/notification/notification.go` around lines 230 - 235, The current loop over for url, g := range groups uses g.name (populated by addTrigger) which depends on unordered map iteration, causing inconsistent "Migrated: ..." labels; instead, after collecting all triggers for the group, build the migration name from the full set of active trigger types (e.g., collect g.triggers or g.types, deduplicate and sort them, then join into a descriptive string like "Migrated: error, live, video") and use that composed name instead of g.name so the label consistently reflects all triggers for that URL (update the code around the for url, g := range groups loop and references to g.name/addTrigger accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/notification/notification.go`:
- Around line 637-661: Update the dummy value for channel_ext_id in
getTestVariableMap to a realistic non-typo string (replace "1234456789" with the
corrected sequence, e.g., "1234567890" or another intended test ID) so test
notifications show realistic data; locate the map literal in function
getTestVariableMap and change the value associated with the "channel_ext_id" key
only.
- Around line 637-661: The test data in getTestVariableMap contains a typo in
the "channel_ext_id" value ("1234456789" with a doubled 4); update the value to
a realistic dummy external ID (e.g., "123456789") in the map returned by
getTestVariableMap so the test data looks consistent—edit the "channel_ext_id"
entry within the getTestVariableMap function accordingly.
- Around line 107-134: UpdateNotification currently skips setting
AppriseType/AppriseFormat when the incoming values are empty, leaving the DB
value unchanged; change the logic so that when the notification is of apprise
type you always set these enum fields on the builder (use
builder.SetAppriseType(entNotification.AppriseType(n.AppriseType)) and
builder.SetAppriseFormat(entNotification.AppriseFormat(n.AppriseFormat))
unconditionally for apprise notifications) so Ent will validate or allow
clearing via its API; adjust the condition around the builder in
UpdateNotification to branch on the notification Type (e.g., n.Type ==
entNotification.TypeApprise) and call the Set* methods even if n.AppriseType or
n.AppriseFormat are empty.
- Around line 107-134: The UpdateNotification method currently skips setting
AppriseType/AppriseFormat when n.AppriseType or n.AppriseFormat is empty,
leaving the DB value unchanged; change UpdateNotification to always call
builder.SetAppriseType(entNotification.AppriseType(n.AppriseType)) and
builder.SetAppriseFormat(entNotification.AppriseFormat(n.AppriseFormat)) (remove
the conditional if-checks) so the update applies the provided value (including
empty/zero) and lets Ent validate/clear the enum fields; update the builder
usage in the UpdateNotification function to always set these enum fields.
- Around line 279-365: Wrap calls to SendLiveArchiveSuccess,
SendVideoArchiveSuccess, and SendError in anonymous goroutines with a defer
recovery block (same pattern used for SendLive): launch notifSvc.Send... inside
go func() { defer func(){ if r:=recover(); r!=nil {
log.Error().Interface("panic", r).Msg("panic in notification") } }();
notifSvc.SendLiveArchiveSuccess(ctx, ... ) }() (and similarly for
SendVideoArchiveSuccess and SendError), optionally using a detached context if
the caller's context should not be tied to the goroutine; ensure each call uses
its own goroutine and panic recovery to avoid blocking the task pipeline.
- Around line 279-365: The HTTP handler currently calls SendTestNotification
synchronously which can block the request; change the handler to invoke
SendTestNotification in a new goroutine (e.g. go func() { defer func(){ if r :=
recover(); r != nil { log.Error().Msgf("panic in SendTestNotification: %v", r) }
}(); s.SendTestNotification(context.Background(), <args>) }()) so delivery runs
asynchronously and any panic inside SendTestNotification is recovered and
logged; ensure you pass a safe context (context.Background or a derived context)
rather than the request's context if you want retries to continue after the
request finishes.
- Around line 493-544: The appriseRequestBody currently defines URLs as a string
which breaks the Apprise OpenAPI contract and prevents sending multiple
endpoints; update appriseRequestBody.URLs to be []string (json:"urls,omitempty")
and in sendAppriseWithTitle build that slice by taking n.AppriseUrls (or
renderTemplate(n.AppriseUrls, variableMap) when variableMap != nil), splitting
on commas, trimming whitespace for each entry, and only appending non-empty
values; leave the rest of the payload logic intact and keep calling
s.postJSONWithRetry(ctx, n.URL, jsonBody, "apprise") after json.Marshal so the
request body now contains a JSON array of urls.
---
Nitpick comments:
In `@internal/notification/notification.go`:
- Around line 230-235: The current loop over "for url, g := range groups" sets
the migrated config name from g.name which is non-deterministic because map
iteration order varies; change the naming logic in that loop (and any related
addTrigger handling) to derive the name deterministically from all active
triggers in the group: collect the set of trigger types/labels present on g
(e.g., from g.triggers or the slice/map field that stores triggers), sort those
labels lexicographically, then build the descriptive name (e.g., "Migrated:
"+strings.Join(sortedLabels, ", ") when multiple triggers exist, or use a single
label when only one) so the resulting name is stable across runs. Ensure you
reference and use the group structure (g) and its trigger collection rather than
g.name alone.
- Around line 230-235: The current loop over for url, g := range groups uses
g.name (populated by addTrigger) which depends on unordered map iteration,
causing inconsistent "Migrated: ..." labels; instead, after collecting all
triggers for the group, build the migration name from the full set of active
trigger types (e.g., collect g.triggers or g.types, deduplicate and sort them,
then join into a descriptive string like "Migrated: error, live, video") and use
that composed name instead of g.name so the label consistently reflects all
triggers for that URL (update the code around the for url, g := range groups
loop and references to g.name/addTrigger accordingly).
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
2 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.
Summary
Resolves #1012
Replaces the old config.json-based single notification setup with a database-backed system supporting multiple named notification configurations with webhook and Apprise providers.
Existing config.json notification settings are automatically migrated to the database on first startup. Triggers sharing the same webhook URL are grouped into a single notification config.
Changes
Backend
Notificationentity (name, enabled, type, URL, 4 triggers, 4 templates, 5 Apprise fields, timestamps) with MaxLen constraints{{variable}}template rendering, webhook and Apprise providersFrontend
/admin/notificationswith DataTable, create/edit drawer, delete confirmation, and test modal