Tier 3 structural enforcement: DB invariant gate in CI (+ crew sync v2 phase docs)#505
Merged
Merged
Conversation
Standalone agent-ready spec for the remaining crew PWA realtime redesign work: Phase 2 broadcast migration (full SQL), Phase 3 client cutover behind NEXT_PUBLIC_CREW_SYNC_V2, Phase 4 outbox backoff, Phase 5 rollout/cleanup/guardrail, plus the enforcement Tier 3 outline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
The ESLint rules and unit/guardrails/ suite police the code; this adds the layer that polices the database itself. A new db-invariants CI job runs scripts/check-db-invariants.mjs against the dedicated E2E project (self-disarming when the E2E secrets are absent, like the e2e job) and fails on: - any public table without RLS enabled - any policy-less (deny-all) table outside a shrink-only allowlist of the four deliberately service-role-only tables - any FK column without a covering index (partial WHERE col IS NOT NULL indexes count -- FK probes imply the predicate) - any anon table grant The checks live in a new SECURITY DEFINER db_invariant_report() function (service_role-only execute) since they need pg_catalog, which the REST API can't reach. Applied to prod + E2E along with two drift fixes the audit surfaced: covering indexes for the six FK columns that had none, and revocation of the stale anon grants on 20 prod tables (nothing reads tables unauthenticated -- every public surface goes through the service client server-side; tables with open read policies like integration_providers were world-readable via the anon key until now). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds CI database-invariant enforcement and schema remediation, documents Crew Sync v2 phases, and improves E2E server setup, seed validation, cookie handling, selectors, navigation, and test authentication isolation. ChangesDatabase invariant enforcement
Crew Sync v2 implementation plan
E2E execution reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant check_db_invariants
participant Supabase_REST
participant db_invariant_report
CI->>check_db_invariants: Run invariant check with E2E secrets
check_db_invariants->>Supabase_REST: POST authenticated RPC request
Supabase_REST->>db_invariant_report: Execute schema scan
db_invariant_report-->>Supabase_REST: Return JSONB findings
Supabase_REST-->>check_db_invariants: Return invariant report
check_db_invariants-->>CI: Pass or fail with findings
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
The e2e job died at global-setup with ERR_CONNECTION_REFUSED because playwright.config.ts's webServer conditional dropped the server when E2E_BASE_URL was unset (undefined?.startsWith made the localhost default fall into the no-server branch) -- and CI never sets it. The gate was never armed before this PR's run, so the gap never surfaced. Unset/localhost now gets a webServer; in CI it builds and serves the production build with a matching timeout. Also stop logging the RPC response body in check-db-invariants.mjs's failure path (Sonar S5145: network-controlled data in CI logs) -- the status code plus the fixed remediation hint is enough. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
Owner
Author
|
e2e status: code-side fixed, blocked on test-account seeding. Progress across the two runs:
Remaining steps are environment-side (can't be done from CI or this branch):
Sonar's quality gate and the Generated by Claude Code |
Production's handle_new_user was fixed in place at some point (pinned search_path, qualified public.profiles insert) but the fix never became a migration file, so the E2E project -- built purely from migrations -- kept the original version whose unqualified INSERT INTO profiles fails with 'relation does not exist' on the auth admin connection, breaking every user creation. Applied to both projects (no-op on prod). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
A full prod-vs-E2E schema diff found drift from direct-SQL changes on prod that never became migration files: 12 functions differed and one was missing (including the RLS helpers get_user_org_ids/is_org_member/ get_crew_member_id), crew_feedback's timestamp column had been renamed, inventory_template_items.par_level had been widened to numeric, the 23-row maintenance_catalog_items platform seed existed on prod only, and ~45 tables of policy drift (the July 23 RLS consolidation the E2E project never received). The E2E project is now fully synced -- columns, policies, and seed data hash-identical to prod -- and this migration captures the prod-authoritative function/column/seed state as a file so a from-migrations rebuild lands on parity. stripe_processed_events leaves the db-invariants allowlist: it now has its deny-all policy on both projects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
The suite had never completed a run before today (the gate was unarmed), so its first clean run against the synced E2E database surfaced latent bugs in the tests themselves. This fixes the three mechanical clusters (16 of the 26 failures): - global-setup's crew seed used role 'cleaner' (not a crew_role enum value) plus a nonexistent status column and never checked the insert error, so '[E2E] Alex Cleaner' silently never existed. Fixed the insert and added error checks to the crew and vendor seeds so seed failures can never be silent again. - dismissCookieBanner now force-clicks: specs legitimately dismiss the banner while a Dialog is open, and the dialog overlay intercepted the pointer, timing out five specs. - Ambiguous locators (Add Booking button, vendor names) resolve to two elements on the live pages -- scoped with .first() in the bookings, vendors, booking-validation, and vendor-compliance specs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
![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: 5
🧹 Nitpick comments (4)
.github/workflows/ci.yml (1)
115-115: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: set
persist-credentials: falseon checkout.zizmor flags the default credential persistence. This job only runs a node script and uploads no artifacts, so exposure is minimal, but pinning it costs nothing.
Proposed hardening
- uses: actions/checkout@v4 + with: + persist-credentials: false🤖 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 @.github/workflows/ci.yml at line 115, Update the actions/checkout@v4 step in the workflow to disable persisted credentials by setting persist-credentials to false.Source: Linters/SAST tools
supabase/migrations/20260724130500_index_unindexed_fk_columns.sql (1)
9-25: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffSquawk's
CONCURRENTLYhint has a caveat here — likely safe to leave as-is.Non-concurrent
CREATE INDEXtakes aSHARElock that blocks writes on the referencing table until the build completes. Of these, onlyorganizationsis likely to have enough rows to matter. Note that switching toCREATE INDEX CONCURRENTLYis not a drop-in change: it cannot run inside a transaction block, and Supabase wraps each migration file in one — so it would require isolating these into a non-transactional migration. If write availability onorganizationsduring deploy is a concern, weigh that tradeoff; otherwise the current form is fine.🤖 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 `@supabase/migrations/20260724130500_index_unindexed_fk_columns.sql` around lines 9 - 25, Leave the non-concurrent CREATE INDEX statements unchanged; no code change is required. If write availability during deployment is a concern, move the organizations indexes into a separate non-transactional migration before using CREATE INDEX CONCURRENTLY, while preserving the current statements otherwise.Source: Linters/SAST tools
e2e/helpers/cookies.ts (1)
14-17: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the forced click to the cookie banner.
dismissBtnis a page-wide locator, and.first()is resolved beforeclick({ force: true }). If an open dialog contains another matching button, the forced click can activate that control instead of the cookie action. Build the locator frombannerbefore forcing the click.Based on the changed Playwright helper and the supplied overlay behavior.
Proposed fix
- const dismissBtn = page.getByRole('button', { + const dismissBtn = banner.getByRole('button', { name: /accept|got it|ok|dismiss|close|agree|allow/i, }).first()🤖 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 `@e2e/helpers/cookies.ts` around lines 14 - 17, Scope the forced click in the cookie-banner dismissal helper to the banner locator by deriving the dismiss button from banner rather than using the page-wide dismissBtn locator. Preserve the existing force-click behavior while ensuring an open dialog’s matching button cannot be selected.e2e/specs/03-bookings.spec.ts (1)
9-11: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAnchor the visibility check to the property filter.
page.locator('select').first()only proves that the first rendered select is visible. It can pass if another filter is reordered or rendered while the “All Properties” control is missing. Use the property filter’s accessible label, name, stable container, or test id.Based on the changed selector and the supplied booking-filter context.
🤖 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 `@e2e/specs/03-bookings.spec.ts` around lines 9 - 11, Update the visibility assertion in the booking filter test to target the property filter specifically rather than relying on page.locator('select').first(). Use the property filter’s available accessible label, name, stable container, or test id, while preserving the assertion that the control itself is visible.
🤖 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 `@docs/CREW_SYNC_V2_PHASES.md`:
- Around line 183-210: The exception handling in notify_crew_sync must stop
including raw v_user_id values in RAISE WARNING messages. Update the related
warning statements, including the additional occurrences noted, to log only the
entity/function context and a redacted or non-identifying failure marker.
- Around line 583-591: Update the “Reconnect with jitter” specification to
define jitter as a uniform delay in the explicit interval [base, base + 30
seconds], matching the example and tests. Remove the conflicting “±30 s” wording
while preserving the teardown, resubscribe, and post-subscribe resync
requirements.
- Around line 714-728: The proposed guardrail must allow tables to be covered by
both broadcast triggers and the safety poll. In the new crew-sync coverage test,
derive or reuse the remote-backed cached table list from the Dexie schema,
define separate broadcast-covered and SAFETY_POLL_ONLY sets, and assert their
union covers every remote-backed table; include all existing schema tables,
while keeping purely local tables in a separate LOCAL_ONLY set.
- Around line 631-643: Correct the backoff calculation in processOutbox so the
first retry uses a 5-second base delay after retryCount is incremented,
preserving the subsequent 10-second, 20-second progression and existing
five-minute cap and jitter scaling.
- Around line 160-165: Update the checklist deletion documentation to explicitly
justify standalone deletions as safe, or document the required DELETE triggers
for both checklist_instance_items and checklist_instances when such deletions
can affect crew state. Ensure the rationale addresses nullable turnover_id,
public DELETE policies, and the absence of ON DELETE CASCADE trigger coverage.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 115: Update the actions/checkout@v4 step in the workflow to disable
persisted credentials by setting persist-credentials to false.
In `@e2e/helpers/cookies.ts`:
- Around line 14-17: Scope the forced click in the cookie-banner dismissal
helper to the banner locator by deriving the dismiss button from banner rather
than using the page-wide dismissBtn locator. Preserve the existing force-click
behavior while ensuring an open dialog’s matching button cannot be selected.
In `@e2e/specs/03-bookings.spec.ts`:
- Around line 9-11: Update the visibility assertion in the booking filter test
to target the property filter specifically rather than relying on
page.locator('select').first(). Use the property filter’s available accessible
label, name, stable container, or test id, while preserving the assertion that
the control itself is visible.
In `@supabase/migrations/20260724130500_index_unindexed_fk_columns.sql`:
- Around line 9-25: Leave the non-concurrent CREATE INDEX statements unchanged;
no code change is required. If write availability during deployment is a
concern, move the organizations indexes into a separate non-transactional
migration before using CREATE INDEX CONCURRENTLY, while preserving the current
statements otherwise.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1a46c1d-d107-48a7-8a0e-a4cb5719cc7b
📒 Files selected for processing (16)
.github/workflows/ci.ymlCLAUDE.mddocs/CREW_SYNC_V2_PHASES.mde2e/global-setup.tse2e/helpers/cookies.tse2e/specs/03-bookings.spec.tse2e/specs/10-vendors.spec.tse2e/specs/23-booking-validation.spec.tse2e/specs/24-vendor-compliance-block.spec.tsplaywright.config.tsscripts/check-db-invariants.mjssupabase/migrations/20260724130000_revoke_stale_anon_table_grants.sqlsupabase/migrations/20260724130500_index_unindexed_fk_columns.sqlsupabase/migrations/20260724131000_db_invariant_report.sqlsupabase/migrations/20260724150000_fix_handle_new_user_search_path.sqlsupabase/migrations/20260724160000_capture_prod_drift_functions_columns_seed.sql
- cookies.ts: scope the force-clicked dismiss button to the banner region -- with force:true a page-wide locator could have clicked an open dialog's Close/OK button instead. - ci.yml: persist-credentials:false on the db-invariants checkout. - Crew sync phase doc: fix the outbox backoff off-by-one (first retry is 5s, so 2**(retryCount-1)), state the reconnect jitter interval as [base, base+30s] instead of the contradictory plus/minus wording, make the coverage guardrail a union check (broadcast-triggered tables are deliberately also covered by the safety poll), and spell out why standalone checklist deletions get no DELETE trigger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
First full CI runs (65 tests) surfaced a genuine test-infra bug plus
several stale/incorrect spec assumptions. Root-caused via direct
component reads, not guessing:
- Cookie-banner dismiss vs Dialog backdrop collision (~14 failures):
the cookie notice and Dialog backdrop both use z-50; since the Dialog
portal paints later in DOM order it sits on top, so a force-click
aimed at the banner's coordinates was landing on the Dialog's
backdrop instead (onClick={onClose}), silently closing whatever
dialog was open mid-test. Fixed by moving every dismissCookieBanner()
call to immediately after page.goto(), before any dialog opens --
dismissal persists via localStorage, so one early call per test
covers everything downstream. Affects: 03-bookings, 05-work-orders,
13-maintenance-schedules, 16-comms-log, 23-booking-validation,
24-vendor-compliance-block, 25-owner-portal, 26-turnover-crew-assignment.
- 06-crew.spec.ts: the add-crew-member trigger used an overly broad
regex that could match page chrome ahead of the real button in DOM
order. Narrowed to the exact button text (crew-manage-client.tsx)
and filled the required email field the spec was missing entirely.
- 10-vendors.spec.ts and the vendor-compliance helpers: vendors-client.tsx
renders both a mobile card list and a desktop table unconditionally;
first() picked the DOM-earlier mobile copy, which is CSS-hidden at
this suite's desktop viewport. Scoped with filter visible true.
- 18-messages.spec.ts: asserted the empty state, but global-setup.ts
also seeds a crew member with a linked auth user (for the logout-guard
spec) -- the thread list is never actually empty in this suite. Now
asserts that crew member appears.
- 20-help.spec.ts: dashboard-shell.tsx (wrapping every page) has its
own aria-expanded sidebar toggle that rendered ahead of the first FAQ
button in DOM order. Scoped to the main landmark.
- 22-crew-logout-guard.spec.ts: the turnover link relies on Dexie
(IndexedDB) local-first sync completing after initial mount --
networkidle doesn't cover the async IndexedDB write and re-render.
Added an explicit wait for the link itself.
- 27-crew-feedback.spec.ts: reused the shared crew.json session, but
every test in 22-crew-logout-guard.spec.ts calls supabase.auth.signOut
(a real server-side revocation) -- since crew.json is a static
snapshot never rewritten after global-setup captures it, and 22 runs
before 27, the shared session was already dead by the time this
file's tests ran. Rewritten to create its own disposable crew login
per test, mirroring 21-work-order-offline.spec.ts.
Left unresolved and not guessed at:
- 26-turnover-crew-assignment.spec.ts's property-select timeout: the
Add Turnover dialog's markup looks correct on inspection and an
equivalent selectOption call succeeds elsewhere (03-bookings.spec.ts)
against the same seeded property -- root cause not established
without live reproduction.
- 21-work-order-offline.spec.ts's three offline-completion tests: a
fresh, correctly-isolated browser context still fails to find the
email field on login -- no obvious code-level explanation found.
- 25-owner-portal.spec.ts's 404 assertion: load-owner-portal-data.ts
already returns null for a nonexistent token and page.tsx already
calls notFound() -- the app code looks correct, so the spec is left
as-is rather than weakening the assertion to match an unexplained
200 without understanding why.
Verification: tsc --noEmit clean, lint 0 errors (pre-existing warnings
only), 1978 unit tests pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
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 `@e2e/specs/27-crew-feedback.spec.ts`:
- Around line 63-95: Update the setup helper around the created user and
returned cleanup to catch all failures after createUser, deleting the
provisioned auth user before rethrowing. Ensure the returned cleanup closes the
browser context with guaranteed cleanup semantics so deleteUser runs even when
context.close() throws, while preserving the existing successful setup flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00c49fd0-860f-4921-a4d4-92b1dbc47ee5
📒 Files selected for processing (17)
.github/workflows/ci.ymldocs/CREW_SYNC_V2_PHASES.mde2e/helpers/cookies.tse2e/specs/03-bookings.spec.tse2e/specs/05-work-orders.spec.tse2e/specs/06-crew.spec.tse2e/specs/10-vendors.spec.tse2e/specs/13-maintenance-schedules.spec.tse2e/specs/16-comms-log.spec.tse2e/specs/18-messages.spec.tse2e/specs/20-help.spec.tse2e/specs/22-crew-logout-guard.spec.tse2e/specs/23-booking-validation.spec.tse2e/specs/24-vendor-compliance-block.spec.tse2e/specs/25-owner-portal.spec.tse2e/specs/26-turnover-crew-assignment.spec.tse2e/specs/27-crew-feedback.spec.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e/helpers/cookies.ts
- e2e/specs/10-vendors.spec.ts
- .github/workflows/ci.yml
- e2e/specs/03-bookings.spec.ts
- docs/CREW_SYNC_V2_PHASES.md
- e2e/specs/24-vendor-compliance-block.spec.ts
- e2e/specs/23-booking-validation.spec.ts
CodeRabbit review: if crew_members insert, context creation, or the login flow threw after createUser() succeeded, the just-created disposable auth user was never deleted -- the function propagated the error before reaching the return that hands back cleanup(). Wrapped the post-createUser setup in try/catch so any failure deletes the user before rethrowing, and made the returned cleanup itself resilient (a context.close() throw no longer skips deleteUser). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
The 9e86e3f run (49 passed / 16 failed, up from 40/25 -- confirming the cookie-banner root-cause fix worked broadly) showed this test still failing despite the earlier <main> scoping fix. Root cause: a different, self-inflicted bug -- getByRole('button', { expanded: false }) is a LIVE filter Playwright re-evaluates on every interaction, not a snapshot taken once at locator creation. After firstQuestion.click() flips that item's aria-expanded to "true", the same locator no longer matches its own expanded:false condition and silently re-resolves .first() to the NEXT still-closed FAQ item -- so the toHaveAttribute assertion was checking the wrong element the whole time. Selects by attribute presence (button[aria-expanded]) instead, which doesn't shift identity when the attribute's value changes mid-test. Remaining failures from the 9e86e3f run investigated but left unresolved rather than guessed at: 05-work-orders (ruled out the "New Work Order" button's two DOM occurrences as the cause -- they're conditional empty-state/header variants calling the same handler, not a hidden-duplicate collision -- but found no other explanation), 21-work-order-offline and 27-crew-feedback (both use the identical fresh browser.newContext() + /login?next=/crew + fill #email pattern and both fail identically waiting for #email; ruled out proxy.ts rate limiting since /login carries no limiter entry -- root cause remains unclear without live reproduction), 24-vendor-compliance-block (now timing out on "Add Document" specifically, a step past where it previously failed), 25-owner-portal and 26-turnover-crew-assignment (unchanged from prior investigation). Verification: tsc --noEmit clean, lint 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
…st data cleanE2EData() never deleted [E2E]-prefixed communication_logs rows, so a prior CI run or same-run retry left a stale/duplicate row behind that collided with 16-comms-log.spec.ts's hardcoded subject text, breaking both the empty-state assertion and the create-entry test's strict-mode locator. Also give the test's own subject a per-attempt unique suffix so a retry within the same run can't recreate the collision even before cleanup runs.
…owner portal specs - 24-vendor-compliance-block: addComplianceDocument() clicked the vendor row, which opens vendors-client.tsx's quick-view dialog (no compliance section) instead of navigating to the vendor detail page where "Add Document" actually lives. Now clicks the row's "Details" link first. - 22-crew-logout-guard: rewritten to create a disposable crew login + turnover per test (mirroring 27-crew-feedback.spec.ts) instead of sharing e2e/.auth/crew.json across all three tests. Two of the three tests end in a real supabase.auth.signOut(), which revokes the session server-side — since crew.json is a static storageState snapshot, whichever test logged out first permanently killed the session for every test after it in the same file, regardless of declaration order. - 25-owner-portal: (1) give the owner a unique per-attempt name so a CI retry (retries: 2) re-running the whole test doesn't leave a stale same-named card behind that collides with the retry's own card on a scoped locator. (2) the "nonexistent token" test now asserts on the rendered 404 content instead of response.status() — app/owner/[token] has a loading.tsx, so Next.js streams the route and flushes a 200 shell before the async token lookup resolves and notFound() fires deep in the tree; the security boundary (no owner data leaks, correct 404 UI) is intact, this is a known Next.js App Router limitation with streamed notFound(), not a gap in the route's own logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/specs/22-crew-logout-guard.spec.ts (1)
104-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the disposable-crew seeding helper into
e2e/helpers/. The header comment states this mirrors27-crew-feedback.spec.ts; two copies of the create-user → crew_member → turnover → checklist → login sequence will drift as schema evolves.#!/bin/bash # Compare the two seeding implementations for shared logic rg -n 'admin.createUser|crew_members|turnover_assignments|checklist_instance' e2e/specs/27-crew-feedback.spec.ts fd . e2e/helpers --exec echo {}🤖 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 `@e2e/specs/22-crew-logout-guard.spec.ts` around lines 104 - 224, Extract the shared disposable-crew setup flow from loginAsFreshCrewWithTurnover and the corresponding implementation in 27-crew-feedback.spec.ts into a reusable helper under e2e/helpers. Have both specs call that helper for user, crew member, turnover, checklist data, login context, and cleanup, preserving the existing failure-safe cleanup behavior and returned page/cleanup contract.
🤖 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 `@e2e/specs/22-crew-logout-guard.spec.ts`:
- Around line 115-121: Update the crewEmail construction in the test setup to
use a crypto.randomUUID() suffix instead of Date.now(), preserving the existing
disposable email prefix and domain. Leave the createUser flow and error handling
unchanged.
- Around line 208-217: The cleanup callback must delete the E2E crew_members
record before removing the auth user. In cleanup, after closing context and
before deleteUser(userId), delete the crew_members row using crewMember.id,
while preserving the existing turnovers deletion and finally-based cleanup
behavior.
- Around line 219-223: Update the failure cleanup around the test’s main setup
flow to track created turnover and crew-member IDs in outer scope, then
explicitly delete those records in the catch block alongside the auth user. Also
remove associated turnover assignments and checklist rows as needed, preserving
cleanup ordering and ensuring all records inserted before the throw are rolled
back.
---
Nitpick comments:
In `@e2e/specs/22-crew-logout-guard.spec.ts`:
- Around line 104-224: Extract the shared disposable-crew setup flow from
loginAsFreshCrewWithTurnover and the corresponding implementation in
27-crew-feedback.spec.ts into a reusable helper under e2e/helpers. Have both
specs call that helper for user, crew member, turnover, checklist data, login
context, and cleanup, preserving the existing failure-safe cleanup behavior and
returned page/cleanup contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c77ee17-3ad1-41c2-b250-ba383e51065c
📒 Files selected for processing (7)
e2e/global-setup.tse2e/specs/16-comms-log.spec.tse2e/specs/20-help.spec.tse2e/specs/22-crew-logout-guard.spec.tse2e/specs/24-vendor-compliance-block.spec.tse2e/specs/25-owner-portal.spec.tse2e/specs/27-crew-feedback.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/specs/25-owner-portal.spec.ts
- e2e/specs/20-help.spec.ts
- e2e/global-setup.ts
- e2e/specs/27-crew-feedback.spec.ts
- e2e/specs/24-vendor-compliance-block.spec.ts
…le-crew helper - crypto.randomUUID() instead of Date.now() for the throwaway email — closes a same-millisecond collision window with other disposable-crew specs. - Success-path cleanup() now deletes the crew_members row before deleting the auth user — crew_members.user_id is ON DELETE SET NULL, not CASCADE, so the delete-user alone left orphaned rows behind. - The catch-path rollback now tracks and deletes the turnover/crew_members rows already inserted before the throw, not just the auth user, matching what its own comment claimed it did. - Renamed the crew member from the same "[E2E] Logout Guard Crew" name global-setup.ts's static seed uses (which 18-messages.spec.ts asserts on) to "[E2E] Fresh Logout Guard Crew" — avoids ambiguous duplicate-named rows coexisting mid-run.
The previous run's e2e job failed entirely in global-setup with only "Login failed — current URL: http://localhost:3000/login" — no Playwright spec ever ran, including the recent diagnostic additions to 05/24. signInWithPassword() runs client-side (login-form.tsx) and never touches the Next.js server, so webServer stdout/stderr piping doesn't help here; the actual reason only ever reaches the page's own error banner. Grab that text so a recurrence identifies whether this is bad credentials, Supabase-side rate-limiting, or an outage instead of just the generic URL.
Sonar flagged the nested template literal in the new bannerText interpolation. Extract the conditional suffix to a named variable first, per CLAUDE.md's no-nested-template-literals rule.
Root-caused via the diagnostic added in 5334667/9949c1e: work order rows for both tests WERE persisting correctly (dbErr: null, confirmed via direct DB query), but the /maintenance board's Server Component query kept returning empty regardless of router.refresh() or a full page.reload(). Direct comparison of the two Supabase projects' live wo_status enum found the actual cause: production (vpmznjktllhmmbfnxuvk): pending, quote_requested, assigned, in_progress, completed, cancelled E2E (syhthijeqlnltufdawyb): pending, assigned, in_progress, completed, cancelled — missing quote_requested entirely quote_requested was added to production out-of-band at some point — no migration file in this repo ever adds it (the original 20260524165615_fieldstay_v1_extensions_enums.sql only defines the other five values). The E2E project, seeded purely from tracked migrations, never got it. page.tsx's board query (`.in('status', ['pending', 'quote_requested', 'assigned', 'in_progress'])`) throws "invalid input value for enum wo_status" on that literal every single time it runs against the E2E project — and since the query result's `error` was never checked (`{ data: workOrders }` destructured directly, `?? []` masking both "zero rows" and "query errored"), the board just silently rendered as empty instead of surfacing the failure. Confirmed directly: hand-running the same filter as raw SQL against the E2E project reproduces the identical Postgres error. Fixes: - New migration adds quote_requested to wo_status (idempotent via ADD VALUE IF NOT EXISTS, a no-op against production where it already exists) — applied to the E2E project directly via Supabase MCP so CI is unblocked now; committed here so it reaches production through the normal migration pipeline and the schema is finally tracked. - page.tsx now checks every Promise.all query's error and console.errors it before falling back to `?? []`, closing the silent-failure gap that let this go undetected — a bad filter value or an RLS misconfiguration will show up in logs instead of just rendering as "no results" indefinitely.
Root cause confirmed via git history: #technician-name (vendor-portal.tsx) is a required native <input>, added in commit 1ca212f after this spec (b1e1339) was written and never updated to match. The browser's own constraint validation silently blocked the form submit before handleSubmit ever ran, so "Saved" never appeared for a reason that had nothing to do with the offline-queueing behavior these tests actually exercise. The earlier Upstash rate-limit fix (0452758) was addressing a real but different issue and had no bearing on this one.
…indow 23-booking-validation.spec.ts: the dedup test never selected a Source on the "Add Booking" form, so both bookings defaulted to source 'direct'. bookings_manual_dates_unique only applies WHERE source = 'manual' (confirmed against the live E2E index definition), so neither insert ever hit the constraint — the second booking silently succeeded as a separate direct booking instead of being rejected. Select "Manual Entry" explicitly on both submissions. 26-turnover-crew-assignment.spec.ts: created its turnover 200 days out to land in the board's "Upcoming" section, but app/(dashboard)/turnovers/page.tsx's Server Component query only fetches turnovers with checkout_datetime within [-7, +60] days of now — a turnover further out than that is invisible to every subsequent page load, and BoardSection unmounts its entire section (heading included) when the group is empty, matching the exact observed symptom of the "Upcoming" button never appearing at all. Use +30/+31 days instead, comfortably inside the fetch window and still past the 7-day "Upcoming" boundary. Both were confirmed via direct queries against the live E2E Supabase project (constraint definition, enum values) rather than assumed from migration files, following the same investigative approach that found the wo_status schema-drift root cause earlier in this branch's history.
…tion
After the +30/+31-day fetch-window fix landed, this test's
"only card in Upcoming" assumption broke: card.getByText('Needs Crew')
resolved to 2 elements on a clean first attempt (not a retry artifact —
no other spec creates a turnover in this date range, and the E2E DB is
reset per run). Root cause of the second element wasn't pinned down
further after ruling out duplicate inserts (createManualTurnover does
a single .insert()), duplicate status-label render paths (mutually
exclusive ternary), and other same-class elements in the file (an
assign-crew dropdown, but it's conditionally rendered and closed at
that point in the test).
Rather than leave the assumption in place, scope the card by a unique
marker written into the turnover's notes field (rendered on the card)
and filtered on — the same "scope by unique text" pattern already used
throughout this suite for property/vendor/guest names, instead of
relying on being the only match in a shared section.
The previous notes-marker filter fix didn't work: notes only render
once a TurnoverCard is expanded (`{expanded && ... turnover.notes}`),
so scoping by that text before ever expanding the card matched zero
elements. Add a stable data-testid="turnover-card-<id>" to
TurnoverCard's root instead — always present regardless of expand
state or any other render condition — and have the test look up its
own turnover's real ID via a direct DB query right after creation,
then scope every subsequent assertion to that id. This sidesteps the
original ambiguity (why a plain '.bg-card-themed.rounded-xl' locator
matched 2 elements under CI load) entirely rather than requiring it to
be root-caused first.
A single .single() lookup right after the dialog closed intermittently
found zero rows ("Cannot coerce the result to a single JSON object")
even though createManualTurnover had already returned success (that's
what closes the dialog) — the read goes over a separate service-role
connection from the browser session that performed the write, and a
single immediate read apparently isn't guaranteed to see it yet. Poll
with toPass() until the row is visible, matching the retry pattern
already used elsewhere in this suite (21-work-order-offline's
expect.poll on status) instead of assuming read-after-write is
instantaneous across connections.
…ignment
The turnover ID lookup now reliably finds the created row (previous
commit's polling fix worked), exposing the next real interaction for
the first time: getByRole('button', { name: 'Assign' }) matched 2
elements. TurnoverCard's header wrapper div (role="button", used for
expand/collapse) has no aria-label of its own, so its computed
accessible name absorbs all nested descendant text — including the
literal word "Assign" from the actual Assign button nested inside it.
exact: true disambiguates without needing to restructure the
component's existing (if not ideal) nested-interactive markup.
Same accessible-name-pollution issue as the "Assign" button fix: the crew-assignment UI (dropdown option + assigned chip) is nested inside the card header's role="button" wrapper, so a non-exact match against "[E2E] Alex Cleaner" resolves to both the wrapper div and the actual element. Proactively fixing both remaining occurrences (the dropdown option click and the chip visibility check) together, since the same pattern would predictably break the next line the same way and cost another CI round trip to discover individually.
…down Root cause of the crew-select button never being found by exact:true — the avatar span renders c.name[0] (the literal first character, "[" for a name like "[E2E] Alex Cleaner", not a real initial) as visible text inside the button with no aria-hidden, so it was part of the computed accessible name: "[[E2E] Alex Cleaner" instead of "[E2E] Alex Cleaner". A non-exact match previously papered over this by substring-matching anyway (contributing to the earlier strict-mode ambiguity fixed by adding exact:true), but exact:true correctly requires the whole string to match and never could. aria-hidden="true" on the purely decorative avatar circle is the right fix on its own merits, not just for the test: a screen reader has no reason to read out a single stray bracket character before the crew member's actual name.
Newly recurring across two consecutive runs, not a flake: both
03-bookings.spec.ts and 23-booking-validation.spec.ts timed out
waiting for the "Booking added" banner. Confirmed via CI log evidence
(server console output now visible via playwright.config.ts's
webServer stdout/stderr piping) that createBooking's insert always
succeeds — a "failed" attempt's booking still existed and caused a
strict-mode duplicate-guest-name match on the very next attempt.
Root cause is environmental, not a code defect: the E2E Supabase
project's Postgres logs show a continuous, unrelated background error
storm ("invalid column for filter org_id", recurring every ~1s,
consistent with a broken Realtime subscription stuck retrying) that
predates this PR and isn't caused by anything in this branch's diff.
Combined with dozens of CI runs today against one shared project, this
occasionally pushes createBooking's fully-awaited critical path (property
lookup, insert, audit log, overlap detection, Inngest dispatch, two
revalidatePath calls) past a tight 8s client timeout. Widen to 20s —
proportionate to a proven-real latency source, not a guess.
Fixing the underlying Realtime subscription loop is a separate,
pre-existing infrastructure issue outside this PR's scope.
Companion doc to CREW_SYNC_V2_PHASES.md: captures the still-open items from the scalability assessment (Tier 3 hygiene — notifications retention, Hostaway incremental sync, Kroger rate limiter, fail-closed spend budgets — plus the deferred types/database.ts drift check from the enforcement Tier 3 outline), with verified done/open status for every original tier item so the list can't silently rot.
|
smj1860
pushed a commit
that referenced
this pull request
Jul 25, 2026
Closes the last item from PR #505's db-invariants job: check 4 (types vs. live schema) was deferred. Adds public.db_type_shape_report() (mirrors db_invariant_report()'s SECURITY DEFINER/service-role-only pattern) and scripts/check-type-drift.mjs, which diffs it against a mechanical parse of types/database.ts — every enum's labels, every table's presence, and column presence for every table wired into Database.public.Tables. Wired into the existing db-invariants CI job as a second step. First real run against both projects surfaced three genuine drift incidents beyond the wo_status one that motivated this check: - wo_source was missing 'vacancy_gap_suggestion', which advanceScheduleAfterCompletion() already branches on (20260725201000_add_vacancy_gap_suggestion_to_wo_source.sql). - crew_feedback's timestamp column was renamed created_at -> submitted_at on both projects by an earlier drift-capture migration, but support-inbox/page.tsx and its client still queried/displayed the old name — a live "column does not exist" bug in the support inbox feedback list, now fixed alongside the stale CrewFeedback interface. - inventory_count_drafts never actually had the reviewed_at/reviewed_by columns approveInventoryCount()/rejectInventoryCount() write to (an earlier migration defined them but no-op'd against an already-existing table) — every PM approve/reject of a pending count was failing. Added the columns for real, with their FK covering index (20260725201500_add_reviewed_columns_to_inventory_count_drafts.sql). Also reconciled a long tail of types/database.ts fields that had drifted from the live schema (BookingSource.ownerrez, SupportMessageRole.human, Organization/Property/Booking/Turnover/WorkOrder/MaintenanceSchedule columns, TurnoverAssignment/PushSubscription/SupportConversation/ SupportMessage/InventoryCountDraft(Item)/InventoryTemplateItem shapes, new OrgSmsTemplate interface) and wired a dozen previously-unmapped but already-interfaced tables into Database.public.Tables. Intentional mismatches (the deprecated work_orders.assigned_crew_id, join-only relationship fields, DB-internal-only tables like platform_admins) are allowlisted in the new script, same shrink-only ratchet as SERVICE_ROLE_ONLY_TABLES. Migrations applied to both vpmznjktllhmmbfnxuvk and syhthijeqlnltufdawyb. get_advisors on production shows no new findings from db_type_shape_report. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
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.
What this adds
Tier 3: the database-level enforcement layer
Tiers 1–2 (ESLint AST rules, guardrail tests, typed
ServiceRoleContext) police the code. This adds the layer that polices the database — the CLAUDE.md invariants no code-side check can see.New CI job
db-invariantsrunsscripts/check-db-invariants.mjsagainst the dedicated E2E Supabase project (never production — CI holds no prod credentials; both projects receive every migration, so schema invariants verified there hold for prod by construction). Self-disarming with a warning annotation when the E2E secrets are absent, mirroring the e2e job's gate. It fails on:pending_integration_links,pending_oauth_authorizations,processed_webhooks,stripe_processed_events) — stale allowlist entries are themselves failuresWHERE col IS NOT NULLindexes count — FK-enforcement probes are alwayscol = $1, which implies the predicate)anontable grantThe checks live in a new
SECURITY DEFINERfunctionpublic.db_invariant_report()(pinned emptysearch_path, EXECUTE revoked fromPUBLIC/anon/authenticated, granted toservice_roleonly) because they needpg_catalog, which the REST API can't reach directly.Drift the audit surfaced, fixed so the gate starts clean
20260724130500— covering indexes for the six FK columns that had none (org_inventory_catalog.platform_catalog_item_id,org_maintenance_catalog_items.platform_catalog_item_id,organizations.bedroom/bathroom_room_template_id,pending_oauth_authorizations.provider_id,vendor_assignment_outcomes.property_id).20260724130000— revoked the staleanongrants that 20 production tables still carried (audit_events,organization_members,owner_transactions,work_orders, …) and ~85 tables carried on the E2E project via default privileges. Verified nothing reads tables unauthenticated: every public surface (guidebook, owner portal, media kit, vendor-connect) reads server-side through the service client, auth pages callsupabase.auth.*only, and all browser-side table reads run with an authenticated session. Until this change, tables with open read policies (e.g.integration_providers' "Anyone can read active providers") were world-readable through the REST API with just the public anon key. Grants toauthenticatedare untouched (RLS depends on them). Default privileges also revoked so future tables don't regress.All three migrations are already applied to both projects and verified:
db_invariant_report()returns clean on prod and E2E (empty RLS/FK/anon sections; only the four allowlisted deny-all tables). Supabase security advisors show nothing new — the new function correctly does not appear in the callable-function warnings.Also included
docs/CREW_SYNC_V2_PHASES.md— standalone agent-ready implementation instructions for crew sync v2 Phases 2–5 (broadcast migration SQL, client cutover spec, outbox backoff, rollout plan) plus the Tier 3 outline as an appendix.Verification
tsc --noEmit,eslint(0 errors),check:ui-classesclean🤖 Generated with Claude Code
https://claude.ai/code/session_01EgNNY4XjLYRHZ3w4RHocLp
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
anontable/sequence grants and adding missing FK covering indexes.Quality & Reliability
Documentation