refactor(integrations): unify OAuth integration routes#3379
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Executive SummaryA dead Overview
Issue Details (click to expand)WARNING
Resolved Issues
Incremental changes reviewed (4 new commits)
Files Reviewed (49 files)
Fix these issues in Kilo Cloud Reviewed by claude-sonnet-4.6 · 1,274,989 tokens Review guidance: REVIEW.md from base branch |
| if (authFailedResponse) { | ||
| return redirectToSignInForOAuthConnect( | ||
| request, | ||
| request.nextUrl.searchParams.has('clientSecret') |
There was a problem hiding this comment.
WARNING: Dead condition — clientSecret is never present in GET query params
The GET handler no longer reads clientSecret from the URL (it was moved to POST-only). This .has('clientSecret') check can never be true, so redirectToSignInForOAuthConnect always receives undefined as its second argument. The branch that resolves to buildGitLabDetailCallbackPath(organizationId) is unreachable dead code.
Either remove the ternary and pass undefined directly, or, if the intent is to restore the self-hosted GitLab sign-in resume behaviour (redirect back to the detail page after sign-in), derive the callback path from instanceUrl or the organizationId that is already read from the query string.
| ? await getDefaultAllowedModel(owner.id, SLACK_DEFAULT_MODEL) | ||
| : SLACK_DEFAULT_MODEL; | ||
| ? await getDefaultAllowedModel(owner.id, DEFAULT_BOT_MODEL) | ||
| : DEFAULT_BOT_MODEL; |
There was a problem hiding this comment.
Is this Slack default model change intended? New personal Slack installations now default to kilo-auto/frontier through DEFAULT_BOT_MODEL instead of the previous kilo-auto/free, which can change credit/cost behavior for Slack bot usage.
| @@ -150,19 +147,9 @@ export function SlackIntegrationDetails({ | |||
| } | |||
| }, [success, error]); | |||
|
|
|||
There was a problem hiding this comment.
Can the removed getOAuthUrl tRPC procedures stay as compatibility shims for one deploy? Existing pages loaded before this rollout still call them when the user clicks Connect/Re-install; this new redirect path only helps freshly loaded clients.
| const instanceUrl = searchParams.get('instanceUrl') || undefined; | ||
| const returnToParam = searchParams.get('returnTo') || undefined; | ||
| const returnTo = returnToParam ? validateReturnPath(returnToParam) : null; | ||
|
|
There was a problem hiding this comment.
Could this path keep a short-lived compatibility branch for authenticated GETs with clientId/clientSecret? Old client bundles still send self-hosted GitLab OAuth credentials as query params, and this now drops them before building the OAuth URL, so those starts fail with oauth_init_failed during rollout.
2 participants
Summary
/api/integrations/[platform]/connectand/api/integrations/[platform]/callbackroutes while keeping GitHub App and Google OAuth on their special routes.getOAuthUrltRPC initiation with shared connect paths, and unified user/org platform detail pages behind shared dynamic UI.returnToredirects, encoded callback errors, and self-hosted GitLab credential handling via an authenticated POST/Redis handoff instead of query-string secrets.Verification
Visual Changes
N/A
Reviewer Notes
returnTopaths closely, especially unauthenticated connect resumes and platform-specific success/error query parameters.clientSecrethandoff; the GET connect route intentionally no longer consumesclientSecret.