Accounts & Groups tabs: UI polish deep dive (dogfood findings, mockup fidelity) #552
Labels
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Cordy/Cairn#552
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Nikola's first pass over the #550 tabs (2026-09-15 screenshots) found the surface working but not at the polish level he wants — parking this for a proper return visit during the beta.
Found in the first dogfood pass (quick fixes shipping as v0.6.201, verify they hold):
Group · Members · Used bycolumn headers are missing — rows are a two-sided list, not the table the mockup shows.409toast instead of the mockup's inline red message under the name field with the button disabled.Deep-dive scope when returning:
—.Reference:
admin-accounts-groups-mockup.htmlrev 1 (approved), #550 decisions, and the v0.6.201 fix-round commit.Fix round 2 shipped as v0.6.202 (PR #555, live on both dogfoods) — this takes the second batch of findings from Nikola's v0.6.201 pass:
autocolumns).Still open here for the deep dive: typeahead member suggestions, inline collision preview while typing, lazy directory member counts in the Members column, dashed directory-member chips with the legend, and the full phone pass.
Fix round 3 shipped as v0.6.203 (PR #556, live on both dogfoods):
#seatdlg .dlgactions button.danger, so only the seat dialog ever got it. Now one.dlgactions button.dangerrule covers every dialog: release seat, remove licence, and delete group all wear the same red from a single place.That closes out Nikola's third screenshot batch; the remaining deep-dive scope (typeahead members, live collision preview, lazy directory counts, phone pass over the new tables) stays parked here for the beta return visit.
v0.6.204 takes two more items off this list (PR #557, live):
GET /api/v1/admin/groups/candidates: local accounts, seat holders, and the whole realm when the IdP admin client exists — the Cairn-peering/spaces-picker idiom Nikola asked for. Server-side, names outside that universe are refused: bulk adds report them per-name asunknown, create returns 400 naming them. With no enumerable source at all, validation degrades to format-only, mirroring how the picker degrades to free-text.luadd-err, no more toast); the seat check exempts local re-creations and config admins.Remaining deep-dive scope here: live collision preview while typing group names, lazy directory member counts in the Members column, and the phone pass over the tables.
Mockup audit against v0.6.204, and the bug it surfaced — fixed in v0.6.205 (PR #558, live on both dogfoods).
The bug: a revoke that silently did nothing
On a directory account whose administrator role came from the admin group, the row still offered Remove admin. The click wrote a revocation to
admins.json, the group kept granting the role, and the row came back unchanged — an action that appears to work and does nothing. Regression from the v0.6.202 table rewrite (the v0.6.196 version showed the grant source and disabled the button).Root cause was upstream of the button:
adminNames()flattened four grant sources — config allow-list, dashboard grant, local flag, IdP group — intomap[string]bool, discarding the source at the API boundary. Neither the page nor the endpoint could tell a revocable grant from one the directory owns, so fixing the button alone would have been a symptom fix.Fixed in both halves, matching the pattern the last-admin guard already uses (server refuses and client disables):
AccessAdmin.Source(group/config/local/grant) from oneadminSources()lookup, the row showing "via the cairn-admins group" with the revoke disabled and explained, anddirectoryAdminSetreturning 409 naming the owner. Three tests written first and watched fail — including the revoke returning 200 where 409 was required, which is the bug reproduced. Dashboard grants stay revocable (pinned by its own test).Audit result: what matches the approved mockup
Tabs and deep links, both tables with house-style headers and aligned columns, the cap meter, lock markers with the mockup sub-copy, the member detail panel with App-owned badge and dashed directory chips plus legend, the delete dialog, inline collision errors, and the degraded state. Three things went past the mockup: the 25/50/100 page-size control, member autocomplete with server-side validation, and the seat-shadow refusal.
Remaining drift (mockup
accounts-groups-polish-decisions.htmlfor Nikola)uavcount is zero); the mockup has them, including the initials rule.Still open here
Live collision preview while typing with the primary button disabled; empty/degraded-state review against mockup variant D; plus whatever Nikola picks from the drift list. Moved out: the phone pass joins the existing mobile issue. Closed as fine: lazy directory member counts (confirmed working in the drill-in).
v0.6.206 — polish mockup items 1–3 landed
Shipped via #559, live on both dogfoods (
cairn-enc,cairn-openbao).1 — Avatars (approved). Account and group rows open with an initial circle from a shared
avatar()helper. Filled accent for administrators, square-cornered for groups so a group never reads as a person. Initials split on. _ -and whitespace, first letter of the first two parts, single-part names fall back to their first two characters.2 — Role badges (approved).
roleCellrenders Admin and User as one badge family instead of a chip beside muted grey text; the green you badge moved up beside the name it describes, leaving the meta line for group membership only.3 — Copy (recommendation: keep). Add account stays. The directory table header becomes Cairn role — that column is strictly about the role Cairn grants, and the person may hold roles in the directory that Cairn neither knows nor controls. The local table keeps plain "Role".
Bug caught by the round's own CI sanity check before merge: the emitted character class was
[._\\s-](escaped backslash + literals), so names split on the letter "s" and never on whitespace —Fall 1→ "FA",case-folder-1→ "CE". Fixed to[._\s-], nine cases now asserted in CI.Item 4 — Used-by column: resolved elsewhere
The Used-by divergence (column promises a share count, today shows only peering/admin/space references) is not a bug to patch here. It is a symptom of the sharing model, and the sharing model is being reworked in #529 — see the comment there: share with people and groups (OpenCloud-style internal sharing), links subdivided into internal link and public link, peering send as a distinct third act. Once group-addressed sharing exists, Used-by gains the real share count the mockup promised, with no separate work in this issue.
Research backing the decision to keep the column at all: no competitor surfaces this in the UI. Nextcloud admins are directed to query the
oc_sharetable directly, nextcloud/server#5586 is an open user request for exactly this listing, and nextcloud/server#25755 shows the failure mode it prevents (membership changes silently destroying shares). It is differentiator work, not parity work.Still open on this issue
Moved out: the full phone pass over all four tabs → existing mobile issue. Closed as fine: lazy directory member counts, confirmed working on the dogfood.
v0.6.208 shipped (PR #561), live on both dogfoods — both remaining items done.
Live collision preview. New group, Rename group and Add account now show a clash while typing: danger border, the reason under the field, primary button disabled. No new endpoint and no debounced request — all three dialogs open from tabs that have already loaded the names they need, so the check is a synchronous lookup against
ncLists, filled by the two loaders./candidateswas left alone; it feeds the member picker with account names, a different list from group names.Four distinct messages, because the reason differs: a directory-group clash is about share ambiguity, a directory-account clash is the v0.6.204 shared-identity case, the same-kind duplicates are plain duplicates.
No green "available" tick, deliberately. The lists are paginated and, with no IdP admin client, only the names seen in sign-in tokens so far. The check can honestly say taken; it can never say free. Silence means nothing is known against the name; the 409 remains the authority.
Variant D review. Empty states were already correct. Two real divergences fixed: filtering the Groups tab said "No accounts match the filter" over a table of groups (new
guNoMatchkey), and the notice shown when Cairn cannot list directory groups never said what to configure to fix it — the whole point of variant D. It now names the IdP admin client and says member counts are unknown.Two things this round caught that are worth recording:
nameCollisiontakes aselfexemption, the same line the server draws (groups.goonly callsdirNameTakenwhen the name actually changed). Three tests cover it.TestI18nKeysDefinedearned its keep. The first implementation built the key by concatenation —tf("nc" + hit.charAt(0).toUpperCase() + ...)— so the static scanner saw the keyncand could not verify any of the four. CI failed the PR. Fixed with one literaltf("ncDirGroup", …)per branch rather than by working around the check: a computed i18n key is invisible to the guard, and all four would have shipped unverified. Worth remembering as a house rule — i18n keys stay literal at the call site.Left undone, for your call: variant D also draws a per-row "seen in sign-in tokens" note in the actions column of every directory group. The notebox below the table already states that once for the whole table, and repeating a global condition on every row reads as noise. Also skipped the mockup's
notebox warnstyling as purely cosmetic. Say the word on either.Verification: red witnessed on the runner before implementation (
index.html has no // --- pure:name-collision --- block), 15 tests inweb/test/name-collision.test.js, full Go and web suites green,node --checkclean, the new CSS rule asserted outside any@mediablock. Both pods booted onversion=v0.6.208.The visual pass is yours — sessions clear on redeploy, so I cannot sign in to check it renders as drawn.
Live visual pass done — v0.6.209
Nikola left a signed-in browser session, so I ran the pass myself on files-bao instead of handing it over. Everything from v0.6.208 verified against the live instance, one real defect found and fixed, and one process problem found that matters more than the defect.
Verified live
test5(existing app group)ncbadapplied, borderrgb(168,84,63)=--danger, button disabled, message "A group named "test5" already exists on this instance."cairn-admins(directory group)project-aare(free name)luinp, message cleared, button re-enableddataset.ncSelf="test5", no flag, Rename enabled — the self-exemption worksFall 2TEST5admin1admin1is in both the directory and local lists, and the directory reason won: "would share its identity". The shadow-precedence test was not hypothetical.dirSource: "client"on bao, so correctly not shown; the new copy including "Configure the IdP admin client to resolve groups fully" is present in the bundleThe defect: a disabled button that did not look disabled
The check was switching the primary button off correctly —
disabled === true— whilegetComputedStyle(btn).opacitywas"1". It looked fully clickable and invited the click it would ignore.Cause was scope, not logic. Every
:disabledrule in the page belonged to something else —.pgn[disabled],.uabtn:disabled,#adminview .verfoot button[disabled]..dlgactions buttonhad none, so the state was invisible in every dialog in the app; it had simply never shown before, because nothing disabled a dialog button until v0.6.208. Fixed in v0.6.209 with one rule beside.dlgactions button.danger, the shared family de-scoped in v0.6.203 — same call as then, so it lands once for every dialog. Nowopacity: .5,cursor: not-allowed, confirmed live.The process problem — this one needs your decision
While chasing the above I found that
mainis not a protected branch ("protected": false), so no status check is required to merge.merge_when_checks_succeed: truetherefore gates on nothing; it merges as soon as it next evaluates.Combined with the house release train that is an active race. The train pushes a one-shot workflow file to the PR branch and dispatches it — and that file push is itself a commit that triggers re-evaluation, so the merge fires seconds before the one-shot has done its work:
a8534bf02:33:34fix-552c5e0af3d02:33:49255c75502:58:38fix-552dfaa503802:58:55d78edf102:59:33Consequence: v0.6.208 and v0.6.209 were both tagged from a tree whose tests failed. The failure was
TestI18nKeysDefined— I had built the message key by concatenation,tf("nc" + …), so the scanner saw the keyncand could verify none of the four. Runtime was always fine, which is exactly why only the static check could catch it. Now fixed on main (#564) with one literaltf("…")per branch, and main is green again — verified by a full CI run before merging, not by scheduling the merge and trusting a gate that was not there.Two changes worth making:
mainand require thecicheck. That makesmerge_when_checks_succeedmean what its name says and kills this class of race outright. This is a repo setting — Nikola's call.Still open on this issue
Only the per-row "seen in sign-in tokens" marker from variant D, which I deliberately left out — the notebox states it once for the whole table and repeating a global condition on every row is noise. And the
notebox warnstyling, skipped as cosmetic. Both are yours to overrule; otherwise this can close.v0.6.210 — the
·blobNikola spotted
test5 · via the cairn-admins groupin the directory-accounts meta line. Fixed (PR #565), live on both dogfoods.What it was:
index.htmlline 10466 carried\\u00b7— an escaped backslash followed byu00b7. JavaScript reads that as a literal backslash plus five characters, so the separator printed as its own escape sequence instead of a·. Exactly one occurrence in the file; every other middle dot in the page is either a real·(62) or a correct single escape (98).Where the extra backslash came from. The house one-shots write
index.htmlthrough a Python heredoc, which puts two escaping layers between what I type and what the browser runs. Typed into a non-raw Python string,"\\u00b7"emits the right thing; inside a raw string, or off by one layer, it emits the doubled form and JavaScript renders the escape.This is the third instance of the same mechanism in this feature:
[._\\s-]avInitialssplit on the letter "s", not whitespace —Fall 1→ "FA"tf("nc" + …)\\u00b7Caught by, respectively: a sanity check I happened to write,
TestI18nKeysDefined, and a human reading the screen. The trend is the point — the third had no automated net under it at all.Proposed guard (follow-up, not done here): a
webcheckassertion thatindex.htmlcontains no\\usequence outside a regex literal. One static check, would have caught all three, costs nothing to run. I'd rather add that than rely on noticing.Both pods on
version=v0.6.210. Note the redeploy cleared sessions, so the browser is signed out again.Remaining on this issue is unchanged: only the per-row "seen in sign-in tokens" marker and the
notebox warnstyling, both deliberately skipped and both yours to overrule.mainis now a protected branchThe process hole from the earlier comment is closed. Rule created 2026-09-16 and verified by reading it back through the API:
Push is left enabled and approvals at 0. Direct pushes were never the failure mode, and disabling them would break
go-tidy.yml/protogen.ymlif either is ever dispatched onmain; requiring approvals would block the only maintainer from merging. The rule does exactly one thing: a PR cannot merge until the CI run on that PR is green.Two traps this had to avoid
The context name is event-suffixed. Forgejo records
ci / test-and-build (push)andci / test-and-build (pull_request)as different contexts, and both appear in the settings page's "found in the last week" list. Requiring the(push)variant would have made every pull request permanently unmergeable, since a PR run never emits it."Enforce this rule for repository admins" has to be on. Merges here go through the owner account. With admin bypass left enabled the rule would have been decorative — it would have permitted precisely the merges that caused the problem. The trade-off is that there is no in-the-moment bypass; the escape hatch is editing or deleting the rule on the settings page, which the rule does not block.
Proved, not assumed
Opening a throwaway PR and calling merge immediately, before CI finished:
Before the rule, that identical call merged instantly — that is how #561 and #562 landed red. The test PR was closed unmerged and its branch deleted.
Root cause, restated more precisely than before
I originally described this as a timing race. The timing made it worse, but the real defect was subtler: PR #561's head commit
943d42dhad no status records at all.merge_when_checks_succeedhad nothing to wait for, so it merged at once. A commit with zero checks was being treated as a commit whose checks had passed. Naming a required context is what closes that, not the timing.Remaining verification: the rule is proven to block a non-green merge. That it permits a green one will be confirmed by the next real PR — if it over-blocks, the symptom will be immediate and the rule can be deleted from the settings page in seconds.
v0.6.211 — variant-D round, both decisions plus a real bug
Live on both dogfoods (PR #567). Decisions taken from
variant-d-remaining-two.html: item 1 the lighter option, item 2 the split.The bug Nikola's question found
He asked whether the "configure the IdP admin client" notice would appear on an instance deliberately run without an IdP. The answer was usually not — but only by accident, which is worse than either a yes or a no.
The Groups tab guard was:
It infers "does this instance have a directory?" from "did any group names turn up in the payload?". Two facts break that:
dirSource: "none"is unreachable.KnownDirNamesis assigned unconditionally inmain.goand never nil, so the server only ever reports"client"or"seen". The!== "none"branch is dead code and the only thing hiding the section wasdir.length === 0.dirincludes peering allow-list group names. It iscfg.Auth.AdminGroupsplus every group named in a peering allow-list — and #550's selling point is that allow-lists work with no IdP whatsoever. One stray allow-list name on a local-only instance therefore renders a "Directory groups" table and tells the admin to configure an identity provider they deliberately do not run.The Accounts tab has always asked the right question one screen away —
if (d.authMode !== "local"). The Groups tab now does the same through a pureshowDirSection(authMode, dirSource, dirCount), with ten cases inweb/test/dir-section.test.jsincluding the stray-name case as an explicit regression test. Red witnessed before implementation.Item 1 — the dash says which dash it is
Not the per-row sentence. The defect was never missing prose, it was that a dash in Members reads equally as "no members" and "Cairn cannot tell" — opposite facts, with the explanation below the fold on a paginated list. In degraded mode the header now reads Members (unknown) and each dash carries the reason as a tooltip.
Item 2 — amber means a fault you can fix
guDirErr(directory unreachable) getsnotebox warn.guSeenNote(no admin client) stays plain: per #533 a no-IdP instance is a supported first-class deployment, and a permanent amber banner would be nagging it for a choice it made on purpose.Two tooling notes
Both splices that failed this round failed loudly, before writing anything, and both are the escaping-layer family this issue keeps producing:
—and·I typed into the splice script were converted to real characters in transit, while the file holds the escape sequences. Fixed by rebuilding that line from its own text so the escapes are never retyped — the fourth instance of this class, now in the tooling rather than the product.r.append(nm, cell(shape. Disambiguated onString(.Separately, a post-condition caught that v0.6.210 shipped with no CHANGELOG entry — I had asserted a version that did not exist. Both entries are in now, and the guard checks the captured previous heading rather than a hard-coded version.
Also of note: this was the first PR merged under the new branch protection, and it permitted a green merge normally — so the rule is now confirmed in both directions.
Where #552 stands
Every item from the polish mockup and the variant-D review is shipped. The only thing I would still like to add is the static guard proposed earlier: a
webcheckassertion thatindex.htmlcontains no\\usequence outside a regex literal. Four escaping bugs in this feature, three of which reached a release. Say the word and it is a short round.Closing — every item from both reviews is shipped and live.
Polish mockup (
accounts-groups-polish-decisions.html) — v0.6.206: initial-circle avatars, one badge family for Admin/User/you, "Cairn role" header on the directory table. Item 4 (Used-by) routed to #529, where the share count becomes real once sharing can address a group.Collision preview — v0.6.208: New group, Rename and Add account flag a taken name while typing, with four distinct reasons, the rename self-exemption, and no green tick (the lists are paginated and partial, so "taken" is honest and "free" would not be). v0.6.209 gave dialog buttons a visible disabled state, which turned out to be missing app-wide rather than just here.
Variant-D review — v0.6.211: the lighter fix for ambiguous member counts (
Members (unknown)+ per-dash tooltip), amber reserved for the directory being unreachable rather than for running without an admin client, and the local-mode bug Nikola's question surfaced — a deliberately IdP-less instance could be shown a "Directory groups" table and told to configure an identity provider it does not run.Also fixed along the way:
·rendering as literal text in the account meta line (v0.6.210), the groups filter saying "No accounts match", and the degraded notice never naming what to configure.Out of scope, tracked elsewhere: the phone pass over all four tabs (#389), the Used-by share count (#529), lazy directory member counts (verified working, closed as fine).
One follow-up is being done now rather than filed: a
webcheckguard against doubled unicode escapes inindex.html. Four escaping bugs came out of this feature and three of them reached a release; the guard is cheap and would have caught all four.