fix(ui,cache): three bulk-action UX/cache bugs - #579
Merged
Conversation
The drawer Actions section in both users_v2.templ and computers_v2.templ previously rendered the Disable form whenever the backend was Active Directory, regardless of the entity's current enabled state. An already-disabled user kept seeing a "Disable" button that, when clicked, asked the directory to re-disable an already-disabled account. Wrap the form in the combined guard (IsAD && Enabled) so the action only surfaces when it can do something. Comment on the IsAD field updated to call out the Enabled requirement. Coverage: new drawer_disable_gating_test.go asserts the IsAD × Enabled matrix for both User and Computer drawer fragments. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Bug symptom: after clicking Delete or Disable on a user/group/computer,
the list+drawer still showed the target entry for several seconds
("took longer than expected"), even though the LDAP op succeeded. Cause
is AD replication delay — Refresh() runs against the readonly-bind DC
which hasn't yet received the mutation from the writing DC.
Solution: bypass the replication round-trip by applying the mutation to
the in-memory cache directly, the moment the per-entity LDAP op returns
without error. Follows the existing OnAddUserToGroup /
OnRemoveUserFromGroup pattern.
New Manager hooks (wired into finaliseBulk* in bulk_handlers.go):
- OnDeleteUser(dn) — drops entry, scrubs from Groups.Members
- OnDeleteGroup(dn) — drops entry, scrubs from User.Groups /
Computer.Groups
- OnDeleteComputer(dn) — drops entry
- OnDisableUser(dn) — flips Enabled=false on the user
- OnDisableComputer(dn) — flips Enabled=false on the computer
Supporting change: new unexported Cache[T].remove(dn) that deletes a
single entry by DN and rebuilds the O(1) indexes. No-op on miss so
callers can call it optimistically.
The trailing full Refresh() in finaliseBulk* is kept as a reconciliation
pass but correctness of the redirected list no longer depends on it
winning the race against replication.
Coverage: hooks_test.go (8 subtests): Cache.remove happy/miss/empty,
OnDeleteUser member-scrub, OnDeleteGroup memberOf-scrub on both users
and computers, OnDeleteComputer idempotence, OnDisable{User,Computer}
Enabled-flip.
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Bug symptom: clicking Disable or Delete on a user from the drawer
(which is reached via /users?ou=Eng&enabled=true&panel=1) redirected
to a bare /users, discarding the OU filter, the enabled filter, and
closing the drawer. The user had to re-apply their context on every
action.
All twelve 303 redirects in bulk_handlers.go now route through a new
bulkRedirectAfter helper that:
- Reads the Referer header (the URL that served the form) instead
of a hard-coded list path.
- Rejects cross-origin and unparseable values, falling back to the
hard-coded list — defence in depth against open-redirect via a
crafted Referer.
- Only honours Referers pointing at the same list surface
(fallbackList or a detail route beneath it).
- Uses EscapedPath so percent-encoded DNs round-trip verbatim.
- When dropPanel=true (delete actions): strips ?panel= and, if the
Referer was a /users/:dn detail page of the now-deleted entity,
collapses back to /users so the user doesn't land on a dangling
detail route.
- When dropPanel=false (disable, add-to-group, remove-from-group,
add-members-to-groups): keeps the Referer verbatim so the drawer
reopens on the same entity (now in its updated state) with all
filter chips intact.
Coverage: bulk_redirect_test.go exercises 10 scenarios including
empty Referer, list-with-query, detail-page-with-query for both
dropPanel modes, cross-origin rejection, unparseable rejection,
and the panel-only-param edge case.
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Contributor
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces optimistic cache updates and improved redirect logic for bulk LDAP operations. It adds methods to the Manager to immediately reflect deletions and account disabling in the local cache, reducing the impact of AD replication delays. It also enhances the web UI by preserving filter parameters after actions and gating the 'Disable' button based on the current account status. Feedback focuses on performance optimizations for bulk updates, specifically regarding redundant index rebuilding and linear scans where indexed lookups are possible, as well as a potential memory leak in slice manipulation and a race condition where synchronous refreshes might overwrite optimistic updates with stale data.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #579 +/- ##
==========================================
+ Coverage 67.15% 67.42% +0.26%
==========================================
Files 29 29
Lines 2780 2864 +84
==========================================
+ Hits 1867 1931 +64
- Misses 783 799 +16
- Partials 130 134 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes three bulk-action regressions introduced/observed after the UI revamp: (1) hide meaningless Disable actions for already-disabled entities, (2) make cache/UI reflect disable/delete immediately despite AD replication lag, and (3) preserve list filters + drawer state across bulk-action redirects.
Changes:
- Gate Disable drawer actions on both backend being AD and the entity being currently enabled.
- Add optimistic cache hooks for delete/disable to avoid stale list/drawer state while replication catches up.
- Route bulk-action redirects through a helper that preserves the originating query string and conditionally retains
?panel=.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/web/templates/users_v2.templ | Hide “Disable” action unless IsAD and user is enabled. |
| internal/web/templates/computers_v2.templ | Hide “Disable” action unless IsAD and computer is enabled. |
| internal/web/templates/drawer_disable_gating_test.go | Adds tests covering the IsAD × Enabled gating matrix for drawers. |
| internal/web/bulk_handlers.go | Adds bulkRedirectAfter and wires redirects through it; adds optimistic cache hooks for delete/disable. |
| internal/web/bulk_redirect_test.go | Unit tests for redirect helper behavior (filters/panel preservation + safety fallbacks). |
| internal/ldap_cache/manager.go | Adds Manager hooks for OnDelete*/OnDisable* optimistic cache updates. |
| internal/ldap_cache/cache.go | Adds Cache.remove(dn) for single-entity eviction + index rebuild. |
| internal/ldap_cache/hooks_test.go | Tests for Cache.remove and the new Manager hooks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ter scrub Post-review follow-up on PR #579. Three correctness/efficiency fixes flagged by Gemini and Copilot: 1. Cache.remove now uses the dnIndex for O(1) location and slices.Delete for the slice shrink. slices.Delete zeroes the vacated slot so pointer fields inside T (e.g. ldap.User.Mail *string) don't keep the deleted entry's data alive indefinitely — the previous append-based removal left the vacated slot pointing at the removed element until another append overwrote it. 2. New Cache.updateByDN(dn, fn) does a dnIndex-backed O(1) targeted mutation. Callers must only perform non-key mutations (the method does not rebuild indexes). OnDisableUser / OnDisableComputer use this instead of full-cache update() scans — flipping Enabled on a directory with tens of thousands of cached users no longer linearly walks the whole slice. 3. OnDeleteComputer now scrubs the computer DN from every cached group's Members list. Computers can be group members in AD (machine accounts in security groups), and PopulateGroupsForComputerFromData derives a computer's group memberships by scanning group Members at display time. Without the scrub, group member counts and drawer listings stayed stale until the next background Refresh picked up the change. Supporting cleanups: OnDeleteUser / OnDeleteGroup now use slices.DeleteFunc for the membership scrubs (same semantics, fewer lines, idiomatic). Test updates: - New TestCacheUpdateByDN covers the mutate-matching-entry and no-op-on-unknown-DN branches. - New TestManagerOnDeleteComputer_ScrubGroupMembership asserts the computer DN is removed from the group's Members list after OnDeleteComputer. - TestManagerOnDisableUser/Computer_FlipsEnabled replaced by TestManagerOnDisable_NoOpOnAbsentDN; the happy-path flip is now covered by TestCacheUpdateByDN above (seeding an ldap.User with a real DN requires reaching into simple-ldap-go's unexported Object fields, which we avoid). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Addresses Gemini's high-priority review finding on PR #579. Before this change, finaliseBulkDelete and finaliseBulkDisable called a.ldapCache.Refresh() immediately after the optimistic OnDelete*/ OnDisable* hooks had applied the mutation locally. Refresh() queries the readonly-bind DC, which under typical AD replication delay still sees the pre-mutation state and overwrites the optimistic update via setAll(). Result: the list page briefly showed the correct state, then flipped back to the pre-op state until the next 30 s background refresh picked up the replicated mutation. That race IS the user-visible "took longer than expected" symptom that motivated the whole optimistic-cache effort. Removing the sync Refresh completes the fix. Correctness is now covered by: - Per-entity optimistic hooks (OnDelete{User,Group,Computer}, OnDisable{User,Computer}) applied in the success branch of the bulk loops — immediate, authoritative, immune to replication lag. - The existing 30 s background Refresh loop in ldap_cache.Manager — picks up anything the hooks don't cover (unrelated mutations, concurrent admin changes on another DC). - The existing templateCache.Clear() in each finalise* — still done so cached HTML responses don't linger. Existing tests continue to pass; no new tests needed since this is removal of a behaviour that was always racing with replication. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Two follow-up nits from the Copilot review on PR #579: - bulkRedirectAfter's same-origin guard compared refURL.Host (hostname:port) against c.Hostname() (hostname only). A same-origin Referer whose URL included the port — common in dev (localhost:3000) and behind some reverse proxies — was incorrectly flagged as cross- origin, falling back to the bare list path and losing filters. Fixed to refURL.Hostname() vs c.Hostname(). New subtest "same-origin with explicit port preserved" locks in the behaviour. - bulk_redirect_test.go read the response body with a single Read into a fixed 256-byte buffer. Read is allowed to return partial data; if the helper output ever grew past 256 bytes or the reader fragmented the response, the assertion would silently truncate and either pass on a partial match or false-positive on a trailing diff. Replaced with io.ReadAll. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Contributor
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
CybotTM
enabled auto-merge
April 24, 2026 15:18
CybotTM
disabled auto-merge
April 24, 2026 15:25
CybotTM
enabled auto-merge
April 24, 2026 15:25
Contributor
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CybotTM
added a commit
that referenced
this pull request
Apr 25, 2026
Follow-up to #579 review feedback. The doc comment on `Cache[T].remove()` claimed "O(1) location lookup via dnIndex" but the implementation: 1. Uses dnIndex only for **presence** detection (the cached pointer) 2. Then **linearly scans** `c.items` to find the matching slice element by pointer comparison 3. Calls `buildIndexes()` afterwards, which is also O(n) Comment now reflects the true O(n) complexity. No behavior change. Resolves the unresolved review thread on #579 (#579 (comment)).
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
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
Three independent bugs surfaced while reviewing the shipped UI revamp (#571).
Each lands as its own atomic commit — bisect stays useful, each commit
builds, vets, tests, and lints independently.
bf8926f45e4ef120db214/usersroute, discarding OU filter, enabled filter, and the open drawer on every actionCommit 1 —
fix(ui): gate Disable action on User/Computer.EnabledWraps the Disable form in
if vm.IsAD && vm.User.Enabled(and the mirror for computers) so it only surfaces when the action is meaningful. One-line template change, 85-line test matrix covering IsAD × Enabled.Commit 2 —
fix(cache): optimistic updates on delete + disableNew
Managerhooks —OnDelete{User,Group,Computer}andOnDisable{User,Computer}— that mutate the in-memory cache synchronously in the per-entity success branch of the bulk handlers. No longer depend on winning the race against AD replication; the trailingRefresh()now runs as a reconciliation pass only.Follows the existing
OnAddUserToGroup/OnRemoveUserFromGrouppattern. Supporting change: a new unexportedCache[T].remove(dn)that drops a single entry by DN and rebuilds the O(1) indexes.Commit 3 —
fix(ui): preserve filters + drawer on bulk action redirectsAll twelve 303 redirects in `bulk_handlers.go` now route through a new `bulkRedirectAfter` helper that:
Out of scope — deliberately deferred
Test plan
Each commit is signed (ED25519) and carries a `Signed-off-by` trailer.