fix(app): close the UI/UX critique cycle across all four public workbenches - #64
Merged
Conversation
…on screen An /impeccable critique of the four public workbenches found the same root cause repeated everywhere: a BLOCK finding rendered identically to a PASS finding (same card, differentiated only by an 11px status word), and the risk/decision rail scrolled out of view while reading the evidence it summarized. - Add a shared FindingsList component (severity-sorted, BLOCK gets distinct red chrome, a blocking/warning/passing summary line, and remediation text rendered for every domain instead of only self-hosted) and use it in all four workbenches, replacing four near-duplicate implementations. - Make the airlock status rail sticky on Network/Terraform/Kubernetes so the verdict stays visible while scrolling the evidence below it. - Stop silently substituting a different scenario when `?scenario=` names an unknown id; show a dismissible notice and normalize the URL instead. - Relabel the primary button "Replay evaluated" instead of leaving a disabled control still reading "Run replay". - self-hosted workbench: add self-hosting guide and repo links to the disconnected banner (previously zero outbound links on the page), address the message to the visitor rather than only the operator, wire aria-describedby from the disabled controls to the explanation, and group the intake picker's 15 flat options into per-domain optgroups.
The Terraform and Kubernetes workbenches' "before/after" and "current/
proposed" panels were two independent BoundedJsonBlock calls — the reviewer
had to eyeball-diff two full JSON objects to find what actually changed.
Both critiques flagged this as the top issue in their domain: the diff is
the entire evidence base for an external-diff domain, and asking the human
to spot a 2-line change inside 82 lines of near-identical JSON is exactly
the failure mode the gate exists to prevent.
- Add components/diffLines.ts: a bounded LCS line diff (falls back to plain
before/after rendering above ~400 lines, so a huge fixture can't make this
expensive) and a bounded leaf-value walk that produces a one-line human
summary ("replicas 2 → 3, image v1 → v2") for genuine field-level changes.
- Add components/DiffBlock.tsx: renders the summary plus a marked-up diff
(+/- prefixes, not color alone) with the same disclosure UX as before.
- Wire it into both workbenches' before/after panels, replacing the paired
BoundedJsonBlock calls. Auto-expand the disclosure for destructive
operations (Terraform delete/replace, Kubernetes replace/remove) per the
critique's suggested fix, and give each disclosure an accessible name that
includes the resource address instead of an identical "Inspect values" on
every row.
- No new dependency: both algorithms are small and bounded on purpose.
The Kubernetes public workbench shipped three examples, none of which exercised K8S_PROTECTED_RESOURCE — the one rule in this domain that would genuinely surprise a Kubernetes operator: a resource annotated changesafe.dev/protected: true cannot change its spec at all, not even a single-replica bump, because Kubernetes has no delete operation for protection to key off of. The corpus already proves this (scenarios/kubernetes/scenario-v-protected-config-change), but the workbench's own picker never demonstrated it. Add a fourth fixture, "Protected resource change": a Deployment (demo/pricing-engine) marked protected, with a proposal that touches nothing but spec.replicas (2 -> 3). It blocks with CRITICAL risk, same as the corpus scenario it mirrors. Left the existing three fixtures in place rather than displacing "Large manifest boundary" — that fixture has its own dedicated test coverage for proposal-table pagination bounds that the protected-resource change doesn't otherwise exercise.
The self-hosted workbench rendered its full three-pane app (intake / detail / queue) even with no gateway configured — every control present but dead. The critique called this the worst of both worlds: it doesn't work and it doesn't explain, and it was the single biggest reason the page read as a broken deploy rather than a deliberate public state. Add SelfHostedDisconnectedExplainer, shown in place of the app shell when `transport` is null: what self-hosting adds (OIDC approver identity, server-recomputed findings, signed receipts, ledger inclusion) and what a resolved review's receipt proof looks like, rendered through the real SelfHostedReceiptProof component against a fictional example. The example data is schema-valid (parses through the production ReceiptProofSchema, not a hand-waved shape) but every value is obviously synthetic and the panel is permanently labeled "Example" — this must never be mistaken for a real review, per the project's provenance-honesty invariant. The configured (transport present) path is untouched: the real three-pane app still renders exactly as before, unit-tested to confirm the explainer never leaks into it and vice versa.
A re-critique of all four public workbenches after the first round of UI/UX fixes found that several of them shipped correct-in-logic but incomplete-in-finish, reproducing identically across pages since they share components: - Status badges (BLOCK/WARN/PASS) failed WCAG AA on pages where a card's own tint stacked with the badge's own translucent tint, roughly doubling BLOCK's effective background opacity and dropping it to 3.75:1. Switch StatusBadge from a translucent border+tint to a solid fill with dark text (the same pairing already used on the primary button), which is immune to ancestor-tint stacking by construction rather than by coincidence — BLOCK now measures 5.65:1, WARN 7.50:1, PASS 7.45:1, verified against the app's real computed styles, not estimated. - DiffBlock's +/- markers were aria-hidden, contradicting the component's own comment that the marker "carries the meaning, not color alone" — screen readers heard identical added/removed lines. Pair the glyph with a visually-hidden "added"/"removed" word instead of hiding it outright. - The sticky airlock rail, and the three-column layout it depends on, only engaged at the `xl` breakpoint (1280px), so both phones and 1024-1279px laptops got the pre-fix "verdict scrolls out of view" experience. Lower the grid and sticky breakpoints to `lg` (1024px) across all three public workbenches. - The Kubernetes manifest-diff panel introduced a new defect: a missing `min-w-0` on its containing section/grid/article let a 432px-wide diff card escape containment, dragging the entire page 91px sideways at a true 390px viewport. Complete the min-w-0 chain from DiffBlock's own root up through its ancestors. Verified against the app's live computed styles and DOM at true (CDP- corrected) viewports, not estimated from source alone.
Persists the /impeccable critique history for the four public workbenches across three passes: the initial production-deployment baseline, and two local-dev re-critiques verifying the fix commits in this branch (severity- distinct findings, sticky verdict rail, real before/after diffs, the self-hosted disconnected-state explainer, and the cross-page contrast/ overflow fixes that followed). Scores moved 25->27, 20->26, 21->27, and 20->27/28 across the four surfaces; several issues carried forward unaddressed are recorded here for the next round rather than re-discovered from scratch.
Terraform's mobile diff table caused a genuine 260px page-level horizontal scroll despite being visually clipped inside its own overflow-x-auto container - a table-layout:auto sizing leak invisible to a plain scrollWidth/clientWidth check, closed with contain-layout on the scroll wrapper. The Kubernetes picker never surfaced a WARN/MEDIUM example, making the product's own "a human decides" step structurally unreachable on that page; added reduced-availability (MEDIUM) and mutable-image-tag (HIGH) fixtures. The sticky authority rail had no max-height, hiding the page's safety statement on ordinary short laptop windows once its breakpoint dropped from xl to lg. ActionBadge, the diff add/remove colors, and the self-hosted receipt-proof panel's claim statuses had each drifted from the solid-fill StatusBadge system the rest of the app just adopted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ee61e6906
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
summarizeLeafChanges returned an empty array both when two values were
truly equal and when it gave up after MAX_VISITED_NODES on a large,
still-differing value - DiffBlock rendered "No leaf value differs" in
both cases. It now returns { changes, truncated } so the truncated case
gets an honest "too large to summarize" message instead of a false
equality claim.
Selecting a scenario from any of the three example pickers (Network,
Terraform, Kubernetes) never cleared a prior unknown-scenario-id
notice, so choosing a different example after landing on a bad
?scenario= link left a stale "showing the default example" banner even
though a real, valid example was now selected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
wonkwonlee
added a commit
that referenced
this pull request
Aug 9, 2026
Three tests broke on main after PR #64 merged, each from a mismatch between an already-merged UI change and stale test expectations that predate this session's work: - The Terraform table's diff-inspect summary text grew to include the resource address ("Inspect values for module.boundary.aws_instance. worker_009"), which now substring-matches the same getByText query used for the address cell itself - a strict-mode violation. Scoped to exact: true. - An earlier commit added a pricing-engine Deployment to the shared Kubernetes snapshot (for the protected-resource-change fixture), taking the snapshot from 153 to 154 resources; the e2e assertion was never updated. - The self-hosted workbench's disabled three-pane shell was replaced by an explainer when no gateway is configured, but its e2e test still exercised the old interactive controls (select, disabled buttons) that no longer render in that state. Rewritten to assert the explainer's actual content instead. Verified by diffing against the pre-#64 commit (355a58d), where all three passed - confirming these are real regressions from that merge, not new breakage from anything in this fix. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Closes out the
/impeccablecritique cycle run against the Network, Terraform, Kubernetes, and self-hosted workbenches, ending with four remaining tech-debt gaps found in the final re-critique round:overflow-x-autowrapper — atable-layout:autointrinsic-sizing leak invisible to a plainscrollWidth/clientWidthcheck. Fixed withcontain-layouton the scroll wrapper (verified live: zero achievable scroll at a true 390px viewport with every diff expanded, vs. 260px before).APPROVAL_REQUIRED) path was structurally unreachable on that page — every non-safe example resolved to CRITICAL. Addedkubernetes-reduced-availability(MEDIUM, single WARN) andkubernetes-mutable-image-tag(HIGH, two WARNs) fixtures to the public picker.max-height, so on ordinary short laptop windows (~1100×550–700) it exceeded the viewport and clipped its own safety statement ("Terraform execution — Not performed or observed" / Kubernetes equivalent) off-screen. Addedlg:max-h-[calc(100vh-2rem)] lg:overflow-y-autoacross all three workbench shells.ActionBadgewas still tinted-outline whileStatusBadgehad moved to solid-fill;DiffBlock's add/remove colors reused thetext-pass/text-blockverdict tokens for an unrelated meaning; the self-hosted receipt-proof panel rendered every claim status identically regardless of pass/fail. Unified all three onto the same solid-fill pass/warn/block system, and gave the diff its own dedicated--diff-add/--diff-removetokens.Also includes the six prior commits from this branch's critique-and-fix cycle (badge contrast, sticky rail functional fix, real before/after diffs, self-hosted disconnected-state explainer, protected-resource rule exposure, cross-page defect pass) that had not yet been merged.
Checked GitHub Issues for other open tech debt — only #55 (Terraform command-smuggling scenario gap) is open, which is a separate, already well-scoped domain-policy design question unrelated to this UI work; left as filed.
Test plan
npm run typechecknpm run lintnpm test(1213 passed, 4 skipped)npm run buildnpm run build:cli && changesafe scenario check/scenario gallery --check(no drift)🤖 Generated with Claude Code