Conversation
|
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:
WalkthroughIntroduce a channel-agnostic NotificationMessagePayload, persist JSON BodyPayload alongside Body in notification history, refactor sending flows (Slack, Shoutrrr, Webhook, SMTP) to use payloads and templating overrides, remove legacy templates, add SMTP E2E test infra, and expose history detail API that renders markdown. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Event Source
participant Events as notification/events
participant Builder as notification/message.go
participant DB as NotificationSendHistory
participant Sender as Sender (Slack/Shoutrrr/Webhook/SMTP)
participant Renderer as FormatNotificationMessage
Event->>Events: Trigger notification event (NotificationEventPayload)
Events->>Builder: buildNotificationHistoryPayload(payload, celEnv)
Builder-->>Events: NotificationMessagePayload (BodyPayload)
Events->>DB: Insert history record with Body and BodyPayload
Events->>Sender: PrepareAndSendEventNotification(payload / raw path)
Sender->>Renderer: FormatNotificationMessage(payload, format)
Renderer-->>Sender: Formatted channel message
Sender->>Sender: Dispatch via channel (Slack/SMTP/Webhook/etc)
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 |
adityathebe
force-pushed
the
feat/clicky-notifications
branch
4 times, most recently
from
6989bf8 to
14ccef0
Compare
|
improve the buttons on email html |
adityathebe
force-pushed
the
feat/clicky-notifications
branch
from
14ccef0 to
2759c71
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
notification/send.go (1)
87-121: Property templates are not rendered (breaks title/subject).Properties with template expressions like
{{.incident.title}}are passed through unchanged. The test setup shows properties should be templated (see line 94 ofnotification_test.go), but the current implementation inPrepareAndSendEventNotificationsendscn.Propertieswithout rendering them throughcelEnv. This breaks webhook titles and email subjects.Add property templating before passing to
sendEventNotificationWithMetrics. SincecelEnvis available in the function signature, render property values using it—either inline or via a helper function—so template expressions resolve correctly.
🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 53: The go.mod entry for module github.com/flanksource/clicky uses a
non-existent semantic version (v1.14.0); update the dependency in go.mod by
replacing that version with the available pseudo-version
(v0.0.0-20250820111116-a1cda798d4da) or, if your org has a proper tag, confirm
and use the correct semantic tag instead; ensure you run `go mod tidy`
afterwards to validate and regenerate the go.sum entries for the
github.com/flanksource/clicky requirement.
In `@notification/message.go`:
- Around line 74-90: The SlackTitle in the EventCheckPassed handler is using
env.Canary.Name while the Title uses env.Check.Name, causing inconsistent
resource names; update the SlackTitle expression in the case
icapi.EventCheckPassed to use safeName(lo.FromPtr(env.Check).Name) (preserving
the ":large_green_circle: *%s* is _healthy_" format) so both Title and
SlackTitle reference the same env.Check.Name, matching the pattern used in
EventCheckFailed.
♻️ Duplicate comments (1)
notification/notification_test.go (1)
154-157: CI failure: title template still literal in webhook payload.This assertion currently fails because the
titleproperty isn’t being rendered (literal{{...}}reaches the webhook). This is the same root cause flagged in the send-path comment.
🧹 Nitpick comments (2)
notification/controllers.go (1)
100-114: Add a comment explaining the backward compatibility fallback for deprecatedBodyfield.Static analysis correctly flags lines 112-113 as using the deprecated
detail.Bodyfield. The fallback is intentional for backward compatibility with existing records that don't haveBodyPayloadpopulated yet, but this intent should be documented.📝 Suggested documentation
if len(detail.BodyPayload) > 0 { var payload NotificationMessagePayload if err := json.Unmarshal(detail.BodyPayload, &payload); err != nil { return api.WriteError(c, ctx.Oops().Wrapf(err, "failed to parse body payload")) } bodyMarkdown, err := FormatNotificationMessage(payload, "markdown") if err != nil { return api.WriteError(c, ctx.Oops().Wrapf(err, "failed to render body payload")) } detail.BodyMarkdown = bodyMarkdown - } else if detail.Body != nil { + } else if detail.Body != nil { //nolint:staticcheck // Backward compatibility for records created before BodyPayload migration detail.BodyMarkdown = *detail.Body }notification/events.go (1)
316-335: Useduty.Now()forNotBeforetimestamps.
NotBeforeis persisted; usingduty.Now()keeps DB timestamps consistent with the rest of the codebase.🔧 Suggested change
- NotBefore: lo.ToPtr(time.Now().Add(*n.WaitFor)), + NotBefore: lo.ToPtr(duty.Now().Add(*n.WaitFor)),As per coding guidelines, use duty.Now() for DB timestamps.
adityathebe
force-pushed
the
feat/clicky-notifications
branch
from
2759c71 to
1092a99
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
playbook/runner/runner.go (1)
230-253: Persist BodyPayload even when no AI headline is set.
BodyPayloadcan be set withoutResourceHealthDescription, but the DB update only runs when the headline is non-empty, so the payload is silently dropped.🔧 Proposed fix
- if len(sendHistoryUpdate.ResourceHealthDescription) > 0 { + if len(sendHistoryUpdate.ResourceHealthDescription) > 0 || len(sendHistoryUpdate.BodyPayload) > 0 { if err := ctx.DB().Updates(sendHistoryUpdate).Error; err != nil { return ctx.Oops().Wrap(err) } }
adityathebe
force-pushed
the
feat/clicky-notifications
branch
from
1092a99 to
8509fb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@notification/notification_test.go`:
- Around line 154-157: The webhook title is not being rendered because template
properties are not processed with a CEL environment before the payload is
assembled; ensure renderTemplateProperties is called with a non-nil celEnv for
team/custom notification properties prior to building/sending the webhook
payload (the code path that constructs webhookPostdata), e.g., invoke
renderTemplateProperties(celEnv, ...) for the notification's property map in the
webhook preparation function (where webhookPostdata is created) so the
"{{.incident.title}}" template is evaluated and the title/message fields are
rendered before the HTTP send.
♻️ Duplicate comments (1)
notification/message.go (1)
74-90: Inconsistent naming between Title and SlackTitle for check passed event.On line 75, the
Titleusesenv.Check.Name, but on line 76, theSlackTitleusesenv.Canary.Name. This inconsistency could confuse users when the same notification shows different resource names across channels. Compare withEventCheckFailed(lines 58-59) which consistently usesenv.Check.Namefor both.🔧 Suggested fix
case icapi.EventCheckPassed: msg.Title = fmt.Sprintf("Check %s has passed", safeName(lo.FromPtr(env.Check).Name)) - msg.SlackTitle = fmt.Sprintf(":large_green_circle: *%s* is _healthy_", safeName(lo.FromPtr(env.Canary).Name)) + msg.SlackTitle = fmt.Sprintf(":large_green_circle: *%s* is _healthy_", safeName(lo.FromPtr(env.Check).Name))
🧹 Nitpick comments (1)
notification/controllers.go (1)
89-98: Consider defensive handling for empty SourceEvent.The
strings.Split(detail.SourceEvent, ".")[0]call assumesSourceEventis non-empty. While this should always be true for valid records, an empty string would return an emptyResourceKind. This is likely acceptable but worth noting if data integrity issues occur.
adityathebe
force-pushed
the
feat/clicky-notifications
branch
from
ee58edc to
70570ec
Compare
adityathebe
force-pushed
the
feat/clicky-notifications
branch
from
113fbce to
d1c94d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@notification/message.go`:
- Around line 150-157: Update the responder notification descriptions to show
the responder's name instead of their ID: in the switch cases handling
icapi.EventIncidentResponderAdded and icapi.EventIncidentResponderRemoved (where
msg.Description is set), use lo.FromPtr(env.Responder).Name as the primary value
and fall back to lo.FromPtr(env.Responder).ID if Name is empty or nil so the
text remains user-friendly and robust.
In `@notification/notification_test.go`:
- Around line 58-64: The shared map webhookPostdata must be cleared before
running assertions to avoid stale entries; inside the test setup (e.g., in the
ginkgo.BeforeAll/BeforeEach block where john is created) reset webhookPostdata
by reinitializing or clearing it so that length checks only reflect new
webhooks—refer to the webhookPostdata variable in notification_test.go and
ensure it is emptied before triggering or asserting on any webhook arrival.
In `@notification/template_legacy.go`:
- Around line 28-51: The switch cases currently call templates.ReadFile and
ignore its error (e.g., content, _ := templates.ReadFile(...)) which leads to
silent empty bodies; update each case handling (for events like
api.EventCheckPassed, api.EventCheckFailed, api.EventConfigHealthy,
api.EventConfigCreated, api.EventComponentHealthy, etc.) to check the returned
error from templates.ReadFile and surface it (either return the error, log and
return, or set a clear fallback message) before assigning body = string(content)
so missing/renamed templates fail fast instead of producing empty notifications.
🧹 Nitpick comments (6)
notification/notification_test.go (2)
898-938: Use duty.Now() for DB timestamps in fixturesCreatedAt uses time.Now(); prefer duty.Now() for database timestamps to keep a consistent time source across the repo. As per coding guidelines, Use
duty.Now()instead oftime.Now()for database timestamps and soft deletes.♻️ Suggested update
import ( "encoding/json" "fmt" "time" // register event handlers "github.com/flanksource/commons/collections" + "github.com/flanksource/duty" "github.com/flanksource/duty/models" "github.com/flanksource/duty/query" "github.com/flanksource/duty/types" @@ - CreatedAt: lo.ToPtr(time.Now().Add(-time.Minute * 3)), + CreatedAt: lo.ToPtr(duty.Now().Add(-time.Minute * 3)), @@ - CreatedAt: lo.ToPtr(time.Now().Add(-time.Minute * 2)), + CreatedAt: lo.ToPtr(duty.Now().Add(-time.Minute * 2)), @@ - CreatedAt: lo.ToPtr(time.Now().Add(-time.Minute)), + CreatedAt: lo.ToPtr(duty.Now().Add(-time.Minute)), @@ - CreatedAt: lo.ToPtr(time.Now().Add(-time.Second * 30)), + CreatedAt: lo.ToPtr(duty.Now().Add(-time.Second * 30)), @@ - CreatedAt: time.Now().Add(-time.Second * 10), + CreatedAt: duty.Now().Add(-time.Second * 10), @@ - CreatedAt: time.Now().Add(-time.Second * 10), + CreatedAt: duty.Now().Add(-time.Second * 10),Also applies to: 1236-1268
115-120: Clean up team/person/component/incident fixturesAfterAll only deletes the notification; leaving the other rows can bleed into later tests. Consider deleting the fixtures created in BeforeAll/It.
🧹 Suggested cleanup
ginkgo.AfterAll(func() { + if incident != nil { + Expect(DefaultContext.DB().Delete(incident).Error).To(BeNil()) + } + if component != nil { + Expect(DefaultContext.DB().Delete(component).Error).To(BeNil()) + } + if team != nil { + Expect(DefaultContext.DB().Delete(team).Error).To(BeNil()) + } + if john != nil { + Expect(DefaultContext.DB().Delete(john).Error).To(BeNil()) + } err := DefaultContext.DB().Delete(¬if).Error Expect(err).To(BeNil()) notification.PurgeCache(notif.ID.String()) })notification/events.go (1)
317-337: Use duty.Now() for NotBefore timestampsNotBefore is persisted to the DB; use duty.Now() to align with the repo’s time source. As per coding guidelines, Use
duty.Now()instead oftime.Now()for database timestamps and soft deletes.♻️ Suggested change
- NotBefore: lo.ToPtr(time.Now().Add(*n.WaitFor)), + NotBefore: lo.ToPtr(duty.Now().Add(*n.WaitFor)),notification/controllers.go (1)
89-98: Consider logging errors from resource resolution instead of silently ignoring them.Errors from
GetResourceAsMapFromEventandjson.Marshalare silently swallowed. While it's acceptable to proceed without resource data, logging these errors would help with debugging issues in production.Proposed fix
resourceKind := strings.Split(detail.SourceEvent, ".")[0] detail.ResourceKind = resourceKind - if resourceMap, err := GetResourceAsMapFromEvent(ctx, detail.SourceEvent, detail.ResourceID.String()); err == nil && resourceMap != nil { - if b, err := json.Marshal(resourceMap); err == nil { + if resourceMap, err := GetResourceAsMapFromEvent(ctx, detail.SourceEvent, detail.ResourceID.String()); err != nil { + ctx.Logger.Warnf("failed to get resource from event: %v", err) + } else if resourceMap != nil { + if b, err := json.Marshal(resourceMap); err != nil { + ctx.Logger.Warnf("failed to marshal resource map: %v", err) + } else { detail.Resource = types.JSON(b) } if resourceType, ok := resourceMap["type"].(string); ok && resourceType != "" { detail.ResourceType = &resourceType } }notification/shoutrrr.go (1)
239-321: Consider extracting duplicated SMTP handling logic.The SMTP handling logic in
shoutrrrSend(lines 254-311) is nearly identical toshoutrrrSendRaw(lines 115-172). ThefirstNonEmptyhelper function is also duplicated inline. Consider extracting the shared SMTP sending logic into a separate helper function to reduce code duplication and maintenance burden.Proposed extraction
// sendSMTPEmail extracts common SMTP sending logic func sendSMTPEmail(ctx *Context, shoutrrrURL string, params *types.Params, title, message string) error { parsedURL, err := url.Parse(shoutrrrURL) if err != nil { return fmt.Errorf("failed to parse shoutrrr URL: %w", err) } firstNonEmpty := func(params *types.Params, q url.Values, keys ...string) string { for _, k := range keys { for p := range *params { if strings.EqualFold(k, p) { return (*params)[p] } } if v := q.Get(k); v != "" { return v } } return "" } query := parsedURL.Query() // ... rest of common SMTP logic }notification/send.go (1)
329-334: Consider extracting the repeated resourceID extraction pattern.The same pattern for extracting
resourceIDfromctx.logis repeated four times acrossSendRawNotificationandSendNotification. This could be extracted into a small helper method on*Context.Proposed helper
// On Context struct or as a helper: func (c *Context) resourceIDString() string { if c.log != nil && c.log.ResourceID != uuid.Nil { return c.log.ResourceID.String() } return "" }Also applies to: 350-354, 391-395, 407-410
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@notification/events.go`:
- Around line 388-414: The re-marshal of Slack blocks in getNotificationMsg
silently ignores json.Marshal errors (when marshaling []any{slackMsg}), leaving
data.Message unchanged; update the block after unmarshaling slackMsg to check
the marshal error and return a wrapped error (e.g., fmt.Errorf("failed to
marshal slack blocks: %w", err)) instead of ignoring it so callers receive the
failure and can handle it appropriately.
🧹 Nitpick comments (4)
notification/events.go (1)
356-386: Consider returning early whenpayload.GroupIDis nil to avoid unnecessary work.The function correctly handles two modes (template vs. payload-based), but when there's no template and no
GroupID, you could skip the grouped resources lookup entirely. Currently, this is handled by the nil check onpayload.GroupID, which is fine.One minor observation: the error message on line 373 references
payload.NotificationIDbut the function receivesnotificationwhich also has an ID. Both should be the same, but usingnotification.IDwould be more consistent with the pattern used elsewhere.♻️ Optional: Use consistent ID reference in error message
if payload.GroupID != nil { groupedResources, err := db.GetGroupedResources(ctx, *payload.GroupID, payload.ID.String()) if err != nil { - return nil, nil, fmt.Errorf("failed to get grouped resources for notification[%s]: %w", payload.NotificationID, err) + return nil, nil, fmt.Errorf("failed to get grouped resources for notification[%s]: %w", notification.ID, err) } env.GroupedResources = groupedResources }notification/send.go (3)
107-114: Consider propagating the marshal error instead of just logging.Currently, if
json.Marshalfails, the function logs a warning and returns without storing the payload. Depending on the criticality of storing the payload, you may want to return an error or at least ensure callers are aware the payload wasn't stored.♻️ Optional: Return error from storeNotificationPayload
-func storeNotificationPayload(ctx *Context, payload NotificationMessagePayload) { +func storeNotificationPayload(ctx *Context, payload NotificationMessagePayload) error { b, err := json.Marshal(payload) if err != nil { - ctx.Logger.Warnf("failed to marshal notification payload: %v", err) - return + return fmt.Errorf("failed to marshal notification payload: %w", err) } ctx.WithBodyPayload(types.JSON(b)) + return nil }Then handle the error at call sites if payload storage is critical.
116-174: Significant code duplication betweenPrepareAndSendEventNotificationandprepareAndSendRawNotification.Both functions share nearly identical recipient-handling logic (PersonID, TeamID, CustomService branches). This duplication increases maintenance burden and risk of divergence.
Consider extracting the common recipient resolution logic into a helper, or using a strategy pattern where the only difference is which send function is called.
♻️ Suggested approach to reduce duplication
One option is to create a helper that handles recipient resolution and accepts a send callback:
type sendFunc func(ctx *Context, celEnv *celVariables, payload NotificationEventPayload, notification *NotificationWithSpec, connection string, url string, properties map[string]string) error func dispatchToRecipients(ctx *Context, payload NotificationEventPayload, celEnv *celVariables, notification *NotificationWithSpec, sender sendFunc) error { if payload.PersonID != nil { ctx.WithRecipient(RecipientTypePerson, payload.PersonID) // ... email lookup ... return sender(ctx, celEnv, payload, notification, "", smtpURL, nil) } // ... similar for TeamID, CustomService ... }This would allow
PrepareAndSendEventNotificationandprepareAndSendRawNotificationto share the recipient logic while differing only in which actual send function is used.Also applies to: 176-220
353-358: Repeated pattern for extracting resourceID from ctx.log.The same nil-check and extraction pattern for
resourceIDappears four times in this file. Consider extracting this to a helper method on*Context.♻️ Optional: Extract resourceID helper
// Add to Context struct or as a method func (c *Context) ResourceIDString() string { if c.log != nil && c.log.ResourceID != uuid.Nil { return c.log.ResourceID.String() } return "" }Then replace the repeated blocks:
- resourceID := "" - if ctx.log != nil && ctx.log.ResourceID != uuid.Nil { - resourceID = ctx.log.ResourceID.String() - } - traceLog("NotificationID=%s Resource=[%s] Sent via slack ...", ctx.notificationID, resourceID) + traceLog("NotificationID=%s Resource=[%s] Sent via slack ...", ctx.notificationID, ctx.ResourceIDString())Also applies to: 374-378, 415-419, 431-435
adityathebe
force-pushed
the
feat/clicky-notifications
branch
from
7e31f79 to
5e8702f
Compare
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)
notification/events.go (1)
317-337:⚠️ Potential issue | 🟡 MinorUse
duty.Now()instead oftime.Now()for theNotBeforetimestamp.
NotBeforeis persisted to the database in line 337; useduty.Now()to maintain consistency with system-wide timestamp handling.Suggested fix
- NotBefore: lo.ToPtr(time.Now().Add(*n.WaitFor)), + NotBefore: lo.ToPtr(duty.Now().Add(*n.WaitFor)),
🤖 Fix all issues with AI agents
In `@notification/send.go`:
- Around line 26-55: DefaultTitleAndBody currently ignores errors from
FormatNotificationMessage and can return an empty body; modify
DefaultTitleAndBody to check the error returned by FormatNotificationMessage and
provide a sensible fallback (e.g., use msgPayload.Message or a concatenation of
msgPayload.Title and msgPayload.Message and/or render groupedResourcesMessage
using msgPayload.GroupedResources) when FormatNotificationMessage returns an
error; keep using BuildNotificationMessagePayload to obtain msgPayload and
respect the slack/non-slack bodyFormat logic (celEnv.Channel) but ensure the
function returns a non-empty body on error and log or surface the formatting
error as appropriate.
In `@notification/suite_test.go`:
- Around line 129-186: In captureSession.Data, don't store the s.to slice
directly into capturedMessage.To because s.to is mutable across sessions;
instead make a new slice copy (e.g., make([]string, len(s.to)) and copy(s.to,
newSlice)) and store that copy in capturedMessage.To before appending to
captureBackend.messages; keep the mutex locking around the append to maintain
thread-safety when writing to captureBackend.messages in the Data method.
In `@notification/webhook.go`:
- Around line 50-52: The grouped-resources footer (groupedResourcesMessage) is
appended after templating so any {{- range .groupedResources }} blocks are left
unrendered; modify the flow so you append groupedResourcesMessage to
data.Message before invoking the templater (or re-run the templater on the
combined message). Specifically, ensure celVars.GroupedResources is available,
then combine data.Message += groupedResourcesMessage prior to calling the
templating function (e.g., Render/Template/templater.RenderTemplate) or call
that templating function again on data.Message after appending so the {{- range
.groupedResources }} block is rendered.
🧹 Nitpick comments (3)
.golangci.yml (1)
22-24: Consider adding a comment explaining why this deprecation is suppressed.Suppressing SA1019 (deprecated API usage) for
WithCredentialsJSONis valid when migration isn't feasible yet, but a brief comment or TODO with a tracking issue would help future maintainers understand the rationale and timeline for addressing the deprecation.+ # TODO: WithCredentialsJSON is deprecated; tracked in <issue-link> - linters: - staticcheck text: 'SA1019:.*WithCredentialsJSON'notification/message.go (1)
379-396: Ensure the first rendered action becomes primary.If the first action is filtered out, the next valid button won’t be marked primary because the index still increments. Consider tracking the count of rendered buttons instead.
♻️ Possible tweak
func actionsToButtonGroup(actions []NotificationAction) api.ButtonGroup { buttons := make([]api.Button, 0, len(actions)) + added := 0 for i, action := range actions { if action.URL == "" || action.Label == "" { continue } variant := action.Style - if variant == "" && i == 0 { + if variant == "" && added == 0 { variant = "primary" } buttons = append(buttons, api.Button{ Label: action.Label, Href: action.URL, Variant: variant, }) + added++ } return api.ButtonGroup{Buttons: buttons} }notification/controllers.go (1)
81-87: Prefer errors.Is for ErrRecordNotFound checks.Gorm may wrap errors; using errors.Is is more resilient and idiomatic.
🔧 Suggested adjustment
import ( "encoding/json" + "errors" "net/http" "strings" @@ - if err := ctx.DB().Model(&models.NotificationSendHistory{}).Where("id = ?", id).First(&detail.NotificationSendHistory).Error; err != nil { - if err == gorm.ErrRecordNotFound { + if err := ctx.DB().Model(&models.NotificationSendHistory{}).Where("id = ?", id).First(&detail.NotificationSendHistory).Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { return api.WriteError(c, api.Errorf(api.ENOTFOUND, "notification history %s not found", id)) }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@notification/message.go`:
- Around line 301-307: FormatNotificationMessage is being called with an
unsupported format string "email" which breaks SMTP notifications; find where
the SMTP/shoutrrr send path passes "email" (the callsite that uses
FormatNotificationMessage and sets UseHTML="true" in the shoutrrr/SMTP send
code) and change that format string to "html" so it matches clicky's supported
formats and the existing error message ("failed to format html message") and
UseHTML flag; verify RenderBodyMarkdown remains using "markdown" and that all
calls to FormatNotificationMessage only use supported formats like "html",
"markdown", "json", etc.
🧹 Nitpick comments (4)
notification/shoutrrr.go (1)
112-175:m.Send(conn)error on Line 164 is returned unwrapped, inconsistent with other error paths.Other error paths in this function (lines 142, 158, 170) wrap errors via
ctx.Oops(), but the SMTPSenderror is returned bare. This means SMTP send failures won't carry the same structured error context (codes, hints) as shoutrrr failures.♻️ Suggested fix
- return m.Send(conn) + if err := m.Send(conn); err != nil { + return ctx.Oops().Wrapf(err, "error sending email via SMTP") + } + return nilnotification/send.go (2)
571-593: Template override forDescription(lines 586–592) is unreachable in the current call flow.In
PrepareAndSendEventNotification(line 171), a non-emptynotification.Templateroutes to the raw path, soapplyTemplateOverridesis only called whenTemplateis empty/whitespace. Consequently, thenotification.Templateoverride block here can never fire. This isn't a bug, but it's dead code that could mislead future readers.♻️ Consider removing the dead branch or adding a comment
func applyTemplateOverrides(ctx *Context, msgPayload *NotificationMessagePayload, notification *NotificationWithSpec, celEnv *celVariables) { if msgPayload == nil || notification == nil || celEnv == nil { return } celEnvMap := celEnv.AsMap(ctx.Context) if strings.TrimSpace(notification.Title) != "" { if rendered, err := renderTemplateString(ctx, celEnvMap, notification.Title); err != nil { ctx.Logger.Warnf("failed to render notification title template %q: %v", notification.Title, err) } else { msgPayload.Title = rendered } } - - if strings.TrimSpace(notification.Template) != "" { - if rendered, err := renderTemplateString(ctx, celEnvMap, notification.Template); err != nil { - ctx.Logger.Warnf("failed to render notification template %q: %v", notification.Template, err) - } else { - msgPayload.Description = rendered - } - } }
295-349: RepeatedresourceIDextraction pattern across both send paths.The pattern of extracting
resourceIDfromctx.logappears four times (lines 322–326, 342–346, 383–386, 399–402). A small helper would reduce duplication.♻️ Optional: extract helper
+func (ctx *Context) resourceIDStr() string { + if ctx.log != nil && ctx.log.ResourceID != uuid.Nil { + return ctx.log.ResourceID.String() + } + return "" +}Then replace each occurrence:
- resourceID := "" - if ctx.log != nil && ctx.log.ResourceID != uuid.Nil { - resourceID = ctx.log.ResourceID.String() - } - traceLog("NotificationID=%s Resource=[%s] Sent via slack ...", ctx.notificationID, resourceID) + traceLog("NotificationID=%s Resource=[%s] Sent via slack ...", ctx.notificationID, ctx.resourceIDStr())Also applies to: 351-406
notification/message.go (1)
43-169: Comprehensive event-to-payload mapping with good nil-safety.The
BuildNotificationMessagePayloadfunction covers all event types and consistently useslo.FromPtr,safeName,stringPtr, andcoalesceStringhelpers for nil-safe access. ThecompactKeyValuescall at the end (lines 166-167) cleans up any empty entries.Minor observation: the
defaultcase (line 162-163) only setsmsg.Title = payload.EventName, producing a payload with no description, attributes, or actions. If a new event type is added without updating this switch, notifications will be quite sparse. Consider logging a warning for unrecognized events.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@notification/send.go`:
- Around line 238-239: The doc comment above sendRawEventNotificationWithMetrics
incorrectly names the function "SendRawEventNotification"; update the comment to
reference the actual function name sendRawEventNotificationWithMetrics (and
adjust capitalization to match exported/unexported status if needed) so the
comment correctly documents sendRawEventNotificationWithMetrics and its purpose
(wrapper around sendRawEventNotification for error handling & metrics).
- Around line 273-274: The top-of-function doc comment currently references the
wrong identifier ("SendEventNotification"); update the comment to reference the
actual function name sendEventNotificationWithMetrics (or reword to a neutral
description) so the doc comment matches the function declaration for
sendEventNotificationWithMetrics; ensure the first word of the comment matches
the exported/unexported naming convention and refers to
sendEventNotificationWithMetrics to keep godoc/lint checks happy.
- Line 619: The warning currently logs both the property key and its value via
ctx.Logger.Warnf, which can leak secrets; change the log to omit or redact the
value and include only the key and the error (e.g., ctx.Logger.Warnf("failed to
render template property %s: %v", key, err)) or otherwise mask the value before
logging; update the call sites where properties/value are passed (the
ctx.Logger.Warnf invocation shown) so only the property name and error are
logged.
🧹 Nitpick comments (2)
notification/send.go (2)
328-332: RepeatedresourceIDextraction pattern across four locations.The same pattern for extracting
resourceIDfromctx.logappears at Lines 328–331, 348–351, 389–392, and 405–408. Consider extracting a small helper to reduce duplication.Example helper
func (ctx *Context) resourceIDString() string { if ctx.log != nil && ctx.log.ResourceID != uuid.Nil { return ctx.log.ResourceID.String() } return "" }Also applies to: 348-352, 389-393, 405-409
577-599: Thenotification.Templatebranch (Line 592–598) is currently unreachable.
applyTemplateOverridesis only called fromPrepareAndSendEventNotification(Line 182), which is gated bynotification.Templatebeing empty (Line 177). So the body-override branch on Line 592 will never execute. If this is intentional defensive code for future callers, a brief comment would help; otherwise it can be removed.
moshloop
force-pushed
the
feat/clicky-notifications
branch
from
8afab8f to
1da7566
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@notification/message.go`:
- Around line 301-307: The FormatNotificationMessage function uses an
unsupported "email" format for SMTP notifications causing clicky.Format to fail;
update FormatNotificationMessage so any branch that currently treats SMTP/email
as "email" instead maps it to "html" (e.g., normalize format="email" to
format="html" before calling clicky.Format or change the conditional that builds
clicky.FormatOptions to use "html" when UseHTML/SMTP is intended), referencing
the FormatNotificationMessage function and the
payload.ToTextList()/payload.ToSlackTextList() call sites to ensure
clicky.Format receives FormatOptions{Format: "html"} for SMTP/email cases.
In `@notification/send.go`:
- Around line 577-599: The Template rendering branch in applyTemplateOverrides
is dead because callers of applyTemplateOverrides already ensure
strings.TrimSpace(notification.Template) != ""; remove the unreachable block
that checks notification.Template, calls renderTemplateString, and sets
msgPayload.Description (or alternatively keep it but add a clear comment above
the if explaining it is intentionally preserved for future call sites and
referencing that current callers in send.go and events.go already guard
Template) so the function isn’t left with misleading dead code; update
applyTemplateOverrides accordingly (modify the if block that references
notification.Template and msgPayload.Description).
In `@playbook/actions/notification.go`:
- Around line 37-44: SendNotification already delivered the Slack notification
before calling notification.FormatNotificationMessage, so a formatting failure
should not be propagated as an error; instead catch the error from
FormatNotificationMessage in the if service == "slack" block, log the formatting
error using the existing logger, set output.Slack to a degraded/empty value (and
ensure output.Message isn't overwritten into a misleading empty string), and
return the successful output object (or a result with a non-fatal warning flag)
rather than returning the error from FormatNotificationMessage; update the code
around SendNotification and FormatNotificationMessage to implement this
non-fatal handling.
🧹 Nitpick comments (3)
notification/shoutrrr.go (2)
112-175: Centralized SMTP dispatch viaThe extraction of SMTP parameters and the
mail.New(...).Send(conn)flow is well-structured. Two minor observations:
- Line 132:
firstNonEmpty(params, query, "to", "ToAddresses", "ToAddress")— iftoresolves to an empty string,mail.Newwill receive an empty recipient list fromstrings.Split("", ",")producing[""], which may cause a confusing send error downstream rather than an explicit validation error.- Line 164:
m.Send(conn)error is returned raw while other errors in this function usectx.Oops().Wrapf(...). Consider wrapping for consistency.
189-241: Significant duplication withPrepareShoutrrrRaw(lines 49–95).Both
PrepareShoutrrrandPrepareShoutrrrRawshare the same pattern: system SMTP credential resolution →shoutrrr.CreateSender→ExtractServiceName→ service-based formatting switch. The main difference is thatRawapplies CEL templating on aNotificationTemplatewhile the new version formats aNotificationMessagePayloadviaFormatNotificationMessage.Consider extracting the shared setup (credential resolution, sender creation, service extraction) into a private helper to reduce duplication. This is not urgent but would simplify future maintenance.
notification/types.go (1)
23-38: Silent error swallowing in rendering fallback.
RenderBodyMarkdownsilently discards errors from bothjson.UnmarshalandFormatNotificationMessage. The fallback-to-Bodybehavior is appropriate for a rendering function, but consider adding a trace/debug log on the error paths to aid troubleshooting when payload rendering silently degrades.
adityathebe
force-pushed
the
feat/clicky-notifications
branch
from
1da7566 to
0b2848b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@notification/controllers.go`:
- Around line 74-103: Import the standard library context as an alias (e.g.,
gocontext) and update usages accordingly: add the import gocontext "context",
keep github.com/flanksource/duty/context, then in
GetNotificationSendHistoryDetail replace the type assertion
c.Request().Context().(context.Context) with
c.Request().Context().(gocontext.Context) (and any other occurrences of the
stdlib Context type in this file) so the two context packages are unambiguous.
In `@notification/send.go`:
- Around line 371-393: The Slack branch in SendNotification fails to persist the
sent message because it never calls ctx.WithMessage; update the Slack path
(after formatting the slackMsg and building NotificationTemplate and before
calling SlackSend) to invoke ctx = ctx.WithMessage(data.Message) (or equivalent
usage of ctx.WithMessage) so the message is recorded in history, mirroring the
behavior used in SendRawNotification and the shoutrrrSend fix; ensure you
reference SendNotification, FormatNotificationMessage, NotificationTemplate,
ctx.WithMessage, and SlackSend when making the change.
In `@notification/shoutrrr.go`:
- Around line 243-251: shoutrrrSend never records the message in history because
it doesn't call ctx.WithMessage like shoutrrrSendRaw does; after PrepareShoutrrr
returns (in shoutrrrSend) call ctx.WithMessage(data.Message) (using the returned
data variable) before invoking dispatchNotification so the sent message body is
persisted to notification history.
🧹 Nitpick comments (2)
notification/message.go (1)
441-455: Minor inconsistency in Slack grouped resources formatting.For the default case, when
GroupedResourcesTitleis set (line 451), the format is"*%s:* - %s"with"\n - "join. But the fallback (line 453) uses the same pattern with "Also Failing". This produces*Also Failing:* - item1\n - item2— the leading space before the dash on the first item creates a slightly different visual than the subsequent items (- item1vs\n - item2). This is cosmetic, but the first item has a preceding space while subsequent items start on a new line with-.notification/shoutrrr.go (1)
97-110:firstNonEmptydoes case-insensitive lookup on params but case-sensitive on query values.
url.Values.Getis case-sensitive per Go'snet/urlpackage, while the params lookup is case-insensitive. This asymmetry is likely intentional (params keys may vary in casing from shoutrrr), but worth a brief doc note for clarity.
- Apply rendered notification.Title/Template to payload title/description when provided - Pass celEnv through send pipeline so templates have event data - Render templated delivery properties before sending - Update playbook action to pass nil celEnv to SendNotification
Remove SlackTitle and Slack emoji formatting so Slack uses Title directly.
Drop SlackActionsDivider from payload and remove its Slack-only divider logic.
Replace Fields/LabelFields with Attributes and Labels key/value lists, removing label map helpers.
…faults - Delete notification/templates/ directory with embedded template files - Delete notification/template_legacy.go - Move DefaultTitleAndBody() to send.go using clicky-generated content - Move getNotificationMsg() to events.go
Add linter exclusion for SA1019 deprecation warnings on option.WithCredentialsJSON. This is a temporary fix - the proper solution requires refactoring to use credentials files instead of inline JSON certificates. Tracked in GitHub issue.
…dpoints Add RenderBodyMarkdown helper that renders markdown from body_payload (clicky) with fallback to legacy body column. Use it in: - MCP get_notification_detail tool - GET /notification/silence_preview API - GET /notification/send_history/:id (refactored to use shared helper) This fixes backward compatibility for consumers that relied on body being non-null, which is now NULL for clicky-based notifications.
…c into dispatchNotification
- Return error when notification name not found in team spec - Return error when no recipient resolved at all - Prevents silent success when notifications fail to send
- Add validation after querying person email from DB - Return error if email is empty or whitespace-only - Prevents building invalid SMTP URL with empty ToAddresses - Uses fmt.Errorf for consistency with surrounding code
- Fall back to description when FormatNotificationMessage fails in DefaultTitleAndBody - Fix doc comments to match function names (sendRawEventNotificationWithMetrics, sendEventNotificationWithMetrics) - Remove property value from log to avoid leaking sensitive data - Don't propagate Slack formatting error after notification already sent
Grouped resources are already rendered by clicky via DefaultTitleAndBody.
The old groupedResourcesMessage append was a leftover that caused
double-rendering and unrendered Go template syntax.
DefaultTitleAndBody()
└─ BuildNotificationMessagePayload() ← sets msg.GroupedResources
└─ FormatNotificationMessage()
└─ payload.ToTextList() / buildTextList()
└─ if len(p.GroupedResources) > 0 { ← renders them into output
out = append(out, opts.renderGroupedRes(p))
}
└─ clicky.Format(...) ← returns final body with grouped resources already in it
moshloop
force-pushed
the
feat/clicky-notifications
branch
from
f79ced2 to
8d23df5
Compare
2 participants
resolves: #2721
Summary by CodeRabbit
New Features
Refactor
Tests