Approval gate glyph: a state machine for the gate, zero-dep (#544) - #545
Conversation
Last child of epic #499, built zero-dep instead of on the Rive runtime. The advantage asked for is the GLYPH — a gate whose states are a machine, so the approval lifecycle reads as a mechanism. The objections raised against the Rive child all attach to the RUNTIME rather than that visual: an unreviewable .riv binary, a state machine holding its own copy of governance state, and a canvas nothing in the suite can assert against. Authoring the same machine in SVG + CSS keeps the advantage and loses all three. The shackle carries the meaning: pending shut the gate is closed, awaiting a human approved OPENS + check passage granted consumed SHUTS AGAIN the one attempt was spent; blocking resumed rejected shut + cross refused The consumed row is why this was worth building. A spent one-shot override is back to blocking, and nothing on the card said so as a mechanism — the copy explained it in a sentence and the colour just faded. A shackle that visibly re-closes states it in the way a reader absorbs without reading. Every part is present in every state, undrawn or transparent rather than absent: a part rendered only for its own state would be a new node at the flip, and a new node cannot transition (the #541 Proof Rail lesson). And transition rather than animation means a card that ARRIVES approved is still, while only a real flip animates — motion never claims an event that did not happen. Transform, opacity and stroke-dash only; a browser test asserts the card neither resizes nor moves when the gate opens. aria-hidden, because the pill beside it already states the status in words. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughApproval cards now include an accessibility-hidden SVG gate glyph. CSS maps pending, approved, rejected, and consumed states to distinct gate visuals and transitions. Unit-style and browser tests validate rendering, state changes, motion behavior, geometry, accessibility, and live-update test control. ChangesApproval gate glyph
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ApprovalPage
participant ApprovalCard
participant CSSStateRules
ApprovalPage->>ApprovalCard: render approval status and gate glyph
ApprovalCard->>CSSStateRules: apply approval state class
CSSStateRules-->>ApprovalCard: update shackle, marks, colors, and spent bar
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoApprovals: add SVG gate glyph state machine for approval lifecycle
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/browser/dashboard-544-gate-glyph.test.js`:
- Around line 106-116: Update the setup around the flip evaluation in
dashboard-544-gate-glyph.test.js to stop overriding window.handleGlobalSnapshot,
since the dashboard uses a module-local handler. Use an interception mechanism
shared with the dashboard client, or invoke the client’s render path directly
during setup, so live snapshot rendering is reliably paused before mutating the
approval card.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 814d3e42-123b-42d5-88fd-b4b43f93a1cb
📒 Files selected for processing (4)
src/observability/dashboard/ui/pages/approvals.jssrc/observability/dashboard/ui/styles.jstests/browser/dashboard-544-gate-glyph.test.jstests/hub-gate-glyph-544.test.js
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
CodeRabbit asked whether `window.handleGlobalSnapshot = ...` actually holds
live ticks. Verified in a real browser rather than reasoned about: the client
bundle is a classic <script>, so its top-level function declarations become
window properties, and assigning over one rebinds the identifier the WS
handler calls. Probe returned {onWindow: true, rebound: true} — the override
works and the comment was accurate.
But three suites depended on that non-obvious fact silently. A switch to
<script type="module"> would scope the declaration and turn every one of
those overrides into a no-op, with the tests still passing for the wrong
reason. Extracted `holdLiveTicks(page)` into the shared harness, which
performs the override AND asserts the rebind took effect, and routed all
three suites through it — the #515 lesson that copies drift, applied to a
line whose correctness nobody could see.
Qodo: unguarded querySelector dereferences inside page.evaluate turned any
selector or fixture regression into a TypeError instead of a readable
failure. Named lookups now throw with the selector and the likely cause.
1973/1973 core, 59/59 browser, lint 0 errors.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both findings addressed — and CodeRabbit's turned out to be worth more than its severity suggestedCodeRabbit: does
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/browser/helpers/harness.js`:
- Around line 190-193: Update the handler check in the surrounding harness
helper to store the replacement function in a local variable, assign that same
function to window.handleGlobalSnapshot, and compare both
window.handleGlobalSnapshot and the bare handleGlobalSnapshot result from new
Function against the stored replacement. This must verify that rebinding
actually takes effect rather than accepting an unchanged handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b05e553-1cc9-4650-a255-acb96e505224
📒 Files selected for processing (4)
tests/browser/dashboard-516-micro.test.jstests/browser/dashboard-541-motion.test.jstests/browser/dashboard-544-gate-glyph.test.jstests/browser/helpers/harness.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/browser/dashboard-544-gate-glyph.test.js
#544) CodeRabbit, and it was right at Major. My check read window.handleGlobalSnapshot and the bare identifier and compared them TO EACH OTHER. If the assignment silently failed — a non-writable or getter-only property — both reads return the original, match, and the check passes while the override does nothing. Both reads are now compared against the replacement function itself, and the failure message names which side did not take. Proved rather than asserted. On a global defined non-writable: old check => true (would have passed, blind) new check => correctly failed A guard that cannot fail is not a guard, and this one is meant to protect three suites from silently asserting on state a live tick already replaced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Right, and at the correct severity — fixedMy check read Both reads now compare against the replacement function itself, and the failure message names which side did not take. Proved rather than asserted. Against a global defined non-writable — the exact hole you described: Worth noting for anyone reading later: the property on the real page is a top-level function declaration, so it is non-configurable and cannot be corrupted in place — I had to reproduce the condition on a blank page to demonstrate it. That is also why the hole was invisible: it cannot occur today, only after a packaging change, which is precisely when a silent no-op would do the most damage. 59/59 browser, lint 0 errors. |
2 participants
Closes #544. Last child of epic #499 — the epic completes at 8/8 with this.
Taken on Richardson's call: "if it brings advantages for our ui/ux then lets develop it."
Built zero-dep, and why that is not a substitution
The advantage being asked for is the glyph — a gate whose states are a machine, so the approval lifecycle reads as a mechanism rather than a colour change. Every objection I raised against the Rive child on #499 attaches to the runtime, not to that visual:
.rivbinaryAuthoring the same state machine in SVG + CSS keeps the advantage and loses all three. If Rive-the-tool is wanted for its own sake, the Studio is where those objections don't bite — that stays open as a separate call.
What it does
The shackle carries the meaning:
pendingapprovedconsumedrejectedThe
consumedrow is why this was worth building. A spent one-shot override is back to blocking, and nothing on the card said so as a mechanism — the copy explains it in a sentence, and after #541 the colour merely faded. A shackle that visibly re-closes states it in the way a reader absorbs without reading.Correctness properties, not styling choices
Every part exists in every state, undrawn or transparent rather than absent. A part rendered only for its own state would be a new node at the flip, and a new node cannot transition — so the change would never animate on the one occasion it means something. That is the #541 Proof Rail lesson applied.
transition, never@keyframes. A card that arrives approved is still; only a card that flips animates. Motion never claims an event nobody watched happen. Both directions are asserted.Transform, opacity and stroke-dash only — a browser test measures that the card neither resizes nor moves when the gate opens, because this list repaints and a shift would move the page under the reader's cursor. A unit test enumerates the transitioned properties and rejects anything outside that set.
aria-hidden— the pill beside it already states the status in words. A second announcement would be noise, not access.Verification
git diff --checkclean.continuewhen a rule is missing, so without the guards they would pass on an unstyled glyph. Caught before implementing — I ran the suite and saw them pass green against nothing.Not built
The Rive runtime and any
.rivasset. Empty-state illustrations — declined on #499: our empty states are deliberately informative ("No telemetry yet", "No runs in this scope") and an illustration adds warmth but no information.Summary by CodeRabbit
New Features
Accessibility