Migrate admin handlers to unified AppContext pattern#1498
Conversation
Agent-Logs-Url: https://github.com/jaypatrick/adblock-compiler/sessions/d9a0bdd5-02fe-4d6d-949e-a15a32697029 Co-authored-by: jaypatrick <1800595+jaypatrick@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Migrates Worker admin endpoints to a unified AppContext handler signature so admin handlers can consistently access request-scoped context (e.g., c.get('prisma'), c.get('authContext')) and align with the rest of the Hono/OpenAPI route ecosystem.
Changes:
- Updated admin route handler function signatures from
(request, env, authContext, ...)to(c: AppContext, ...)across admin handler modules. - Updated
worker/routes/admin.routes.tsto pass the Hono context (c) directly to the migrated handlers. - Added a Vitest Workers config + npm script/deps to support Workers-runtime-style integration testing via
@cloudflare/vitest-pool-workers.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| worker/routes/admin.routes.ts | Updated admin route invocations to call handlers with c (AppContext) instead of decomposed params. |
| worker/handlers/auth-config.ts | Migrated auth-config admin handler to accept AppContext and use c.env / c.get('authContext'). |
| worker/handlers/admin-users.ts | Migrated admin user management handlers to AppContext; switched Prisma access to c.get('prisma') where applicable. |
| worker/handlers/admin-usage.ts | Migrated usage handler to AppContext and used c.req.url / c.env. |
| worker/handlers/admin-neon.ts | Migrated Neon admin handlers + helper to AppContext and used c.req / c.env. |
| worker/handlers/admin-agents.ts | Migrated agent handlers to AppContext and switched to request-scoped Prisma from c.get('prisma'). |
| vitest.worker.config.ts | Added Vitest configuration for Workers integration tests via @cloudflare/vitest-pool-workers. |
| package.json | Added test:worker:vitest script and Vitest-related devDependencies. |
|
@copilot apply changes based on the comments in this thread |
|
@copilot Fix CI |
- Update worker/handlers/admin-users.test.ts: replace (req, env, authContext) calls with makeAppContext(req, env, authContext, prisma) — also eliminates _internals.createPrismaClient stubs for delete/ban/unban tests - Update worker/handlers/admin-agents.test.ts: same pattern; 503 tests now omit prisma from context instead of using makeEnvNoHyperdrive(); remove stub/_internals usage entirely; remove unused makeEnvNoHyperdrive helper - Update worker/handlers/admin-usage.test.ts: add makeAppContext import and update all 6 handler invocations - Update worker/handlers/admin-neon.test.ts: add makeAppContext import and update all 22 handler invocations across guard-rail and happy-path tests - Update worker/handlers/auth-config.test.ts: add makeAppContext import and update all 13 handler invocations - Fix package.json: correct @vitest/ui and vitest versions from ^2.2.7 (non-existent) to ^2.1.9 (compatible with @cloudflare/vitest-pool-workers@^0.5.25) Agent-Logs-Url: https://github.com/jaypatrick/adblock-compiler/sessions/9f75c48a-9d70-4321-937c-6cbc1d0a36ce Co-authored-by: jaypatrick <1800595+jaypatrick@users.noreply.github.com>
…k conflict Co-authored-by: jaypatrick <1800595+jaypatrick@users.noreply.github.com>
…-workers) Co-authored-by: jaypatrick <1800595+jaypatrick@users.noreply.github.com>
Agent-Logs-Url: https://github.com/jaypatrick/adblock-compiler/sessions/9f75c48a-9d70-4321-937c-6cbc1d0a36ce Co-authored-by: jaypatrick <1800595+jaypatrick@users.noreply.github.com>
Done. All 5 test files have been updated to use the
Also fixed All 380 handler tests pass (commit |
…workers entries pnpm install --frozen-lockfile was failing in CI because package.json added @cloudflare/vitest-pool-workers, @vitest/ui, and vitest to devDependencies but pnpm-lock.yaml was never updated. This caused all CI jobs that run `pnpm install --frozen-lockfile` to fail with "3 dependencies were added" error. Agent-Logs-Url: https://github.com/jaypatrick/adblock-compiler/sessions/12bf17e7-9022-4747-adf5-6b5b67d09c65 Co-authored-by: jaypatrick <1800595+jaypatrick@users.noreply.github.com>
Fixed in commit
The lockfile is now updated and |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
@copilot fix CI |
…o fix frontend CI - Upgrades @cloudflare/vitest-pool-workers from ^0.5.25 to ^0.14.2 (vitest@4.x compat) - Changes root vitest from ^2.1.9 to ^4.1.0 to match frontend, eliminating dual-vitest version conflict that caused vite defaultClientConditions ESM export error - Removes @vitest/ui from root devDependencies (no longer needed, was causing @vitest/ui@2.1.9 to be wired as peer of vitest@4.1.0) - Updates vitest.worker.config.ts to new 0.14.x API (cloudflareTest plugin instead of defineWorkersConfig; defineConfig from vitest/config instead of defineWorkersProject) - Regenerates pnpm-lock.yaml with minimal diff: adds @cloudflare/vitest-pool-workers@0.14.2 + workerd@1.20260405.1 snapshots; removes old 0.5.x vitest@2.1.9 entries; no Angular hash changes; better-auth stays on vitest@4.1.0 Agent-Logs-Url: https://github.com/jaypatrick/adblock-compiler/sessions/af38f403-44e9-47fa-a6b7-21f2d6819ecb Co-authored-by: jaypatrick <1800595+jaypatrick@users.noreply.github.com>
Fixed in commit Root cause: the previous lockfile introduced two conflicting Fix:
|
| * - Tests that need the Workers runtime environment | ||
| * | ||
| * ## When NOT to use | ||
| * - Pure TypeScript unit tests (use Deno test runner via `deno task test:src`) |
There was a problem hiding this comment.
The header comment says to use deno task test:src for “pure TypeScript unit tests”, but the worker unit tests in this repo run via deno task test:worker (and deno task test runs both). Updating this avoids confusing readers about the intended runner for worker tests.
| * - Pure TypeScript unit tests (use Deno test runner via `deno task test:src`) | |
| * - Pure TypeScript unit tests (use Deno test runner via `deno task test:worker`) |
| export function makeAppContext( | ||
| request: Request, | ||
| env: Env, | ||
| authContext: IAuthContext, | ||
| prisma?: unknown, | ||
| ): AppContext { | ||
| const vars: Record<string, unknown> = { authContext }; | ||
| if (prisma !== undefined) { | ||
| vars.prisma = prisma; | ||
| } | ||
| return { | ||
| req: { | ||
| url: request.url, | ||
| raw: request, | ||
| json: () => request.json(), | ||
| text: () => request.text(), | ||
| header: (name: string) => request.headers.get(name) ?? undefined, | ||
| method: request.method, | ||
| path: new URL(request.url).pathname, | ||
| }, | ||
| env, | ||
| get: (key: string) => vars[key], | ||
| set: (key: string, value: unknown) => { | ||
| vars[key] = value; | ||
| }, | ||
| } as unknown as AppContext; |
There was a problem hiding this comment.
makeAppContext() currently builds vars as Record<string, unknown> and exposes get: (key: string) => vars[key], then casts to AppContext. This weakens type-safety in tests (e.g., typos in variable keys or wrong value shapes won’t be caught). Consider typing vars as Partial<Variables> (from worker/routes/shared.ts) and making get/set key-based (<K extends keyof Variables>(key: K) => Variables[K]) so tests stay aligned with the real Hono context contract.
| import { assertEquals } from '@std/assert'; | ||
| import { stub } from '@std/testing/mock'; | ||
| import { handleAdminGetAgentSession, handleAdminListAgentAuditLog, handleAdminListAgentSessions, handleAdminTerminateAgentSession } from './admin-agents.ts'; | ||
| import { type Env, type HyperdriveBinding, type IAuthContext, UserTier } from '../types.ts'; | ||
| import { _internals } from '../lib/prisma.ts'; | ||
| import { makeAppContext } from '../test-helpers.ts'; |
There was a problem hiding this comment.
The file header still says “Prisma is stubbed at the createPrismaClient boundary”, but these tests now pass a Prisma mock via makeAppContext(..., prisma) and no longer stub createPrismaClient. Please update the header comment to reflect the new testing approach to avoid misleading future readers.
Description
Migrates all admin route handlers from decomposed parameter passing
(request: Request, env: Env, authContext: IAuthContext, ...params)to unified AppContext pattern(c: AppContext, ...params). This enables request-scoped Prisma client access viac.get('prisma')and aligns with Hono ecosystem best practices.Also updates all corresponding Deno unit tests to use the new
makeAppContext(req, env, authContext, prisma?)helper and fixes the CI failure caused by a dual-vitest-version conflict introduced by the initial dependency additions.Changes
Handler Signature Updates
Converted 20 admin handler functions across 5 files to accept
AppContextas the first parameter:worker/handlers/admin-users.ts (7 handlers):
handleAdminListUsers(c)handleAdminGetUser(c, userId)handleAdminUpdateUser(c, userId)handleAdminDeleteUser(c, userId)handleAdminBanUser(c, userId)handleAdminUnbanUser(c, userId)worker/handlers/admin-agents.ts (5 handlers):
handleAdminListAgentSessions(c)handleAdminGetAgentSession(c, sessionId)handleAdminListAgentAuditLog(c)handleAdminTerminateAgentSession(c, sessionId)worker/handlers/auth-config.ts (1 handler):
handleAdminAuthConfig(c)worker/handlers/admin-usage.ts (1 handler):
handleAdminGetUserUsage(c, userId)worker/handlers/admin-neon.ts (8 handlers):
handleAdminNeonGetProject(c)handleAdminNeonListBranches(c)handleAdminNeonGetBranch(c, branchId)handleAdminNeonCreateBranch(c)handleAdminNeonDeleteBranch(c, branchId)handleAdminNeonListEndpoints(c)handleAdminNeonListDatabases(c, branchId)handleAdminNeonQuery(c)Route Call Updates
Updated all 20 corresponding handler invocations in worker/routes/admin.routes.ts:
Context Access Pattern
Handlers now access request primitives through AppContext:
Helper functions updated to accept
AppContext:Test Updates
All 5 admin handler test files updated to use
makeAppContext(req, env, authContext, prisma?)fromworker/test-helpers.ts:(req, env, authContext)calls; eliminated_internals.createPrismaClientstubs by passing mock prisma directly into contextmakeEnvNoHyperdrive()approach; removed allstub/_internalsusageDependency Fix
The initial addition of
@cloudflare/vitest-pool-workers@^0.5.25+vitest@^2.1.9introduced a dual-vitest-version conflict: the frontend workspace already usesvitest@4.1.0, and havingvitest@2.1.9alongside it caused pnpm'sauto-install-peersto wire@vitest/ui@2.1.9as a peer ofvitest@4.1.0. This produced a brokenvitesnapshot that failed withSyntaxError: The requested module 'vite' does not provide an export named 'defaultClientConditions'in thefrontend-lint-testCI job.Resolution:
@cloudflare/vitest-pool-workersfrom^0.5.25→^0.14.2(first version supportingvitest@^4.1.0); changed rootvitestfrom^2.1.9→^4.1.0to match the frontend, eliminating the dual-vitest conflict; removed@vitest/ui(no longer needed)defineWorkersConfig/@cloudflare/vitest-pool-workers/configAPI (removed in0.13.x) to the newcloudflareTestplugin +defineConfigfromvitest/config@cloudflare/vitest-pool-workers@0.14.2andworkerd@1.20260405.1snapshots; removes oldvitest@2.1.9entries;better-authstays onvitest@4.1.0; no Angular builder hash changesTesting
Zero Trust Architecture Checklist
Worker / Backend
*) on write/authenticated endpoints (N/A - no CORS changes)[vars]) (N/A - no secret access changed).prepare().bind()(no string interpolation) (N/A - no D1 queries changed)Frontend / Angular
CanActivateFnauth guards (N/A - backend only)localStorage) (N/A - backend only)