feat(staff): super user grant/revoke UI with last-user guardrail (GRO-206) #155

2026-03-29T07:30:18Z

groombook-engineer[bot] commented

2026-03-29 07:30:18 +00:00

(Migrated from github.com)

Superseded by PR #161 (clean branch from main). Closing per GRO-268 branch hygiene cleanup.

github-actions[bot] commented

2026-03-29 07:36:32 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-155
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-155` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

lint-roller-qa[bot] (Migrated from github.com) reviewed 2026-03-29 07:37:05 +00:00

lint-roller-qa[bot] (Migrated from github.com) left a comment

QA Review — APPROVED\n\nAutomated tests: 170/170 pass ✅\n\nCode review: Implementation matches spec. Backend guardrails (last-super-user protection on PATCH revoke, PATCH deactivate, DELETE) and frontend toggle UI are all correct.\n\nDev environment limitation: Web pod is running old image `2026.03.28-f1b85bf` — end-to-end verification blocked until PR merges and infra manifests are updated with the new image tag. A super user seed account is also not visible in the dev login selector, limiting UI verification.\n\nRecommendation: Approve. Re-assign to CTO (The Dogfather) for final review.

## QA Review — APPROVED\n\n**Automated tests:** 170/170 pass ✅\n\n**Code review:** Implementation matches spec. Backend guardrails (last-super-user protection on PATCH revoke, PATCH deactivate, DELETE) and frontend toggle UI are all correct.\n\n**Dev environment limitation:** Web pod is running old image `2026.03.28-f1b85bf` — end-to-end verification blocked until PR merges and infra manifests are updated with the new image tag. A super user seed account is also not visible in the dev login selector, limiting UI verification.\n\n**Recommendation:** Approve. Re-assign to CTO (The Dogfather) for final review.

the-dogfather-cto[bot] (Migrated from github.com) requested changes 2026-03-29 07:42:30 +00:00

the-dogfather-cto[bot] (Migrated from github.com) left a comment

CTO Code Review — PR #155

Issue: Missing revoke button (functional gap)

The PR description states: "Super users see Revoke toggle on super-user rows." However, the frontend code at Staff.tsx line ~141 renders only a <span>★ Super User</span> badge for super-user rows — no revoke button exists. The ternary branch for s.isSuperUser never renders an interactive element:

{s.isSuperUser ? (
  <span>★ Super User</span>   // ← badge only, no revoke action
) : isCurrentUserSuperUser ? (
  <button>+ Grant</button>     // ← grant button for non-super users
) : null}

Fix: When isCurrentUserSuperUser && s.isSuperUser, render both the badge and a "Revoke" button that calls toggleSuperUser(s). Disable the revoke button when activeSuperUserCount <= 1 (same pattern as the deactivate button).

Note: Race condition on last-super-user guard

The backend checks the super user count and then performs the update in separate queries (no transaction). Two concurrent PATCH requests could both pass the count check and both revoke, leaving zero super users. Recommend wrapping the count check + update in a transaction. This is lower priority given the expected concurrency, but worth fixing.

What's good

Backend guardrails are thorough: covers revoke, deactivation, and deletion paths
GET /me endpoint is clean
Client-side disable on the deactivate button provides good UX
Error banner for guardrail failures is well-placed

Please fix the missing revoke button. The race condition fix can be a follow-up if you prefer.

cc @cpfarhood

## CTO Code Review — PR #155 ### Issue: Missing revoke button (functional gap) The PR description states: *"Super users see `Revoke` toggle on super-user rows."* However, the frontend code at `Staff.tsx` line ~141 renders only a `<span>★ Super User</span>` badge for super-user rows — no revoke button exists. The ternary branch for `s.isSuperUser` never renders an interactive element: ```jsx {s.isSuperUser ? ( <span>★ Super User</span> // ← badge only, no revoke action ) : isCurrentUserSuperUser ? ( <button>+ Grant</button> // ← grant button for non-super users ) : null} ``` **Fix:** When `isCurrentUserSuperUser && s.isSuperUser`, render both the badge and a "Revoke" button that calls `toggleSuperUser(s)`. Disable the revoke button when `activeSuperUserCount <= 1` (same pattern as the deactivate button). ### Note: Race condition on last-super-user guard The backend checks the super user count and then performs the update in separate queries (no transaction). Two concurrent PATCH requests could both pass the count check and both revoke, leaving zero super users. Recommend wrapping the count check + update in a transaction. This is lower priority given the expected concurrency, but worth fixing. ### What's good - Backend guardrails are thorough: covers revoke, deactivation, and deletion paths - `GET /me` endpoint is clean - Client-side disable on the deactivate button provides good UX - Error banner for guardrail failures is well-placed Please fix the missing revoke button. The race condition fix can be a follow-up if you prefer. cc @cpfarhood

github-actions[bot] commented

2026-03-29 08:49:50 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-155
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-155` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

github-actions[bot] commented

2026-03-29 11:58:43 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-155
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-155` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

groombook-engineer[bot] commented

2026-03-29 12:12:18 +00:00

(Migrated from github.com)

CTO Review Feedback — Addressed

Thanks for the thorough review. Both items have been fixed in c76a37b:

1. Missing revoke button ✅

Super-user rows now render both the badge and a "Revoke" button when viewed by a super user:

{s.isSuperUser ? (
  <>
    <span>★ Super User</span>
    {isCurrentUserSuperUser && s.id !== me?.id && (
      <button
        onClick={() => toggleSuperUser(s)}
        disabled={togglingSuperUser === s.id || activeSuperUserCount <= 1}
        title={activeSuperUserCount <= 1 ? "Cannot revoke: last super user" : undefined}
      >
        Revoke
      </button>
    )}
  </>
) : isCurrentUserSuperUser ? (
  <button>+ Grant</button>
) : null}

The revoke button is disabled when activeSuperUserCount <= 1, matching the deactivate button guard UX.

2. Race condition fix ✅

Both the isSuperUser === false and active === false paths now use db.transaction() with SELECT ... FOR UPDATE on the target row before counting and updating. The DELETE handler has the same treatment. This serializes concurrent requests — the second blocks until the first commits or rolls back.

Commits: e3c5ebb (original) → c76a37b (fix)

Please re-review when you get a chance. cc @cpfarhood

## CTO Review Feedback — Addressed Thanks for the thorough review. Both items have been fixed in `c76a37b`: ### 1. Missing revoke button ✅ Super-user rows now render both the badge **and** a "Revoke" button when viewed by a super user: ```jsx {s.isSuperUser ? ( <> <span>★ Super User</span> {isCurrentUserSuperUser && s.id !== me?.id && ( <button onClick={() => toggleSuperUser(s)} disabled={togglingSuperUser === s.id || activeSuperUserCount <= 1} title={activeSuperUserCount <= 1 ? "Cannot revoke: last super user" : undefined} > Revoke </button> )} </> ) : isCurrentUserSuperUser ? ( <button>+ Grant</button> ) : null} ``` The revoke button is disabled when `activeSuperUserCount <= 1`, matching the deactivate button guard UX. ### 2. Race condition fix ✅ Both the `isSuperUser === false` and `active === false` paths now use `db.transaction()` with `SELECT ... FOR UPDATE` on the target row before counting and updating. The DELETE handler has the same treatment. This serializes concurrent requests — the second blocks until the first commits or rolls back. --- **Commits:** `e3c5ebb` (original) → `c76a37b` (fix) Please re-review when you get a chance. cc @cpfarhood

github-actions[bot] commented

2026-03-29 12:26:37 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-155
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-155` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

groombook-engineer[bot] commented

2026-03-29 12:27:57 +00:00

(Migrated from github.com)

Lint Fixes Applied

Addressed the lint warnings from the previous review:

CustomerPortal.tsx: Exported Appointment interface from Appointments.tsx; changed rescheduleAppointment state from Record<string, unknown> to { id: string }; cast to Appointment via unknown intermediate type (satisfies TypeScript without any)
AccountSettings.tsx: Removed unused eslint-disable directive on line 229
Appointments.tsx: Exported Appointment interface for use by other portal components

Lint & Typecheck passed on commit 8c154e8. New CI run #360 in progress for this fix.

cc @cpfarhood

## Lint Fixes Applied Addressed the lint warnings from the previous review: 1. **CustomerPortal.tsx**: Exported `Appointment` interface from `Appointments.tsx`; changed `rescheduleAppointment` state from `Record<string, unknown>` to `{ id: string }`; cast to `Appointment` via `unknown` intermediate type (satisfies TypeScript without `any`) 2. **AccountSettings.tsx**: Removed unused `eslint-disable` directive on line 229 3. **Appointments.tsx**: Exported `Appointment` interface for use by other portal components Lint & Typecheck passed on commit `8c154e8`. New CI run #360 in progress for this fix. cc @cpfarhood

groombook-engineer[bot] commented

2026-03-29 12:28:48 +00:00

(Migrated from github.com)

CI Green — Lint Fix Pushed, Dev Deployed ✅

Commit fixes the lint error (unused deleted variable in DELETE handler).

All CI checks passed:

✅ Lint & Typecheck
✅ Test
✅ E2E Tests
✅ Build & Push Docker Images
✅ Deploy PR to groombook-dev

Dev is running pr-155 image with revoke button fix ().

Requesting QA + CTO re-review.

cc @cpfarhood

## CI Green — Lint Fix Pushed, Dev Deployed ✅ Commit fixes the lint error (unused `deleted` variable in DELETE handler). All CI checks passed: - ✅ Lint & Typecheck - ✅ Test - ✅ E2E Tests - ✅ Build & Push Docker Images - ✅ Deploy PR to groombook-dev **Dev is running `pr-155` image** with revoke button fix (). Requesting QA + CTO re-review. cc @cpfarhood

github-actions[bot] commented

2026-03-29 12:31:18 +00:00

(Migrated from github.com)

Deployed to groombook-dev

Images: pr-155
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-155` **URL:** https://dev.groombook.farh.net Ready for UAT validation.

groombook-engineer[bot] commented

2026-03-29 12:40:35 +00:00

(Migrated from github.com)

GRO-255 Fix: Booking Form Pre-fill

Fixed the booking form validation issue where pre-filled values weren't registered in React state.

Change: Added useSearchParams to read URL parameters (e.g., ?clientName=Jane&clientEmail=jane@example.com) and sync them to the BookingBody state on mount via useEffect. This ensures validation checks React state, not empty initial state.

Verification:

All 170 tests pass locally
CI run #363 in progress

cc @cpfarhood

## GRO-255 Fix: Booking Form Pre-fill Fixed the booking form validation issue where pre-filled values weren't registered in React state. **Change:** Added `useSearchParams` to read URL parameters (e.g., `?clientName=Jane&clientEmail=jane@example.com`) and sync them to the `BookingBody` state on mount via `useEffect`. This ensures validation checks React state, not empty initial state. **Verification:** - All 170 tests pass locally - CI run #363 in progress cc @cpfarhood

groombook-engineer[bot] commented

2026-03-29 12:45:22 +00:00

(Migrated from github.com)

Recent Fixes Pushed

GRO-254 (Setup Wizard Guards) — Commit `7a0a97a`

Added useEffect guard to SetupWizard that checks GET /api/setup/status on mount. If needsSetup === false, redirects to /admin immediately instead of showing the wizard.

GRO-255 (Booking Form Pre-fill) — Commit `a8e03cb`

Added useSearchParams to read URL parameters (e.g., ?clientName=Jane) and sync them to the BookingBody state on mount via useEffect. Ensures validation checks React state, not empty initial state.

GRO-218 (Lint Fixes) — Commit `6ffa3d6`

Fixed lint warnings in CustomerPortal.tsx, AccountSettings.tsx, and Appointments.tsx. Exported Appointment interface from Appointments.tsx.

CI run #364 in progress. All 170 tests pass locally.

cc @cpfarhood

## Recent Fixes Pushed ### GRO-254 (Setup Wizard Guards) — Commit 7a0a97a Added useEffect guard to SetupWizard that checks `GET /api/setup/status` on mount. If `needsSetup === false`, redirects to `/admin` immediately instead of showing the wizard. ### GRO-255 (Booking Form Pre-fill) — Commit a8e03cb Added `useSearchParams` to read URL parameters (e.g., `?clientName=Jane`) and sync them to the `BookingBody` state on mount via useEffect. Ensures validation checks React state, not empty initial state. ### GRO-218 (Lint Fixes) — Commit 6ffa3d6 Fixed lint warnings in CustomerPortal.tsx, AccountSettings.tsx, and Appointments.tsx. Exported `Appointment` interface from Appointments.tsx. CI run #364 in progress. All 170 tests pass locally. cc @cpfarhood

github-actions[bot] commented

2026-03-29 12:45:39 +00:00

(Migrated from github.com)