Screen wireframes: 15 UX rethinks across the admin app - #261
Merged
Conversation
Two items from the screen-wireframe board. 1y — an error page told the user something broke but gave them nothing to quote in a support report. The correlation id is already on every log line for the request and echoed as X-Correlation-ID; render it too, so the page and the logs can be joined. Adds a shared CopyableId component, since retyping a uuid by hand is the failure mode it exists to prevent. 1g — dashboard module tiles were inert. Each now links to its module's own screen and shows that module's worst health status. Health checks gain a `module` attribution, stamped by the host around register_health_checks so module authors keep the existing add() signature. Link targets are gated client-side against the per-user `menus` prop rather than server-side: the stats payload is process-wide cached for 30s, so any per-user filtering there would leak across sessions.
…upload progress 1t — the executions table had no status counts, so spotting a pile of failed jobs meant paging through the list. Adds a counts strip that doubles as a filter: clicking a tile filters to it, clicking again clears. Counts honour the search box but deliberately ignore the status filter, since the strip is how you pick a status. Failed/stuck go red only when non-zero — a permanent red zero trains people to ignore it. 1s — files could not be searched or filtered by type, and past 20 rows were simply unreachable (the page had no pager at all). Adds filename search, content-type filter with a `image/`-style family option, facet counts drawn from what is actually in the bucket, and the missing pagination controls. Filter clauses are shared between the page query and its count so a filter can never narrow the rows without narrowing the total. Uploads now report per-file byte progress as rows above the table, via XMLHttpRequest — fetch exposes no upload progress events in any current browser. Failed rows persist until dismissed rather than vanishing.
The audit log showed `user_id` as a bare uuid and left `entity_id` unlinked, so a row proved something changed without saying who did it or offering any route to the record. Actors now resolve to a display name (full name, else email) via one batched query per page — an audit page is 50 rows authored by a handful of admins. Deleted accounts keep showing the raw id rather than blanking the row: the id is still the truthful record of who acted. Entity ids link to the owning module's screen through a new AuditLinkRegistry and `register_audit_links` hook, following the existing register_design_packs precedent. Modules declare their own table -> URL mapping, so audit_log never learns about anyone else's routes. Users, Settings and BackgroundTasks register theirs; tables with no per-record screen (join rows, stored files) simply render unlinked, which the registry treats as normal rather than an error. Resolution happens at render time only — the stored row keeps the bare id, which is what makes it durable. Links widen no access: the target route enforces its own permissions.
1n — the user-grants switch reflected only the direct grant, which is correct (it is the only thing the form can change) but it was also the row's only signal. A permission the user genuinely held through a role rendered identically to one they did not hold, with a small badge as the sole clue. Rows now answer two questions separately: a leading indicator says whether the user has the permission at all, and the switch says whether it is granted here. The badge names the granting role, because "inherited" does not tell an admin which role to edit — `inherited_by` maps each key to its sources, and deliberately includes keys that are also direct, so a permission granted both ways cannot look purely direct. 1r — the flags scope was a free-text tenant id, where a typo silently showed an empty scope rather than an error. Replaced with a picker over the tenants that have overrides. The list cannot be closed — the framework has no tenant registry, ids arrive on auth claims — so naming a new tenant by hand stays possible for creating a tenant's first override. The currently-viewed tenant is folded into the options, or it would vanish from its own picker.
The preview was a logo tile and the app name, so the two surfaces branding is most visible on — the sidebar every authenticated page carries, and the site-wide banner — could only be checked by saving and waiting for a reload. The preview now renders both from form state, so it updates as you type. It uses the same near-black `bg-app-sidebar` token as the real shell, so the dark logo variant is judged against the surface it will actually sit on, and it lists the viewer's own sidebar entries rather than invented ones. The sidebar mark is rendered inline instead of through BrandingMark: that component takes its badge colour as a Tailwind class, and the preview has to show whatever hex is currently in the colour field. The logo tile is kept below — the sidebar shows the dark variant, so it is the only place the light-surface logo appears.
…nance 1o — /settings/ was the raw key/value store: a database view, keyed by dotted strings, shown to anyone who clicked "Settings". The per-module forms now own the section root and the store moves to /settings/store. /settings/modules redirects (308) so existing links and bookmarks keep working. 1p — the setting key was free text, and a typo produced a row that looked saved and was silently never read: a failure mode with no feedback at all. The field now suggests every <package>.<field> an installed module declares, and selecting one also fills in its declared value type. It stays free text — a module can read keys this screen cannot enumerate — so an unrecognised key gets an advisory warning rather than a validation error. 1q — a field showed its value and its env var name but never which one was in force. Each field now reports its source (stored override / environment / default), mirroring hydrate_settings' real precedence, and calls out the genuinely confusing case: a stored override silently shadowing a set env var. "Test connection" runs the module's health checks on demand rather than inventing a parallel mechanism, so settings never learns what SMTP or S3 is. Users and FileStorage gain the checks that makes this real: an SMTP session that authenticates and hangs up without sending, and a HEAD for a key that cannot exist. Both re-read live settings on every run rather than pinning the boot-time instance, and both report the reason — "connection refused" and "authentication failed" call for different fixes.
1j — create and invite were separate pages behind separate buttons, so an admin chose between them before seeing what either involved. They take nearly the same inputs and differ in exactly one respect (who sets the password), which makes it a mode switch, not a fork in the navigation. Both old URLs redirect into the merged form with their mode preselected. 1k — folded into that flow rather than into the standalone invite page it replaces. Addresses are pasted as a block (newlines, commas, semicolons) and reported per-address: one already-registered address in a list of twenty must not discard the other nineteen. Repeats are collapsed and addresses lowercased so a pasted column cannot mint two invites for one person. Capped at 100 per submit, so one request cannot mint unbounded live tokens. The copy-link panel appears only when the server says delivery did not happen — the console mailer writes invite URLs to stdout and nowhere else, so an admin could otherwise create an invite with no way to deliver it. A mailer that does not declare itself is assumed to deliver, so a third-party mailer never leaks tokens by omission. Delivery failures also fall back to a link rather than stranding a half-finished invite. 1f — the accept card asked for a password while naming neither the invitee nor the access granted, so a forwarded link was indistinguishable from the right one. It now shows both, and says so when an invite has already been used instead of presenting a form guaranteed to fail. The preview decodes the token read-only: UserManager.verify marks the account verified as a side effect, so routing the preview through it would spend the invite just by looking at the page. A test pins that. 1l — details and roles now share one dirty state and one Save, with an unsaved-changes marker and a navigation guard; only changed sections are sent, so saving a renamed user does not rewrite every role assignment's audit trail. Status changes stay immediate on purpose: disable/enable and mark-verified are actions, not edits, and putting an account lockout behind a Save button would be worse than the inconsistency it removes. Also splits the boot-time module registration loop into _registrations.py to stay under the 300-line cap.
Applied by /code-review --fix:
- SECURITY: POST /settings/test-connection/{package} was on the view router,
which carries no permission dependency. Any authenticated user could force
outbound SMTP AUTH / S3 requests and read raw exception text back
(hostnames, bucket names, credential-failure reasons). Now gated on
PERM_EDIT.
- audit_log imported users.models without declaring simple_module_users;
a standalone install would ModuleNotFoundError at route registration.
- _overrides_by_package re-collected module settings and issued one full
settings-table read per package (~15 per render). Replaced with a single
SettingsStore.all_override_fields() bucketed by key prefix.
- Dashboard tile reachability matched view_prefix exactly, so Users (prefix
/users, menu entry /users/admin) stayed permanently inert. Now resolves the
first menu entry at or under the prefix.
- File search debounce was reset by every upload-progress re-render, so
typing while uploading never fired a search. navigate/applyFilters memoised.
- Upload rows were cleared when router.reload() was issued rather than when it
landed, flashing "No files yet" on a first upload.
- TestConnectionButton hardcoded a path the same change had added to ROUTES.
Fixed here, having been reported and skipped:
- Health checks that reach a third party no longer run on automatic pollers.
/health/ready and the dashboard both ran the new SMTP login and S3 request
on every poll — a k8s probe at 10s intervals means an SMTP AUTH every 10s,
which earns a rate-limit and binds probe latency to someone else's uptime.
Such dependencies are not readiness signals anyway: the app serves pages
fine while its mailer is down. HealthCheck gains `probe`; "Test connection"
still runs everything on demand.
- Bulk invite silently truncated at 100 addresses: paste 150 and the UI said
"100 sent" while 50 people were never contacted. Overflow now comes back as
explicit failed results naming the limit.
- Bulk invite's blanket except kept using a session that a real DB error had
left needing rollback, so one bad row turned every later address into
PendingRollbackError — the opposite of the documented partial success. Now
rolls back before continuing, which is safe because the user manager commits
each invite as it goes.
- The audit log's actor link hardcoded /users/admin/{id}, duplicating what
AuditLinkRegistry owns; it now resolves through the registry and degrades to
plain text when no module claims the users table.
- Edit user kept reporting "Unsaved changes" for details that had already
persisted when the subsequent roles save failed. The dirty baseline now
advances per section, and Discard reverts to what is persisted.
SECURITY — the settings *view* router carried no permission dependency, so any signed-in account could GET /settings/ and read every module's configuration (values, env var names, and the new source/env_set provenance), plus /settings/create's catalog of every registered key. The equivalent GET /api/settings/modules returned 403 for the same user, so the screen and the API disagreed. Settings was the only module not guarding its view routes, and moving the module-settings screen onto the section root widened the exposure. Now RequiresPermission(PERM_VIEW) at the router, with CREATE/EDIT/DELETE on the form actions, mirroring module_api.py. Also fixes a bug the previous pass introduced: rolling back on a per-address failure discarded flushed-but-uncommitted work from *earlier* successful invites in the same request. UserManager.create commits the user row, but the UserRole rows are only flushed — so inviting ["fresh", "taken"] with a role left `fresh` created with no roles and nothing reported. Each invite is now committed before the loop moves on. The old comment claimed "the user manager commits each invite as it goes", which was true only of the user row. Bulk invite no longer takes list[EmailStr]: pydantic rejected the entire body over one malformed address, returning a 422 the UI could only report as a generic failure — so a typo on one line of a pasted column discarded every other address, defeating the per-address contract. Addresses are validated one at a time and malformed ones come back as failed rows. Two low findings reported and skipped upstream, closed here: - The audit log showed "This account no longer exists" for any unresolved actor id, including ids from another id space (celery-worker-1) that never named an account. The copy is now neutral about why it did not resolve. - The add-people screen reported mailer_delivers=true when no mailer existed at all, promising delivery in the one case that certainly cannot deliver.
Applied by /code-review --fix: - Upload rows: clearing on reload kept only errors, so a file dropped while a previous reload was in flight had its in-progress row wiped. The XHR kept running but every patch() was then a no-op — upload with no progress and no completion feedback. In-flight jobs are now preserved. - Retrying a task reloaded executions but not status_counts, so the red "Failed" tile kept its pre-retry number until a full navigation. - bulk_invite treated a None mailer as delivering, so send_invite raised AttributeError and the handler surfaced "'NoneType' object has no attribute 'send_invite'" verbatim in the admin UI — and disagreed with the add-people page, which already reported mailer_delivers: False for that case. - The locale key `scope_other` collided with the CLDR `_other` plural suffix the i18n generator strips, emitting a phantom `feature_flags.browse.scope` key backed by no resource. Renamed to `scope_custom`. - Dashboard tile targets could fall back to a POST-only menu entry (Logout), rendering a GET link to a 405. POST entries are now excluded. - _package_of_module duplicated _module_settings._package_of; the two had to agree exactly or the "Test connection" button silently never appeared. Fixed here, having been reported and skipped: gating /settings/ in pass 2 left its sidebar entry visible to every authenticated account, 403ing on click. MenuRegistry filtered on roles only, so there was no way to express "show this to whoever holds settings.view" — roles=["admin"] would have hidden it from a custom role that legitimately holds the permission while still showing it to admin-adjacent roles that cannot open it. MenuItem gains `permissions`, and get_for_user drops entries whose keys the caller lacks. The middleware already had the expanded permission list one line above the call. Settings and AuditLog now declare theirs — audit_log shipped the same ungated-menu bug before the filter existed. Entries declaring nothing are unaffected, and a caller passing no permissions fails closed.
- Feature Flags shipped the third instance of the ungated-menu bug: its
sidebar entry declared neither roles nor permissions while its view router
guards every route, so any signed-in non-admin saw the entry and got a 403
on click. Now declares feature_flags.view, like Settings and Audit Log.
- The unsaved-changes guard on Edit user only hooked `beforeunload`, which
never fires for an Inertia visit — and "Back to Users", "Cancel", "Manage
permissions" and every sidebar link are Inertia visits. So every ordinary
way of leaving the page silently discarded the edit, which is the exact
accident the merged dirty state exists to prevent. Adds a router.on('before')
confirm, with a savingRef so the page's own post-save reload isn't prompted.
- The file-storage empty state lost its `pagination.total === 0` guard, so
deleting the last file on page 2 — or following a stale ?page=3 link —
rendered "No files yet / Upload your first file" over a full bucket.
- Dashboard tile targets could adopt another module's menu entry: the prefix
fallback did not check whether a different module owned a longer prefix, so
a module at /admin would claim background_tasks' /admin/background-tasks.
- bulk invite now commits after bus.publish. UserInvited handlers run inline
on the same request-scoped session, so a later address's rollback silently
voided the previous invite's handler side effects — contradicting the
"nothing durable is pending" comment the rollback relies on.
- KeyField's 150ms blur timer was never cleared, so navigating away inside
that window set state on an unmounted component.
Applied by /code-review --fix: - The env-provenance badge was inverted. `env_set` tested whether the SM_* *label* was in os.environ, but no settings class on that screen reads env vars: all declare SettingsConfigDict(extra="ignore") with no env_prefix and are built from defaults + DB hydration. A stale SM_USERS_SMTP_HOST therefore badged the field "From environment" while the live value was the pydantic default — exactly backwards from the "why isn't my setting taking effect" question the feature exists to answer. Now derived from the class's own env_prefix, so it is honest today and starts working by itself for any class that declares one. The old test only asserted the flag flipped, never that the value changed, so it passed on a false claim; replaced. - The bulk-invite copy link was built from request.base_url while both mailers use the configured base_url, so behind a proxy without SM_TRUSTED_PROXY the link was the internal origin — and it is surfaced only when mail could not be delivered, i.e. exactly when the admin must pass it on by hand. - The file search box adopted the server value unconditionally: typing "report" fires the debounce at "repo", and the reply landing mid-word rewrote the input and swallowed "rt". - VIEW_BROWSE lacked a trailing slash, so every filter change and page click paid a 307 before the real request. - The bulk-invite body was unbounded: MAX_ADDRESSES caps tokens minted, but validation and the response array are per submitted address. Capped at 1000, far above MAX_ADDRESSES so the over-the-limit outcomes stay visible. Fixed here, having been reported and skipped: both health checks this branch adds are probe=False, and they were the only checks in the tree — so probe_checks was always empty, the new per-module health dot could never render, and /health/ready answered "healthy" from an empty check set, a green light proving nothing. test_every_module_entry_reports_health passed vacuously on "". Adds the host's own database check (SELECT 1, probe-safe), which is the one dependency no request can do without and the right thing for a readiness probe to ask. Also corrected a stale claim: a docstring and test name said the audit log "still renders without the users module installed", which its module-scope `from users.models import User` and declared dependency make unreachable. The None case is a users module that ships no audit link, not an absent one.
Two bugs found by driving the app in a browser. Neither is reachable from
unit tests as written — both need a real login and a rendered page.
BUG-001 (P1) — every admin screen showed raw translation keys
("dashboard.home.title", "Total Users" → "dashboard.home.stats.total_users")
from the moment of login until the user happened to hard-refresh.
Two causes, both fixed:
* host/client_app/i18n.ts applied an incoming catalog only when the *locale*
changed. Audience-scoped catalogs mean the same locale carries a different
payload after login — the anonymous snapshot withholds admin-only modules
— so the catalog arrived and was dropped. A non-null `messages` IS the
server's "you need this" signal; it sends null when the cache is good.
* react-i18next binds to no store events by default, so the subsequent
addResourceBundle updated the store without re-rendering anything that had
already mounted. Set `react.bindI18nStore: 'added'`.
Pre-existing on main (i18n.ts is untouched by this branch) but latent until
audience scoping landed, and it defeated every screen this branch adds.
BUG-002 (P1) — audit entity ids never linked. `snapshot_changes` records
`type(obj).__name__`, but the AuditLink registrations were keyed by
`__tablename__` ("users_user"), so every lookup missed. The failure was
invisible: `entity_link` falls back to showing `entity_type` as the label, so
a User row rendered "User <id>" and looked right while silently never
linking — only the missing anchor gave it away. Registrations now use the
model class name, and the docstring says why. Adds a regression test that
asserts registered keys are class names and that a real audit payload comes
back with entity URLs populated.
Verified in-browser after each fix: dashboard renders "Dashboard" immediately
post-login, and audit rows link both actor and entity to their records.
Round 2 reviewed the QA fixes. No high-severity correctness bugs; the audit-link class-name fix, menu permission filter, settings authz, i18n audience fix and bulk-invite partial success all verified sound. - The audit_links module docstring still said entries store "the table name" and pointed at app.state.audit_links — contradicting the entity_type docstring rewritten in the same commit, and describing exactly the misreading that caused the bug it was fixing. - USER_ENTITY_TYPE re-hardcoded "User" while every module registration had moved to Model.__name__; a rename would have silently unlinked every actor cell, since unmatched lookups degrade to plain text rather than erroring. Now derived from the class, so both ends move together. - bulk_invite's post-publish commit sat outside the per-address try, so an event handler writing something the DB rejects would 500 the whole request and discard every result already collected — the total-failure outcome this endpoint exists to prevent, and the invites are durable by then anyway. - host/client_app/i18n.ts kept `activeLocale` as write-only dead state after the guard was removed, and its docstring still described the removed check. - _module_settings docstrings asserted "every settings class on this screen declares no env_prefix". False — the host's Settings declares SM_ and the module scaffold ships SM_<PACKAGE>_, both of which legitimately report "From environment". Only the comments were wrong; the code handles both. Known and accepted, documented rather than papered over: with both module health checks now probe=False (they reach SMTP/S3 and must not run on a probe timer), the host database check is the only probe-safe one in a default install, and it is owned by the host rather than a module. So the per-module health dot has no data source until a module registers a cheap check of its own, and renders nothing — which is the honest "nothing is watching this" state. Restoring per-module health would mean either polling third parties on a timer (the hazard being avoided) or caching on-demand results, which is a design change beyond this branch.
The existing parametrised check could not catch the raw-key bug this branch fixes: it does `page.goto(path)` after logging in, and a full page load re-bootstraps i18n from scratch — the path that always worked. The broken one is the client-side navigation login itself performs, where the audience changes while the locale does not. This asserts on whatever the app navigated to after login, touching nothing. Verified it fails without the fix (heading renders 'dashboard.home.title') and passes with it.
Both are local verification audit trails (server logs, screenshots, reports), not source. .qa/ was already ignored; .verify/ was not, so a /vf run left an untracked server log in the tree.
Deploying simple-module-python with
|
| Latest commit: |
081243d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9c57842a.simple-module-python.pages.dev |
| Branch Preview URL: | https://worktree-wireframe-rethinks.simple-module-python.pages.dev |
Found while photographing the bulk-invite screen for the wireframe gallery: an already-registered address rendered in red with nothing beside it. `str(UserAlreadyExists())` is the empty string — fastapi-users carries the meaning in the exception type, not its message — and the endpoint passed that straight into `detail`. So the single most common failure was the one case that never explained itself, on the screen built to explain failures. Map it to "Already registered", and fall back to the exception class name for anything else whose `str()` is empty: a class name is a poor message but still a lead, where a blank cell is nothing at all. The existing test asserted only `status == "failed"`, which is what let this ship. The new test asserts on the reason, and was confirmed to fail without the fix. Claude-Session: https://claude.ai/code/session_01QpbArQUekPoF8Svyge6LmB
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
1 participant
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.
Implements every "Rethink" annotation from the
Screen Wireframes.dc.htmldesign board. The board documented 26 already-shipped screens, so its 15 captions were the actual delta.What changed
/settings/leads with per-module forms; the raw key/value store moves to/settings/store(308 from the old path)X-Correlation-IDheaderAlso fixed en route: file storage had no pagination controls at all, so anything past the first 20 files was unreachable.
Two new framework extension points
Both follow the existing
register_design_packsprecedent:register_audit_links— modules declare where their own records live, soaudit_lognever learns anyone else's routes.MenuItem.permissions— the sidebar stops offering entries that 403 on click. Roles alone could not express this:roles=["admin"]hides a screen from a custom role that legitimately holds the permission.Health checks also gained module attribution and a
probeflag, so checks that reach a third party are not run on a readiness-probe timer.Verification
Six code-review passes and a browser QA cycle ran over this branch. Everything found was fixed; the notable ones:
POST /settings/test-connection/{package}was mounted on a router with no permission dependency, letting any signed-in account trigger outbound SMTP/S3 requests and read the raw exception text back. Separately, the settings view router was ungated entirely, so any authenticated user could read every module's configuration — while the equivalent API returned 403 for the same user.main, latent until audience-scoped catalogs landed.__tablename__, but the audit trail records the model class name. Invisible on screen, because an unmatched lookup falls back to showingentity_typeas the label — so rows read "User <id>" and looked correct while never linking.13 of the 15 items were exercised directly in a browser; the other two are covered by unit tests.
make lint(ruff, ty, biome, 13 tsc projects, file-size, metadata, READMEs), 1901 pytest, 48 vitest, production build — all greenpage.goto()after login, which always worked. The new test was verified to fail without the fix.make doctor: 0 errors (its one SM003 warning predates this branch)Known limitation
Both module health checks are
probe=False(they reach SMTP/S3 and must not run on a probe timer), so the host database check is the only probe-safe one in a default install — and it is owned by the host, not a module. The per-module health dot therefore has no data source until a module registers a cheap check of its own, and renders nothing, which is the honest "nothing is watching this" state. Restoring it would mean either polling third parties on a timer or caching on-demand results.Test plan
/settings/as a non-admin and confirm both the screen and the sidebar entry are absent