Add /admin/platform-integrations operator UI - #1343
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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)
📝 WalkthroughWalkthroughAdds an admin platform integrations page, API, loader data, routing, OAuth app CRUD operations, logo storage handling, validation, and client-side forms for configuration management. ChangesPlatform integrations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdminBrowser
participant AdminPlatformIntegrationsApiHandler
participant PlatformOAuthAppStorage
participant ObjectStorage
AdminBrowser->>AdminPlatformIntegrationsApiHandler: POST save or delete action
AdminPlatformIntegrationsApiHandler->>PlatformOAuthAppStorage: validate and persist integration
AdminPlatformIntegrationsApiHandler->>ObjectStorage: upload or remove logo asset
AdminPlatformIntegrationsApiHandler->>PlatformOAuthAppStorage: reload integration data
AdminPlatformIntegrationsApiHandler-->>AdminBrowser: refreshed JSON response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 28ad04e. Configure here.
Admin page for managing platform (built-in) OAuth apps, following the /admin/feature-flags pattern: - app cards with logo, enabled state, connection counts, credential status, endpoints, and scope/host lists - enable/disable, edit, and delete actions; delete surfaces the connections-exist guard error - create/edit form covering the full field set; clientSecret and logoBase64 are write-only (omit retains), matching the admin capability semantics so partial saves never clear stored state - role-gated (admin) page + JSON API with audit events - registered in the admin nav, lazy route areas, and loader payloads Handler tests cover retain-on-omit, the delete guard, logo upload/clear, secret non-exposure, and non-admin rejection. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
The client renderer applies defaultValue as an attribute, which text inputs honor but selects ignore, so editing a confidential app showed the Flow dropdown as pkce (caught in manual testing). Options now set selected explicitly for both the flow and token-exchange-style dropdowns; verified in the browser. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
28ad04e to
0993210
Compare
|
🔎 Preview deployed: https://kody-pr-1343.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
packages/worker/src/app/handlers/admin-platform-integrations.ts (3)
156-167: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider auditing failed save and delete attempts.
Both handlers log an audit event only on success. A failed credential change on this surface is a useful signal. Add a
result: 'failure'audit event in each catch block so the audit trail shows attempted changes to platform OAuth credentials.Also applies to: 212-223
🤖 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-platform-integrations.ts` around lines 156 - 167, Update the save and delete handlers’ catch blocks to record an audit event with result: 'failure' before returning the existing 400 response. Reuse each handler’s existing audit-event structure and context so failed platform OAuth credential changes are captured consistently with successful attempts.
144-155: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCap the decoded logo size before writing to R2.
logoBase64goes straight tobase64ToByteswith no length check. A large string allocates the full byte array in the worker beforesetPlatformOauthAppLogoprocesses it. Add a maximum byte length and return 400 when the payload exceeds it.🤖 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-platform-integrations.ts` around lines 144 - 155, Add a maximum decoded logo byte-length check in the handler before calling base64ToBytes or setPlatformOauthAppLogo. Reject oversized logoBase64 payloads with a 400 response, while preserving the existing handling for valid strings and null/undefined values; reuse the handler’s established response pattern and define the limit near the surrounding validation logic.
96-115: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate
tokenUrlandauthorizeUrlas absolute HTTPS URLs.The handler only checks that these fields are non-empty.
upsertPlatformOauthAppinpackages/worker/src/integrations/platform-apps.tsalso only trims them. The worker later uses both values as OAuth endpoints. A typo or a non-HTTP scheme is stored and fails later at token exchange with an opaque error. The surface is admin-only, so this is hardening rather than an exploitable flaw.♻️ Proposed check
+function isHttpsUrl(value: string) { + try { + return new URL(value).protocol === 'https:' + } catch { + return false + } +}if (!clientId || !tokenUrl || !authorizeUrl) { return jsonResponse( { ok: false, error: 'Client id, token URL, and authorize URL are required.', }, 400, ) } + if (!isHttpsUrl(tokenUrl) || !isHttpsUrl(authorizeUrl)) { + return jsonResponse( + { ok: false, error: 'Token URL and authorize URL must be https URLs.' }, + 400, + ) + }🤖 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-platform-integrations.ts` around lines 96 - 115, Update the validation in the admin integration handler around the slug, clientId, tokenUrl, and authorizeUrl checks to require tokenUrl and authorizeUrl to be absolute HTTPS URLs, not merely non-empty strings. Reject malformed, relative, or non-HTTPS values with the existing 400 validation response before calling the persistence flow, while preserving the current required-field and OAuth-flow validation behavior.packages/worker/src/app/admin-platform-integrations-data.ts (1)
15-42: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider one grouped count query instead of one query per app.
Each app triggers a separate
countConnectionsForPlatformAppcall. The loader also runs after every save and delete, so the fan-out repeats on each mutation. Platform apps are operator-provisioned and few, so the current cost is small. If the table grows, replace the per-app counts with a single grouped query overuser_integrationskeyed byplatform_app_slug, then look up each slug in aMap.🤖 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-platform-integrations-data.ts` around lines 15 - 42, Replace the per-app countConnectionsForPlatformApp calls in the withCounts mapping with one grouped user_integrations count query keyed by platform_app_slug, build a Map from the grouped results, and populate each app’s connectionCount from that Map with zero for missing slugs.packages/worker/client/routes/admin-platform-integrations.tsx (3)
44-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the paths from the route table.
Line 44 and line 49 hardcode strings that
packages/worker/universal/routes.tsalready defines asadminPlatformIntegrationsApiandadminPlatformIntegrations.account-management-components.tsxline 310 showsroutes.<name>.href()is available on the client. Using the route objects keeps the page in step with any path change.🤖 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-platform-integrations.tsx` around lines 44 - 51, Update adminPlatformIntegrationsApiPath and isAdminPlatformIntegrationsPath to derive both API and page paths from the corresponding routes.adminPlatformIntegrationsApi and routes.adminPlatformIntegrations href() values, replacing the hardcoded strings while preserving the existing URL pathname comparison.
359-374: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a confirmation step before delete.
handleDeletefires the request on the first click. The Delete button sits next to Edit and Disable. The server rejects deletion while connections exist, but an unused integration is removed without any prompt. Add a confirmation, for example a second click state or a dialog.🤖 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-platform-integrations.tsx` around lines 359 - 374, Update handleDelete to require explicit user confirmation before calling submitAdminAction, using the component’s existing dialog or confirmation pattern where available. Keep the deletion request and existing cleanup behavior unchanged after confirmation, while ensuring the initial Delete click does not submit immediately.
249-268: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueHandle a
FileReadererror and cap the file size.
reader.onerroris not set. If the read fails,pendingLogoBase64staysundefinedand the user gets no message. The handler also accepts any file size and base64-encodes it into the JSON body. Add anonerrorbranch that sets an error message, and reject files above a fixed byte limit before reading.🤖 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-platform-integrations.tsx` around lines 249 - 268, Update handleLogoFileChange to reject files exceeding a fixed byte limit before creating or reading the FileReader, set an appropriate error message, and preserve the existing state update flow. Add reader.onerror to set the same or a clear read-failure error and call handle.update(), while retaining successful base64 processing in reader.onload.packages/worker/src/app/handlers/admin-platform-integrations.node.test.ts (1)
145-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the unknown-slug delete and the invalid-flow save.
The delete path returns 404 when
deletePlatformOauthAppreports no rows changed, and the save path returns 400 for an invalidflow. Neither branch is exercised. Both cases are one extra request each in this harness.🤖 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-platform-integrations.node.test.ts` around lines 145 - 181, Add test coverage in the existing admin platform integrations handler test for deleting an unknown slug, asserting the 404 response from deletePlatformOauthApp when no rows change, and for saving with an invalid flow, asserting the 400 response. Reuse the existing harness and handler setup, adding one request for each branch.
🤖 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/client/routes/admin-platform-integrations.tsx`:
- Around line 275-281: Update the validation returns in the form submit handler
around slug, clientId, tokenUrl, authorizeUrl, and flow to set an appropriate
message and messageTone, then call handle.update() before returning. Ensure both
missing required fields and invalid flow values provide visible feedback despite
noValidate being enabled.
- Around line 525-558: Track the currently mutating app slug with a page-level
pendingSlug state: set it from the app in handleToggleEnabled and handleDelete,
and clear it in submitAdminAction’s finally block. Update the button labels in
the card rendering to show “Saving…” or “Deleting…” only when both actionState
matches and pendingSlug equals app.slug; keep other cards’ labels unchanged.
- Around line 310-315: Update the tokenExchangeStyle handling in the form
submission so selecting "default" sends null in body.tokenExchangeStyle instead
of omitting the key; preserve sending the selected non-default
TokenExchangeStyleOption values unchanged.
- Around line 631-639: Update the slug input and handleSaveFormSubmit flow so
editing preserves the existing slug in submitted form data. Prefer using
readOnly for isEditing while retaining disabled behavior during mutation, or
explicitly restore editingApp.slug into the request body before validation and
save.
In `@packages/worker/src/app/handlers/admin-platform-integrations.ts`:
- Around line 192-223: Move logo cleanup in the deletion handler so failures
from deletePlatformOauthAppLogoAsset do not enter the main 400-response catch
path after the row has been deleted. Declare existing outside the try block,
retain the successful deletion response and audit flow, and perform
deletePlatformOauthAppLogoAsset as best-effort cleanup with its error isolated
from the primary deletion result.
---
Nitpick comments:
In `@packages/worker/client/routes/admin-platform-integrations.tsx`:
- Around line 44-51: Update adminPlatformIntegrationsApiPath and
isAdminPlatformIntegrationsPath to derive both API and page paths from the
corresponding routes.adminPlatformIntegrationsApi and
routes.adminPlatformIntegrations href() values, replacing the hardcoded strings
while preserving the existing URL pathname comparison.
- Around line 359-374: Update handleDelete to require explicit user confirmation
before calling submitAdminAction, using the component’s existing dialog or
confirmation pattern where available. Keep the deletion request and existing
cleanup behavior unchanged after confirmation, while ensuring the initial Delete
click does not submit immediately.
- Around line 249-268: Update handleLogoFileChange to reject files exceeding a
fixed byte limit before creating or reading the FileReader, set an appropriate
error message, and preserve the existing state update flow. Add reader.onerror
to set the same or a clear read-failure error and call handle.update(), while
retaining successful base64 processing in reader.onload.
In `@packages/worker/src/app/admin-platform-integrations-data.ts`:
- Around line 15-42: Replace the per-app countConnectionsForPlatformApp calls in
the withCounts mapping with one grouped user_integrations count query keyed by
platform_app_slug, build a Map from the grouped results, and populate each app’s
connectionCount from that Map with zero for missing slugs.
In `@packages/worker/src/app/handlers/admin-platform-integrations.node.test.ts`:
- Around line 145-181: Add test coverage in the existing admin platform
integrations handler test for deleting an unknown slug, asserting the 404
response from deletePlatformOauthApp when no rows change, and for saving with an
invalid flow, asserting the 400 response. Reuse the existing harness and handler
setup, adding one request for each branch.
In `@packages/worker/src/app/handlers/admin-platform-integrations.ts`:
- Around line 156-167: Update the save and delete handlers’ catch blocks to
record an audit event with result: 'failure' before returning the existing 400
response. Reuse each handler’s existing audit-event structure and context so
failed platform OAuth credential changes are captured consistently with
successful attempts.
- Around line 144-155: Add a maximum decoded logo byte-length check in the
handler before calling base64ToBytes or setPlatformOauthAppLogo. Reject
oversized logoBase64 payloads with a 400 response, while preserving the existing
handling for valid strings and null/undefined values; reuse the handler’s
established response pattern and define the limit near the surrounding
validation logic.
- Around line 96-115: Update the validation in the admin integration handler
around the slug, clientId, tokenUrl, and authorizeUrl checks to require tokenUrl
and authorizeUrl to be absolute HTTPS URLs, not merely non-empty strings. Reject
malformed, relative, or non-HTTPS values with the existing 400 validation
response before calling the persistence flow, while preserving the current
required-field and OAuth-flow validation behavior.
🪄 Autofix
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: 4591e829-af9b-4ca0-ac4b-6d377b8d7d0b
📒 Files selected for processing (11)
packages/worker/client/lazy-route.tsxpackages/worker/client/routes/account-management-components.tsxpackages/worker/client/routes/admin-area.tspackages/worker/client/routes/admin-platform-integrations.tsxpackages/worker/client/routes/index.tsxpackages/worker/src/app/admin-platform-integrations-data.tspackages/worker/src/app/handlers/admin-platform-integrations.node.test.tspackages/worker/src/app/handlers/admin-platform-integrations.tspackages/worker/src/app/router.tspackages/worker/universal/loader-data.tspackages/worker/universal/routes.ts
| try { | ||
| const existing = await getPlatformOauthAppBySlug({ | ||
| db: input.env.APP_DB, | ||
| slug, | ||
| includeDisabled: true, | ||
| }) | ||
| const deleted = await deletePlatformOauthApp({ | ||
| db: input.env.APP_DB, | ||
| slug, | ||
| }) | ||
| if (!deleted) { | ||
| return jsonResponse( | ||
| { ok: false, error: 'Platform integration not found.' }, | ||
| 404, | ||
| ) | ||
| } | ||
| await deletePlatformOauthAppLogoAsset({ | ||
| env: input.env, | ||
| logoKey: existing?.logoKey ?? null, | ||
| }) | ||
| } catch (error) { | ||
| return jsonResponse( | ||
| { | ||
| ok: false, | ||
| error: | ||
| error instanceof Error | ||
| ? error.message | ||
| : 'Unable to delete platform integration.', | ||
| }, | ||
| 400, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Move the logo asset delete out of the failure path.
deletePlatformOauthApp commits the row delete at line 198. If deletePlatformOauthAppLogoAsset then throws, the catch block returns 400. The client reports a failure, keeps the stale list, and skips the success audit event, but the integration is already gone. Treat the asset cleanup as best effort.
🐛 Proposed fix
if (!deleted) {
return jsonResponse(
{ ok: false, error: 'Platform integration not found.' },
404,
)
}
- await deletePlatformOauthAppLogoAsset({
- env: input.env,
- logoKey: existing?.logoKey ?? null,
- })
} catch (error) {
return jsonResponse(
{
ok: false,
error:
error instanceof Error
? error.message
: 'Unable to delete platform integration.',
},
400,
)
}
+
+ try {
+ await deletePlatformOauthAppLogoAsset({
+ env: input.env,
+ logoKey: existing?.logoKey ?? null,
+ })
+ } catch (error) {
+ console.error('platform-oauth-app-logo-delete-failed', slug, error)
+ }Note: existing must be declared outside the try block for this to compile.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const existing = await getPlatformOauthAppBySlug({ | |
| db: input.env.APP_DB, | |
| slug, | |
| includeDisabled: true, | |
| }) | |
| const deleted = await deletePlatformOauthApp({ | |
| db: input.env.APP_DB, | |
| slug, | |
| }) | |
| if (!deleted) { | |
| return jsonResponse( | |
| { ok: false, error: 'Platform integration not found.' }, | |
| 404, | |
| ) | |
| } | |
| await deletePlatformOauthAppLogoAsset({ | |
| env: input.env, | |
| logoKey: existing?.logoKey ?? null, | |
| }) | |
| } catch (error) { | |
| return jsonResponse( | |
| { | |
| ok: false, | |
| error: | |
| error instanceof Error | |
| ? error.message | |
| : 'Unable to delete platform integration.', | |
| }, | |
| 400, | |
| ) | |
| } | |
| if (!deleted) { | |
| return jsonResponse( | |
| { ok: false, error: 'Platform integration not found.' }, | |
| 404, | |
| ) | |
| } | |
| } catch (error) { | |
| return jsonResponse( | |
| { | |
| ok: false, | |
| error: | |
| error instanceof Error | |
| ? error.message | |
| : 'Unable to delete platform integration.', | |
| }, | |
| 400, | |
| ) | |
| } | |
| try { | |
| await deletePlatformOauthAppLogoAsset({ | |
| env: input.env, | |
| logoKey: existing?.logoKey ?? null, | |
| }) | |
| } catch (error) { | |
| console.error('platform-oauth-app-logo-delete-failed', slug, error) | |
| } |
🤖 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-platform-integrations.ts` around lines
192 - 223, Move logo cleanup in the deletion handler so failures from
deletePlatformOauthAppLogoAsset do not enter the main 400-response catch path
after the row has been deleted. Declare existing outside the try block, retain
the successful deletion response and audit flow, and perform
deletePlatformOauthAppLogoAsset as best-effort cleanup with its error isolated
from the primary deletion result.
- edit submits work: the disabled slug input is excluded from FormData, so the slug now comes from the editing state (bug caught by review; verified in the browser end to end) - selecting the 'default' token exchange style clears a stored style (the key is always included; the handler maps non-literals to null) - invalid submits surface an error message instead of returning silently - pending action labels apply only to the acted-on card (pendingSlug) - typed vi.fn in the handler test mock Skipped: the 'logo delete in the failure path' finding — deletePlatformOauthAppLogoAsset already catches internally and never throws, so a committed row delete cannot be reported as a failure. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
3 participants
Operator UI for managing platform (built-in) OAuth integrations, stacked on #1303 (it needs that PR's schema and service layer; GitHub retargets this to
mainwhen #1303 merges).What changed
/admin/platform-integrationspage + JSON API, following the/admin/feature-flagspattern: admin-role-gated (requirePageUserWithRole/requireUserWithRole), audited actions, loader payload inAppLoaderData.hasClientSecretonly — values never leave the server), flow/exchange style, endpoints, scope menu, and required hosts.clientSecretandlogoBase64are write-only exactly like theadmin_platform_oauth_app_savecapability (omit retains, explicit clear for logos), so a partial save can never wipe the scope menu, hosts, or stored credential. Logo upload via file input (SVG rasterized server-side by the existing pipeline).Testing
npm run validatefully green.System recap — composes existing primitives (low risk)
Mode: recap · Base:
cursor/platform-oauth-integrations-c0a2@eb19ae7d· Head:8331e288Classification: composes — wires the existing platform-integrations service and logo pipeline into a new admin page; no primitive behavior changes.
Primitives touched
app-uiintegrationsupsertPlatformOauthApp, delete, logo set)rbacSystem map
An operator manages built-in providers from the admin page; every mutation flows through the same service functions the MCP capabilities use.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Summary by CodeRabbit