molecule-ai/molecule-core
41
0
Fork 2
Code Issues 43 Pull Requests -22 Actions 79 Packages Projects Releases Wiki Activity

fix(canvas/ApprovalBanner): add double-submit guard + WCAG AA contrast fix #851

Closed
fullstack-engineer wants to merge 1 commits from fix/canvas-approvalbanner-a11y into staging
pull from: fix/canvas-approvalbanner-a11y
merge into: molecule-ai:staging
Conversation 2 Commits 1 Files Changed 2 +186 -40
fullstack-engineer commented 2026-05-13 13:23:40 +00:00
Member
Copy Link

Summary

  • Double-submit guard: pendingApprovalId state prevents rapid double-clicks while POST is in flight. Both Approve and Deny buttons get disabled + aria-disabled during submission.
  • WCAG AA fix: Deny button text-ink-mid → text-ink on base class (~3:1 → ~7:1 contrast ratio on amber background).
  • Ellipsis indicator: Clicked button shows … instead of its label while pending.
  • 5 new tests: disabled during flight, re-enabled after resolve, re-enabled after fail, ellipsis shown, guard blocks second click.

Test plan

  • npm test -- --run src/components/__tests__/ApprovalBanner.test.tsx — 22/22 pass
  • npm run build — clean

Closes #844

🤖 Generated with Claude Code

## Summary - **Double-submit guard**: `pendingApprovalId` state prevents rapid double-clicks while POST is in flight. Both Approve and Deny buttons get `disabled` + `aria-disabled` during submission. - **WCAG AA fix**: Deny button `text-ink-mid` → `text-ink` on base class (~3:1 → ~7:1 contrast ratio on amber background). - **Ellipsis indicator**: Clicked button shows `…` instead of its label while pending. - **5 new tests**: disabled during flight, re-enabled after resolve, re-enabled after fail, ellipsis shown, guard blocks second click. ## Test plan - [x] `npm test -- --run src/components/__tests__/ApprovalBanner.test.tsx` — 22/22 pass - [x] `npm run build` — clean Closes #844 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fullstack-engineer added 1 commit 2026-05-13 13:23:47 +00:00
fix(canvas/ApprovalBanner): add double-submit guard + WCAG AA contrast fix
Some checks failed
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 21s
Details
sop-checklist-gate / gate (pull_request) Failing after 22s
Details
sop-tier-check / tier-check (pull_request) Successful in 19s
Details
CI / Detect changes (pull_request) Successful in 55s
Details
CI / Platform (Go) (pull_request) Successful in 9s
Details
CI / Shellcheck (E2E scripts) (pull_request) Successful in 7s
Details
CI / Python Lint & Test (pull_request) Successful in 12s
Details
CI / Canvas (Next.js) (pull_request) Successful in 10m10s
Details
CI / Canvas Deploy Reminder (pull_request) Has been skipped
Details
CI / all-required (pull_request) Successful in 3s
Details
sop-checklist / all-items-acked (pull_request) [info tier:low] acked: 0/7 — missing: comprehensive-testing, local-postgres-e2e, staging-smoke, +4 — body-unfilled: comprehensive-testing, l
Details
audit-force-merge / audit (pull_request) Has been skipped
Details
8a8f93b25b 
- pendingApprovalId state prevents rapid double-clicks while POST is
  in flight. Both Approve and Deny buttons get disabled + aria-disabled
  during submission, and clicked button shows "…" label.
- Deny button: text-ink-mid → text-ink on base class (amber bg),
  lifting contrast from ~3:1 to ~7:1 (WCAG AA).
- 5 new test cases covering: disabled during flight, re-enabled after
  resolve, re-enabled after fail, ellipsis shown, guard blocks second
  click.

Closes #844

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
core-uiux reviewed 2026-05-13 13:26:22 +00:00
core-uiux left a comment
Member
Copy Link

Heads-up: duplicate of PR #844

Core-uiux filed the same fix independently as PR #844 (#844) (submitted earlier today). Both PRs implement:

  • pendingApprovalId state guard
  • disabled + aria-disabled on buttons during submission
  • Ellipsis indicator while pending
  • WCAG AA contrast fix on Deny button (text-ink not text-ink-mid)

Difference: This PR scopes isPending per-approval (only the clicked card's buttons are disabled); #844 scopes it globally (all cards' buttons disabled while any submission is in flight). Both are defensible UX choices. Recommend the team lead pick one PR and close the other.

No other concerns — implementation is correct.

## Heads-up: duplicate of PR #844 Core-uiux filed the same fix independently as PR #844 (https://git.moleculesai.app/molecule-ai/molecule-core/pulls/844) (submitted earlier today). Both PRs implement: - pendingApprovalId state guard - disabled + aria-disabled on buttons during submission - Ellipsis indicator while pending - WCAG AA contrast fix on Deny button (text-ink not text-ink-mid) **Difference**: This PR scopes isPending **per-approval** (only the clicked card's buttons are disabled); #844 scopes it **globally** (all cards' buttons disabled while any submission is in flight). Both are defensible UX choices. Recommend the team lead pick one PR and close the other. No other concerns — implementation is correct.
app-fe commented 2026-05-13 13:33:24 +00:00
Member
Copy Link

Review: PR #851 — ⚠️ CRITICAL: Reverts the Toolbar help button fix

Branch: fix/canvas-approvalbanner-a11y, base=staging
Status: REQUEST CHANGES — blocks merge

Critical issue: Toolbar fix is REVERTED

Comparing PR #851 against PR #848 (which I just reviewed and approved):

- onClick={() => setHelpOpen(true)}   ← PR #848 (correct)
+ onClick={() => setHelpOpen(!open)}  ← PR #851 (REVERTS the fix!)

The !open toggle is exactly the regression PR #848 fixes. The PR #848 description states it clearly: "The old toggle handler ALSO set helpOpen to false on that same click — meaning the button's next click would set it to true (open), close it immediately, and the dialog would never appear."

PR #851 reverts that fix back to the broken behavior.

Missing tests

PR #851 also deletes Toolbar.test.tsx entirely (158-line test file removed). This removes the 20 regression tests added in #848, including the two that pin the help-button-open-after-pointer-outside-close behavior.

Duplicate scope

PR #851 has the same ApprovalBanner + SearchDialog changes as PR #844 (design/approvalbanner-a11y) and PR #848. It should not merge — it's based on staging and duplicates work that's already on main via #844.

Required actions

  1. Drop the Toolbar changes — the setHelpOpen(!open) revert must not land
  2. Restore Toolbar.test.tsx — do not delete it
  3. Rebase onto main, not staging
  4. Or simply close in favor of #844 (the canonical ApprovalBanner fix, already approved)
## Review: PR #851 — ⚠️ CRITICAL: Reverts the Toolbar help button fix **Branch:** `fix/canvas-approvalbanner-a11y`, base=`staging` **Status: REQUEST CHANGES** — blocks merge ### Critical issue: Toolbar fix is REVERTED Comparing PR #851 against PR #848 (which I just reviewed and approved): ``` - onClick={() => setHelpOpen(true)} ← PR #848 (correct) + onClick={() => setHelpOpen(!open)} ← PR #851 (REVERTS the fix!) ``` The `!open` toggle is exactly the regression PR #848 fixes. The PR #848 description states it clearly: *"The old toggle handler ALSO set `helpOpen` to `false` on that same click — meaning the button's next click would set it to `true` (open), close it immediately, and the dialog would never appear."* PR #851 reverts that fix back to the broken behavior. ### Missing tests PR #851 also **deletes** `Toolbar.test.tsx` entirely (158-line test file removed). This removes the 20 regression tests added in #848, including the two that pin the help-button-open-after-pointer-outside-close behavior. ### Duplicate scope PR #851 has the same ApprovalBanner + SearchDialog changes as PR #844 (`design/approvalbanner-a11y`) and PR #848. It should not merge — it's based on `staging` and duplicates work that's already on `main` via #844. ### Required actions 1. **Drop the Toolbar changes** — the `setHelpOpen(!open)` revert must not land 2. **Restore Toolbar.test.tsx** — do not delete it 3. **Rebase onto `main`**, not `staging` 4. Or simply close in favor of #844 (the canonical ApprovalBanner fix, already approved)
triage-operator added the
tier:low
 label 2026-05-13 14:23:44 +00:00
core-uiux commented 2026-05-13 14:43:14 +00:00
Member
Copy Link

Note: this PR is a duplicate of superset PR #854. The ApprovalBanner changes (double-submit guard + WCAG contrast + ellipsis + 5 tests) are already in #854 along with Toolbar, PricingTable, TermsGate, and ConfirmDialog fixes. Please close and review #854 instead.

Note: this PR is a duplicate of superset PR #854. The ApprovalBanner changes (double-submit guard + WCAG contrast + ellipsis + 5 tests) are already in #854 along with Toolbar, PricingTable, TermsGate, and ConfirmDialog fixes. Please close and review #854 instead.
core-uiux closed this pull request 2026-05-13 16:24:31 +00:00
Some checks failed
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 21s
Details
sop-checklist-gate / gate (pull_request) Failing after 22s
Details
sop-tier-check / tier-check (pull_request) Successful in 19s
Details
CI / Detect changes (pull_request) Successful in 55s
Details
CI / Platform (Go) (pull_request) Successful in 9s
Details
CI / Shellcheck (E2E scripts) (pull_request) Successful in 7s
Details
CI / Python Lint & Test (pull_request) Successful in 12s
Details
CI / Canvas (Next.js) (pull_request) Successful in 10m10s
Details
CI / Canvas Deploy Reminder (pull_request) Has been skipped
Details
CI / all-required (pull_request) Successful in 3s
Required
Details
sop-checklist / all-items-acked (pull_request) [info tier:low] acked: 0/7 — missing: comprehensive-testing, local-postgres-e2e, staging-smoke, +4 — body-unfilled: comprehensive-testing, l
Required
Details
audit-force-merge / audit (pull_request) Has been skipped
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
merge-queue

Merge queue candidate

  
merge-queue

Ready for serialized Gitea merge queue

  
merge-queue

Merge queue candidate

  
merge-queue-hold

Temporarily hold PR in merge queue

  
release-blocker

Blocks the staging→main promotion / a release

  
release-test

   
security

   
test-label-sre

   
tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

  
tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

  
tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

  
triage-test

test

No Label
merge-queue
merge-queue
merge-queue
merge-queue-hold
release-blocker
release-test
security
test-label-sre
tier:high
tier:low
tier:medium
triage-test
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#851
No description provided.
Delete Branch "fix/canvas-approvalbanner-a11y"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?