UX review P1+P2: cycle-ordered nav, one save model, feed delivery reasons - #266
Closed
richardmhope wants to merge 3 commits into
Closed
UX review P1+P2: cycle-ordered nav, one save model, feed delivery reasons#266richardmhope wants to merge 3 commits into
richardmhope wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
IcebergAutoReview
Verdict: request_changes
Three blocking correctness regressions remain in the editor and feed behavior.
Findings
- [P1] Unsaved report changes can be lost when advancing the lifecycle. The editor defers autosave for 1.2 seconds (
static/js/tags.js:166-183), while the transition controls remain ordinary navigating forms (templates/report_edit.html:16-20,622-630). Editing and immediately selecting “Submit for review” can unload the page before autosave starts or completes, transition the stale report, and make the draft non-editable. The removed explicit save button was the only deterministic escape. Transition/navigation must await a successful save or block while dirty/saving. - [P1] The feed becomes empty without Alpine after items have been read. Read rows and buckets receive
x-cloak(templates/feed.html:42,49), while global CSS permanently hides cloaked elements until Alpine initializes. On a subsequent visit, every event is already read, so the complete server-rendered feed is hidden when JavaScript is disabled or fails—contradicting the stated no-Alpine fallback. - [P1]
delivery_reason()cannot reliably describe why a historical dissemination event occurred. It derives from mutable current relationships (services/feed.py:27-62): stakeholders can change subscriptions/preferences, and report tags and requirements remain editable after publication. Consequently, a tag or requirement added after delivery can be presented as its cause, while the actual matching tag can disappear. Store the routing reason at dissemination time, preserve the relevant publish-time inputs, or label this as current relevance rather than delivery provenance.
Validation
git diff --check origin/m4/ux-p0...HEAD— passed.- Focused pytest invocation via
uv runcould not start because the read-only workspace prevented creation of.venv. - Focused system
pytestinvocation could not start because FastAPI is not installed in the system environment. Current-head GitHub CI remains the broad green baseline.
Residual risks / optional notes
- None identified.
Automated review by Codex d34dc3cd470d using IcebergAutoReview.
…row (review) `ai_settings.resolve` fail-closes an invalid provider selection to "none" — an OpenAI backend with no API key or model, an Ollama base URL that violates the operator pin. The effective-config tile read `AISettings.backend` straight off the row and called any non-"none" value available, so the page could show a green, enabled AI backend while assist was in fact disabled at runtime. That is exactly backwards for the one page an operator opens to ask "why is AI doing nothing?". The tile now mirrors the resolution: an invalid selection reads "none (selected openai: invalid)" and is not ok. An advisory names the provider and every reason it was rejected, so the red pill comes with the fix. Also closes the review's residual note: SECRET_FIELDS is a manual allowlist while the row set is built automatically, so a future `*_api_key` would be rendered in full unless someone remembered to classify it. A test now asserts every secret-looking Settings field is classified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, /admin hub, notebook phases Implements the four P0 items from the Claude Design UX handoff (design_handoff_iceberg_ux/README.md) as template + CSS + Alpine changes on the existing stack. No route removed, no asset pin touched, no new design tokens. Editor · markings promoted to the header. Intel level, TLP and analytic confidence move out of the Citations dock footer into always-visible chips, so a product's markings are one source of truth and are legible while writing. Each chip lays its real <select form="reportform"> transparently over itself: a click opens the native dropdown, so it stays keyboard/screen-reader navigable and renders correctly before Alpine hydrates. The component only mirrors the chosen option's own text back into the chip, so there is no label map to drift from the enums. Dashboard · actionable. Every KPI card links to the work behind it, and a "Needs you now" queue leads: the review queue (for anyone who can review) ahead of the viewer's own drafts, capped at five. Both halves are existing lifecycle state surfaced as actions — no new model. Notebook creation is demoted to a collapsed <details> so content leads. Admin · Settings & integrations hub at GET /admin. One tile per configurable subsystem with a live status pill, grouped Outbound integrations / Governance. Status comes from effective_config.admin_hub_tiles(), which reads the same settings singletons the config pages edit, so the hub cannot disagree with the page it links to. The pill separates OFF (opt-in integration disabled) from NOT CONFIGURED (enabled but missing an endpoint or its env-only secret) — the state an operator most needs flagged. Secrets are consulted as set/not-set only. Notebook · phase tabs. The nine stacked sections become Collect / Analyze / Produce / Trace, one phase at a time, with the collection strip re-cut as the tab bar. Nothing is gated: the tabs are real #section anchors, Collect is the server-rendered default, and a <noscript> rule cancels x-cloak so a browser without Alpine still gets every section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… feed delivery reasons
The second half of the Claude Design UX handoff, on top of the P0 change set.
Same routes, same forms, same Alpine bindings — reorganisation only.
Nav · ordered by the intelligence cycle. Analysts get Workspace · Collect
(notebooks, feed reader, tasking) · Produce (reports) · Discover; Tasking moves
out of Administration, where it never belonged. The admin rail's eleven links
collapse to four, with the six integration/governance consoles behind the /admin
hub. Nothing becomes unreachable: every collapsed console is still a ⌘K jump
target, and a new test walks each role's palette and asserts every destination
resolves. "Matrix" becomes "ATT&CK coverage", and the palette trigger gets its
own glyph — it and Search were both magnifiers, which read as one feature.
Editor · one save model, one place to publish. "Save draft" is gone: the editor
already autosaved, so two controls reported one thing. A <noscript> submit keeps
the form usable if Alpine never loads. The dock tabs are verb-labelled and kept
to four — Cite / Classify / Link / Publish (plus Assist, conditional on AI being
enabled). Link merges the requirement and audience forms; Publish holds the
lifecycle stepper and the rendered PDFs, with the one available transition pinned
in the dock footer, replacing the separate subhead stepper + submit row. The
repeated per-tab ARIA wiring becomes a single Jinja macro.
Notebook · every add-form sits behind a consistent "+ Add …" <details>, opened by
default only when its section is empty. Native, so it costs no JS and no CSP
surface.
Feed · every row says why it reached you — own requirement ("Answers your RFI") →
tag subscription → audience group → level preference. The reason is re-derived
from the same rules dissemination.matched_stakeholders routes by rather than
stored on the event, so there is no second copy to drift. Rows bucket into Today
/ This week / Earlier server-side and an All ⇄ Unread pill filters them
client-side. A row answering your own requirement offers "Mark satisfied →",
which deep-links to the product's #feedback card with that requirement
preselected — deliberately not a one-click POST, because the feedback endpoint
requires a usefulness rating and inventing one would put a verdict in the
reader's mouth. The ?requirement= parameter only preselects a requirement the
reader owns and the report answers.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
IcebergAutoReview
Verdict: request_changes
The follow-up delta only changes effective-configuration reporting; all three prior blocking areas are unchanged and remain unresolved.
Findings
- [P1] Unsaved report edits can still be lost during lifecycle transitions. Autosave waits 1.2 seconds (
src/iceberg/static/js/tags.js:166-204), but transition controls remain independent navigating forms that neither awaitsaveNow()nor block while dirty/saving (src/iceberg/templates/report_edit.html:16-20,622-630). An immediate submit can discard edits; publishing can freeze the stale version permanently. - [P1] The no-Alpine feed fallback still hides read items. Read rows and all-read buckets receive
x-cloak(src/iceberg/templates/feed.html:42,49), while global CSS applies[x-cloak] { display: none !important; }(src/iceberg/static/css/iceberg.css:68). If JavaScript is unavailable, a previously read feed can render empty despite All being the claimed default. - [P1] Delivery reasons still claim historical causation from mutable current state.
delivery_reason()examines current requirements, subscriptions, tags, groups, and preferences (src/iceberg/services/feed.py:27-65), while dissemination records only report and stakeholder IDs (src/iceberg/services/dissemination.py:75-95). Requirements are not even part ofmatched_stakeholders()routing, and post-delivery relationship changes can replace or invent the displayed reason. Persist publish-time routing provenance or describe this explicitly as current relevance rather than why delivery occurred.
Validation
git diff d34dc3cd470dc5d4c89aaec5747ac4eef30de86f..HEADandgit diff --check— passed; delta contains only effective-config code and tests.git diff --quietacross the four prior-blocker files — confirmed they are unchanged from the previously reviewed head.git diff --check origin/m4/ux-p0...HEAD— passed.- Focused effective-config pytest selection could not start because
uvcould not create.venvon the read-only workspace; current-head GitHub CI remains the authoritative green baseline.
Residual risks / optional notes
- None identified.
Automated review by Codex 008e93bc18e6 using IcebergAutoReview.
Collaborator
Author
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.
The second half of the Claude Design UX review. Same routes, same forms, same Alpine bindings — reorganisation only.
Nav — ordered by the intelligence cycle
Analysts get Workspace · Collect (notebooks, feed reader, tasking) · Produce (reports) · Discover. Tasking moves out of Administration, where it never belonged.
The admin rail's eleven links collapse to four, with the six integration/governance consoles behind the
/adminhub. Nothing becomes unreachable: every collapsed console is still a ⌘K jump target, and a new test walks each role's palette and asserts every destination resolves."Matrix" becomes "ATT&CK coverage". The ⌘K trigger gets its own glyph — it and Search were both magnifiers, which read as one feature.
Editor — one save model, one place to publish
"Save draft" is gone: the editor already autosaved, so two controls reported one thing. A
<noscript>submit keeps the form usable if Alpine never loads.Dock tabs are verb-labelled and kept to four — Cite / Classify / Link / Publish (plus Assist, conditional on AI being enabled). Link merges the requirement and audience forms; Publish holds the lifecycle stepper and the rendered PDFs, with the one available transition pinned in the dock footer — replacing the separate subhead stepper + submit row. The repeated per-tab ARIA wiring becomes a single Jinja macro.
Notebook — consistent add-forms
Every add-form sits behind a
+ Add …<details>, opened by default only when its section is empty. Native, so it costs no JS and no CSP surface.Feed — why it reached you, and how to close the loop
Every row now says why it was delivered: own requirement ("Answers your RFI") → tag subscription → audience group → level preference. The reason is re-derived from the same rules
dissemination.matched_stakeholdersroutes by, rather than stored onDisseminationEvent— no schema change, and no stored copy that can drift from the rule that actually applied.Rows bucket into Today / This week / Earlier server-side; an All ⇄ Unread pill filters them client-side (All is the default, so the server render is complete without Alpine).
A row answering your own requirement offers "Mark satisfied →", which deep-links to the product's
#feedbackcard with that requirement preselected.One deliberate deviation from the handoff
The design asked for an inline POST here.
POST /reports/{id}/feedbackrequires ausefulnessrating, so a one-click version would have to fabricate a rating the reader never gave. The deep-link uses the same endpoint and costs one extra click, but the recorded feedback is theirs. The?requirement=parameter only ever preselects a requirement the reader owns and the report answers — a test pins that it cannot surface anyone else's.Verification
Full suite green (612 passed, 11 skipped) plus
ruff/bandit/vulture/mypy/djlint/biome. New tests cover the rail's cycle ordering, the four-link admin rail, every ⌘K destination per role, the editor's single save path and four-tab dock, and each delivery-reason branch. Also smoke-run against the real app per role.🤖 Generated with Claude Code