Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Google "dev credentials" mode to CLI auth client flows: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Prompt as "Interactive Prompt"
participant API as "Instant API"
participant Logger
User->>CLI: run `instant auth client add --provider google ...`
CLI->>Prompt: resolve credential mode (auto from flags or ask)
alt dev mode selected
Prompt-->>CLI: "dev"
CLI->>API: POST /oauth/clients { provider: google, use_shared_credentials: true, clientId: null, redirectUri: null }
API-->>CLI: 201 created (client with use_shared_credentials: true)
CLI->>Logger: log "Credentials: Instant dev credentials" and dev-specific messages
else custom mode selected
Prompt-->>CLI: "custom"
CLI->>API: POST /oauth/clients { provider: google, clientId, clientSecret, redirectUri }
API-->>CLI: 201 created (client with use_shared_credentials: false)
CLI->>Logger: log custom credential lines and redirect instructions
end
CLI-->>User: command output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/packages/cli/src/commands/auth/client/update.ts`:
- Around line 228-235: The updateGoogleToDevCredentials function currently only
sets useSharedCredentials and redirectTo; modify the payload passed to
updateOAuthClient to also wipe stored custom Google credentials by setting the
client_id/client_secret (clientId and clientSecret) fields to null/empty so the
secret is removed when switching back to dev mode; keep existing fields
(oauthClientId, useSharedCredentials, redirectTo) and add explicit clientId:
null and clientSecret: null in the updateOAuthClient call.
- Around line 301-330: The code currently still calls promptFlag for missing
Google credential fields when switchingFromShared even in --yes mode; update the
logic so --yes never triggers interactive prompts: either (A) add an early check
after computing switchingFromShared that if yes && switchingFromShared &&
!hasAnyUpdateFlag (or specifically missing required flags as determined by
hasFlag(opts, 'client-id')/hasFlag(opts, 'client-secret')/hasFlag(opts,
'custom-redirect-uri')/hasFlag(opts, 'dev-credentials')) then return yield*
BadArgsError.make(...) indicating required flags are missing, or (B) make each
promptFlag call’s promptIf and required conditions include !yes (e.g. promptIf:
!yes && (promptAll || switchingFromShared && !hasFlag(...)), required: !yes &&
(promptAll || switchingFromShared)), so promptFlag is never invoked when yes is
true; reference symbols: yes, hasAnyUpdateFlag, switchingFromShared, promptAll,
promptFlag, hasFlag, and BadArgsError.make.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c1cc8acd-b7d6-41b4-9ddd-3e27486ae20b
📒 Files selected for processing (10)
client/packages/cli/__tests__/authClientAddGoogle.test.tsclient/packages/cli/__tests__/authClientList.test.tsclient/packages/cli/__tests__/authClientUpdate.test.tsclient/packages/cli/src/commands/auth/client/add.tsclient/packages/cli/src/commands/auth/client/list.tsclient/packages/cli/src/commands/auth/client/update.tsclient/packages/cli/src/index.tsclient/packages/cli/src/lib/oauth.tsserver/src/instant/dash/routes.cljserver/src/instant/model/app_oauth_client.clj
Comment on lines
+228
to
+235
| const updateGoogleToDevCredentials = Effect.fn(function* ( | ||
| client: OAuthClientRow, | ||
| ) { | ||
| const response = yield* updateOAuthClient({ | ||
| oauthClientId: client.id, | ||
| useSharedCredentials: true, | ||
| redirectTo: null, | ||
| }); |
Contributor
There was a problem hiding this comment.
Clear stored custom Google credentials when switching back to dev mode.
This update only flips useSharedCredentials and clears redirectTo. The previous client_id / client_secret will still be retained unless the backend does extra cleanup, which defeats the point of switching away from custom credentials and unnecessarily keeps a secret around. Please wipe those fields as part of this transition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/packages/cli/src/commands/auth/client/update.ts` around lines 228 -
235, The updateGoogleToDevCredentials function currently only sets
useSharedCredentials and redirectTo; modify the payload passed to
updateOAuthClient to also wipe stored custom Google credentials by setting the
client_id/client_secret (clientId and clientSecret) fields to null/empty so the
secret is removed when switching back to dev mode; keep existing fields
(oauthClientId, useSharedCredentials, redirectTo) and add explicit clientId:
null and clientSecret: null in the updateOAuthClient call.
Comment on lines
+301
to
+330
| if (yes && !hasAnyUpdateFlag) { | ||
| return yield* BadArgsError.make({ | ||
| message: | ||
| 'Must specify at least one of --client-id, --client-secret, --custom-redirect-uri, or --dev-credentials.', | ||
| }); | ||
| } | ||
|
|
||
| const selectedMode = | ||
| isWeb && !hasAnyUpdateFlag && !yes | ||
| ? yield* selectGoogleCredentialMode() | ||
| : ('custom' as const); | ||
|
|
||
| if (selectedMode === 'dev') { | ||
| return yield* updateGoogleToDevCredentials(client); | ||
| } | ||
|
|
||
| const promptAll = !hasAnyUpdateFlag && !yes; | ||
| const switchingFromShared = Boolean(client.use_shared_credentials); | ||
| const clientId = yield* promptFlag(opts, 'client-id', { | ||
| promptIf: promptAll || (switchingFromShared && !hasFlag(opts, 'client-id')), | ||
| required: promptAll || switchingFromShared, | ||
| prompt: clientIdPrompt( | ||
| link('https://console.developers.google.com/apis/credentials'), | ||
| ), | ||
| }); | ||
| const clientSecret = yield* promptFlag(opts, 'client-secret', { | ||
| promptIf: | ||
| isWeb && | ||
| (promptAll || (switchingFromShared && !hasFlag(opts, 'client-secret'))), | ||
| required: isWeb && (promptAll || switchingFromShared), |
Contributor
There was a problem hiding this comment.
Don't prompt in --yes mode when leaving shared Google credentials.
For a shared Google web client, switchingFromShared forces promptFlag(...) for any missing custom credential fields. That means instant-cli auth client update --yes --name google-shared --client-id ... still blocks on an interactive prompt for --client-secret. In --yes mode this should fail fast with a BadArgsError, not prompt.
Suggested direction
if (yes && !hasAnyUpdateFlag) {
return yield* BadArgsError.make({
message:
'Must specify at least one of --client-id, --client-secret, --custom-redirect-uri, or --dev-credentials.',
});
}
+
+ if (yes && switchingFromShared && !devCredentialsFlag) {
+ if (!hasFlag(opts, 'client-id') || (isWeb && !hasFlag(opts, 'client-secret'))) {
+ return yield* BadArgsError.make({
+ message:
+ 'When disabling Google dev credentials with --yes, you must also provide --client-id and --client-secret.',
+ });
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/packages/cli/src/commands/auth/client/update.ts` around lines 301 -
330, The code currently still calls promptFlag for missing Google credential
fields when switchingFromShared even in --yes mode; update the logic so --yes
never triggers interactive prompts: either (A) add an early check after
computing switchingFromShared that if yes && switchingFromShared &&
!hasAnyUpdateFlag (or specifically missing required flags as determined by
hasFlag(opts, 'client-id')/hasFlag(opts, 'client-secret')/hasFlag(opts,
'custom-redirect-uri')/hasFlag(opts, 'dev-credentials')) then return yield*
BadArgsError.make(...) indicating required flags are missing, or (B) make each
promptFlag call’s promptIf and required conditions include !yes (e.g. promptIf:
!yes && (promptAll || switchingFromShared && !hasFlag(...)), required: !yes &&
(promptAll || switchingFromShared)), so promptFlag is never invoked when yes is
true; reference symbols: yes, hasAnyUpdateFlag, switchingFromShared, promptAll,
promptFlag, hasFlag, and BadArgsError.make.
Contributor
|
View Vercel preview at instant-www-js-cli-codex-jsv.vercel.app. |
stopachka
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/packages/cli/src/commands/auth/client/add.ts (1)
156-162: 💤 Low valueDocument the implicit dev-credentials default with
--yes.When
--yesis passed without any credential flags, the code defaults to dev credentials for web apps. This is tested behavior but could surprise users expecting an error for missing credentials.Consider adding a brief comment explaining this intentional default, or emitting a log message when dev credentials are auto-selected via
--yes.💡 Optional: Add clarifying comment
const credentialMode = appType !== 'web' || hasCustomCredentialFlag ? 'custom' : devCredentialsFlag || yes - ? 'dev' + ? 'dev' // With --yes and no custom credentials, default to dev credentials for quick setup : yield* selectGoogleCredentialMode();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/commands/auth/client/add.ts` around lines 156 - 162, The branch that sets credentialMode (symbols: credentialMode, useSharedCredentials, selectGoogleCredentialMode, appType, hasCustomCredentialFlag, devCredentialsFlag, yes) implicitly selects 'dev' for web apps when --yes is provided; add either a brief inline comment above this block documenting that --yes auto-selects dev credentials for web apps, or emit a warning/info log when credentialMode is set to 'dev' because yes is true (e.g., log that dev credentials were auto-selected due to --yes) so users aren’t surprised by the implicit default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/packages/cli/src/commands/auth/client/add.ts`:
- Around line 156-162: The branch that sets credentialMode (symbols:
credentialMode, useSharedCredentials, selectGoogleCredentialMode, appType,
hasCustomCredentialFlag, devCredentialsFlag, yes) implicitly selects 'dev' for
web apps when --yes is provided; add either a brief inline comment above this
block documenting that --yes auto-selects dev credentials for web apps, or emit
a warning/info log when credentialMode is set to 'dev' because yes is true
(e.g., log that dev credentials were auto-selected due to --yes) so users aren’t
surprised by the implicit default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00a2a78e-06c4-4936-9402-d9481c7524a4
📒 Files selected for processing (6)
client/packages/cli/__tests__/authClientAddGoogle.test.tsclient/packages/cli/__tests__/authClientList.test.tsclient/packages/cli/src/commands/auth/client/add.tsclient/packages/cli/src/commands/auth/client/list.tsclient/packages/cli/src/index.tsclient/packages/cli/src/lib/oauth.ts
✅ Files skipped from review due to trivial changes (1)
- client/packages/cli/src/lib/oauth.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- client/packages/cli/tests/authClientList.test.ts
- client/packages/cli/src/index.ts
stopachka
force-pushed
the
cli-codex
branch
2 times, most recently
from
f9b11a1 to
2743584
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
drew-harris
reviewed
Apr 30, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/packages/cli/src/lib/oauth.ts (1)
110-139: ⚡ Quick winEncode the shared-credentials invariant inside
addOAuthClient.
client/packages/cli/src/commands/auth/client/add.tscurrently has to blank outclientId,clientSecret, andredirectTobefore calling this helper. Moving that branch here would keep the newuseSharedCredentialscontract from drifting in future add/update callers.Suggested refactor
export const addOAuthClient = Effect.fn(function* (params: { appId?: string; providerId: string; clientName: string; clientId?: string; clientSecret?: string; authorizationEndpoint?: string; tokenEndpoint?: string; discoveryEndpoint?: string; redirectTo?: string; meta?: unknown; useSharedCredentials?: boolean; }) { const http = (yield* InstantHttpAuthed).pipe(withCommand('auth')); const targetAppId = params.appId ?? (yield* CurrentApp).appId; + const usingSharedCredentials = params.useSharedCredentials === true; return yield* http .post(`/dash/apps/${targetAppId}/oauth_clients`, { body: HttpBody.unsafeJson({ provider_id: params.providerId, client_name: params.clientName, - client_id: params.clientId, - client_secret: params.clientSecret, authorization_endpoint: params.authorizationEndpoint, token_endpoint: params.tokenEndpoint, discovery_endpoint: params.discoveryEndpoint, - redirect_to: params.redirectTo, + ...(usingSharedCredentials + ? {} + : { + client_id: params.clientId, + client_secret: params.clientSecret, + redirect_to: params.redirectTo, + }), meta: params.meta, use_shared_credentials: params.useSharedCredentials, }), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/lib/oauth.ts` around lines 110 - 139, The addOAuthClient helper should enforce the useSharedCredentials invariant rather than relying on callers; inside the Effect.fn addOAuthClient ensure that when params.useSharedCredentials is true you do not send client_id, client_secret, or redirect_to in the HTTP body (omit or set them to null/undefined) and optionally validate/throw if callers supplied conflicting values; update the body construction in addOAuthClient to conditionally include provider_id, client_name, meta and use_shared_credentials always but omit client_id, client_secret, and redirect_to when params.useSharedCredentials is true so callers (e.g., commands/auth/client/add.ts) no longer need to blank those fields.client/packages/cli/__tests__/authClientAddGoogle.test.ts (1)
251-315: ⚡ Quick winAdd a happy-path test for the explicit
--dev-credentialsflag.This suite covers the
--yesfallback, incompatibility errors, and the interactive selector, but not the primary success case for the new flag itself. Locking that down would protect the exact branch this PR adds.Suggested test shape
describe('web: dev credentials', () => { + test('--dev-credentials → creates dev credentials client', async () => { + await run(withEntry(webDevFlags, 'dev-credentials', 'true'), { yes: true }); + expect(addedClients).toHaveLength(1); + expect(addedClients[0].useSharedCredentials).toBe(true); + expect(addedClients[0].clientId).toBeUndefined(); + expect(addedClients[0].clientSecret).toBeUndefined(); + expect(addedClients[0].redirectTo).toBeUndefined(); + expect(logs.join('\n')).toContain('Credentials: Instant dev credentials'); + }); + test('--yes with no Google credentials → creates dev credentials client', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/__tests__/authClientAddGoogle.test.ts` around lines 251 - 315, Add a new test in client/packages/cli/__tests__/authClientAddGoogle.test.ts that asserts the happy-path when passing the explicit --dev-credentials flag: call run(...) with webDevFlags plus ['dev-credentials','true'] and yes: true, then verify addedClients has one entry whose clientName is 'google-web' (or matches existing dev case), useSharedCredentials is true, clientId/clientSecret/redirectTo are undefined, and logs include the same messages checked in the existing '--yes with no Google credentials' test (e.g., 'Credentials: Instant dev credentials', 'No Google Console setup required', and no guidance about updating credentials or redirect URIs); use the same helpers/constants (run, webDevFlags, addedClients, logs, prompts) so the new test targets the exact branch implemented for --dev-credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/packages/cli/src/commands/auth/client/add.ts`:
- Around line 143-145: The current hasProvidedSomeCustomCredentials uses
Boolean(opts['client-id'] || ...) which treats empty strings as not provided;
change the check to test presence of the flags by key instead of truthiness: use
Object.prototype.hasOwnProperty.call(opts, 'client-id') (or opts['client-id']
!== undefined) for each of 'client-id', 'client-secret', and
'custom-redirect-uri' and combine them the same way to set
hasProvidedSomeCustomCredentials so flags like --client-id "" are considered
provided.
---
Nitpick comments:
In `@client/packages/cli/__tests__/authClientAddGoogle.test.ts`:
- Around line 251-315: Add a new test in
client/packages/cli/__tests__/authClientAddGoogle.test.ts that asserts the
happy-path when passing the explicit --dev-credentials flag: call run(...) with
webDevFlags plus ['dev-credentials','true'] and yes: true, then verify
addedClients has one entry whose clientName is 'google-web' (or matches existing
dev case), useSharedCredentials is true, clientId/clientSecret/redirectTo are
undefined, and logs include the same messages checked in the existing '--yes
with no Google credentials' test (e.g., 'Credentials: Instant dev credentials',
'No Google Console setup required', and no guidance about updating credentials
or redirect URIs); use the same helpers/constants (run, webDevFlags,
addedClients, logs, prompts) so the new test targets the exact branch
implemented for --dev-credentials.
In `@client/packages/cli/src/lib/oauth.ts`:
- Around line 110-139: The addOAuthClient helper should enforce the
useSharedCredentials invariant rather than relying on callers; inside the
Effect.fn addOAuthClient ensure that when params.useSharedCredentials is true
you do not send client_id, client_secret, or redirect_to in the HTTP body (omit
or set them to null/undefined) and optionally validate/throw if callers supplied
conflicting values; update the body construction in addOAuthClient to
conditionally include provider_id, client_name, meta and use_shared_credentials
always but omit client_id, client_secret, and redirect_to when
params.useSharedCredentials is true so callers (e.g.,
commands/auth/client/add.ts) no longer need to blank those fields.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 759cefea-5ee1-4962-bb50-d56ac458c47b
📒 Files selected for processing (6)
client/packages/cli/__tests__/authClientAddGoogle.test.tsclient/packages/cli/__tests__/authClientList.test.tsclient/packages/cli/src/commands/auth/client/add.tsclient/packages/cli/src/commands/auth/client/list.tsclient/packages/cli/src/index.tsclient/packages/cli/src/lib/oauth.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- client/packages/cli/tests/authClientList.test.ts
- client/packages/cli/src/commands/auth/client/list.ts
- client/packages/cli/src/index.ts
drew-harris
reviewed
Apr 30, 2026
drew-harris
reviewed
Apr 30, 2026
| use_shared_credentials: NullableBoolean, | ||
| }); | ||
|
|
||
| export type OAuthClient = Schema.Schema.Type<typeof OAuthClient>; |
Contributor
There was a problem hiding this comment.
tiny nit / fun fact: typeof OAuthClient.Type also works.
I generally prefer not to extract out effect schema types since they can be piped / modified but thats irrelevant here
Contributor
|
I feel like the warning "For production, use your own Google OAuth credentials." should be underlined/bolded |
drew-harris
reviewed
Apr 30, 2026
drew-harris
reviewed
Apr 30, 2026
drew-harris
reviewed
Apr 30, 2026
Contributor
Author
@drew-harris fixed! |
drew-harris
approved these changes
May 4, 2026
Contributor
drew-harris
left a comment
drew-harris
left a comment
There was a problem hiding this comment.
Everything works and looks good
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
2 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.
Our Google Clients now support shared dev credentials!
This PR updates
npx instant-cli auth client addandnpx instant-cli auth client listto support shared dev credentials.Note: You may wonder: how do you upgrade a client from dev-credentials? A follow-on PR will implement the
updatecommand.add
--dev-credentials
You can pass in a
--dev-credentialsflag to explicitly choose dev-credentials:When you do, we will include a notice in the prompt output saying you have dev credentials
--yes defaults to --dev-credentials
If you create with --yes and don't provide a
--client-idor a--client-token, we will default to--dev-credentialsInteractive mode asks you for your choice
For interactive runs, Google web asks whether to use dev credentials or your own Google credentials. If you pass
--client-id,--client-secret, or--custom-redirect-uri, the CLI treats that as the custom credentials path.This is how the output boxes look:
list
auth client listnow explicitly notes that you are usingdev-credentials.This is how the output looks:
Testing