fix(webui): keep saved filters intact when editing a filter preset#98
Conversation
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu The core fix is right and well targeted: routing saveNewPreset() through buildPresetFilterData() means editing a preset's name or description no longer rewrites its filter_data from whatever happens to be on the dashboard, and the summary panel now describes what will actually be persisted instead of what is on screen. The defensive copies in editPreset() are a good catch too. I verified go build ./... passes and that both *_templ.go files are regenerated in sync with their .templ sources.
Two non-blocking follow-ups, both about the boundary of the new opt-in rather than the branch itself: one path still lets dashboard-side state into the "keep saved filters" update, and the checkbox copy does not match the set of fields the opt-in actually replaces. Details inline.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
|
Looper fixer round complete — f288c7f
🔁 Powered by Looper · runner=fixer · agent=claude-code · An autonomous AI dev team for your GitHub repos. |
|
🔀 Rebased on |
f288c7f to
99c8575
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu The core of this fix reads correctly now. Routing saveNewPreset() through buildPresetFilterData() means a description-only edit no longer rewrites filter_data from whatever happens to be on the dashboard, the copy-on-read in applyFilterPreset() and editPreset() closes the aliasing that let dashboard-side hides leak into the cached preset, and folding the branch into willReplaceSavedFilters() keeps the modal labels and the persistence decision from drifting apart. The reworded checkbox copy also matches the scope that is actually replaced. I verified the generated *_templ.go files are in sync with the .templ sources for the new bindings.
One remaining gap on the opt-in path, left inline: column_configs can still be cleared rather than preserved. It is narrower than the original report and needs a deliberate user action, so I am not treating it as a merge blocker.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
🧪 QA ReportEnv:
Exploratory: adjacent flows intact — creating a new preset still captures the live state ( Verdict: PASS |
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu The column-layout fix in this head lands in the right place. buildPresetFilterData() now copies the preset's stored column_configs through when the user opts into re-capture with the columns box off, so the opt-in can only ever replace a saved layout, never clear one, and the guard is at the persistence boundary rather than relying on the form seeding a page-lifetime flag. The willReplaceSavedFilters() predicate from the previous round is holding up well: the modal labels, the include-columns visibility and the persistence branch all read from one place now, which is what keeps this class of bug from coming back.
Verification on my side: I re-ran templ generate in the review worktree and the working tree came back clean, so both generated files match their .templ sources exactly, and go build ./... is green.
One non-blocking follow-up, left inline on the modal: the column-layout fix made the re-capture checkbox's own copy stale, and the nested columns checkbox now means two different things depending on create-vs-edit. Nothing is destroyed in either state anymore, so this is not a merge blocker.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
|
Looper fixer round complete — f11bf12
🔁 Powered by Looper · runner=fixer · agent=claude-code · An autonomous AI dev team for your GitHub repos. |
🧪 QA ReportRe-test after the rebase (head Env:
Also verified for the new head's extra commit: with the opt-in on but "Include current column configuration" off, the preset's saved column layout is now preserved ( Exploratory: adjacent flows intact — creating a new preset still captures the live state ( Verdict: PASS |
|
🔀 Rebased on |
f11bf12 to
caad6d8
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Reviewed the final state of this PR (head caad6d8). Across the three rounds already on this PR, buildPresetFilterData() now correctly returns the preset's own stored filter_data on a description/sharing-only edit, willReplaceSavedFilters() is the single source of truth driving both the modal labels and the persistence branch, editPreset()/applyFilterPreset() copy hidden_alerts/hidden_rules instead of aliasing them, and the re-capture opt-in preserves the saved column layout unless the columns checkbox is explicitly checked. I re-read the current diff against main independently and didn't find anything new beyond what was already caught and fixed in the prior rounds — the checkbox wording now matches what it actually replaces in every state (create vs. edit vs. edit+recapture).
Verification I ran myself: templ generate produced zero diff against the committed *_templ.go files (generated code is in sync with the .templ sources), and go build ./... is green.
Nice work tightening this down over the review rounds — the fix is well targeted and the final wording cleanup makes the opt-in's scope unambiguous. Thanks for the thorough iteration!
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
|
🔀 Rebased on |
caad6d8 to
48e6625
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu This head (48e6625) is byte-identical, in the reviewed files, to caad6d8 from the previous review round — I diffed them directly and the only change between the two commits is an unrelated factory-tui commit picked up by a rebase onto main, which is already merged upstream and outside this PR's diff.
I re-verified the substance independently rather than just trusting the prior rounds: buildPresetFilterData() correctly returns the preset's own stored filter_data on a description/sharing-only edit and only pulls in the live dashboard state when willReplaceSavedFilters() is true (creating a new preset, or the user explicitly checks the re-capture opt-in). That predicate is also the single source driving the modal labels and the include-columns checkbox visibility, so the UI and the persistence branch can't drift apart. editPreset() and applyFilterPreset() now copy hidden_alerts/hidden_rules with spread instead of aliasing, so editing the form or hiding an alert on the dashboard no longer mutates the cached preset object. And the column-layout opt-in only ever replaces column_configs when the columns checkbox is checked during re-capture — otherwise it falls back to the preset's stored layout, so it can't be silently cleared.
Verification I ran myself in this worktree: templ generate ./internal/webui/templates/... produced zero diff against the committed *_templ.go files, and go build ./... is green.
Nice, methodical fix for a subtle data-loss bug — thanks for iterating through the review rounds to close the column-layout and wording edge cases too.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
|
🔀 Rebased on |
48e6625 to
9c2bd07
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Reviewed the final head (9c2bd07) after the last review round. The only change since the previously reviewed head (48e6625) in the four files this PR touches is none at all in code — the diff between those two heads is entirely the unrelated 209bc0f (Sentry encryption key) commit picked up by a rebase onto main; I confirmed with git diff that the reviewed webui files are byte-identical between the two heads. The actual new commit on top, 9c2bd07, is a small, correct fix to the include-columns checkbox label: it now reads "Also replace the saved column layout with my current one" when editing (via x-text="editingPreset ? ... : 'Include current column configuration'" in filter_preset_modal.templ), which matches the column-preserving behaviour buildPresetFilterData() already implements — closing the last non-blocking wording gap flagged in the previous round.
I independently re-verified the substance rather than relying only on prior rounds: buildPresetFilterData() returns the preset's own stored filter_data on a description/sharing-only edit and only pulls in live dashboard state when willReplaceSavedFilters() is true (new preset, or the user explicitly checks the re-capture opt-in); that same predicate drives the modal labels and the include-columns visibility so they can't drift from the persistence branch; editPreset()/applyFilterPreset() copy hidden_alerts/hidden_rules with spread instead of aliasing, so dashboard-side hides or form edits no longer mutate the cached preset; and the re-capture opt-in only replaces column_configs when the columns checkbox is checked, otherwise it falls back to the preset's stored layout.
Verification I ran myself in this worktree: go build ./... is green, and make webui-templates (templ generate) produced zero diff against the committed *_templ.go files.
Nice, thorough iteration closing out the last wording nit — this is ready to merge. Thanks for the careful work across all the review rounds!
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
Editing a preset only copied its name, description and share flag into the form, so "Update Filter" persisted whatever filters were live on the dashboard, silently replacing the preset's saved definition (and wiping its column configs when the column checkbox was off). Renaming a shared team preset therefore corrupted it for every teammate using it. The edit form now submits the preset's own stored filter_data, with an explicit "Replace saved filters with my current dashboard filters" opt-in (default off) for the re-capture flow. The filter summary and the column config checkbox follow that opt-in so the form shows what will actually be saved. Hidden alerts/rules stay editable from the form and are copied on load so form edits no longer mutate the cached preset. Generated-By: looper 0.11.0 (runner=worker, agent=claude-code)
applyFilterPreset() aliased the active preset's hidden_alerts/hidden_rules arrays, so dashboard hides mutated the cached preset and were written back on the next description-only edit. Copy on read, matching editPreset(). Reword the re-capture checkbox to name the real persisted scope (filters, display and sort options, column layout) and expose the branch condition as willReplaceSavedFilters() so the labels and the persistence branch cannot drift apart. Generated-By: looper 0.11.0 (runner=fixer, agent=claude-code)
…ters Generated-By: looper 0.11.0 (runner=fixer, agent=claude-code)
The re-capture label claimed it replaced the column layout, but that only holds when "Include current column configuration" is also ticked. Drop the column clause from the parent label and let the nested checkbox describe the branch it belongs to: when editing it now reads "Also replace the saved column layout with my current one", while creating a preset keeps the original wording. Generated-By: looper 0.11.0 (runner=fixer, agent=claude-code)
|
🔀 Rebased on |
9c2bd07 to
2e84fd5
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Reviewed the current head (2e84fd5). I diffed it against the previously reviewed and approved head (9c2bd07) and the four files this PR touches — filter_preset_modal.templ/_templ.go and dashboard_filter_presets.templ/_templ.go — are byte-identical between the two. The only difference is that this branch was rebased onto a newer main, picking up two unrelated, already-merged backend fixes (57647f7 re-firing-alert acknowledgement fix, 23693e7 RemoveAllResolvedAlerts admin restriction), neither of which touches this PR's diff.
So the substance is unchanged from the prior round: buildPresetFilterData() correctly returns the preset's own stored filter_data on a description/sharing-only edit and only pulls in live dashboard state when willReplaceSavedFilters() is true; that same predicate drives the modal labels and the include-columns checkbox visibility so they can't drift from the persistence branch; editPreset()/applyFilterPreset() copy hidden_alerts/hidden_rules with spread instead of aliasing; and the re-capture opt-in only replaces column_configs when the columns checkbox is checked, otherwise falling back to the preset's stored layout.
Verification I ran myself in this worktree: go build ./... is green, and make webui-templates (templ generate) produced zero diff against the committed *_templ.go files.
Nothing new here — ready to merge. Thanks for keeping the branch current with main.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
🧪 QA ReportRe-test after the clean rebase (head Env:
New in this head: with the opt-in on, the column checkbox now relabels itself per branch — Exploratory: adjacent flows intact — creating a new preset still captures the live state ( Verdict: PASS |
Problem
The Edit button in the Saved Filters modal only copied a preset's name, description, share flag and hidden alerts/rules into the form — never its filter definition.
saveNewPreset()then built the PUT payload fromcaptureCurrentFilterState(), so clicking Update Filter persisted whatever filters were live on the dashboard.Fixing a typo in a shared preset's description while browsing unfiltered replaced its
filter_datawith the unfiltered state (and wipedcolumn_configswhen the column checkbox was off), silently changing the dashboard for every teammate using it.Changes
buildPresetFilterData()(new): returns the preset's storedfilter_datawhen editing, the live dashboard state when creating a new preset or when the user opts in. Used bysaveNewPreset().recaptureFiltersstate (default off, reset on edit/cancel/save) backing an explicit "Replace saved filters with my current dashboard filters" checkbox, shown only in edit mode with a note that leaving it unchecked keeps the saved criteria untouched.editPreset()copieshidden_alerts/hidden_rulesinstead of aliasing them, so form edits no longer mutate the cached preset object.Behaviour is unchanged for creating a new preset and for
updateActiveFilterPreset(), which still deliberately re-captures the live state.Validation
make webui-templates(only.templsources edited;*_templ.goregenerated)go build ./...filter_dataunchanged; edit with opt-in on → live state.Closes #96
🔁 Powered by Looper · runner=worker · agent=claude-code · An autonomous AI dev team for your GitHub repos.