Repository navigation
Add admin user plan updates across API, MCP, and admin UI - #682
Conversation
- users admin API: new update_plan action on POST /admin/users.json (update:user:any), validating against the plan registry and audit logging target_user_id and plan - new admin_user_update MCP capability (admin role, audited) to set or clear a user's entitlement plan by id or email - admin users payload and MCP metadata schema now expose plan; the /admin/users page gains a Manage plan panel and Plan metadata field - tests: handler actions, MCP capability, parsePlanName, and the admin RBAC E2E now covers plan editing and the widened payload pin - docs: authorization privacy boundary and entitlements plan assignment
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds entitlement "plan" support to admin user management: ChangesAdmin plan assignment feature
Estimated code review effort: 3 (Moderate) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant AdminUsersRoute
participant AdminUsersHandler
participant AdminUsersData
participant AuditLog
Admin->>AdminUsersRoute: select plan, click Save plan
AdminUsersRoute->>AdminUsersHandler: POST action=update_plan
AdminUsersHandler->>AdminUsersHandler: readPlanUpdate(body.plan)
AdminUsersHandler->>AdminUsersData: updateAdminUserPlan(userId, plan)
AdminUsersData-->>AdminUsersHandler: updated user or null
AdminUsersHandler->>AuditLog: logAuditEvent(update_plan)
AdminUsersHandler-->>AdminUsersRoute: refreshed users payload
AdminUsersRoute-->>Admin: show updated plan
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
🔎 Preview deployed: https://kody-pr-682.kody-a99.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b1b453f. Configure here.
Bugbot flagged that an unsaved plan selection survived pagination, route preloads, and role mutations, so Save plan could apply a stale draft. Any refresh of the users list now drops the draft and the render pass reseeds the select from the refreshed record.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/worker/src/app/admin-users-data.ts (1)
59-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared SELECT column list.
The
id, username, email, email_verified_at, plan, created_at, updated_atcolumn list is duplicated across the list query, the by-id query, and the by-email query. This PR already needed three manual edits to addplan; a future column addition/removal is one easy edit away from silently drifting across these queries.♻️ Proposed refactor
+const adminUserRowColumns = + 'id, username, email, email_verified_at, plan, created_at, updated_at' + export async function loadAdminUsersData( env: Env, requestUrl: string, ): Promise<AdminUsersLoaderData> { ... env.APP_DB.prepare( - `SELECT id, username, email, email_verified_at, plan, created_at, updated_at - FROM users - ORDER BY id ASC - LIMIT ? OFFSET ?`, + `SELECT ${adminUserRowColumns} + FROM users + ORDER BY id ASC + LIMIT ? OFFSET ?`, )Apply similarly to the two queries in
loadAdminUserByIdOrEmail.Also applies to: 91-109
🤖 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 `@packages/worker/src/app/admin-users-data.ts` around lines 59 - 67, The admin user queries in admin-users-data.ts repeat the same SELECT column list in the paginated list query and the lookup queries, which makes future schema changes easy to drift. Extract that shared column list into a single reusable constant or helper near loadAdminUsers/loadAdminUserByIdOrEmail, then update all three queries to reference it so changes to columns like plan only need one edit.packages/worker/client/routes/admin-users.tsx (1)
168-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated fetch/error-handling between
submitRoleActionandsubmitPlanAction.Both functions repeat the same query-string preservation, fetch options, 401 redirect, JSON parsing, and
users/availableRoles/availablePlans/pagination sync logic. Consider extracting a shared helper that performs the POST + response handling, parameterized by request body and success message, to avoid the two copies drifting apart as more admin actions are added.🤖 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 `@packages/worker/client/routes/admin-users.tsx` around lines 168 - 281, `submitRoleAction` and `submitPlanAction` duplicate the same POST/query-string preservation, 401 redirect, JSON parsing, and state-sync flow, so factor that work into a shared helper. Extract the common fetch-and-handle logic from these methods in `admin-users.tsx`, parameterizing the request body and the success/message update, then have both `submitRoleAction` and `submitPlanAction` call it so pagination, users, roles, and plans stay in sync in one place.packages/worker/src/app/loader-data.ts (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
AdminPlanNamefrom#worker/entitlements/plans.ts
AdminPlanNameduplicatesPlanName; re-exporting keeps the loader types aligned with the shared plan source.♻️ Proposed refactor
-export type AdminPlanName = 'partner' | 'personal' | 'pro' +export type { PlanName as AdminPlanName } from '`#worker/entitlements/plans.ts`'🤖 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 `@packages/worker/src/app/loader-data.ts` at line 37, The loader type is depending on a duplicated AdminPlanName instead of the shared plan type source. Update the plans module referenced by AdminPlanName so it re-exports the shared PlanName from `#worker/entitlements/plans.ts`, then ensure loader-data’s plan field continues to use AdminPlanName without redefining a separate type. Keep the change centered around the AdminPlanName and PlanName symbols so all plan-related types stay aligned.
🤖 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 `@packages/worker/src/app/handlers/admin-users.ts`:
- Around line 302-311: The fire-and-forget audit event logging calls in the
assign_role, remove_role, and update_plan action handlers are not guaranteed to
complete before the response is returned, risking dropped audit writes. Either
await the logAuditEvent calls directly to ensure they complete before
responding, or if async execution is preferred, pass the promise to
ctx.waitUntil to keep the request alive until the audit write completes. Apply
this fix to all three action handlers.
---
Nitpick comments:
In `@packages/worker/client/routes/admin-users.tsx`:
- Around line 168-281: `submitRoleAction` and `submitPlanAction` duplicate the
same POST/query-string preservation, 401 redirect, JSON parsing, and state-sync
flow, so factor that work into a shared helper. Extract the common
fetch-and-handle logic from these methods in `admin-users.tsx`, parameterizing
the request body and the success/message update, then have both
`submitRoleAction` and `submitPlanAction` call it so pagination, users, roles,
and plans stay in sync in one place.
In `@packages/worker/src/app/admin-users-data.ts`:
- Around line 59-67: The admin user queries in admin-users-data.ts repeat the
same SELECT column list in the paginated list query and the lookup queries,
which makes future schema changes easy to drift. Extract that shared column list
into a single reusable constant or helper near
loadAdminUsers/loadAdminUserByIdOrEmail, then update all three queries to
reference it so changes to columns like plan only need one edit.
In `@packages/worker/src/app/loader-data.ts`:
- Line 37: The loader type is depending on a duplicated AdminPlanName instead of
the shared plan type source. Update the plans module referenced by AdminPlanName
so it re-exports the shared PlanName from `#worker/entitlements/plans.ts`, then
ensure loader-data’s plan field continues to use AdminPlanName without
redefining a separate type. Keep the change centered around the AdminPlanName
and PlanName symbols so all plan-related types stay aligned.
🪄 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: 892fbf96-9664-43f5-b7d2-ea3c998d5ec8
📒 Files selected for processing (15)
docs/contributing/adding-capabilities.mddocs/contributing/architecture/authorization.mddocs/contributing/architecture/entitlements.mde2e/admin-rbac.spec.tspackages/worker/client/navigation-data.node.test.tspackages/worker/client/routes/admin-users.tsxpackages/worker/src/app/admin-users-data.tspackages/worker/src/app/handlers/admin-users.node.test.tspackages/worker/src/app/handlers/admin-users.tspackages/worker/src/app/loader-data.tspackages/worker/src/entitlements/entitlements.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-capabilities.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-shared.tspackages/worker/src/mcp/capabilities/admin/admin-user-update.tspackages/worker/src/mcp/capabilities/admin/domain.ts
| const requestIp = getRequestIp(input.request) ?? undefined | ||
| void logAuditEvent({ | ||
| category: 'admin', | ||
| action: 'update_plan', | ||
| result: 'success', | ||
| email: input.actor.email, | ||
| ip: requestIp, | ||
| path: input.url.pathname, | ||
| reason: `target_user_id=${targetUserId};plan=${planUpdate.plan ?? 'null'}`, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "logAuditEvent" packages/worker/src/app/handlers/admin-users.ts
rg -n "waitUntil" packages/worker/src --type=tsRepository: kentcdodds/kody
Length of output: 5184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== admin-users.ts relevant sections =="
sed -n '150,340p' packages/worker/src/app/handlers/admin-users.ts
echo
echo "== request pipeline waitUntil usage in worker src =="
rg -n "waitUntil\\(" packages/worker/src --type=ts
echo
echo "== handler entrypoints near admin-users pipeline =="
rg -n "handleUpdatePlanAction|handleAssignRole|handleRemoveRole|create.*Handler|ctx\\.waitUntil|waitUntil\\(" packages/worker/src/app packages/worker/src/index.ts --type=tsRepository: kentcdodds/kody
Length of output: 39701
Keep admin audit writes alive. assign_role, remove_role, and update_plan all fire-and-forget logAuditEvent(...), but this request path never uses ctx.waitUntil, so the audit write can be dropped once the response returns. If these events need to be durable, await the call or hand it to ctx.waitUntil.
🤖 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 `@packages/worker/src/app/handlers/admin-users.ts` around lines 302 - 311, The
fire-and-forget audit event logging calls in the assign_role, remove_role, and
update_plan action handlers are not guaranteed to complete before the response
is returned, risking dropped audit writes. Either await the logAuditEvent calls
directly to ensure they complete before responding, or if async execution is
preferred, pass the promise to ctx.waitUntil to keep the request alive until the
audit write completes. Apply this fix to all three action handlers.
What
Admins can now modify a user account record's entitlement plan — previously the only way to set
users.planwas a manual SQL update.POST /admin/users.jsongains anupdate_planaction (guarded byupdate:user:any) that sets or clears (plan: null→ legacy/unlimited) a user's plan. Unknown plan strings are rejected with400rather than coerced to null. Success writes anadmin-category audit event with reasontarget_user_id=…;plan=….admin_user_updatecapability (admin domain,requiredRole: 'admin', audited viaauditAdminCapabilityInvocation) updates one user byidoremailwithplan: PlanName | nulland returns the updated account metadata record./admin/usersshows the plan in the metadata grid and adds a "Manage plan" panel with a plan select (including "Legacy/unlimited (no plan)") and a Save button that stays disabled while the selection matches the stored plan.plan(validated throughparsePlanName, so unknown stored values read back asnull) andavailablePlans; the privacy-boundary field pin and its tests were widened accordingly.Username editing was deliberately left out: usernames drive
{username}@<platform domain>email routing and public identity lookups, so an admin rename has side effects beyond account metadata.Testing
update_planhandler actions (set / clear / invalid plan 400 / unknown user 404 / audit reasons),admin_user_updateMCP capability (set/clear by email/id, audit rows, non-admin rejection, unknown-plan rejection), andparsePlanNameparsing.admin-rbac.spec.tsnow changes a plan through the UI, asserts the success message, and verifiesplanvia the API alongside the widened payload key pin.pro, then cleared it back to legacy/unlimited; audit log recordedupdate_planevents with the expected reasons.npm run validatepasses (format, lint, typecheck, unit, Playwright E2E, MCP E2E).Docs
docs/contributing/architecture/authorization.md— POST actions, privacy boundary (plan is account metadata), widened field pin.docs/contributing/architecture/entitlements.md— new "Assigning plans" section covering both audited admin surfaces.docs/contributing/adding-capabilities.md—admin_user_updateadded to the admin capability list.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@80e35c57· Head:b1b453f0Classification: extends — the RBAC admin surface gains a plan mutation
(HTTP action + MCP capability) and the admin users payload contract widens to
include
plan/availablePlans. No primitives added;primitives.yamlunchanged.
Primitives touched
rbacupdate_planaction onPOST /admin/users.json, newadmin_user_updateMCP capability, payload pin widened withplanentitlementsplanNames/parsePlanNamefor validation; plan config and enforcement unchangedapp-ui/admin/usersgains a Manage plan panel and Plan metadata fieldmcp-serverSystem map
Change flow
sequenceDiagram participant Admin participant UI as /admin/users participant API as POST /admin/users.json participant MCP as admin_user_update participant D1 as users.plan Admin->>UI: select plan, Save plan UI->>API: { action: update_plan, userId, plan | null } Admin->>MCP: { id | email, plan | null } API->>D1: updateAdminUserPlan (parsePlanName-validated) MCP->>D1: updateAdminUserPlan (zod planNames-validated) API-->>Admin: refreshed users payload + audit event MCP-->>Admin: updated user metadata + audit eventBefore / after
users.planwritesadmin_user_updateMCPplan(parsed), +availablePlansInvariants
per-user-isolation: the new mutation stays inside the documented RBACexception — admin-only guards (
update:user:any/requiredRole: 'admin'),account metadata only, audit-logged; the privacy-boundary field pin test was
widened deliberately to include
planand still rejects any other widening.Summary by CodeRabbit
New Features
Bug Fixes