feat/fix(graph): post-Phase-3 polish — Table view, persistent selection, weighting, dark-mode + bug fixes - #588
Merged
Merged
Conversation
… dark mode) - Collapse: clicking expanded node's badge (now -) removes added sub-tree transitively, restores +. Keyboard parity via -/_. - Scroll: graph page now uses baseV2PageScroll so the relationships table below the canvas is reachable. - Zoom anchor: convert mouse to SVG user-space via getScreenCTM before applying scale-around-point; previous mix of screen pixels and viewBox units drifted noticeably. - Hover/focus highlight: hovering or focusing a node now darkens every incident edge via a JS-toggled .graph-edge--hover-related class. - Dark-mode discs: per-type fills shifted one tone away from --bg-subtle (canvas), which previously matched user-disc fill exactly and made discs invisible. - Removed the stale +/- canvas-zoom shortcuts (they collide with the new node expand/collapse keys). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
…ngles Two static enhancements that give the graph a "weighted" feel without animation, motion-reduce concerns, or new dependencies. - Node.Degree is computed once from the rendered edge set (assignDegrees). Both SSR (graphNode templ) and dynamic JS (renderNode) scale the disc radius by degree (base 22, +1.5 per edge, capped at 32) so high- connectivity hubs read as visually weightier than leaves. - assignConcentric now does parent-anchored placement on rings 2+: each node's "parent" is the ring-(N-1) node it shares an edge with; children of the same parent occupy a contiguous arc centred on the parent's angle, with arc width proportional to the parent's child count. Result: clusters of related nodes form spokes radiating outward from the focus, replacing the previous alphabetical shuffle. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
simple-ldap-go's FindUsers uses (|(objectClass=user)(objectClass=inetOrgPerson)(objectClass=person)) — and in Active Directory computer accounts inherit objectClass=user, so they leak into the users cache. The /users page (and BuildGraph, BuildListGraph, anything else built on FindUsers) showed machine accounts alongside real users. Filter at the cache layer: any DN also present in the Computers cache is dropped from the user iteration. Single O(1) lookup per user via the existing dnIndex. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The List|Graph toggle from Slice 5 only stuck per-page; switching from /users to /groups in the topnav reset to default List. Adds: - A third "Table" view for users/groups/computers — full-width scannable table without OU filter rail or detail drawer. Each row links straight to the entity's detail page. Same data as List view, optimised for browsing many rows at once. - Cookie-backed view persistence (graph-view, 30 days, SameSite=Strict, HttpOnly). pickView() reads the ?view= query when present (and refreshes the cookie) or falls back to the cookie value, so picking Table on /users sticks across /groups and /computers. - Three-segment toggle (List | Table | Graph) with always-explicit ?view= so clicking re-asserts the user's choice and refreshes the cookie even when clicking the segment that's already active. - Template-cache key now includes the graph-view cookie so /users (no query) caches separately per view preference. Without this the first cached render won regardless of cookie, making the toggle appear broken — see the "sometimes works" review feedback. - Content-Type explicitly set on the new Table renders. The list view's existing render does this; the new branches needed it too, otherwise the browser was shown the literal HTML source. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Subheader inconsistency: List view had title+count on the left and
toggle on the right; Table view had title alone + toggle; Graph view
had title + toggle + slider. The toggle moved between renders. List
page was max-width 72rem auto-centered, Table/Graph were full-width.
- All four list-bearing templates (users/groups/computers/graph) now
use the same .list-page__head structure: .list-page__head-titles
cluster on the left, .list-page__head-controls cluster auto-margined
to the right. Toggle is always the last child of __head-controls so
it stays in a fixed screen position regardless of view.
- .list-table-page and .graph-page now share .list-page's max-width
(72rem) + auto-centering so all three views align gutter-to-gutter.
Dark/console mode (the dark theme also swaps body to monospace —
:root[data-theme="dark"] body { font-family: var(--font-mono); }):
- Native depth slider gets accent-color: var(--accent) so the track
and thumb tint to the theme accent instead of the OS-default white.
- .graph-table and .list-table get explicit color: var(--fg) +
font-family: inherit so headers and body text honour the theme.
- "Relationships" h2 in the graph view gets explicit color: var(--fg)
(Pico's defaults didn't pick it up in dark mode).
- .graph-node__label SVG text gets font-family: inherit so node
labels switch to monospace alongside the body in dark mode.
Active-tab hover: the segmented option used --bg-subtle on hover,
which equals --bg in dark mode and rendered the active option as
black-on-black. Added :not(.--active) so the active option stays
solid through hover.
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 a new "Table" view for entity lists and implements a persistent view preference via cookies. The graph visualization is significantly improved with parent-anchored layouts, node degree-based scaling, and the ability to collapse expanded nodes. Key fixes include resolving zoom drift in the graph and filtering computer accounts from the user list. Feedback focuses on optimizing the graph layout algorithm's complexity, restoring keyboard zoom accessibility, and ensuring transitive edges are correctly removed during node collapse.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #588 +/- ##
==========================================
+ Coverage 69.26% 70.00% +0.73%
==========================================
Files 32 33 +1
Lines 3254 3357 +103
==========================================
+ Hits 2254 2350 +96
- Misses 844 849 +5
- Partials 156 158 +2
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
Adds final “Phase 3” graph-view polish plus a new Table mode for entity lists, with persistent List/Table/Graph selection across pages and some AD-specific correctness fixes.
Changes:
- Add Table view templates for users/groups/computers and update handlers to render it (including correct HTML Content-Type).
- Persist view selection via
graph-viewcookie and include it in the template-cache key to avoid cross-user cache collisions. - Improve graph UX/layout (degree-based sizing, parent-anchored angles, hover-related edge highlighting, expand/collapse behavior, cursor-anchored zoom) and dark/console-mode styling; fix AD computer accounts leaking into
/users.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/web/users_v2_handler.go | Uses pickView() and renders new Table view branch for /users. |
| internal/web/groups_v2_handler.go | Uses pickView() and renders new Table view branch for /groups. |
| internal/web/computers_v2_handler.go | Uses pickView() and renders new Table view branch for /computers. |
| internal/web/graph_view.go | Introduces cookie-backed view selection helpers (graph-view). |
| internal/web/template_cache.go | Adds graph-view cookie into template cache key for correct per-view caching. |
| internal/web/templates/users_v2.templ | Unifies list-page header structure (titles vs controls cluster). |
| internal/web/templates/groups_v2.templ | Unifies list-page header structure (titles vs controls cluster). |
| internal/web/templates/computers_v2.templ | Unifies list-page header structure (titles vs controls cluster). |
| internal/web/templates/list_table_v2.templ | New full-width Table view templates for users/groups/computers. |
| internal/web/templates/graph_v2.templ | Scrollable base, unified header layout, and degree-scaled node discs. |
| internal/web/templates/graph_toggle.templ | Extends toggle to List/Table/Graph and always emits ?view= for persistence. |
| internal/web/static/js/v2-graph.js | Adds hover edge highlighting, expand/collapse, cursor-anchored zoom fixes, and degree-scaled discs for JS-rendered nodes. |
| internal/web/static/app.css | Header layout unification, graph/table styling, dark/console-mode polish, and new table-view CSS. |
| internal/ldap_cache/manager.go | Filters AD computer accounts out of cached “users” results via DN intersection with Computers cache. |
| internal/ldap_cache/graph.go | Adds Node.Degree and parent-anchored ring layout; assigns degrees before layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Six concrete fixes from Copilot + gemini-code-assist review: - Cookie Secure flag: setViewCookie now sets Secure when the request arrives over HTTPS (auto-detected via c.Protocol()) so the graph-view preference cookie can't be sent over a downgraded HTTP connection on TLS deployments. - collapseNode transitive edges: edges introduced by sub-expansions (addedBy a transitive descendant) where both endpoints survive the collapse used to orphan; now filtered out via the ancestors set. - Degree recomputation after expand/collapse: new recomputeDegrees walks state.edges and refreshes both Node.degree and the rendered <circle r="..."> attribute so disc weighting stays accurate after interactive mutations. - discRadius rounding sync: Go uses math.Round and JS uses Math.round so SSR-rendered and JS-rendered nodes share the same radius for the same degree (was off-by-1 for odd degrees). - Filter QS preservation in toggle: switching between List/Table/ Graph from a filtered URL no longer drops the active ?ou=, ?memberOf=, ?show-disabled= filters. Each handler computes the filter QS once and passes it to GraphPageVM.FilterQS / the table template signature, both of which thread it into listGraphToggle. - Tests: TestHandleUsersV2_TableMode + groups + computers cover the new ?view=table branch (status, content-type, marker assertions); TestHandleUsersV2_PersistentViewViaCookie covers the cookie-only resolution path that the template-cache key fix protects. Reviewer notes acknowledged but not coded: - gemini's O(N×E) Jaccard suggestion: acceptable at the current 60-per-ring / 200-total caps; revisit if caps grow. - gemini's keyboard zoom regression: by design — +/- now drive node expand/collapse (the documented affordance per the SSR aria-label). Canvas zoom remains via wheel + ctrl/meta. 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.
6 tasks
CybotTM
added a commit
that referenced
this pull request
Apr 25, 2026
…589) ## Summary Closes the deferred follow-up from the live-review polish session ([#588](#588) handoff): the Table view now has a search input above the table and clickable column headers that toggle a server-side sort. ## What ships ### Client-side filter - Search input above each Table view (`<input data-search-input>` inside a `data-search-filter` / `data-search-scope` container). - Hooks into the existing `v2-search-filter.js` (no new JS needed). Rows hide via the `hidden` attribute when the haystack doesn't match. - `data-search-text` on each `<tr>` concatenates CN, SAM, mail (where present), and DN so the substring filter matches visible and adjacent attributes. - The visible row count in the subheader updates live via the existing `data-search-count` mechanism. - `Esc` clears the input. ### Server-side sortable headers - Column headers are anchor links carrying `?sort=col&dir=asc|desc` plus the preserved filter QS — so back/forward navigation and bookmarking work, no JS required. - Clicking the active column toggles the direction; clicking another column resets to `asc`. The active column shows `↑` / `↓`. - Sort columns: - Users: cn / sam / mail / status - Groups: cn / members / dn - Computers: cn / sam / status / dn - Status sort: enabled rows first under asc — the more useful default than alphabetical "Disabled" / "Enabled". ### Tests - `TestNormaliseSortDir` + per-entity sort tests (ascending/descending, status semantics, unknown-key fallback to CN, members count desc). - `TestHandleUsersV2_TableMode` marker assertion updated to match the new sort-link / search-input markup. ## Out of scope - Sort persistence ACROSS views (sort is meaningful only in Table view; the cookie-persisted view selection from PR #588 is unaffected). - Multi-column sort (single column for v1). ## Test plan - [x] `go test ./internal/web/ -count=1` — pass - [x] `golangci-lint run ./...` — 0 issues - [x] `go build ./...` — clean - [x] `templ generate` — clean - [ ] CI verification on push - [ ] Visual smoke (after merge): /users → Table → click "Status" header → enabled rows top; "Filter users…" → type "ad" → only matching rows visible; click "Graph" → return to /users → still Table view (cookie still works)
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
Live-review polish on top of the Phase 3 graph view, plus a long-requested Table list mode and a real bug fix on AD-backed deployments. All driven by an interactive review session against the
ldap.netresearch.nrAD.What ships
Bug fixes
FindUsersfilter(|(objectClass=user)(objectClass=inetOrgPerson)(objectClass=person))matches AD computer accounts (which inheritobjectClass=user), so/userswas showing machine accounts alongside real users. Filter at the cache layer via DN intersection with the Computers cache.Content-Type: text/html.graph-viewcookie, so the first cached/usersrender stuck regardless of the per-user preference.Phase 3 polish (from live-review feedback)
−badge now removes the added sub-tree transitively and restores+. Keyboard parity via−/_.baseV2PageScrollso the relationships table below the canvas is reachable.getScreenCTM().inverse()before scaling — previous mix of screen pixels and viewBox units drifted noticeably..graph-edge--hover-relatedclass).--bg-subtle(canvas background), which previously matched user-disc fill exactly and made discs invisible.Weighting (the "the graph feels static" feedback)
Degreeis computed once from the rendered edge set; both SSR and JS-rendered nodes scale disc radius from base 22 (+1.5 per edge, capped at 32) so high-connectivity hubs read as visually weightier than leaves.Persistent List | Table | Graph view selection
graph-view, 30 days, SameSite=Strict, HttpOnly): pickView() reads?view=when present (refreshing the cookie) and falls back to the cookie value otherwise. Picking Table on/userssticks across/groupsand/computers.?view=so each click re-asserts the user's choice.UI consistency
.list-page__headstructure with.list-page__head-titles(left) +.list-page__head-controlsauto-margined right. The toggle stays in a fixed screen position regardless of view.list-table-pageandgraph-pagenow share.list-page'smax-width: 72rem+ auto-centering so all three subheaders align gutter-to-gutter.Dark/console-mode polish
accent-color: var(--accent)instead of the OS-default white.color: var(--fg)+font-family: inheritso headers and body text honour the dark/console theme (which swaps body to monospace).color: var(--fg)so it picks up the theme..graph-node__labeltext inherits the body font so node labels switch to monospace in dark mode alongside the page text.Active-tab hover
--bg-subtle(which equals--bgin dark mode and rendered as black-on-black).:not(.--active)guard on the hover rule.Commits (atomic, signed + DCO)
Test plan
go test ./internal/web/ ./internal/ldap_cache/...— no regressionsgolangci-lint run ./...— 0 issuesgo build ./...+go build -tags e2e ./internal/e2e/...— cleantempl generate— cleanldap.netresearch.nr, 181 users / 108 groups / 73 computers) covering: List/Table/Graph toggle, view persistence across pages, Computer exclusion from /users, dark-mode visibility of all elements, expand/collapse on graph nodes, cursor-anchored zoom.