fix(ui): regroup the rail by privilege, unify breadcrumbs, one search box#315
Merged
icebergai-review-bot[bot] merged 8 commits intoJul 20, 2026
Merged
Conversation
… box
Seven scoped UX fixes from the review in handover/. No restyling, no new CSS
rules, no IA change beyond the rail split; all classes already ship.
Navigation
- The single "Admin" rail group held per-user settings (alerts, API keys, help)
alongside the conditionally-rendered admin-only pages, so the heading was wrong
for regular members and the two privilege levels looked identical. Split into
Account (everyone) and Administration (gated on is_admin).
- iceberg.css shipped .topbar-crumb/.crumb-* with no markup rendering them, while
extension_detail and account each hand-rolled a different inline crumb and the
dashboard had none. Render one crumb in the topbar via {% block crumb %} and
delete the two bespoke ones.
Dashboard
- The page carried its own search form submitting to the same place as the
top-bar box. Removed it in favour of the top bar plus a clear-chip. That form's
hidden inputs were what preserved store/risk/sort across a search, so
topbar-search.js now forwards the existing query string instead of hard-coding
/?q= — otherwise searching would silently reset the active filters.
- Store and risk filters read as one nine-pill strip once the row wrapped and the
divider was lost; each axis now sits in its own .label-cap-prefixed group.
- Rows navigate on click but only the Refresh button said so; added a muted
trailing chevron, outside the @click.stop group so the row click still fires.
Extension detail
- Delete sat flush with Refresh/Store page; isolated it behind the same divider
the watchlist switch already uses.
- The per-signal note lives in a `hidden md:block` column, so below md the six
score bars carried no explanation; carried it into a native title too.
Tests: tests/test_nav_structure.py covers the rail groups per role (a member sees
Account and none of the admin links), the single search input, the clear-chip,
and the shell breadcrumb. Verified failing against the pre-change templates.
Browser-driven against the dev app: all assertions pass, zero console errors.
- help.html: search moved to the top bar, so the dashboard blurb now says where
it is (and that it preserves the active filters/sort) instead of listing
"searched" among the list's own controls.
- CHANGELOG: the seven UI fixes under Changed.
- .claude/rules/frontend.md: a new "Shell contracts" section, because two of
these are now invariants a future page template can break silently — the rail
groups split by privilege (admin links stay inside the is_admin guard), and
{% block crumb %}, where OMITTING the block inherits the default "Dashboard"
leaf with no error. Also records that topbar-search.js's query-string
forwarding is what preserves filters now that the in-page form is gone.
…ducible All five shots were taken before the docs site shipped and showed a UI that no longer exists: the retired "Admin" rail group, the removed in-page dashboard search box, the ungrouped filter strip, no breadcrumb — plus a rail tagline that changed back in #272. Retaken against the current shell, same geometry (1440x900 @ DPR 2 = 2880x1800), written to docs/screenshots/ and copied into website/docs/assets/ so the two can't drift. Also adds scripts/screenshots/, because these went stale silently and the demo dataset had to be reverse-engineered from the old PNGs to retake them: - seed_demo.py builds the synthetic workspace (no store is contacted). Each cached install_footprint is backed by real InstallObservation rows, so the dashboard's Top-exposure ranking agrees with the detail page's org-footprint breakdown instead of contradicting it. - shoot.py drives the five pages and exits non-zero on any console/page error, so a CSP violation or a dead Alpine component fails the run rather than being quietly baked into an image. - README.md documents the process and flags that nothing gates these: a shell change means refreshing them in the same PR.
![icebergai-review-bot[bot]](https://avatars.githubusercontent.com/u/290878036?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
IcebergAutoReview
Verdict: request_changes
The UI changes appear sound, but the new screenshot tooling has a destructive data-loss risk and a portability defect.
Findings
- [P1] scripts/screenshots/seed_demo.py:289-298 selects the first admin and deletes all of that user’s alert rules, destinations, and extensions. This is not limited to prior demo rows, and extension deletion cascades into history and inventory data. Use a dedicated demo user/database or uniquely mark and delete only seeded records, with a fail-closed development-database guard.
- [P2] scripts/screenshots/shoot.py:15-17 hard-codes
/home/richard/Projects/marvin. The documented regeneration command therefore writes outside this checkout, usually fails for other contributors, and could overwrite files in an unrelated directory. Derive the repository root from__file__instead.
Validation
- Inspected the complete main...HEAD diff and affected path-scoped instructions.
- Ran
git diff --check main...HEADsuccessfully. - Verified every updated
docs/screenshots/*.pngis byte-identical to its correspondingwebsite/docs/assets/copy. - Relied on the reported fully green current-head GitHub CI as the broad validation baseline.
Residual risks / optional notes
- The new query-preservation behavior in
topbar-search.jsis only described as manually browser-tested; the committed tests validate markup but do not directly exercise preservation of store/risk/sort parameters.
Automated review by Codex ca013fae740e using IcebergAutoReview.
…(bot review #315) Both findings were real. [P1] seed_demo.py cleared ALL of the first admin's extensions, destinations and rules — not just its own rows — and deleting an Extension cascades into its FetchLog, InstallCountHistory, InstallObservation, AlertRule and AlertLog history. A README warning is not a control. Now: - deletion is scoped to the exact (store, extension_id) pairs the script creates and its single destination label, via tuple_().in_() rather than raw SQL; - the target database is guarded fail-closed: it refuses unless ICEBERG_EBS_SCREENSHOT_SEED=1 is set AND the resolved host is local. Verified by seeding alongside an unrelated extension + destination owned by the same admin: both survived, and the demo rows were replaced (13) rather than duplicated (26), so it stays idempotent. Both guards confirmed to refuse. [P2] shoot.py hard-coded an absolute path from the machine it was written on, so the documented command would write outside another contributor's checkout. The repo root now comes from __file__, with an existence check on both output directories. Verified by running it from /tmp. Residual note from the review: the topbar-search filter-preservation behaviour was only manually browser-tested. Added e2e/test_ui.py::test_topbar_search_preserves_ active_filters — it asserts store/sort/order survive a search and that page resets, which only a browser can prove since the logic lives in the JS keydown handler. Screenshots regenerated (unchanged in content; the canonical and website copies remain byte-identical).
Contributor
There was a problem hiding this comment.
IcebergAutoReview
Verdict: request_changes
The hard-coded screenshot path is fixed, but the prior destructive data-loss blocker remains unresolved.
Findings
- [P1] scripts/screenshots/seed_demo.py:324-340 still does not uniquely identify seeded records. DEMO_KEYS contains popular real extension IDs, so running this against an admin who already watches one deletes that legitimate Extension and cascades through its history, observations, alert rules, and logs. Likewise, any existing destination named "SOC Slack #ext-alerts" is deleted with its rules. The opt-in/local-host checks reduce accidental execution but do not make these rows script-owned; use a dedicated demo user/database or a durable seed marker and refuse collisions.
Validation
- Inspected the complete ca013fa..HEAD follow-up diff and affected model constraints/cascades.
- git diff --check ca013fa..HEAD passed.
- Verified the three changed docs screenshots are byte-identical to their website asset copies.
- Verified shoot.py resolves the current repository root from file; the prior portability blocker is fixed.
- Relied on current-head green GitHub CI for broad validation.
Residual risks / optional notes
- The added browser test now directly covers preservation of active search filters, resolving the prior optional test-coverage note.
Automated review by Codex 662e8a7d7ea5 using IcebergAutoReview.
, round 2) The bot was right that my first fix did not resolve the P1. Scoping deletes by (store, extension_id) is not ownership: DEMO_KEYS holds REAL store IDs — uBlock Origin's actual Chrome ID, ms-python.python, and nine more — and this is an extension-tracking app, so a developer's database plausibly already watches one. The "scoped" delete would still have destroyed a legitimate Extension and cascaded through its fetch history, inventory observations, rules and alert log. Same for any destination that happened to be labelled "SOC Slack #ext-alerts". Ownership is now structural rather than inferred: - All demo data belongs to a dedicated account (`demo`, overridable via ICEBERG_EBS_SCREENSHOT_DEMO_USER). The real admin account is never read or written, so no amount of overlap with real extension IDs can reach it. - If that username already exists and owns anything outside the seed set, the script REFUSES and names the offending row instead of assuming it is ours. - Deleting everything that account owns is then safe by construction. - The demo password has no committed default (it is a real admin login) — both scripts read it from the environment, and seeding fails closed without it. Verified end to end: a clean seed creates the account; a re-seed stays idempotent (13 extensions, not 26); and planting a non-demo extension under the demo username makes the script abort with that row named — with the row still present afterwards. Screenshots retaken (the rail now shows the demo account); canonical and website copies remain byte-identical.
Contributor
There was a problem hiding this comment.
IcebergAutoReview
Verdict: request_changes
The follow-up introduces a dedicated demo username, but still cannot prove that a pre-existing account is script-owned. The prior destructive-data blocker therefore remains, now with an account-takeover path.
Findings
- [P1] scripts/screenshots/seed_demo.py:346-398 —
_ensure_demo_usertreats any existing account with no non-demo extensions or differently labelled destinations as script-owned. An ordinary local or SSO user nameddemowith no such rows passes; the script overwrites its password, potentially enabling password login for an SSO identity, then deletes any legitimate extensions whose IDs overlapDEMO_KEYSand the matching destination. It also ignores other ownership signals such as API keys. Refuse every unmarked pre-existing account and recognize reruns through a durable seed marker, or require a dedicated disposable database.
Validation
git diff 662e8a7d7ea5e3c50a0ca82b1e63f609bed04bf6..HEADreviewed with affected model and authentication context.git diff --check 662e8a7d7ea5e3c50a0ca82b1e63f609bed04bf6..HEADpassed.- Verified all five changed documentation screenshots are byte-identical to their website asset copies.
- No broad suites rerun; current-head green GitHub CI was accepted as the authoritative baseline.
Residual risks / optional notes
- Unchanged UI areas from the prior reviewed head were not revalidated, per follow-up review scope.
Automated review by Codex 835039ad3617 using IcebergAutoReview.
… ownership Third round on the same P1, and the bot was right each time. My previous fix introduced an account-takeover path while trying to fix data loss: _ensure_demo_user treated any existing account with no non-demo rows as script-owned, so an ordinary user named `demo` with no extensions passed the check and had their password overwritten — and for an SSO account (password_hash IS NULL) that would have ENABLED password login on an SSO identity. It also ignored other ownership signals such as API keys. The root error was trying to infer ownership at all. Two attempts failed for the same reason: the seed uses REAL store IDs, so a matching (store, extension_id) proves nothing about who created the row, and no property of an existing account proves the script created it either. So ownership is no longer inferred. seed_demo.py now requires a database whose name contains "screenshot" — one that exists for this and nothing else — on top of the existing opt-in and local-host guards, all fail-closed. Everything in such a database is expendable by construction, so the script: - never creates a user, never writes a password, never reads an account to guess its provenance (the whole _ensure_demo_user function is gone); - deletes extensions and alert config wholesale, which is now sound; - leaves the real dev database untouched — verified: it refuses `iceberg_ebs` by name with instructions. Screenshots regenerated through the documented path against a freshly created iceberg_ebs_screenshots database; canonical and website copies byte-identical. README rewritten with the two extra setup commands and why the requirement exists.
…ved tag zizmor's online ref-version-mismatch audit began failing lint-workflows: the SHA pinned in docs.yml is now reachable specifically from the freshly-published actions/checkout v7.0.0 tag, so the bare `# v7` comment no longer matches. This is upstream tag drift, not a change on this branch — main's last CI run predates the tag and would fail the same audit if re-run. docs.yml was the only checkout pin still commented `# v7`; the other ten pins of the identical SHA already say `# v7.0.0`. Normalised it to match zizmor's named tag and the rest of the repo. Verified: `uvx zizmor@1.27.0 .github/workflows/` reports no findings.
Contributor
There was a problem hiding this comment.
IcebergAutoReview
Verdict: request_changes
The prior destructive-data/account-takeover blocker is resolved, but the documented screenshot workflow now fails because the browser process does not receive the admin credentials.
Findings
- [P2] scripts/screenshots/README.md:31-48 —
source .envcreates shell variables but does not export them.shoot.pynow requiresICEBERG_EBS_ADMIN_USERNAMEandICEBERG_EBS_ADMIN_PASSWORDthroughos.environ, so following the documented commands ends with aKeyErrorbefore login. Export the dotenv variables (for example withset -aaroundsource .env) or explicitly pass both variables to the Python command.
Validation
- Reviewed
git diff 835039ad3617d150e09eb8b5ee265a9c1a23d644..HEADand affected model/auth/Compose context. git diff --check 835039ad3617d150e09eb8b5ee265a9c1a23d644..HEADpassed.- Secret-safe clean-shell probe confirmed both required admin variables remain unexported after
source .env. - Compiled both changed screenshot Python scripts successfully.
- Verified all five screenshots are 2880×1800 and byte-identical to their website asset copies.
Residual risks / optional notes
- Unchanged UI areas from the prior reviewed head were not revalidated, per follow-up scope.
- The screenshot scripts remain outside the repository's configured CI lint/test paths.
Automated review by Codex a2d716f7f95b using IcebergAutoReview.
bot review #315 [P2]: the README's `source .env` sets shell variables but does not export them, so shoot.py — which reads ICEBERG_EBS_ADMIN_USERNAME/PASSWORD through os.environ — would KeyError before login when following the steps as written. Wrapped the source in `set -a` / `set +a` so the child Python process inherits them. Verified in a clean `env -i` shell: all three required vars reach os.environ after the change.
Contributor
There was a problem hiding this comment.
IcebergAutoReview
Verdict: approve
The prior blocker is resolved. The documented set -a; source .env; set +a sequence exports the screenshot credentials so shoot.py can read them from os.environ. No regressions were introduced by the follow-up change.
Findings
- No blocking findings.
Validation
- Reviewed
git diff a2d716f7f95b8a30f9e409606e21471655cee5c2..HEAD; onlyscripts/screenshots/README.mdchanged. - Confirmed the prior reviewed SHA is an ancestor of HEAD.
git diff --check a2d716f7f95b8a30f9e409606e21471655cee5c2..HEADpassed.- Secret-safe clean-shell probe confirmed variables loaded under
set -aremain exported afterset +aand are visible to a child Python process.
Residual risks / optional notes
- The workspace has no configured
.env, so the exact local dotenv file could not be sourced; the shell export behavior was verified with synthetic values matching the documented variable names.
Automated review by Codex 1de51dd047fe using IcebergAutoReview.
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.
Seven scoped UX fixes from a UI review, plus the doc/screenshot alignment they made
necessary. No restyling of the fixed family palette/type, no new CSS rules, and no
IA change beyond the rail split — every class used already ships.
Navigation
Adminrail group held per-user settings. Alerts & webhooks, API keys and Helpwere shown to every signed-in user under a heading labelled Admin, while the genuinely
admin-only items (Users, Proxy, SSO) were conditionally rendered inside the same
block — so the label was wrong for regular members and the two privilege levels were
visually indistinguishable. Split into Account (everyone) and Administration
(wrapped as a whole in
is_admin).iceberg.cssalreadyshipped
.topbar-crumb/.crumb-*— dead CSS, because nothing rendered it. Meanwhileextension_detail.htmlandaccount.htmleach built a different inline crumb and thedashboard had none. One crumb now renders in the shell; pages set only the leaf via
{% block crumb %}.Dashboard
both submitted to
/?q=…. The in-page form is gone; an active query shows as aclear-chip. That form's hidden inputs were what carried
store/risk/sortthrougha search, so
topbar-search.jsnow forwards the existing query string instead ofhard-coding
/?q=— without that, removing the form would silently reset the user'sfilters on every search. This is the one behavioural change in the PR.
the 1px divider was lost. Each axis now sits in its own
.label-cap-prefixed group.chevron, outside the
@click.stopgroup so the row's own@clickstill fires.Extension detail
Delete), same size and spacing. Isolated behind the divider the watchlist switch already
uses.
deleteExtalready confirms, so no behaviour change.md— the per-signal explanation lives in ahidden md:blockcolumn, so on a laptop split-view six bars appear with nothing sayingwhat each measures. Carried into a native
title(adapted to the post-UI re-inlines single-source constants: detail-template permission-tier lists (already drifted) and risk-band thresholds #281risk_rowsshape).
Docs & screenshots
help.html: search moved, so the dashboard blurb now says where it is..claude/rules/frontend.md: a Shell contracts section — two of these are nowinvariants a future page template can break silently. Notably, a page that omits
{% block crumb %}inherits the default leaf "Dashboard" with no error.Admingroup, the removedsearch box, no breadcrumb, and a rail tagline that changed back in chore(ui): tagline → 'EBS Control' in the rail and on login #272. Retaken at the
same geometry, written to
docs/screenshots/and copied towebsite/docs/assets/.scripts/screenshots/(seed + shoot + README), because these went stale silentlyand the demo dataset had to be reverse-engineered from the old PNGs.
shoot.pyexitsnon-zero on any console error, so a CSP violation can't be baked into an image.
Verification
tests/test_nav_structure.py(7 tests): rail groups per role (a member seesAccount and none of the admin links), exactly one search input, the clear-chip, and one
crumb per page with the right leaf. Confirmed failing against the pre-change
templates.
ruff check/format --check/mypy appclean.errors (the CSP backstop), including that search still preserves
store/sortandthat a row click still opens the detail page through the new chevron cell.
🤖 Generated with Claude Code