feat(integrations): Add builtin provider install flow#700
feat(integrations): Add builtin provider install flow#7002witstudios merged 2 commits intomasterfrom
Conversation
Builtin providers (GitHub, Generic Webhook) are defined in code but never inserted into the database, so the settings UI shows nothing. This adds an explicit install step so users can activate builtins before connecting. - GET /api/integrations/providers/available — returns uninstalled builtins - POST /api/integrations/providers/install — copies builtin config into DB - useAvailableBuiltins SWR hook for the new endpoint - "Built-in Integrations" card section in /settings/integrations UI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces functionality to discover and install built-in integration providers. It adds two API routes: one to list available (not-yet-installed) built-in providers, and another to handle provider installation. The frontend integrations page is updated to display available built-ins with install buttons, while new hooks manage data fetching and state synchronization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant API as GET /api/.../available
participant Auth as Auth/Session
participant DB as Database
Client->>API: Request available providers
API->>Auth: Verify session scope
Auth-->>API: Session valid
API->>DB: Fetch enabled providers
DB-->>API: Provider list
API->>API: Compute uninstalled built-ins
API-->>Client: Return { providers: [...] }
sequenceDiagram
participant Client as Client (Browser)
participant API as POST /api/.../install
participant Auth as Admin Auth
participant DB as Database
Client->>API: POST { builtinId }
API->>Auth: Verify admin authentication
Auth-->>API: Admin confirmed
API->>API: Validate schema
API->>DB: Resolve builtin provider
DB-->>API: Builtin details
API->>DB: Check existing installation
DB-->>API: Not found (available)
API->>DB: Create provider record
DB-->>API: New provider created
API-->>Client: 201 { id, slug, name }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdf9f26cc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const auth = await authenticateRequestWithOptions(request, AUTH_OPTIONS); | ||
| if (isAuthError(auth)) return auth.error; |
There was a problem hiding this comment.
This handler allows any authenticated session to create a new global integration_providers row (isSystem: true, driveId: null) without an admin check, so any regular user can change integration availability for all users. That bypasses the admin gate used by other provider-creation endpoints and creates an authorization gap in multi-user deployments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this was indeed an authorization gap.
Fixed in e8dce77: Replaced the session-only authenticateRequestWithOptions with verifyAdminAuth + isAdminAuthError, which handles session authentication, CSRF validation, and admin role-version validation in one call.
The previous inline pattern (if (!adminAuth)) was also a bug: verifyAdminAuth returns VerifiedUser | NextResponse, both truthy, so the check never caught errors. Now using the correct isAdminAuthError() type guard (same as the withAdminAuth HOF uses internally).
Changes:
apps/web/src/app/api/integrations/providers/install/route.ts— lines 3, 16-17: switched toverifyAdminAuth+isAdminAuthError, removed redundantauthenticateRequestWithOptions+AUTH_OPTIONScreatedBynow usesadminUser.idfrom the verified admin user
| ) : builtins.length === 0 ? ( | ||
| <p className="text-sm text-muted-foreground text-center py-6"> | ||
| All built-in integrations have been installed. | ||
| </p> |
There was a problem hiding this comment.
When the /api/integrations/providers/available request fails, the hook falls back to an empty array and this branch renders “All built-in integrations have been installed,” which is incorrect and hides a real backend/auth error from the user. In that failure mode users are told setup is complete even though installable builtins may exist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in e8dce77.
Changes in apps/web/src/app/settings/integrations/page.tsx:
- Line 36: Now destructures
error: builtinsErrorfromuseAvailableBuiltins() - Lines 309-313: Added error state check (
builtinsError ?) before the empty-array check, showing a red error banner ("Failed to load available integrations") consistent with howprovidersErrorandconnectionsErrorare displayed in the other cards on this page.
The "All built-in integrations have been installed" message now only renders when the fetch actually succeeded and returned an empty array.
- Install endpoint: Replace session-only auth with verifyAdminAuth + isAdminAuthError to properly restrict builtin installs to admins. The previous pattern (`!adminAuth`) never caught errors since both VerifiedUser and NextResponse are truthy. - Settings UI: Surface builtinsError state instead of showing misleading "all installed" message when the /available fetch fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/web/src/hooks/useIntegrations.ts (1)
60-65: Consider exportingAvailableBuiltinfor downstream consumers.The interface is only used internally by
useAvailableBuiltins, but exporting it would let consuming components type-annotate variables or props explicitly, consistent with howSafeProvider,SafeConnection, etc. are exported fromtypes.ts.-interface AvailableBuiltin { +export interface AvailableBuiltin {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useIntegrations.ts` around lines 60 - 65, Export the AvailableBuiltin interface so downstream consumers can reference it; update the declaration of AvailableBuiltin to be exported (export interface AvailableBuiltin ...) in the same module where useAvailableBuiltins is defined and ensure any local imports/usages still compile — this lets components import AvailableBuiltin from the hook file just like other exported types (e.g., SafeProvider/SafeConnection).apps/web/src/app/api/integrations/providers/install/route.ts (2)
56-56: Double cast throughunknowncircumvents type safety.
builtin as unknown as Record<string, unknown>is effectively an unchecked cast. If theconfigcolumn is meant to hold the full builtin definition, consider defining a shared type or using a proper serialization step so the compiler can verify the shape. This also stores redundant data (name, description, etc.) already saved in dedicated columns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/integrations/providers/install/route.ts` at line 56, The assignment config: builtin as unknown as Record<string, unknown> bypasses type checks; replace the double-cast by giving config a proper type or serializing only the intended data: either define and import a shared BuiltinConfig (or Builtin) interface and use config: builtin as BuiltinConfig, or explicitly build a minimal object/JSON to store (e.g., pick only the fields needed) and assign config: JSON.parse(JSON.stringify(minimalBuiltin)) or similar; update the assignment referencing the builtin variable and the config property so the compiler can verify shape and you avoid storing redundant full builtin metadata.
25-34: Malformed JSON body returns 500 instead of 400.If the client sends a non-JSON body,
request.json()on line 26 throws and falls through to the generic catch on line 64, returning a 500. Parsing the body before thetryblock or adding a dedicated catch would yield a more accurate 400 response.Proposed fix
+ let body: unknown; + try { + body = await request.json(); + } catch { + return NextResponse.json({ error: 'Invalid JSON body' }, { status: 400 }); + } + try { - const body = await request.json(); const validation = installSchema.safeParse(body);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/integrations/providers/install/route.ts` around lines 25 - 34, The handler currently calls request.json() and lets any JSON parse errors escape to the outer catch (causing a 500); wrap the body parsing in its own try/catch (or add a dedicated catch for request.json()) before running installSchema.safeParse so that a malformed/non-JSON body returns NextResponse.json({ error: 'Invalid JSON' }, { status: 400 }) instead of falling through to the generic 500; update the code around the request.json() call and subsequent use of installSchema.safeParse to return a 400 on parse errors.apps/web/src/app/settings/integrations/page.tsx (1)
90-102: Concurrent installs can clobberinstallingBuiltinstate.Since
installingBuiltinis a singlestring | null, clicking Install on a second builtin while the first is still in-flight overwrites the tracking state — the first button loses its spinner/disabled state. This is unlikely with only a few builtins, but if you want to be safe:Track multiple in-flight installs with a Set
- const [installingBuiltin, setInstallingBuiltin] = useState<string | null>(null); + const [installingBuiltins, setInstallingBuiltins] = useState<Set<string>>(new Set()); const handleInstallBuiltin = async (builtinId: string) => { - setInstallingBuiltin(builtinId); + setInstallingBuiltins((prev) => new Set(prev).add(builtinId)); try { await post('/api/integrations/providers/install', { builtinId }); toast.success('Integration installed'); mutateProviders(); mutateBuiltins(); } catch { toast.error('Failed to install integration'); } finally { - setInstallingBuiltin(null); + setInstallingBuiltins((prev) => { + const next = new Set(prev); + next.delete(builtinId); + return next; + }); } };And update the button check:
- disabled={installingBuiltin === builtin.id} + disabled={installingBuiltins.has(builtin.id)}- {installingBuiltin === builtin.id ? ( + {installingBuiltins.has(builtin.id) ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/settings/integrations/page.tsx` around lines 90 - 102, The handler handleInstallBuiltin uses a single installingBuiltin string which gets clobbered by concurrent installs; change the state from installingBuiltin: string | null to a Set<string> (e.g., installingBuiltins) and update handleInstallBuiltin to add the builtinId to the set before the async post, remove it in finally, and use set state updates (prev => new Set(prev).add(...) / delete(...)) to avoid races; also update the button rendering logic that currently checks installingBuiltin === id to instead check installingBuiltins.has(id) so each install button shows its own spinner/disabled state without being overwritten.
🤖 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/web/src/app/api/integrations/providers/install/route.ts`:
- Line 56: The assignment config: builtin as unknown as Record<string, unknown>
bypasses type checks; replace the double-cast by giving config a proper type or
serializing only the intended data: either define and import a shared
BuiltinConfig (or Builtin) interface and use config: builtin as BuiltinConfig,
or explicitly build a minimal object/JSON to store (e.g., pick only the fields
needed) and assign config: JSON.parse(JSON.stringify(minimalBuiltin)) or
similar; update the assignment referencing the builtin variable and the config
property so the compiler can verify shape and you avoid storing redundant full
builtin metadata.
- Around line 25-34: The handler currently calls request.json() and lets any
JSON parse errors escape to the outer catch (causing a 500); wrap the body
parsing in its own try/catch (or add a dedicated catch for request.json())
before running installSchema.safeParse so that a malformed/non-JSON body returns
NextResponse.json({ error: 'Invalid JSON' }, { status: 400 }) instead of falling
through to the generic 500; update the code around the request.json() call and
subsequent use of installSchema.safeParse to return a 400 on parse errors.
In `@apps/web/src/app/settings/integrations/page.tsx`:
- Around line 90-102: The handler handleInstallBuiltin uses a single
installingBuiltin string which gets clobbered by concurrent installs; change the
state from installingBuiltin: string | null to a Set<string> (e.g.,
installingBuiltins) and update handleInstallBuiltin to add the builtinId to the
set before the async post, remove it in finally, and use set state updates (prev
=> new Set(prev).add(...) / delete(...)) to avoid races; also update the button
rendering logic that currently checks installingBuiltin === id to instead check
installingBuiltins.has(id) so each install button shows its own spinner/disabled
state without being overwritten.
In `@apps/web/src/hooks/useIntegrations.ts`:
- Around line 60-65: Export the AvailableBuiltin interface so downstream
consumers can reference it; update the declaration of AvailableBuiltin to be
exported (export interface AvailableBuiltin ...) in the same module where
useAvailableBuiltins is defined and ensure any local imports/usages still
compile — this lets components import AvailableBuiltin from the hook file just
like other exported types (e.g., SafeProvider/SafeConnection).
1 participant
Summary
/settings/integrationslists uninstalled builtins with Install buttonsintegrationProviderstable, after which the existing Connect → OAuth flow worksChanges
GET /api/integrations/providers/available— returns builtins not yet in DBPOST /api/integrations/providers/install— copies builtin config into DB (admin-only viaverifyAdminAuth+isAdminAuthError, which handles session + CSRF + role-version validation)useAvailableBuiltins()SWR hook inuseIntegrations.tsReview feedback addressed (e8dce77)
verifyAdminAuth+isAdminAuthErrorinstead of session-only auth. Also fixed the!adminAuthbug (bothVerifiedUserandNextResponseare truthy, so the check never caught errors)builtinsErrorwith a red error banner instead of showing misleading "all installed" when the fetch failsTest plan
/settings/integrations— confirm "Built-in Integrations" section shows GitHub and Generic Webhook🤖 Generated with Claude Code
Summary by CodeRabbit