Repository navigation
Redirect bare /connect/oauth visits to the OAuth guide - #1352
Merged
Merged
Conversation
Every working visit carries provider (agent-built setup URLs and built-in connects), code (the provider's success redirect; config is restored from sessionStorage), or error (the provider's denial redirect). A visit with none of them has no flow to set up or resume, so send it to /guides/oauth instead of rendering the blank config form. The server handler checks before the session gate (the guide is public) and the client route mirrors the check for SPA-internal navigations. Covered by handler tests for all three param shapes. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughBare ChangesOAuth guide redirect
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
|
🔎 Preview deployed: https://kody-pr-1352.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: 1
🤖 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/connect-oauth.ts`:
- Around line 15-18: Replace value-based get() checks with URLSearchParams.has()
in isBareConnectOauthVisit and the matching client-side OAuth classification at
packages/worker/client/routes/connect-oauth.tsx:96-105, so empty-but-present
provider, code, and error parameters are recognized as OAuth flows. Add coverage
in packages/worker/src/app/handlers/connect-oauth.node.test.ts:19-33 for each
empty-but-present parameter case.
🪄 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: f71fd27d-f9c6-420e-af2b-92287697fc1c
📒 Files selected for processing (3)
packages/worker/client/routes/connect-oauth.tsxpackages/worker/src/app/handlers/connect-oauth.node.test.tspackages/worker/src/app/handlers/connect-oauth.ts
Comment on lines
+15
to
+18
| export function isBareConnectOauthVisit(url: URL): boolean { | ||
| const params = url.searchParams | ||
| return !params.get('provider') && !params.get('code') && !params.get('error') | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg '(^|/)connect-oauth(\.ts|\.tsx|\.node\.test\.ts)$|connect-oauth' || true
printf '\nRelevant snippets:\n'
for f in \
packages/worker/src/app/handlers/connect-oauth.ts \
packages/worker/client/routes/connect-oauth.tsx \
packages/worker/src/app/handlers/connect-oauth.node.test.ts
do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,140p'
fi
done
printf '\nSearch for isBareConnectOauthVisit/tests:\n'
rg -n "isBareConnectOauthVisit|bareConnectOauth|has\\(['\"](provider|code|error)\\)|get\\(['\"](provider|code|error)\\)" packages/worker || true
printf '\nURLSearchParams behavior probe:\n'
node - <<'JS'
for (const value of ["provider", "code", "error"]) {
const params = new URLSearchParams(`${value}=`)
console.log(`${value}=: get='${params.get(value)}' has=${params.has(value)} !get=${!!params.get(value)}`)
}
console.log('empty: get=', new URLSearchParams('provider=code=error=').get('provider'), ', has=', new URLSearchParams('provider=code=error=').has('provider'))
JSRepository: kentcdodds/kody
Length of output: 658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant snippets:\n'
for f in \
packages/worker/src/app/handlers/connect-oauth.ts \
packages/worker/client/routes/connect-oauth.tsx \
packages/worker/src/app/handlers/connect-oauth.node.test.ts
do
if [ -f "$f" ]; then
echo "--- $f"
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,140p'
fi
done
printf '\nSearch for isBareConnectOauthVisit/tests:\n'
rg -n "isBareConnectOauthVisit|bareConnectOauth|has\\(['\"](provider|code|error)\\)|get\\(['\"](provider|code|error)\\)" packages/worker || true
printf '\nURLSearchParams behavior probe:\n'
node - <<'JS'
for (const value of ["provider", "code", "error"]) {
const params = new URLSearchParams(`${value}=`)
console.log(`${value}=: get='${params.get(value)}' has=${params.has(value)} !get=${!!params.get(value)}`)
}
console.log('empty: get=', new URLSearchParams('provider=code=error=').get('provider'), ', has=', new URLSearchParams('provider=code=error=').has('provider'))
JSRepository: kentcdodds/kody
Length of output: 9284
Use URLSearchParams.has() for OAuth flow classification.
get() returns '' for a present empty parameter, so ?provider=, ?code=, and ?error= are treated as bare visits and redirect to /guides/oauth. This contradicts the comments that describe visits containing provider, code, or error as OAuth flows.
Update the server-side check at packages/worker/src/app/handlers/connect-oauth.ts#L15-L18, the matching client-side check at packages/worker/client/routes/connect-oauth.tsx#L96-L105, and add coverage for the empty-but-present parameter cases.
Proposed fix
- return !params.get('provider') && !params.get('code') && !params.get('error')
+ return !params.has('provider') && !params.has('code') && !params.has('error')📍 Affects 3 files
packages/worker/src/app/handlers/connect-oauth.ts#L15-L18(this comment)packages/worker/client/routes/connect-oauth.tsx#L96-L105packages/worker/src/app/handlers/connect-oauth.node.test.ts#L19-L33
🤖 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/connect-oauth.ts` around lines 15 - 18,
Replace value-based get() checks with URLSearchParams.has() in
isBareConnectOauthVisit and the matching client-side OAuth classification at
packages/worker/client/routes/connect-oauth.tsx:96-105, so empty-but-present
provider, code, and error parameters are recognized as OAuth flows. Add coverage
in packages/worker/src/app/handlers/connect-oauth.node.test.ts:19-33 for each
empty-but-present parameter case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
/connect/oauthwith no meaningful query parameters now 302s to/guides/oauthinstead of rendering the blank config form.Why
Every working visit to the page carries at least one of three parameters:
provider(agent-built setup URLs and built-in connects),code(the provider's success redirect — the redirect URI strips the config query, which sessionStorage restores client-side), orerror(the provider's denial redirect, e.g.access_denied). A visit with none of them has no flow to set up or resume — someone typed the URL or followed a bare link — and the blank form assumes an agent is driving. The guide explains how to actually use the page.How
isBareConnectOauthVisitin the server handler, checked before the session gate (the guide is public, so anonymous bare visitors skip the login bounce).Testing
npm run validategreen. Live smoke test on the dev server: bare visit →302 /guides/oauth;?provider=github(anonymous) → 302 to login as before.Summary by CodeRabbit
New Features
Bug Fixes