Approval gate glyph: a state machine for the gate, zero-dep (#544) - #545
Open
richard-devbot wants to merge 1 commit into
Open
Approval gate glyph: a state machine for the gate, zero-dep (#544)#545richard-devbot wants to merge 1 commit into
richard-devbot wants to merge 1 commit into
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. |
Contributor
📝 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, and accessibility. 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}
Contributor
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
Comment on lines
+106
to
+116
| const flip = await page.evaluate(() => { | ||
| window.handleGlobalSnapshot = function () {}; // hold live ticks | ||
| const card = document.querySelector('#page-approvals .approval-card'); | ||
| card.querySelector('.gate-check').__rs544 = 'original'; | ||
| card.className = 'approval-card approved'; | ||
| const check = card.querySelector('.gate-check'); | ||
| return { | ||
| sameNode: check.__rs544 === 'original', | ||
| offsetAtFlip: parseFloat(getComputedStyle(check).strokeDashoffset), | ||
| }; | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the definition and all call sites of the snapshot handler.
fd -t f -e js . src/observability/dashboard/ui | xargs rg -n -C4 'handleGlobalSnapshot'
# Check whether it is assigned onto window/globalThis anywhere.
rg -n -C3 --type=js 'window\.handleGlobalSnapshot|globalThis\.handleGlobalSnapshot'
# Check whether other browser tests already rely on this override.
fd -t f -e js . tests/browser | xargs rg -n 'handleGlobalSnapshot'Repository: richard-devbot/SDLC-rstack
Length of output: 1459
🏁 Script executed:
#!/bin/bash
set -u
echo "== client.js relevant lines =="
sed -n '388,432p' src/observability/dashboard/ui/client.js | cat -n
echo
echo "== all handleGlobalSnapshot references =="
rg -n -C3 'handleGlobalSnapshot' .
echo
echo "== browser test references =="
rg -n -C2 'handleGlobalSnapshot' tests/browser || true
echo
echo "== window/globalThis assignments matching handleGlobalSnapshot =="
rg -n --type-add 'jsfile':\*.js --type-add 'jsfile':\*.mjs --type-add 'jsfile':\*.ts --type-add 'jsfile':\*.tsx \
'(\b(window|globalThis)\s*\.)\s*handleGlobalSnapshot\s*=' . || true
python3 - <<'PY'
from pathlib import Path
import re
paths = [Path(x) for x in Path('.').glob('src/**/*.js') if Path(x).is_file()]
paths += [Path(x) for x in Path('.').glob('**/*.js') if Path(x).is_file()][len(paths):]
for path in Path('.').rglob('*'):
if str(path).startswith(('.git', 'node_modules', 'dist', 'build')) or path.is_dir():
continue
if not path.suffix in {'.js','.mjs','.ts','.tsx'}:
continue
try:
text = path.read_text(encoding='utf-8', errors='ignore')
except Exception:
continue
lines = text.splitlines()
for i,line in enumerate(lines,1):
if 'handleGlobalSnapshot' in line:
print(f'[{path}:{i}]\t{line.strip()}')
if re.search(r'(window|globalThis)\s*\.\s*handleGlobalSnapshot\s*=', line):
print(f'ASSIGNMENT [{path}:{i}]\t{line.strip()}')
PYRepository: richard-devbot/SDLC-rstack
Length of output: 18639
Remove the window.handleGlobalSnapshot override assumption.
handleGlobalSnapshot is a module-local function invoked by the dashboard client through handleGlobalSnapshot(JSON.parse(event.data)), so replacing window.handleGlobalSnapshot does not intercept live snapshot renders. Use an interception mechanism shared by the dashboard and test, or render the client directly from this file during setup.
🤖 Prompt for 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.
In `@tests/browser/dashboard-544-gate-glyph.test.js` around lines 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.
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}
Comment on lines
+52
to
+60
| const card = document.querySelector('#page-approvals .approval-card'); | ||
| const out = {}; | ||
| for (const state of wanted) { | ||
| const clone = card.cloneNode(true); | ||
| clone.className = 'approval-card ' + state; | ||
| card.parentNode.appendChild(clone); | ||
| const shackle = clone.querySelector('.gate-shackle'); | ||
| const style = getComputedStyle(shackle); | ||
| out[state] = { |
There was a problem hiding this comment.
4. Unguarded dom dereferences 🐞 Bug ⚙ Maintainability
The new Playwright suite dereferences querySelector() results inside page.evaluate() (e.g., cloneNode/getComputedStyle) without checking for null, so selector/fixture regressions will fail as TypeErrors instead of actionable assertions. This makes CI failures harder to diagnose and slower to fix.
Agent Prompt
### Issue description
The new browser tests in `dashboard-544-gate-glyph.test.js` directly dereference DOM nodes returned by `querySelector()` inside `page.evaluate()`. If the fixture changes or the DOM structure regresses, the test will throw an unhelpful `TypeError` (e.g., calling `cloneNode` on null) rather than failing with a clear assertion about which element is missing.
### Issue Context
This repo has had prior review guidance to harden browser tests against null DOM lookups so failures are actionable (avoid TypeErrors inside `page.evaluate`).
### Fix Focus Areas
- tests/browser/dashboard-544-gate-glyph.test.js[51-70]
- tests/browser/dashboard-544-gate-glyph.test.js[103-116]
- tests/browser/dashboard-544-gate-glyph.test.js[152-164]
### Suggested change
Inside each `page.evaluate()`:
- Assert required nodes exist before dereferencing (e.g., `if (!card) throw new Error('...')`).
- Optionally add a small helper like `must(sel)` that throws a descriptive error when a selector is missing, and use it for `.approval-card`, `.gate-check`, `.gate-shackle`, `.gate-glyph`, etc.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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.
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