Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The integration harness and migration lock changes include a couple of cleanup/correctness gaps (notably teardown/process and lock/connection usage) that should be addressed to avoid flaky runs and unnecessary DB pool pressure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a Vitest-based integration-test harness for the Nuxt/Nitro app (one build + one long-lived server against real PostgreSQL) and introduces two initial suites that exercise the setup wizard and validate documented authorization rules, alongside a fix for an advisory-lock leak discovered by the new tests.
Changes:
- Introduces
pnpm test:integrationwith a dedicated Vitest config, global setup, HTTP client, DB reset helpers, and fixtures. - Adds integration suites for the setup wizard flow and an authorization/permission matrix.
- Reworks Postgres advisory locking in migrations and setup completion to use transaction-scoped advisory locks.
File summaries
| File | Description |
|---|---|
| ROADMAP.md | Updates roadmap status to reflect integration harness + first suites delivered. |
| pnpm-lock.yaml | Locks new dev dependency additions (notably @nuxt/test-utils). |
| package.json | Adds top-level test:integration script. |
| CONTRIBUTING.md | Documents how to run integration tests locally via TEST_DATABASE_URL. |
| apps/app/vitest.integration.config.ts | Adds Vitest config dedicated to integration tests (globalSetup, timeouts, serialization). |
| apps/app/vitest.config.ts | Excludes integration tests from the default unit-test run. |
| apps/app/tests/integration/support/global-setup.ts | Builds, resets schema, boots server, waits for health, and provides base URL to tests. |
| apps/app/tests/integration/support/fixtures.ts | Test fixtures for setup/sign-up/users/pastes (DB + HTTP helpers). |
| apps/app/tests/integration/support/env.ts | Integration-only env wiring (DB URL, Turnstile test keys, server env). |
| apps/app/tests/integration/support/database.ts | Provides a Drizzle client + resetDatabase() truncate helper. |
| apps/app/tests/integration/support/client.ts | Cookie-preserving HTTP client with per-identity IP simulation. |
| apps/app/tests/integration/setup-wizard.test.ts | Covers setup wizard + sign-up paths over real HTTP/DB. |
| apps/app/tests/integration/permissions.test.ts | Encodes and asserts the permission matrix via status-code expectations. |
| apps/app/server/plugins/migrate.ts | Updates migration-time advisory locking approach. |
| apps/app/server/api/setup/complete.post.ts | Updates setup completion to use transaction-scoped advisory locking. |
| apps/app/package.json | Adds app-level test:integration script. |
| .github/workflows/ci.yml | Adds CI job running integration tests with a Postgres service container. |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 16/17 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The setup completion route can still leave an instance in a “setup complete” state if settings persistence fails after role promotion, and the fix should be made transactional to avoid locking out recovery.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
What this changes
Closes #33
An integration harness — one build and one server for the whole run, a real PostgreSQL, tables truncated between tests — plus the first two suites from ROADMAP.md: the setup wizard (11 tests) and the permission matrix (80). A
postgres:18service container runs them in CI.It also fixes an advisory-lock leak the matrix suite uncovered. See below.
Why
Lint, types, 152 unit tests, both builds and both Docker images all pass without a single request ever reaching a handler. That is how v1.1.0 and v1.1.1 shipped unable to complete their own setup wizard.
The harness
vitest.integration.config.tsvitest.config.ts, sopnpm teststays database-freesupport/global-setup.ts/api/healthsupport/database.tsresetDatabase()by truncatesupport/client.tssupport/fixtures.tsMigrations are applied by the server's own boot plugin against a schema dropped a moment earlier, so the first thing the suite proves is that a fresh instance comes up at all. Each file attaches to the shared server with
setup({ host }):setup()on its own rebuilds per file, and five more suites are meant to follow.Turnstile is exercised for real on Cloudflare's always-passes test keys, so the job needs egress. Mail stays off — a supported configuration, and the only one where sign-up does not wait on a verification click. That is why suites 4 and 7 (invitations, account deletion) are not here; ROADMAP.md now records what they need.
The advisory-lock leak
One matrix test failed three runs out of six, always the same one. The cause was not in the tests.
pg_advisory_lockandpg_advisory_unlockwere two separatedb.execute()calls on the postgres-js pool, so under any concurrent query they landed on different connections. The unlock then failed with aWARNINGthatonnotice: () => {}inserver/utils/database.tsswallows, and the lock outlived the request.pg_locksduring a stuck run:isSetupComplete()is checked inside the lock, so a secondPOST /api/setup/completeblocked forever instead of answering 409, holding a pool connection while it did. The route takes no session. A second container booting blocked on migrations the same way — the exact scenario the lock was written for.Both are now
pg_advisory_xact_lockinside adb.transaction, which pins one connection and releases on commit. Regression test: five concurrent wizard runs, one 201, four 409, one super admin.One mismatch left for you
GET /api/admin/storageis super admin only, asAdminNav.vuealready says withsuperAdminOnly. The table inapps/docs/content/2.self-hosting/3.security.mdputs "view stats" on the same row as settings, which reads as admin-and-up. The test encodes the code; the docs row is the one to fix.Checks
pnpm lint,pnpm typecheckandpnpm testpasspnpm test:integrationpasses — three consecutive full runs, 91 tests eachpnpm db:generate) — no schema changeserver/api/setup/complete.post.tsis rewritten around the transaction, and the whole permission matrix suite asserts on authorization outcomes. Both deserve a close read.AI assistance
pg_locksoutput, not guessed.