fix: fixes multiple bugs with alerting 🔔

[amir20]

No description provided.

[claude]

Good fix for the JSON escaping issue. A couple of observations:

• w.Template field in WebhookDispatcher is now only used as a nil sentinel — executeJSONTemplate re-parses TemplateText from scratch, making the pre-parsed w.Template field dead weight. Could simplify the check to w.TemplateText != "" and drop the Template field, or cache the parse result and use it.
• The fallback path re-parses the template even though it was already validated in NewWebhookDispatcher — minor but redundant.
• No test for executeJSONTemplate with a template that contains invalid Go template syntax (e.g., {{ .Missing.Field }}) to verify error propagation.

The core approach (unmarshal JSON → walk + resolve templates → re-marshal) is correct and handles escaping properly.

[claude]

Review

webhook.go:76: Condition checks w.Template != nil but immediately uses w.TemplateText. Since both are set together in NewWebhookDispatcher, the check works, but checking w.TemplateText != "" would be more semantically correct and eliminates the now-unused compiled template from the hot path.

executeJSONTemplate: Re-parses JSON and recompiles templates on every call. The pre-compiled w.Template is now unused for JSON templates. Minor perf overhead but acceptable for a notification dispatcher.

Non-JSON fallback still executes via raw text/template, meaning the original quote-escaping bug persists for non-JSON custom templates. Seems intentional but worth a comment clarifying this limitation.

formatLogMessage in processing.go: Clean improvement over fmt.Sprintf("%v") for structured log entries.

Test coverage is solid — quotes, newlines, backslashes, nested structures, and the non-JSON fallback path are all covered.

One untested edge case: a JSON template where a placeholder renders a number (e.g., "count": "{{ .SomeInt }}"). The output stays a JSON string rather than a number. Unlikely to be a practical issue but worth awareness.

[claude]

Review

The approach is correct — parsing the template as JSON, resolving placeholders per-field, then re-marshaling is the right way to ensure proper escaping.

Issues:

• webhook.go:71 — w.Template != nil is now used only as a boolean sentinel; the pre-parsed *template.Template in w.Template is never executed. w.Template could be dropped in favor of w.TemplateText != "". Minor waste but creates confusion about the struct's contract.

• webhook.go:120-132 (fallback path) — when templateText is not valid JSON, it falls back to raw text/template execution, which has the original escaping bug. This is intentional for non-JSON custom templates but worth a comment clarifying that callers accept the risk of unescaped output.

• resolveTemplateValues re-parses each template string on every Send call. For high-frequency alerts this is noticeable. Pre-parsing per-field at construction time (or caching) would be better.

Good:

• formatLogMessage fix in processing.go is correct — json.Marshal for non-string messages instead of %v.
• Test coverage is solid across the key edge cases (quotes, newlines, backslashes, nested objects, fallback).

[claude]

Review

Core fix is correct — parsing template as JSON, resolving placeholders in string values, then re-marshaling is the right approach for proper escaping. Good test coverage.

Issues:

• Race condition log_listener.go:125-130: Load + Store is not atomic. If UpdateStreams and the containerChan goroutine both call startListening for the same container concurrently, both could pass the Load check and store their entries, resulting in duplicate streams. cleanupStream correctly uses Compute for atomic identity comparison — startListening should do the same:

  l.activeStreams.Compute(c.ID, func(old *streamEntry, loaded bool) (*streamEntry, xsync.ComputeOp) {
      if loaded { old.cancel() }
      return entry, xsync.StoreOp
  })

• w.Template is now dead code for execution webhook.go:47-51: Constructor still parses into w.Template (useful for validation), but execution now uses w.TemplateText directly — so the template is re-parsed on every SendTest call. Not a correctness issue, just wasteful.

• Non-JSON fallback may send invalid JSON webhook.go:127-133: If the text/template execution produces output that isn't valid JSON, it's sent to the webhook without validation. Callers expecting JSON will silently receive garbage.

[claude]

Code review

• webhook.go: Send delegates to SendTest, so the fix is complete. executeJSONTemplate correctly handles JSON escaping by separating structure parsing from template execution — solid approach.
• webhook.go: Template is re-parsed per call in resolveTemplateValues (one template.New per string field). Not a hot path for notifications, acceptable.
• webhook.go: Non-JSON templates with numeric placeholders (e.g. {"count": {{ .Number }}}) will fail JSON parse and fall back to raw text/template, which may produce valid JSON. Edge case but worth documenting.
• log_listener.go: Compute + context identity comparison (entry.ctx == streamCtx) is the right approach for the stale-stream race. Clean fix.
• log_listener.go: containerClients.Delete inside a Compute callback on a different map is fine, but worth noting that the callback should stay non-blocking.
• processing.go: formatLogMessage is a straightforward improvement over fmt.Sprintf("%v", ...) for JSON log entries.
• Tests: Good coverage of escaping edge cases (quotes, newlines, backslashes, nested structures, fallback).
• container_store.go: Trace→Debug level bump — no concerns.

[claude]

Previous reviews covered the key points. Confirming the final state looks correct:

• log_listener.go: Compute-based atomic swap with context identity check in cleanupStream is the right solution — no race condition.
• webhook.go: executeJSONTemplate correctly fixes escaping by separating JSON structure parsing from template resolution.
• processing.go: formatLogMessage is a clean improvement.

Remaining dead code: WebhookDispatcher.Template (*template.Template) is parsed in the constructor for validation but never executed — execution goes through executeJSONTemplate which re-parses TemplateText. The field can be dropped and validation kept inline. Not a blocker.