feat: github oauth #972
feat: github oauth #972
Conversation
Updated GitHub sign-in to include a callback URL for redirecting users to the dashboard after authentication. Refactored `apiAuth` configuration for better modularity and flexibility. BREAKING CHANGE: Adjustments in auth configuration may require updates to dependent modules.
WalkthroughIntroduces GitHub social authentication across API and UI, updates the post-sign-up initialization flow, adjusts tests to reflect an extra API key, adds related environment variables in docker-compose files, and aligns dev scripts to load env files via nodemon. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI (Login/Signup)
participant Auth as API Auth Service
participant GitHub as GitHub OAuth
participant DB as Database
User->>UI: Click "Sign in/Sign up with GitHub"
UI->>Auth: signIn.social(provider="github", callback=dashboard)
Auth->>GitHub: OAuth authorize (client_id, redirect_uri, scopes)
GitHub-->>Auth: OAuth callback (code)
Auth->>GitHub: Exchange code for token
GitHub-->>Auth: Access token + user info
alt First-time user (no active org)
Auth->>DB: Begin transaction
Auth->>DB: Create user (if needed) & verify email (self-hosted)
Auth->>DB: Create default org & link user
Auth->>DB: Create default project (credits mode)
Auth->>DB: Create playground API key (prefixed token)
Auth->>DB: Commit transaction
else Returning user with active org
Auth-->>Auth: Skip org/project/key creation
end
Auth-->>UI: Session + redirect to dashboard
UI-->>User: Redirect to dashboard
note right of Auth: Brevo contact creation still triggered on email verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![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 (2)
apps/api/src/auth/config.ts (1)
629-640: Hardcoded playground key description may cause confusion.The API key is created with a hardcoded description "Auto-generated playground key", but this key is also usable outside the playground. Consider a more generic description or making it configurable.
await tx.insert(tables.apiKey).values({ projectId: project.id, token: token, - description: "Auto-generated playground key", + description: "Auto-generated API key", usageLimit: null, // No limit for playground key });apps/ui/src/app/login/page.tsx (1)
202-219: Consider extracting GitHub OAuth logic to reduce duplication.The GitHub sign-in flow is duplicated between
apps/ui/src/app/login/page.tsxandapps/ui/src/app/signup/page.tsx. Both implement nearly identical logic for callingsignIn.social, error handling, and loading state management.Consider creating a shared hook or utility function:
// apps/ui/src/hooks/useGithubAuth.ts export function useGithubAuth() { const { signIn } = useAuth(); const router = useRouter(); const signInWithGithub = async (callbackURL: string) => { const res = await signIn.social({ provider: "github", callbackURL, }); if (res?.error) { throw new Error(res.error.message || "Failed to sign in with GitHub"); } return res; }; return { signInWithGithub }; }Then use it in both login and signup pages to eliminate duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/api/package.json(1 hunks)apps/api/src/auth/config.ts(3 hunks)apps/gateway/package.json(1 hunks)apps/playground/src/app/login/page.tsx(2 hunks)apps/playground/src/app/signup/page.tsx(0 hunks)apps/ui/src/app/login/page.tsx(2 hunks)apps/ui/src/app/signup/page.tsx(3 hunks)apps/worker/package.json(1 hunks)infra/docker-compose.split.local.yml(1 hunks)infra/docker-compose.split.yml(1 hunks)infra/docker-compose.unified.local.yml(1 hunks)infra/docker-compose.unified.yml(1 hunks)
💤 Files with no reviewable changes (1)
- apps/playground/src/app/signup/page.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never useanyoras anyin this pure TypeScript project unless absolutely necessary
Always use top-levelimport; never userequireor dynamic imports
Files:
apps/ui/src/app/login/page.tsxapps/playground/src/app/login/page.tsxapps/api/src/auth/config.tsapps/ui/src/app/signup/page.tsx
apps/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/ui/**/*.{ts,tsx}: Usenext/linkfor links andnext/navigation's router for programmatic navigation in the UI
Use cookies for user settings not stored in the database to ensure SSR works
apps/ui/**/*.{ts,tsx}: Usenext/linkfor links and Next.jsnext/navigationrouter for programmatic navigation
Use cookies for user settings that are not saved in the database to ensure SSR works
Files:
apps/ui/src/app/login/page.tsxapps/ui/src/app/signup/page.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Always use top-level import; never use require() or dynamic imports
Files:
apps/ui/src/app/login/page.tsxapps/playground/src/app/login/page.tsxapps/api/src/auth/config.tsapps/ui/src/app/signup/page.tsx
{apps/{api,gateway},packages/db}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
For reads, use
db().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/api/src/auth/config.ts
{apps/{api,gateway},packages/db}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
.findMany() or db().query.
{apps/{api,gateway},packages/db}/**/*.ts: Use Drizzle ORM with the latest object syntax for all database access
For reads, use db().query..findFirst() Files:
apps/api/src/auth/config.tsapps/{api,gateway}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Zod schemas for validation in backend services
Files:
apps/api/src/auth/config.ts🧬 Code graph analysis (1)
apps/ui/src/app/login/page.tsx (1)
apps/ui/src/lib/components/use-toast.ts (1)
toast(194-194)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build-split (playground, linux/amd64)
- GitHub Check: build-split (ui, linux/amd64)
- GitHub Check: build-split (gateway, linux/amd64)
- GitHub Check: build-split (docs, linux/amd64)
- GitHub Check: build-split (api, linux/amd64)
- GitHub Check: build-unified (linux/amd64)
- GitHub Check: generate / run
- GitHub Check: build / run
- GitHub Check: test / run
- GitHub Check: lint / run
- GitHub Check: e2e-shards (4)
- GitHub Check: e2e-shards (5)
- GitHub Check: e2e-shards (2)
- GitHub Check: e2e-shards (1)
- GitHub Check: e2e-shards (3)
- GitHub Check: autofix
🔇 Additional comments (11)
apps/worker/package.json (1)
20-20: Consistent dev script pattern across services.The updated dev script aligns with the pattern used in gateway and api services, ensuring consistent environment variable loading during development.
apps/api/package.json (1)
12-12: Dev script updated to support GitHub OAuth configuration.The updated dev script ensures environment variables (including the new GitHub OAuth credentials) are loaded during development, supporting the better-auth GitHub social provider configuration.
apps/gateway/package.json (1)
18-18: Verify Node.js version compatibility for --env-file flag.The
--env-fileflag requires Node.js 20.6.0 or higher. The repository does not currently specify a minimum Node.js version in package.json engines fields, .nvmrc, or .node-version files. Ensure all developers and deployment environments use a compatible Node.js version, and consider documenting this requirement in the root package.json engines field or a .nvmrc file.apps/playground/src/app/login/page.tsx (1)
124-124: Consistent redirect target for passkey authentication.The passkey sign-in now redirects to "/" matching the email sign-in flow at line 84. This ensures both authentication methods provide a consistent user experience.
apps/api/src/auth/config.ts (3)
564-589: LGTM! Idempotent organization setup prevents duplicate resources.The refactored after-sign-up flow correctly checks for existing organizations before creating new ones, ensuring idempotency across both email and social sign-in flows. The early return pattern when active organizations exist is clean and efficient.
594-603: Auto-verification for self-hosted: ensure this aligns with security requirements.The code automatically verifies emails for self-hosted deployments without sending a verification email. While this improves UX for self-hosted installations, ensure this bypass is intentional and documented for security-conscious deployments.
Confirm that auto-verification for self-hosted installations is the desired behavior, especially when combined with GitHub OAuth (which provides some identity verification). Consider documenting this behavior in deployment guides.
592-642: Confirmed: Race condition can create duplicate organizations for concurrent sign-ins.The check at lines 573-589 occurs outside the transaction, allowing two concurrent first-time sign-ins to both see zero organizations and proceed to create duplicates. The
userOrganizationtable lacks a unique constraint on(userId, organizationId), so the database won't prevent this.Evidence from verification:
- Schema shows only separate indexes on
userIdandorganizationId(lines packages/db/src/schema.ts:138-139), not a composite unique constraint- Other tables use
unique().on(table.col1, table.col2)for composite constraints (e.g.,providerKeyat line 319), butuserOrganizationdoes not- The codebase demonstrates awareness of concurrency issues elsewhere (apps/worker/src/worker.ts:319 mentions "row-level locking to prevent concurrent processing")
Solutions to consider:
- Add a unique constraint:
unique().on(table.userId, table.organizationId)in the schema- Move the organizations check inside the transaction with
FOR UPDATElocking- Use an upsert pattern with
ON CONFLICT DO NOTHINGinfra/docker-compose.split.yml (1)
79-81: LGTM! Production config correctly requires explicit GitHub OAuth credentials.The environment variables are properly configured without default values, ensuring that production deployments explicitly provide these credentials. This aligns with security best practices for production configurations.
Note: Ensure deployment documentation is updated to reflect these new required environment variables.
infra/docker-compose.unified.yml (1)
32-33: LGTM! Ensure production values override placeholders.The GitHub OAuth credentials follow the established pattern for auth environment variables. The placeholder defaults are appropriate for documentation purposes.
Ensure that production deployments override these placeholder values with actual GitHub OAuth credentials.
infra/docker-compose.split.local.yml (1)
94-95: LGTM!The GitHub OAuth environment variables are correctly added to the API service. The approach of sourcing from shell environment without defaults is consistent with other sensitive configuration in this local deployment file.
apps/ui/src/app/signup/page.tsx (1)
176-183: Nice UI addition!The separator/divider between email sign-up and social sign-in is a good UX pattern that clearly distinguishes the two authentication methods.
| socialProviders: { | ||
| github: { | ||
| clientId: process.env.GITHUB_CLIENT_ID!, | ||
| clientSecret: process.env.GITHUB_CLIENT_SECRET!, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Verify GitHub OAuth credentials are provided at startup.
The GitHub OAuth configuration uses non-null assertions (!) on process.env.GITHUB_CLIENT_ID and process.env.GITHUB_CLIENT_SECRET. If these environment variables are missing, the application will crash at runtime.
Consider adding explicit validation at startup to provide a clearer error message:
+const githubClientId = process.env.GITHUB_CLIENT_ID;
+const githubClientSecret = process.env.GITHUB_CLIENT_SECRET;
+
+if (!githubClientId || !githubClientSecret) {
+ throw new Error(
+ "GitHub OAuth configuration missing: GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET must be set",
+ );
+}
+
socialProviders: {
github: {
- clientId: process.env.GITHUB_CLIENT_ID!,
- clientSecret: process.env.GITHUB_CLIENT_SECRET!,
+ clientId: githubClientId,
+ clientSecret: githubClientSecret,
},
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/auth/config.ts around lines 423-428, the code uses non-null
assertions for GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET which will cause an
unclear runtime crash if env vars are missing; replace the assertions with an
explicit startup validation: check process.env.GITHUB_CLIENT_ID and
process.env.GITHUB_CLIENT_SECRET, and if either is missing log/throw a clear
error naming the missing variable(s) (or exit the process with a descriptive
message) so the app fails fast with actionable diagnostics instead of a generic
runtime crash.
| <div className="grid grid-cols-1 gap-3"> | ||
| <Button | ||
| onClick={async () => { | ||
| setIsLoading(true); | ||
| try { | ||
| const res = await signIn.social({ | ||
| provider: "github", | ||
| callbackURL: | ||
| location.protocol + "//" + location.host + "/dashboard", | ||
| }); | ||
| if (res?.error) { | ||
| toast({ | ||
| title: res.error.message || "Failed to sign in with GitHub", | ||
| variant: "destructive", | ||
| }); | ||
| } | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }} | ||
| variant="outline" | ||
| className="w-full" | ||
| disabled={isLoading} | ||
| > | ||
| {isLoading ? ( | ||
| <Loader2 className="mr-2 h-4 w-4 animate-spin" /> | ||
| ) : ( | ||
| <Github className="mr-2 h-4 w-4" /> | ||
| )} | ||
| Sign in with GitHub | ||
| </Button> | ||
| <Button | ||
| onClick={handlePasskeySignIn} | ||
| variant="outline" | ||
| className="w-full" | ||
| disabled={isLoading} | ||
| > | ||
| {isLoading ? ( | ||
| <Loader2 className="mr-2 h-4 w-4 animate-spin" /> | ||
| ) : ( | ||
| <KeySquare className="mr-2 h-4 w-4" /> | ||
| )} | ||
| Sign in with passkey | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
Shared loading state will disable both sign-in buttons simultaneously.
Both the GitHub and passkey sign-in buttons share a single isLoading state. When a user clicks either button, both become disabled. This creates a confusing UX where the user cannot tell which authentication method is processing.
Consider introducing separate loading states:
export default function Login() {
const queryClient = useQueryClient();
const router = useRouter();
const posthog = usePostHog();
- const [isLoading, setIsLoading] = useState(false);
+ const [isEmailLoading, setIsEmailLoading] = useState(false);
+ const [isGithubLoading, setIsGithubLoading] = useState(false);
+ const [isPasskeyLoading, setIsPasskeyLoading] = useState(false);
const { signIn } = useAuth();Then update each button to use its specific loading state and disable when any method is loading:
<Button
onClick={async () => {
- setIsLoading(true);
+ setIsGithubLoading(true);
try {
const res = await signIn.social({
provider: "github",
callbackURL: location.protocol + "//" + location.host + "/dashboard",
});
if (res?.error) {
toast({
title: res.error.message || "Failed to sign in with GitHub",
variant: "destructive",
});
}
} finally {
- setIsLoading(false);
+ setIsGithubLoading(false);
}
}}
variant="outline"
className="w-full"
- disabled={isLoading}
+ disabled={isEmailLoading || isGithubLoading || isPasskeyLoading}
>
- {isLoading ? (
+ {isGithubLoading ? (
<Loader2 className="mr-2 h-4 w-4 animate-spin" />
) : (
<Github className="mr-2 h-4 w-4" />
)}
Sign in with GitHub
</Button>Apply similar changes to the passkey button and email form submit button.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/ui/src/app/login/page.tsx around lines 200 to 244, both GitHub and
passkey sign-in buttons use a shared isLoading state causing both to disable
when one is clicked; create separate loading flags (e.g., isLoadingGithub,
isLoadingPasskey, and update email form to use isLoadingEmail), wire each button
and the email submit to its own flag (set true at start of its handler, reset in
finally), and change each button's disabled prop and spinner conditional to use
its specific loading flag so only the initiating method shows loading and is
disabled.
| <Button | ||
| onClick={async () => { | ||
| setIsLoading(true); | ||
| try { | ||
| const res = await signIn.social({ | ||
| provider: "github", | ||
| callbackURL: | ||
| location.protocol + "//" + location.host + "/dashboard", | ||
| }); | ||
| if (res?.error) { | ||
| toast({ | ||
| title: res.error.message || "Failed to sign up with GitHub", | ||
| variant: "destructive", | ||
| }); | ||
| } | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }} | ||
| variant="outline" | ||
| className="w-full" | ||
| disabled={isLoading} | ||
| > | ||
| {isLoading ? ( | ||
| <Loader2 className="mr-2 h-4 w-4 animate-spin" /> | ||
| ) : ( | ||
| <Github className="mr-2 h-4 w-4" /> | ||
| )} | ||
| Sign up with GitHub | ||
| </Button> |
There was a problem hiding this comment.
Shared loading state will disable both sign-up options simultaneously.
The GitHub and email sign-up flows share a single isLoading state. When a user clicks the GitHub button, the email form's submit button also becomes disabled, creating a confusing UX.
Apply the same fix as recommended for the login page:
export default function Signup() {
const queryClient = useQueryClient();
const router = useRouter();
const posthog = usePostHog();
- const [isLoading, setIsLoading] = useState(false);
+ const [isEmailLoading, setIsEmailLoading] = useState(false);
+ const [isGithubLoading, setIsGithubLoading] = useState(false);
const { signUp, signIn } = useAuth();Then update the GitHub button:
<Button
onClick={async () => {
- setIsLoading(true);
+ setIsGithubLoading(true);
try {
const res = await signIn.social({
provider: "github",
callbackURL: location.protocol + "//" + location.host + "/dashboard",
});
if (res?.error) {
toast({
title: res.error.message || "Failed to sign up with GitHub",
variant: "destructive",
});
}
} finally {
- setIsLoading(false);
+ setIsGithubLoading(false);
}
}}
variant="outline"
className="w-full"
- disabled={isLoading}
+ disabled={isEmailLoading || isGithubLoading}
>
- {isLoading ? (
+ {isGithubLoading ? (
<Loader2 className="mr-2 h-4 w-4 animate-spin" />
) : (
<Github className="mr-2 h-4 w-4" />
)}
Sign up with GitHub
</Button>And update the email form to use isEmailLoading.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/ui/src/app/signup/page.tsx around lines 184 to 213, the GitHub and email
signup flows share a single isLoading state which disables both buttons
simultaneously; split the loading state into dedicated flags (e.g.,
isGithubLoading and isEmailLoading), set the appropriate flag in the respective
handlers (set true before the async call, reset in finally), update the GitHub
Button to use isGithubLoading for disabled and spinner rendering, and update the
email form submit to use isEmailLoading instead of the shared isLoading so each
control is disabled only when its own action is running.
Adjusted the expected `apiKeys` length from 1 to 2 in the test case for `keys-api` to reflect the updated functionality or data changes.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/routes/keys-api.spec.ts (2)
127-157: Fix the test logic to account for the playground API key.With the additional playground API key created during
createTestUser(), the starting count is now 2 keys (test key + playground key). The loop creates 4 more keys (i=2 to i=5), resulting in 6 total keys before the POST attempt. This exceeds the free plan limit of 5, making the test logic incorrect.Apply this diff to fix the test:
test("POST /keys/api should enforce plan limits", async () => { - // Create 4 more API keys to reach the free plan limit of 5 - for (let i = 2; i <= 5; i++) { + // Create 3 more API keys to reach the free plan limit of 5 (2 existing + 3 new = 5) + for (let i = 2; i <= 4; i++) { await db.insert(tables.apiKey).values({ id: `test-api-key-id-${i}`, token: `test-token-${i}`, projectId: "test-project-id", description: `Test API Key ${i}`, status: "active", }); } // Try to create the 6th API key, should fail for free plan const res = await app.request("/keys/api", {
159-195: Fix the test logic to account for the playground API key.With the additional playground API key created during
createTestUser(), the starting count is now 2 keys (test key + playground key). The loop creates 19 more keys (i=2 to i=20), resulting in 21 total keys before the POST attempt. This exceeds the pro plan limit of 20, making the test logic incorrect.Apply this diff to fix the test:
test("POST /keys/api should allow more keys for pro plan", async () => { // Update organization to pro plan await db .update(tables.organization) .set({ plan: "pro" }) .where(eq(tables.organization.id, "test-org-id")); - // Create 19 more API keys to reach 20 total (pro plan limit) - for (let i = 2; i <= 20; i++) { + // Create 18 more API keys to reach 20 total (2 existing + 18 new = 20) + for (let i = 2; i <= 19; i++) { await db.insert(tables.apiKey).values({ id: `test-api-key-id-${i}`, token: `test-token-${i}`, projectId: "test-project-id", description: `Test API Key ${i}`, status: "active", }); } // Try to create the 21st API key, should fail even for pro plan const res = await app.request("/keys/api", {
🧹 Nitpick comments (1)
apps/api/src/routes/keys-api.spec.ts (1)
85-96: Verify API key ordering assumption and validate both keys.The test now expects 2 API keys but only validates
apiKeys[0].description. This assumes the "Test API Key" will always be first in the array, which may not hold if keys are sorted by timestamp, ID, or other criteria.Consider using a more robust assertion:
test("GET /keys/api", async () => { const res = await app.request("/keys/api", { headers: { Cookie: token, }, }); expect(res.status).toBe(200); const json = await res.json(); expect(json).toHaveProperty("apiKeys"); expect(json.apiKeys.length).toBe(2); - expect(json.apiKeys[0].description).toBe("Test API Key"); + const descriptions = json.apiKeys.map(key => key.description); + expect(descriptions).toContain("Test API Key"); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/routes/keys-api.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never useanyoras anyin this pure TypeScript project unless absolutely necessary
Always use top-levelimport; never userequireor dynamic imports
Files:
apps/api/src/routes/keys-api.spec.ts
{apps/{api,gateway},packages/db}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
For reads, use
db().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/api/src/routes/keys-api.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Name unit test files with the
.spec.tssuffixPlace unit tests in files named *.spec.ts (Vitest)
Files:
apps/api/src/routes/keys-api.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Always use top-level import; never use require() or dynamic imports
Files:
apps/api/src/routes/keys-api.spec.ts
{apps/{api,gateway},packages/db}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
.findMany() or db().query.
{apps/{api,gateway},packages/db}/**/*.ts: Use Drizzle ORM with the latest object syntax for all database access
For reads, use db().query..findFirst() Files:
apps/api/src/routes/keys-api.spec.tsapps/{api,gateway}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Zod schemas for validation in backend services
Files:
apps/api/src/routes/keys-api.spec.ts⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-split (playground, linux/amd64)
- GitHub Check: build-split (ui, linux/amd64)
- GitHub Check: build-split (docs, linux/amd64)
- GitHub Check: build-unified (linux/amd64)
- GitHub Check: build / run
- GitHub Check: test / run
- GitHub Check: lint / run
- GitHub Check: generate / run
- GitHub Check: autofix
- GitHub Check: e2e-shards (2)
- GitHub Check: e2e-shards (1)
- GitHub Check: e2e-shards (4)
- GitHub Check: e2e-shards (5)
- GitHub Check: e2e-shards (3)
Corrected the index of the `apiKeys` array in the test case to validate the correct key description. This ensures the test aligns with the actual data structure.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/src/routes/keys-api.spec.ts(1 hunks)infra/docker-compose.split.local.yml(1 hunks)infra/docker-compose.split.yml(1 hunks)infra/docker-compose.unified.local.yml(1 hunks)infra/docker-compose.unified.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- infra/docker-compose.unified.yml
- infra/docker-compose.split.yml
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never useanyoras anyin this pure TypeScript project unless absolutely necessary
Always use top-levelimport; never userequireor dynamic imports
Files:
apps/api/src/routes/keys-api.spec.ts
{apps/{api,gateway},packages/db}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
For reads, use
db().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/api/src/routes/keys-api.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Name unit test files with the
.spec.tssuffixPlace unit tests in files named *.spec.ts (Vitest)
Files:
apps/api/src/routes/keys-api.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Always use top-level import; never use require() or dynamic imports
Files:
apps/api/src/routes/keys-api.spec.ts
{apps/{api,gateway},packages/db}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
.findMany() or db().query.
{apps/{api,gateway},packages/db}/**/*.ts: Use Drizzle ORM with the latest object syntax for all database access
For reads, use db().query..findFirst() Files:
apps/api/src/routes/keys-api.spec.tsapps/{api,gateway}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Zod schemas for validation in backend services
Files:
apps/api/src/routes/keys-api.spec.ts⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-split (ui, linux/amd64)
- GitHub Check: build-split (api, linux/amd64)
- GitHub Check: build-split (docs, linux/amd64)
- GitHub Check: build-split (playground, linux/amd64)
- GitHub Check: build-unified (linux/amd64)
- GitHub Check: test / run
- GitHub Check: lint / run
- GitHub Check: build / run
- GitHub Check: generate / run
- GitHub Check: e2e-shards (5)
- GitHub Check: e2e-shards (3)
- GitHub Check: e2e-shards (1)
- GitHub Check: e2e-shards (4)
- GitHub Check: e2e-shards (2)
- GitHub Check: autofix
🔇 Additional comments (3)
infra/docker-compose.split.local.yml (1)
93-94: Good approach for required credentials.These environment variables correctly omit placeholder defaults, ensuring the container fails fast if GitHub OAuth credentials aren't provided. This makes misconfiguration immediately apparent, which is preferable to runtime errors with placeholder values.
apps/api/src/routes/keys-api.spec.ts (2)
159-195: Verify the API key count logic accounts for the playground key.Similar to the free plan test, this test now has 2 initial keys. The comment on line 166 states "Create 19 more API keys to reach 20 total (pro plan limit)", but 2 + 19 = 21 keys, not 20.
If the playground key counts toward the limit, the loop should create only 18 additional keys (lines 167-175) to reach exactly 20, so the 21st creation attempt fails as expected.
127-157: Ignore the raised concern:createTestUser()does not insert any API keys, so the spec’s loop (i=2..5) correctly adds four keys to the one created inbeforeEach—totaling five before the POST, and making the sixth request fail as intended.Likely an incorrect or invalid review comment.
| expect(json.apiKeys.length).toBe(2); | ||
| expect(json.apiKeys[1].description).toBe("Test API Key"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Verify both API keys explicitly instead of relying on array index.
The test now assumes that the playground key is at index 0 and the test key is at index 1. This assumption is fragile and could break if the ordering changes (e.g., if the database query returns results in a different order).
Consider using a more robust assertion that verifies both keys are present:
expect(res.status).toBe(200);
const json = await res.json();
expect(json).toHaveProperty("apiKeys");
- expect(json.apiKeys.length).toBe(2);
- expect(json.apiKeys[1].description).toBe("Test API Key");
+ expect(json.apiKeys.length).toBe(2);
+ const testKey = json.apiKeys.find(k => k.description === "Test API Key");
+ const playgroundKey = json.apiKeys.find(k => k.description === "Playground");
+ expect(testKey).toBeDefined();
+ expect(playgroundKey).toBeDefined();📝 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.
| expect(json.apiKeys.length).toBe(2); | |
| expect(json.apiKeys[1].description).toBe("Test API Key"); | |
| expect(res.status).toBe(200); | |
| const json = await res.json(); | |
| expect(json).toHaveProperty("apiKeys"); | |
| expect(json.apiKeys.length).toBe(2); | |
| const testKey = json.apiKeys.find(k => k.description === "Test API Key"); | |
| const playgroundKey = json.apiKeys.find(k => k.description === "Playground"); | |
| expect(testKey).toBeDefined(); | |
| expect(playgroundKey).toBeDefined(); |
🤖 Prompt for AI Agents
In apps/api/src/routes/keys-api.spec.ts around lines 94 to 95, the test assumes
a stable ordering of apiKeys by indexing into json.apiKeys[1]; replace this
fragile index-based assertion with order-independent checks that both expected
keys are present — e.g., assert json.apiKeys.length === 2 and then verify that
one entry has description "Playground API Key" and another has description "Test
API Key" using existence checks (array.some/find) or by mapping descriptions to
a set and asserting it equals the expected set; update the assertions
accordingly so the test no longer relies on array order.
| - GITHUB_CLIENT_ID=${GITHUB_CLIENT_ID:-your-github-client-id} | ||
| - GITHUB_CLIENT_SECRET=${GITHUB_CLIENT_SECRET:-your-github-client-secret} |
There was a problem hiding this comment.
Past review concern remains unaddressed.
The placeholder defaults (your-github-client-id and your-github-client-secret) can still cause runtime errors if GitHub OAuth is attempted without proper configuration. This issue was previously flagged but hasn't been resolved.
Additionally, there's an inconsistency: infra/docker-compose.split.local.yml (lines 93-94) omits these placeholder defaults and relies on environment variables directly, which is the preferred approach—it fails fast if credentials aren't provided.
Recommendation: Remove the placeholder defaults to match the approach in docker-compose.split.local.yml:
# Authentication
- PASSKEY_RP_ID=${PASSKEY_RP_ID:-localhost}
- PASSKEY_RP_NAME=${PASSKEY_RP_NAME:-LLMGateway}
- AUTH_SECRET=${AUTH_SECRET:-your-secret-key}
-- GITHUB_CLIENT_ID=${GITHUB_CLIENT_ID:-your-github-client-id}
-- GITHUB_CLIENT_SECRET=${GITHUB_CLIENT_SECRET:-your-github-client-secret}
+# GitHub OAuth credentials (required for GitHub sign-in)
+- GITHUB_CLIENT_ID=${GITHUB_CLIENT_ID}
+- GITHUB_CLIENT_SECRET=${GITHUB_CLIENT_SECRET}This ensures the container fails immediately if the credentials aren't set, making misconfiguration obvious during development.
📝 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.
| - GITHUB_CLIENT_ID=${GITHUB_CLIENT_ID:-your-github-client-id} | |
| - GITHUB_CLIENT_SECRET=${GITHUB_CLIENT_SECRET:-your-github-client-secret} | |
| # Authentication | |
| - PASSKEY_RP_ID=${PASSKEY_RP_ID:-localhost} | |
| - PASSKEY_RP_NAME=${PASSKEY_RP_NAME:-LLMGateway} | |
| - AUTH_SECRET=${AUTH_SECRET:-your-secret-key} | |
| # GitHub OAuth credentials (required for GitHub sign-in) | |
| - GITHUB_CLIENT_ID=${GITHUB_CLIENT_ID} | |
| - GITHUB_CLIENT_SECRET=${GITHUB_CLIENT_SECRET} |
🤖 Prompt for AI Agents
In infra/docker-compose.unified.local.yml around lines 33-34, the environment
entries currently use placeholder defaults which mask missing GitHub OAuth
credentials; remove the fallback defaults so the variables are referenced
directly (e.g., GITHUB_CLIENT_ID=${GITHUB_CLIENT_ID} and
GITHUB_CLIENT_SECRET=${GITHUB_CLIENT_SECRET}) to match
infra/docker-compose.split.local.yml and ensure the container fails fast when
credentials are not provided.
Summary by CodeRabbit
New Features
Tests
Chores