fix(webui): keep acknowledgement state in SSE alert updates

[SoulKyu]

Problem

convertToDashboardAlert builds an alert from what Alertmanager knows, so IsAcknowledged, AcknowledgedBy and CommentCount are always zero on it. hasAlertChanged compared exactly those fields against the cache entry, so every acknowledged or commented alert was "changed" on every poll — and the object queued for SSE was that stripped alert. Open dashboards lost the acknowledged badge and comment count once per refresh cycle, and UpdatedAt advanced forever on precisely the alerts someone was working on.

Changes

internal/webui/services/alert_cache.go

• hasPolledStateChanged compares only what a poll populates: Status.State, Summary, Annotations. EndsAt is deliberately excluded — Alertmanager keeps pushing it forward for firing alerts, which would mark everything changed every cycle. hasAlertChanged is unchanged and still used by UpdateAlert, which does write collaboration fields.
• The refresh merge branch queues a shallow copy of the merged cache entry (same approach as GetAllAlerts) instead of dashAlert, so the SSE payload carries real ack and comment state.
• updateExistingAlert now copies Summary too. It never did; leaving it stale would make the new comparison report a change forever once a summary changed.
• Acknowledgements no longer reach browsers through the poll diff, so they are emitted where they happen: MutateAlert stamps UpdatedAt and pushes the merged alert to subscribers, and loadAcknowledgmentsEfficiently / loadCommentCountsEfficiently push only the alerts whose backend state actually moved rather than rewriting every alert every cycle.

No template or JS changes — the browser side was already rendering whatever the payload said.

Validation

TestAlertCache_RefreshKeepsAcknowledgementState seeds an acknowledged alert, asserts the acknowledgement itself is pushed over SSE with isAcknowledged: true, then runs three refresh cycles with an unchanged Alertmanager payload and asserts no SSE update fires, the collaboration state survives and UpdatedAt does not move. A final cycle with a changed annotation asserts the push happens and carries both the new summary and the ack state.

go build ./..., go vet ./internal/webui/..., go test ./... -count=1 and go test ./internal/webui/... -race all pass.

Closes #102

_{🔁 Powered by Looper · runner=worker · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

@SoulKyu The diagnosis here is exactly right, and splitting the comparison into hasPolledStateChanged (what a poll can observe) versus hasAlertChanged (what a full write carries) is the correct shape for this. Verified locally on bec0fee: go build ./..., go vet ./internal/webui/..., and go test ./internal/webui/services/... -count=1 -race all pass, and the new test does fail against the old comparison.

Three non-blocking follow-ups, all inline: a residual ordering window in refreshAlerts that can still push a stale ack snapshot, the newAlertsForSSE path that does not yet honour the copy invariant the new notifyAlertsUpdated helper documents, and the fact that the two new dedup guards in the backend loaders are currently untestable. None of these block merge.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

Looper fixer round complete — a6ffb6f

• ✅ Review comment on internal/webui/services/alert_cache.go:270 (@SoulKyu) — thread
  • Moved the notifySubscribers block in refreshAlerts to immediately after ac.mu.Unlock(), ahead of ac.loadBackendData() and go ac.RefreshAllCachedColors(), so the poll diff is always emitted before any collaboration push that could supersede it. It stays outside the lock as before.
• ✅ Review comment on internal/webui/services/alert_cache.go:1193 (@SoulKyu) — thread
  • The new-alert branch in refreshAlerts now appends a shallow copy of dashAlert to newAlertsForSSE instead of the cache-resident pointer, matching the existing-alert branch, so the notifyAlertsUpdated copy invariant holds on every notify path in the file.
• ✅ Review comment on internal/webui/services/alert_cache.go:577 (@SoulKyu) — thread
  • Added a collabLoader interface (IsConnected/GetAllAcknowledgedAlerts/GetCommentCountsBatch) in its own collabClient field mirroring alertFetcher, and two tests over a fake: a table-driven ack test (identical pushes once; changed reason, CreatedAt and user each push) plus a comment-count test covering 0 to 2 with ack state intact, unchanged count silent, and the error branch zeroing counts without notifying.

_{🔁 Powered by Looper · runner=fixer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

@SoulKyu Thanks for the follow-up — this is a solid fix. I re-reviewed the full diff in internal/webui/services/alert_cache.go and alert_cache_test.go at the current PR head.

What I verified:

• hasPolledStateChanged correctly narrows the poll comparison to Status.State, Summary, and Annotations, so acknowledged/commented alerts no longer look "changed" on every refresh cycle just because the poll result carries no collaboration state.
• The refresh-cycle SSE push now snapshots (alertCopy := *dashAlert / *existingAlert) before queuing for newAlertsForSSE/updatedAlertsForSSE, and pushes the merged cache entry rather than the stripped poll result — so ack/comment state survives.
• MutateAlert, loadAcknowledgmentsEfficiently, and loadCommentCountsEfficiently now emit SSE updates directly where collaboration state actually changes, with dedup guards to avoid re-pushing unchanged ack/comment data.
• The reordering in refreshAlerts (poll-diff notify now happens before loadBackendData) addresses the ordering window flagged in the previous review round, and the new collabLoader interface plus its fake implementation finally makes the dedup guards testable.

I ran a full build, vet, and the alert cache test suite with the race detector locally — everything compiles cleanly and all tests pass, including the new tests covering acknowledgement state survival across refresh cycles and the dedup guards in the backend loaders.

I didn't find any blocking or non-blocking issues in the changed ranges — the previous round's follow-ups all look properly addressed. Nice work tightening this up.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

@SoulKyu Re-reviewed at the current head. This PR's actual content in internal/webui/services/alert_cache.go and alert_cache_test.go is byte-identical to the commit that already passed a full clean review round (the branch was rebased onto a newer main, which is why the head SHA changed without any code changes) — I diffed the two commits directly (git diff <prev-reviewed-sha>..<current-head> -- internal/webui/services/) and confirmed zero differences in these files.

I still did an independent full pass rather than just trusting that:

• hasPolledStateChanged correctly narrows the poll comparison to Status.State, Summary, and Annotations (via maps.Equal), so acknowledged/commented alerts stop looking "changed" on every refresh just because a poll result carries no collaboration state. EndsAt is still copied into the cache on every cycle in updateExistingAlert, it's just excluded from the comparison, which matches the stated rationale.
• The refresh cycle now snapshots (alertCopy := *dashAlert / *existingAlert) before queuing into newAlertsForSSE/updatedAlertsForSSE, so the SSE payload for updated alerts carries the merged cache state (with ack/comment data) instead of the stripped poll result.
• The poll-diff notify was moved before loadBackendData(), closing the ordering window from the previous round where a stale pre-backend-load snapshot could overwrite a fresher ack push in the browser.
• MutateAlert now stamps UpdatedAt, copies the alert, releases the lock, then notifies — avoiding notifying subscribers while holding ac.mu.
• loadAcknowledgmentsEfficiently / loadCommentCountsEfficiently now dedup against the existing cached state before pushing, and the collabLoader seam makes both dedup guards testable via fakeCollabLoader.

I ran go build ./..., go vet ./internal/webui/..., and go test ./internal/webui/services/... -race -count=1 locally against 64096b0 — everything compiles and all tests pass, including the new tests covering acknowledgement-state survival across refresh cycles and the dedup guards.

No blocking or non-blocking issues in the changed ranges. Nice work carrying this through multiple review rounds to a clean state.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🔀 Rebased on main (clean, build green). — rebaser

[SoulKyu]

@SoulKyu Re-reviewed at the current head (d2f6007). I diffed the changed files against the previously clean-reviewed head (64096b0) and confirmed the content of internal/webui/services/alert_cache.go and alert_cache_test.go is byte-identical — this is another rebase-only head move, no code delta.

I still did an independent full pass rather than relying on that diff alone:

• hasPolledStateChanged correctly narrows the poll comparison to Status.State, Summary, and Annotations (via maps.Equal), so an acknowledged/commented alert no longer looks "changed" on every refresh just because the fresh poll result carries no collaboration state. EndsAt stays copied into the cache every cycle but is excluded from the comparison, matching the documented rationale (Alertmanager keeps advancing it for firing alerts).
• The refresh cycle now snapshots (alertCopy := *dashAlert / *existingAlert) before queuing into newAlertsForSSE/updatedAlertsForSSE, so the SSE payload for both new and updated alerts carries the merged cache state instead of the collaboration-free poll result.
• The poll-diff notify was moved before loadBackendData(), closing the ordering window where a stale pre-backend-load snapshot could race a fresher ack push to the browser and win.
• MutateAlert releases ac.mu before calling notifyAlertsUpdated, and loadAcknowledgmentsEfficiently/loadCommentCountsEfficiently now dedup against the already-cached ack/comment state before pushing, backed by the new collabLoader seam and fakeCollabLoader in tests.

I ran go build ./..., go vet ./internal/webui/..., and go test ./internal/webui/services/... -race -count=1 locally against d2f6007 — everything compiles and all tests pass, including TestAlertCache_RefreshKeepsAcknowledgementState and the dedup-guard tests for the collaboration loaders.

No blocking or non-blocking issues in the changed ranges. Good, careful fix for a real user-visible regression — thanks for carrying it cleanly through the rebases.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🔀 Rebase (conflicts resolved): rebased onto origin/main (now includes #105, #113, #110).

One conflict: internal/webui/services/alert_cache.go — loadAcknowledgmentsEfficiently.

• main (fix(backend): a re-firing alert no longer inherits the previous incident's acknowledgement #105) had extracted the ack-application into applyAcknowledgments(map) and inverted the loop to iterate ac.alerts, clearing ack fields for any alert the backend no longer reports (so a re-firing alert does not inherit the previous incident's ack).
• This PR iterated the backend map, added a no-op dedup guard, and pushed only changed alerts to SSE.

Resolution: kept main's applyAcknowledgments signature and mirror-the-backend loop (both branches of the exists check preserved), and layered this PR's behaviour on top: the dedup guard short-circuits an unchanged ack, and the clear branch got a symmetric guard so already-clean alerts do not generate spurious updates. Both branches stamp UpdatedAt, snapshot the alert, and the collected set is pushed via notifyAlertsUpdated after the mutex is released. The "successfully updated" log stayed in the caller.

Second commit applied cleanly. go build ./..., go vet ./... pass; go test ./internal/webui/... green (99 tests), including TestAlertCache_AcknowledgmentDoesNotSurviveResolution from #105. No *_templ.go touched.

[SoulKyu]

🧪 QA Report

Environment: make test (docker-compose: postgres + fake alertmanager + backend + webui), all 4 containers healthy, webui on :8081, 22 seeded alerts, 10s refresh interval. Criteria taken from #102.

Criterion	Result	Evidence
Badge and comment count stable across ≥3 refresh cycles with a dashboard open, no page reload	✅	Headless Chromium logged in, Acknowledged view, sampled every 11s for 5 cycles: the 4 acknowledged alerts stayed ack=true by=qaagent cc=1, no Acknowledge Alert button ever reappeared on them, ACK badge count constant at 6
SSE update payload for an acknowledged alert carries isAcknowledged: true + correct acknowledgedBy / commentCount	✅	curl -N /api/v1/dashboard/stream while acking HighMemoryUsage: push at 10:34:36 with isAcknowledged:true, commentCount:1; the later real Alertmanager-side change (active→resolved, 10:35:25) still carried isAcknowledged:true, acknowledgedBy:"qaagent", commentCount:1
Acknowledged-but-unchanged alert absent from updatedAlerts on a quiet refresh cycle	✅	Two SSE captures totalling ~20 refresh cycles: the long-acked HighCPUUsage and HighMemoryUsage never appear in updatedAlerts, while unacked alerts (CriticalCPUUsage, …) churn every cycle. Each ack produces exactly two pushes — the ack itself, then one backend reconciliation (userID→username, DB timestamp, reason) — and then goes silent
UpdatedAt no longer advances every cycle for acknowledged alerts	✅	Polled /api/v1/dashboard/data?displayMode=acknowledge every 10s ×9: updatedAt frozen at 10:36:55 (HighCPUUsage) and 10:35:25 (HighMemoryUsage) throughout; HighMemoryUsage still read 10:35:25 twelve minutes later
Test in alert_cache_test.go asserting absence from UpdatedAlerts after an unchanged refresh	✅	TestAlertCache_RefreshKeepsAcknowledgementState seeds an acked alert, runs 3 refresh cycles and fails on any SSE update / any UpdatedAt move; go test ./internal/webui/services/... -race -count=1 → 67 tests pass

Exploratory: unacknowledge still pushes immediately (isAcknowledged:false, commentCount:2) and does not flip back over the following 4 cycles — the alert correctly leaves the Acknowledged view; free-text search still narrows the table (9 alerts → 5, all HighCPUUsage); the alert detail modal still opens. One pre-existing quirk unrelated to this diff: non-acked new alerts can slip into the Acknowledged view via SSE newAlerts, because alertMatchesFilters (dashboard_data.templ:501) has no displayMode check — same on main.

Verdict: PASS

[SoulKyu]

@SoulKyu Re-reviewed at the current PR head. This is another rebase-only head move: the PR-scoped diff (from gh pr diff --patch, the two commits this PR owns) is unchanged in content from the last clean-reviewed head. A raw two-commit-range diff also picks up unrelated main-branch fixes that landed separately (a re-firing-alert acknowledgement fix, an encryption-key requirement, and an admin-gated resolved-alert deletion restriction) which were pulled in by the rebase, not authored by this PR.

I still did an independent full pass on the PR's own diff rather than relying on that comparison alone:

• hasPolledStateChanged correctly narrows the poll comparison to Status.State, Summary, and Annotations (via maps.Equal), so an acknowledged/commented alert no longer looks "changed" on every refresh just because the fresh poll result carries no collaboration state. EndsAt is still copied into the cache every cycle in updateExistingAlert but stays out of the comparison, matching the documented rationale.
• The refresh cycle snapshots (alertCopy := *dashAlert / *existingAlert) before queuing into newAlertsForSSE/updatedAlertsForSSE, so the SSE payload for both new and updated alerts carries the merged cache state instead of the collaboration-free poll result.
• The poll-diff notify was moved before loadBackendData() in refreshAlerts, narrowing (though, as flagged in the first review round, not fully eliminating — a small window remains between the cache unlock and the subscriber notify) the ordering window where a stale pre-backend-load snapshot could race a fresher acknowledgement push to the browser.
• MutateAlert releases the cache lock before calling notifyAlertsUpdated, avoiding notifying subscribers while holding the lock.
• loadAcknowledgmentsEfficiently/loadCommentCountsEfficiently dedup against already-cached state before pushing, backed by the new collabLoader seam and its fake implementation in tests, with dedicated tests covering identical-push, changed-reason, changed-timestamp, different-user, and comment-count transitions.

I ran a full build, go vet on the webui package, and the alert cache test suite with the race detector locally against the current head — everything compiles and all tests pass, including the tests covering acknowledgement-state survival across refresh cycles and the dedup guards for the backend collaboration loaders.

No blocking or non-blocking issues in the changed ranges. Good, careful fix for a real user-visible regression — thanks for carrying it cleanly through the repeated rebases.

_{🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.}

[SoulKyu]

🧪 QA Report

Env: make test (full rebuild, docker-compose: postgres + fake alertmanager + backend + webui), webui on :8081, cache sync interval 10s. Browser flow driven with Playwright/Chromium (chrome-devtools MCP was not permitted in this run).

Criterion	Result	Evidence
Badge + comment count stay stable across ≥3 refresh cycles with no page reload	✅	Logged in as qauser, clicked the row's Acknowledge button, filled the reason modal, submitted, switched to the Acknowledged view. Sampled Alpine state + rendered DOM every 10s for 70s (7 cycles): domAck=true, commentCount=1 at every sample, updatedAt frozen at 11:03:17. No JS console errors. Screenshot shows the three ACK badges with their comment counts.
SSE update payload for an acknowledged alert carries isAcknowledged: true + correct acknowledgedBy / commentCount	✅	Raw curl -N /api/v1/dashboard/stream capture. Ack of e809b5a5… produced {"isAcknowledged":true,"acknowledgedBy":"3ylfbtim…","commentCount":1} immediately (MutateAlert push), then one follow-up {"isAcknowledged":true,"acknowledgedBy":"qauser","commentCount":1} when applyAcknowledgments normalised the username — a real transition, then silence.
An acknowledged, otherwise unchanged alert does not appear in updatedAlerts on a quiet cycle	✅	65s SSE window (6+ refresh cycles) after the ack: 7 events captured (new alerts, firing→resolved transitions, heartbeats), zero carrying e809b5a5…. On main this alert would be in updatedAlerts every single cycle.
UpdatedAt no longer advances every cycle	✅	GET /api/v1/dashboard/data polled repeatedly over ~8 min: the three acknowledged alerts held 10:59:37.660, 11:04:59.800, 11:05:37.664 unchanged across every subsequent cycle. Each timestamp maps to a real event (ack normalisation, added comment, firing→resolved).
Test in alert_cache_test.go seeds an acked alert, refreshes with an unchanged payload, asserts absence from UpdatedAlerts	✅	go test ./internal/webui/services/ -run TestAlertCache_RefreshKeepsAcknowledgementState -v → PASS; full TestAlertCache* suite: 39 tests pass.

Extra collaboration check: adding a comment on the already-acked alert (POST /alert/:fp/comments) pushed exactly one SSE update with commentCount: 2 and isAcknowledged: true preserved, then went quiet — loadCommentCountsEfficiently's new dedup guard behaves.

Exploratory: adjacent flows are intact — the poll diff still pushes real changes (firing→resolved, new alerts observed live over SSE), and the silence bulk action (another MutateAlert call site, now notifying outside the lock) pushed immediately and the alert reached state=silenced two cycles later. One pre-existing wart unrelated to this diff: the client-side alertMatchesFilters has no acknowledge branch (dashboard_data.templ:586), so un-acked alerts arriving via SSE newAlerts leak into the Acknowledged view (screenshot header reads "Showing 7 alerts" vs "1 to 3 of 3 results"). The JS is untouched by this PR and reproduces the same way on main — worth a separate issue, not a blocker here.

Verdict: PASS