feat(webapp): extend admin workers endpoint and unify admin api auth#3390
feat(webapp): extend admin workers endpoint and unify admin api auth#3390
Conversation
Adds a GET loader to the admin worker groups endpoint, exposes type, hidden, workloadType, cloudProvider, location, staticIPs, and enableFastPath on creation, and introduces authenticateAdminRequest and requireAdminApiRequest helpers in personalAccessToken.server.ts. The generic authenticateAdminRequest returns a discriminated result so callers can shape failures to fit their context; requireAdminApiRequest is the Remix loader/action wrapper that throws a Response.
Replaces duplicated inline PAT + admin checks across 24 admin api routes with the shared requireAdminApiRequest helper. Refactors platform-notifications.ts to compose the generic authenticateAdminRequest with neverthrow instead of duplicating the PAT + admin check.
|
WalkthroughThis pull request introduces a centralized authentication and authorization pattern for admin API endpoints. A new set of helper functions— Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Additional notesThe changes follow a consistent and repetitive pattern across most routes (replacement of inline auth logic with a centralized helper call), which reduces review complexity for those files. However, the distributed scope across 20+ files, combined with the need to validate the new service functions and verify the expanded workers endpoint implementation, presents a moderate overall review burden. Pay particular attention to: (1) the new 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/routes/admin.api.v1.platform-notifications.ts (1)
9-10: SimplifyauthenticateAdminreturn type toResult<void, AuthError>.The success payload (
id/admin) is unused inaction, so returningok(undefined)would reduce coupling and keep intent tighter.♻️ Optional refactor
-type AdminUser = { id: string; admin: boolean }; type AuthError = { status: number; message: string }; -async function authenticateAdmin(request: Request): Promise<Result<AdminUser, AuthError>> { +async function authenticateAdmin(request: Request): Promise<Result<void, AuthError>> { const result = await authenticateAdminRequest(request); - return result.ok - ? ok({ id: result.user.id, admin: result.user.admin }) - : err({ status: result.status, message: result.message }); + return result.ok ? ok(undefined) : err({ status: result.status, message: result.message }); }Also applies to: 12-16
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/admin.api.v1.platform-notifications.ts` around lines 9 - 10, Change the authenticateAdmin function to return Result<void, AuthError> instead of Result<AdminUser, AuthError> and update its successful return to ok(undefined); update the AdminUser/related return points (authenticateAdmin and any callers like action) to stop relying on a returned { id, admin } payload and instead use only the void success. Ensure the AuthError type remains unchanged and adjust any code that destructures authenticateAdmin's result (e.g., places that read result.value.id or result.value.admin) to use the existing request/context or other sources for id/admin checks rather than the function return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/webapp/app/routes/admin.api.v1.platform-notifications.ts`:
- Around line 9-10: Change the authenticateAdmin function to return Result<void,
AuthError> instead of Result<AdminUser, AuthError> and update its successful
return to ok(undefined); update the AdminUser/related return points
(authenticateAdmin and any callers like action) to stop relying on a returned {
id, admin } payload and instead use only the void success. Ensure the AuthError
type remains unchanged and adjust any code that destructures authenticateAdmin's
result (e.g., places that read result.value.id or result.value.admin) to use the
existing request/context or other sources for id/admin checks rather than the
function return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 95bb0786-e0f7-4eb4-9a5c-482823e98b9d
📒 Files selected for processing (29)
.server-changes/admin-workers-endpoint.mdapps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.repair-queues.tsapps/webapp/app/routes/admin.api.v1.environments.$environmentId.engine.report.tsapps/webapp/app/routes/admin.api.v1.environments.$environmentId.schedules.recover.tsapps/webapp/app/routes/admin.api.v1.environments.$environmentId.tsapps/webapp/app/routes/admin.api.v1.feature-flags.tsapps/webapp/app/routes/admin.api.v1.gc.tsapps/webapp/app/routes/admin.api.v1.llm-models.$modelId.tsapps/webapp/app/routes/admin.api.v1.llm-models.missing.tsapps/webapp/app/routes/admin.api.v1.llm-models.reload.tsapps/webapp/app/routes/admin.api.v1.llm-models.seed.tsapps/webapp/app/routes/admin.api.v1.llm-models.tsapps/webapp/app/routes/admin.api.v1.migrate-legacy-master-queues.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.concurrency.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.environments.staging.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.feature-flags.tsapps/webapp/app/routes/admin.api.v1.orgs.$organizationId.runs.enable.tsapps/webapp/app/routes/admin.api.v1.platform-notifications.tsapps/webapp/app/routes/admin.api.v1.runs-replication.$batchId.backfill.tsapps/webapp/app/routes/admin.api.v1.runs-replication.$batchId.cancel.tsapps/webapp/app/routes/admin.api.v1.runs-replication.backfill.tsapps/webapp/app/routes/admin.api.v1.runs-replication.create.tsapps/webapp/app/routes/admin.api.v1.runs-replication.start.tsapps/webapp/app/routes/admin.api.v1.runs-replication.stop.tsapps/webapp/app/routes/admin.api.v1.runs-replication.teardown.tsapps/webapp/app/routes/admin.api.v1.snapshot.tsapps/webapp/app/routes/admin.api.v1.workers.tsapps/webapp/app/services/personalAccessToken.server.tsapps/webapp/app/v3/services/worker/workerGroupService.server.ts
|
ready |
Extends the admin worker groups endpoint with a GET loader and more fields on POST (type, hidden, workloadType, cloudProvider, location, staticIPs, enableFastPath), and pulls the PAT + admin check that was inlined or locally duplicated across every admin.api route into a shared helper in personalAccessToken.server.ts. The generic authenticateAdminRequest returns a discriminated result; requireAdminApiRequest is the thin Remix loader/action wrapper that throws. The neverthrow-style route (platform-notifications.ts) now composes the generic helper instead of duplicating the check. Verified locally against GET (listing) and POST (new fields, invalid enum, minimal backwards-compat).