Add firebase clients via cli (#2593) [bump to v1.0.17]#2593
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds shared OAuth helper functions for provider lookup/name generation, refactors tests to use a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI
participant OAuthLib as OAuth Lib
participant AuthAPI as Auth API
User->>CLI: run `auth client add --type <provider> [--project-id]`
CLI->>OAuthLib: getClientNameAndProvider(type, opts)
OAuthLib->>AuthAPI: getAppsAuth()
AuthAPI-->>OAuthLib: oauth_service_providers & oauth_clients
OAuthLib->>OAuthLib: findName(suggested, usedNames)
OAuthLib->>CLI: optOrPrompt(suggestedName)
CLI-->>OAuthLib: chosen clientName
OAuthLib->>AuthAPI: addOAuthProvider(...) [if provider missing]
AuthAPI-->>OAuthLib: provider
OAuthLib->>AuthAPI: addOAuthClient({ providerId, clientName, discovery })
AuthAPI-->>CLI: created client info
CLI->>User: print client details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
View Vercel preview at instant-www-js-drewh-firebase-oauth-cli-jsv.vercel.app. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
client/packages/cli/src/commands/auth/client/add.ts (2)
725-727: Handler signature is inconsistent with its siblings.All other
handle*Clientfunctions in this file takeopts: Record<string, unknown>, buthandleFirebaseClienttakesOptsFromCommand<typeof authClientAddDef> & Record<string, unknown>. The dispatcher at line 814 passes the sameoptsto every branch, so there's no type benefit here — this just makes the intersection withRecord<string, unknown>effectively the latter. Align with siblings for consistency.♻️ Proposed fix
-const handleFirebaseClient = Effect.fn(function* ( - opts: OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>, -) { +const handleFirebaseClient = Effect.fn(function* (opts: Record<string, unknown>) {🤖 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 725 - 727, The handler signature for handleFirebaseClient is inconsistent: it currently declares opts as OptsFromCommand<typeof authClientAddDef> & Record<string, unknown> while all other handle*Client functions accept opts: Record<string, unknown>; change handleFirebaseClient's parameter to opts: Record<string, unknown> to match siblings and the dispatcher usage, removing the unnecessary OptsFromCommand<typeof authClientAddDef> intersection so the function signature aligns with the other handlers (locate the function named handleFirebaseClient and the type reference to authClientAddDef to update).
760-769: Consider adding follow-up instructions in the success output.Other providers print actionable next steps after creation (e.g., Google/GitHub/LinkedIn print where to register the redirect URI; Clerk prints session-token configuration instructions). The Firebase summary prints only ID/name/projectId — a user who just ran this command has no indication of what (if anything) to configure on the Firebase side or how to complete the sign-in flow. If no extra steps are needed, a short confirmation line would still be helpful; otherwise, consider surfacing the relevant Firebase console link.
🤖 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 760 - 769, The success output currently logged via Effect.log(boxen(...)) only shows the Firebase OAuth client name/ID/projectId (response.client.client_name, response.client.id, projectId); update the boxed message to include actionable next steps — either a short confirmation that no further action is required or explicit instructions and the Firebase Console link and redirect URI registration guidance (e.g., where to add the redirect URI) so users know how to complete setup; modify the string passed to boxen in the same Effect.log call to append these follow-up lines and ensure the message references response.client and projectId for clarity.client/packages/cli/__tests__/oauthMock.ts (2)
13-61: Consider importing the real helpers instead of re-implementing them.
findName,getOrCreateProvider, andgetClientNameAndProviderhere are near-verbatim copies of the production helpers insrc/lib/oauth.ts. Because the mock only needs to substitute the primitives (getAppsAuth,addOAuthProvider,addOAuthClient), you could import the real implementations and inject the mocked primitives, avoiding duplicated logic that can silently drift (e.g., if prompt wording,findNamestarting index, or duplicate-name error handling changes in production, tests could still pass against stale mock logic).♻️ Sketch
// Import the real helpers' pure logic, but override the low-level Effects they depend on. // One approach: export `findName`, `getOrCreateProvider`, `getClientNameAndProvider` from // src/lib/oauth.ts such that they are parameterized over (or import and rely on) // `getAppsAuth`/`addOAuthProvider`/`addOAuthClient`. Then makeOAuthMock can re-export the // real helpers while the provided mocks satisfy the underlying module imports via vi.mock.Not blocking — per internal norms test utilities may be simpler than production — but this particular helper is close enough to production that the duplication has measurable maintenance cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/__tests__/oauthMock.ts` around lines 13 - 61, Duplicate logic: findName, getOrCreateProvider, and getClientNameAndProvider in the test mock duplicate prod helpers and risk drifting; replace them by importing the real helper implementations from src/lib/oauth.ts and adapt the test to inject or mock only the low-level primitives (getAppsAuth, addOAuthProvider, addOAuthClient) those helpers rely on (either by parameterizing the helpers or using vi.mock to stub the underlying Effect calls), so keep the helper names findName, getOrCreateProvider, and getClientNameAndProvider but call the production implementations with the test-provided primitives.
8-12: Looseanytyping weakens mock signal.The mock typings (
AnyEffect,params: any) drop the shape ofgetAppsAuth/addOAuthProvider/addOAuthClientreturn values. TypingmakeOAuthMockagainst the actual exports ofsrc/lib/oauth.ts(e.g.,Parameters<typeof addOAuthClient>[0],ReturnType<typeof getAppsAuth>) would catch schema drift between the production module and the tests at compile time. Optional; fine to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/__tests__/oauthMock.ts` around lines 8 - 12, The mock signatures in makeOAuthMock use loose AnyEffect and params: any which hides mismatches with the real module; replace those broad types by importing the actual functions (getAppsAuth, addOAuthProvider, addOAuthClient) and type the mock shape using TypeScript utility types (e.g., ReturnType<typeof getAppsAuth> for its return, Parameters<typeof addOAuthClient>[0] for the client params, etc.) so the mock types track the real exports and will surface schema drift at compile time.
🤖 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 754-758: The discoveryEndpoint currently interpolates projectId
directly when calling addOAuthClient (provider.id, clientName,
discoveryEndpoint), which can produce malformed URLs; update the prompt
validation for projectId to enforce the Firebase project ID regex
/^[a-z][a-z0-9-]{5,29}$/ (or equivalent) and additionally use
encodeURIComponent(projectId) when constructing the discoveryEndpoint string
passed to addOAuthClient so spaces/special chars are escaped and invalid input
fails fast with a clear validation error.
- Around line 733-747: The optOrPrompt call for the Firebase project ID uses
simpleName: 'project-id' which yields inconsistent error text; update the
simpleName to use the double-dash form simpleName: '--project-id' so the
missing-value message matches other handlers and existing tests (look for the
optOrPrompt invocation that sets simpleName, prompt text, and validate:
validateRequired in add.ts).
In `@client/packages/cli/src/lib/oauth.ts`:
- Around line 186-198: The doc comment above findName is misleading: it shows
"google-web-1/google-web-2/google-web-3" but the implementation concatenates the
index directly to prefix and starts at 2, producing names like "google",
"google2", "google3". Update the comment/example to reflect the actual behavior
of findName (mention prefix param and used Set) — e.g., "If used contains
'google' and 'google2', findName('google', used) returns 'google3'"; or
alternatively adjust the description to state that the first duplicate keeps the
plain prefix and subsequent collisions append integers starting at 2. Ensure the
updated comment references the findName(prefix, used) behavior.
---
Nitpick comments:
In `@client/packages/cli/__tests__/oauthMock.ts`:
- Around line 13-61: Duplicate logic: findName, getOrCreateProvider, and
getClientNameAndProvider in the test mock duplicate prod helpers and risk
drifting; replace them by importing the real helper implementations from
src/lib/oauth.ts and adapt the test to inject or mock only the low-level
primitives (getAppsAuth, addOAuthProvider, addOAuthClient) those helpers rely on
(either by parameterizing the helpers or using vi.mock to stub the underlying
Effect calls), so keep the helper names findName, getOrCreateProvider, and
getClientNameAndProvider but call the production implementations with the
test-provided primitives.
- Around line 8-12: The mock signatures in makeOAuthMock use loose AnyEffect and
params: any which hides mismatches with the real module; replace those broad
types by importing the actual functions (getAppsAuth, addOAuthProvider,
addOAuthClient) and type the mock shape using TypeScript utility types (e.g.,
ReturnType<typeof getAppsAuth> for its return, Parameters<typeof
addOAuthClient>[0] for the client params, etc.) so the mock types track the real
exports and will surface schema drift at compile time.
In `@client/packages/cli/src/commands/auth/client/add.ts`:
- Around line 725-727: The handler signature for handleFirebaseClient is
inconsistent: it currently declares opts as OptsFromCommand<typeof
authClientAddDef> & Record<string, unknown> while all other handle*Client
functions accept opts: Record<string, unknown>; change handleFirebaseClient's
parameter to opts: Record<string, unknown> to match siblings and the dispatcher
usage, removing the unnecessary OptsFromCommand<typeof authClientAddDef>
intersection so the function signature aligns with the other handlers (locate
the function named handleFirebaseClient and the type reference to
authClientAddDef to update).
- Around line 760-769: The success output currently logged via
Effect.log(boxen(...)) only shows the Firebase OAuth client name/ID/projectId
(response.client.client_name, response.client.id, projectId); update the boxed
message to include actionable next steps — either a short confirmation that no
further action is required or explicit instructions and the Firebase Console
link and redirect URI registration guidance (e.g., where to add the redirect
URI) so users know how to complete setup; modify the string passed to boxen in
the same Effect.log call to append these follow-up lines and ensure the message
references response.client and projectId for clarity.
🪄 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: ecff75d9-3cb9-42bf-8596-0c30282ea195
📒 Files selected for processing (9)
client/packages/cli/__tests__/authClientAddApple.test.tsclient/packages/cli/__tests__/authClientAddClerk.test.tsclient/packages/cli/__tests__/authClientAddGithub.test.tsclient/packages/cli/__tests__/authClientAddGoogle.test.tsclient/packages/cli/__tests__/authClientAddLinkedin.test.tsclient/packages/cli/__tests__/oauthMock.tsclient/packages/cli/src/commands/auth/client/add.tsclient/packages/cli/src/index.tsclient/packages/cli/src/lib/oauth.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
client/packages/cli/src/commands/auth/client/add.ts (2)
733-767: Deduplicate the Firebase project-id regex and error message.The regex literal
/^[a-z][a-z0-9-]{4,28}[a-z0-9]$/and the 40-word "Invalid Firebase project ID…" error string are each written out twice — once inside the prompt'svalidatecallback (lines 733, 750-751) and again in the post-optOrPromptfallback that catches values passed via--project-id(lines 762-767). Any tweak to the rule (e.g., tightening/loosening the length or copy) now has to be kept in sync in two places, and the regex is re-compiled on every invocation.Extracting both to module-scope constants keeps the two enforcement paths aligned and avoids recompilation:
♻️ Proposed refactor
+const FIREBASE_PROJECT_ID_REGEX = /^[a-z][a-z0-9-]{4,28}[a-z0-9]$/; +const FIREBASE_PROJECT_ID_ERROR = + 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.'; + const handleFirebaseClient = Effect.fn(function* ( opts: OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>, ) { const { clientName, provider } = yield* getClientNameAndProvider( 'firebase', opts, ); - const firebaseProjectIdRegex = /^[a-z][a-z0-9-]{4,28}[a-z0-9]$/; const projectId = yield* optOrPrompt(opts['project-id'], { simpleName: '--project-id', required: true, skipIf: false, prompt: { prompt: 'Firebase project ID: (From Project Settings page on https://console.firebase.google.com/)', placeholder: '', modifyOutput: UI.modifiers.piped([ UI.modifiers.topPadding, UI.modifiers.dimOnComplete, ]), validate: (val) => { if (!val) { return 'Project ID is required'; } - if (!firebaseProjectIdRegex.test(val)) { - return 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.'; - } + if (!FIREBASE_PROJECT_ID_REGEX.test(val)) { + return FIREBASE_PROJECT_ID_ERROR; + } }, }, }); // typeguard if (!clientName || !projectId) { return yield* BadArgsError.make({ message: 'Missing required arguments', }); } - if (!firebaseProjectIdRegex.test(projectId)) { - return yield* BadArgsError.make({ - message: - 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.', - }); - } + if (!FIREBASE_PROJECT_ID_REGEX.test(projectId)) { + return yield* BadArgsError.make({ message: FIREBASE_PROJECT_ID_ERROR }); + }🤖 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 733 - 767, The Firebase project-id regex and its long validation message are duplicated; extract them to module-scope constants (e.g., FIREBASE_PROJECT_ID_REGEX and FIREBASE_PROJECT_ID_ERROR) and use those constants inside the optOrPrompt validate callback and the post-optOrPrompt check that calls BadArgsError to ensure a single source of truth; update references to firebaseProjectIdRegex and the literal error string in the validate function and the BadArgsError.make call so both paths share the same compiled regex and message.
725-727: Nit: Type annotation inconsistent with sibling handlers.Every other
handle*Clientin this file takesopts: Record<string, unknown>. This one usesOptsFromCommand<typeof authClientAddDef> & Record<string, unknown>, but since--project-idis not registered as a Commander option forauthClientAddDef(the command relies on.allowUnknownOption(true)), theOptsFromCommand<…>side contributes nothing to the field-level type ofopts['project-id']and the intersection just widens everything back tounknownvia theRecord. Dropping it makes the signature match the other handlers.♻️ Proposed refactor
-const handleFirebaseClient = Effect.fn(function* ( - opts: OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>, -) { +const handleFirebaseClient = Effect.fn(function* ( + opts: Record<string, unknown>, +) {Based on learnings:
authClientAddDefintentionally uses.allowUnknownOption(true)with provider-specific flags documented viaaddHelpTextrather than.option(), soOptsFromCommand<typeof authClientAddDef>doesn't carry per-flag types for flags like--project-id.🤖 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 725 - 727, The handler signature for handleFirebaseClient uses a widened intersection type (OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>) that is inconsistent with sibling handlers and gives no benefit because authClientAddDef uses allowUnknownOption(true); change the parameter type to opts: Record<string, unknown> (matching other handle*Client functions) by removing the OptsFromCommand<typeof authClientAddDef> part so the function signature is consistent and simpler.client/packages/cli/src/lib/oauth.ts (1)
15-15: Circular dependency / layering:lib/oauth.tsshould not depend oncommands/auth/client/add.ts.
oauth.tsnow pulls inClientTypeSchemaas a type-only import from../commands/auth/client/add.ts, whileadd.tsimports runtime values (addOAuthClient,findName,getClientNameAndProvider,getOrCreateProvider) from this file.import typeerases at runtime so it won't break execution, but it inverts the intended dependency direction (a lower-levellibmodule reaching into a command module) and future edits that accidentally convert the import to a value import — or tooling that doesn't fully respectisolatedModules/verbatimModuleSyntax— will introduce a real cycle.Consider moving
ClientTypeSchema(and itsType) down tolib/oauth.ts— it's a data model that's now used in two lib-level helpers here anyway — and re-export fromadd.tsif you want to preserve the current public-facing location.♻️ Proposed refactor (sketch)
In
lib/oauth.ts:-import type { ClientTypeSchema } from '../commands/auth/client/add.ts'; +export const ClientTypeSchema = Schema.Literal( + 'google', + 'github', + 'apple', + 'linkedin', + 'clerk', + 'firebase', +);In
commands/auth/client/add.ts:-export const ClientTypeSchema = Schema.Literal( - 'google', - 'github', - 'apple', - 'linkedin', - 'clerk', - 'firebase', -); +export { ClientTypeSchema } from '../../../lib/oauth.ts';Also applies to: 170-184
🤖 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` at line 15, Move the ClientTypeSchema (and its associated Type) out of the command module into the library module so lib/oauth.ts owns the data model: define ClientTypeSchema and its Type in lib/oauth.ts, update any code that referenced the type import from commands/auth/client/add.ts to import from lib/oauth.ts, and add a re-export from commands/auth/client/add.ts if you need to preserve the existing public API; ensure this breaks the circular dependency between lib/oauth.ts and the functions addOAuthClient, findName, getClientNameAndProvider, and getOrCreateProvider by making lib/oauth.ts the canonical holder of the schema/type.
🤖 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 733-767: The Firebase project-id regex and its long validation
message are duplicated; extract them to module-scope constants (e.g.,
FIREBASE_PROJECT_ID_REGEX and FIREBASE_PROJECT_ID_ERROR) and use those constants
inside the optOrPrompt validate callback and the post-optOrPrompt check that
calls BadArgsError to ensure a single source of truth; update references to
firebaseProjectIdRegex and the literal error string in the validate function and
the BadArgsError.make call so both paths share the same compiled regex and
message.
- Around line 725-727: The handler signature for handleFirebaseClient uses a
widened intersection type (OptsFromCommand<typeof authClientAddDef> &
Record<string, unknown>) that is inconsistent with sibling handlers and gives no
benefit because authClientAddDef uses allowUnknownOption(true); change the
parameter type to opts: Record<string, unknown> (matching other handle*Client
functions) by removing the OptsFromCommand<typeof authClientAddDef> part so the
function signature is consistent and simpler.
In `@client/packages/cli/src/lib/oauth.ts`:
- Line 15: Move the ClientTypeSchema (and its associated Type) out of the
command module into the library module so lib/oauth.ts owns the data model:
define ClientTypeSchema and its Type in lib/oauth.ts, update any code that
referenced the type import from commands/auth/client/add.ts to import from
lib/oauth.ts, and add a re-export from commands/auth/client/add.ts if you need
to preserve the existing public API; ensure this breaks the circular dependency
between lib/oauth.ts and the functions addOAuthClient, findName,
getClientNameAndProvider, and getOrCreateProvider by making lib/oauth.ts the
canonical holder of the schema/type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 070dc00f-6465-422f-adc5-e7f304714628
📒 Files selected for processing (2)
client/packages/cli/src/commands/auth/client/add.tsclient/packages/cli/src/lib/oauth.ts
| return 'Project ID is required'; | ||
| } | ||
| if (!firebaseProjectIdRegex.test(val)) { | ||
| return 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.'; |
There was a problem hiding this comment.
Q: why is this both here and on L762?
There was a problem hiding this comment.
This only validates the interactive TUI text input, the other validation covers the case where its provided by flag.
drew-harris
force-pushed
the
drewh/firebase-oauth-cli
branch
from
f645601 to
9e870e5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
client/packages/cli/__tests__/oauthMock.ts (1)
1-69: LGTM — and the minor duplication is a reasonable trade-off.Because
vi.mock('../src/lib/oauth.ts', …)replaces the whole module, the helpers (findName,getOrCreateProvider,getClientNameAndProvider) have to be re-provided, and doing so with implementations that mirrorsrc/lib/oauth.tsis the simplest path.Optional: a short comment at the top noting "Keep these helpers behaviorally in sync with src/lib/oauth.ts" would flag the maintenance expectation for future readers, since silent drift here would make the test suite stop exercising the real orchestration logic.
Based on learnings: test helper utilities under
__tests__directories may implement simpler logic than production code, which supports keeping this mock lean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/__tests__/oauthMock.ts` around lines 1 - 69, Add a short top-of-file comment in oauthMock.ts stating that the helper implementations (findName, getOrCreateProvider, getClientNameAndProvider) are intentionally mirrored from src/lib/oauth.ts and must be kept behaviorally in sync with that module so tests remain valid; place the comment above the imports and mention the three function names to signal maintenance expectations to future readers.client/packages/cli/src/commands/auth/client/add.ts (1)
725-767: Extract the Firebase project ID validator to eliminate duplication.The regex pattern and error message are repeated verbatim between the prompt's
validatefunction (lines 746–753) and the post-prompt guard (lines 762–767). The post-prompt check is necessary becausevalidateonly runs in interactive mode, not when--project-idis provided via flag in--yesmode. A shared validator function keeps both paths in sync and simplifies future updates.Additionally, the type signature
opts: OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>is inconsistent with every other handler (opts: Record<string, unknown>). SinceauthClientAddDefuses.allowUnknownOption(true)and does not register--project-idvia.option(), theOptsFromCommand<…>part adds no typing benefit.♻️ Suggested refactor
+const FIREBASE_PROJECT_ID_REGEX = /^[a-z][a-z0-9-]{4,28}[a-z0-9]$/; +const FIREBASE_PROJECT_ID_ERROR = + 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.'; +const validateFirebaseProjectId = (val: string | undefined) => { + if (!val) return 'Project ID is required'; + if (!FIREBASE_PROJECT_ID_REGEX.test(val)) return FIREBASE_PROJECT_ID_ERROR; +}; + const handleFirebaseClient = Effect.fn(function* ( - opts: OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>, + opts: Record<string, unknown>, ) { const { clientName, provider } = yield* getClientNameAndProvider( 'firebase', opts, ); - const firebaseProjectIdRegex = /^[a-z][a-z0-9-]{4,28}[a-z0-9]$/; const projectId = yield* optOrPrompt(opts['project-id'], { simpleName: '--project-id', required: true, skipIf: false, prompt: { prompt: 'Firebase project ID: (From Project Settings page on https://console.firebase.google.com/)', placeholder: '', modifyOutput: UI.modifiers.piped([ UI.modifiers.topPadding, UI.modifiers.dimOnComplete, ]), - validate: (val) => { - if (!val) { - return 'Project ID is required'; - } - if (!firebaseProjectIdRegex.test(val)) { - return 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.'; - } - }, + validate: validateFirebaseProjectId, }, }); - // typeguard - if (!clientName || !projectId) { - return yield* BadArgsError.make({ - message: 'Missing required arguments', - }); - } - if (!firebaseProjectIdRegex.test(projectId)) { - return yield* BadArgsError.make({ - message: - 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.', - }); - } + if (!clientName || !projectId) { + return yield* BadArgsError.make({ message: 'Missing required arguments' }); + } + const err = validateFirebaseProjectId(projectId); + if (err) return yield* BadArgsError.make({ message: err });🤖 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 725 - 767, Extract the Firebase project ID validation into a single helper and update handleFirebaseClient to use it in both places: replace the inline firebaseProjectIdRegex and duplicate error message with a shared function (e.g., isValidFirebaseProjectId(projectId: string) => boolean and getFirebaseProjectIdError(projectId?: string) => string | undefined) and call that helper from the optOrPrompt.validate callback and from the post-prompt guard before yielding BadArgsError; also simplify the handler signature from opts: OptsFromCommand<typeof authClientAddDef> & Record<string, unknown> to opts: Record<string, unknown> to match other handlers. Ensure you reference and update the uses in handleFirebaseClient, the optOrPrompt(...) validate, and the post-prompt check that calls BadArgsError.make.
🤖 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/__tests__/oauthMock.ts`:
- Around line 1-69: Add a short top-of-file comment in oauthMock.ts stating that
the helper implementations (findName, getOrCreateProvider,
getClientNameAndProvider) are intentionally mirrored from src/lib/oauth.ts and
must be kept behaviorally in sync with that module so tests remain valid; place
the comment above the imports and mention the three function names to signal
maintenance expectations to future readers.
In `@client/packages/cli/src/commands/auth/client/add.ts`:
- Around line 725-767: Extract the Firebase project ID validation into a single
helper and update handleFirebaseClient to use it in both places: replace the
inline firebaseProjectIdRegex and duplicate error message with a shared function
(e.g., isValidFirebaseProjectId(projectId: string) => boolean and
getFirebaseProjectIdError(projectId?: string) => string | undefined) and call
that helper from the optOrPrompt.validate callback and from the post-prompt
guard before yielding BadArgsError; also simplify the handler signature from
opts: OptsFromCommand<typeof authClientAddDef> & Record<string, unknown> to
opts: Record<string, unknown> to match other handlers. Ensure you reference and
update the uses in handleFirebaseClient, the optOrPrompt(...) validate, and the
post-prompt check that calls BadArgsError.make.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 91b82e21-8e2d-4191-83ea-da53cfebb521
📒 Files selected for processing (9)
client/packages/cli/__tests__/authClientAddApple.test.tsclient/packages/cli/__tests__/authClientAddClerk.test.tsclient/packages/cli/__tests__/authClientAddGithub.test.tsclient/packages/cli/__tests__/authClientAddGoogle.test.tsclient/packages/cli/__tests__/authClientAddLinkedin.test.tsclient/packages/cli/__tests__/oauthMock.tsclient/packages/cli/src/commands/auth/client/add.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/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- client/packages/cli/tests/authClientAddApple.test.ts
- client/packages/cli/tests/authClientAddGithub.test.ts
- client/packages/cli/tests/authClientAddClerk.test.ts
- client/packages/cli/tests/authClientAddGoogle.test.ts
drew-harris
force-pushed
the
drewh/firebase-oauth-cli
branch
from
9e870e5 to
bcd08a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/packages/cli/src/lib/oauth.ts (1)
227-231: Minor UX: surface duplicate-name check in the prompt validator.When run interactively, a user who overrides the suggested name with one that's already taken only sees the error after submitting. Moving the
usedClientNames.has(…)check into the prompt'svalidate(alongsidevalidateRequired) would surface the error in-place and allow the user to correct without re-running. The post-prompt check is still needed for the--name-via-flag path.🤖 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 227 - 231, The duplicate-name check currently done after prompting (usedClientNames.has(clientName)) should be added into the interactive prompt's validate function (the same place validateRequired is used) so the prompt validator returns a user-facing error if the entered name exists; implement validate to call usedClientNames.has and return a string like "The unique name 'X' is already in use." when true, while retaining the existing post-prompt guard that yields BadArgsError.make for the --name flag path to keep non-interactive behavior unchanged.client/packages/cli/src/commands/auth/client/add.ts (2)
725-784: Optional: DRY the project-ID regex and error message.The Firebase project ID regex and error message are defined twice (once inline in the prompt
validate, once as the post-hoc check on line 762). Since both must stay in sync (one covers the--project-idflag path, the other the interactive prompt path, as confirmed in the previous review discussion), extracting them to a single constant/helper reduces the risk of drift.♻️ Proposed refactor
+const FIREBASE_PROJECT_ID_REGEX = /^[a-z][a-z0-9-]{4,28}[a-z0-9]$/; +const FIREBASE_PROJECT_ID_ERROR = + 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.'; + +const validateFirebaseProjectId = (val: string | undefined) => { + if (!val) return 'Project ID is required'; + if (!FIREBASE_PROJECT_ID_REGEX.test(val)) return FIREBASE_PROJECT_ID_ERROR; +}; + const handleFirebaseClient = Effect.fn(function* ( opts: OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>, ) { const { clientName, provider } = yield* getClientNameAndProvider( 'firebase', opts, ); - const firebaseProjectIdRegex = /^[a-z][a-z0-9-]{4,28}[a-z0-9]$/; const projectId = yield* optOrPrompt(opts['project-id'], { simpleName: '--project-id', required: true, skipIf: false, prompt: { prompt: 'Firebase project ID: (From Project Settings page on https://console.firebase.google.com/)', placeholder: '', modifyOutput: UI.modifiers.piped([ UI.modifiers.topPadding, UI.modifiers.dimOnComplete, ]), - validate: (val) => { - if (!val) { - return 'Project ID is required'; - } - if (!firebaseProjectIdRegex.test(val)) { - return 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.'; - } - }, + validate: validateFirebaseProjectId, }, }); // typeguard if (!clientName || !projectId) { return yield* BadArgsError.make({ message: 'Missing required arguments', }); } - if (!firebaseProjectIdRegex.test(projectId)) { + if (!FIREBASE_PROJECT_ID_REGEX.test(projectId)) { return yield* BadArgsError.make({ - message: - 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.', + message: FIREBASE_PROJECT_ID_ERROR, }); }🤖 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 725 - 784, Extract the Firebase project ID regex and its error message into shared constants (e.g., FIREBASE_PROJECT_ID_REGEX and FIREBASE_PROJECT_ID_ERROR) and use those constants inside handleFirebaseClient: replace the inline regex and message used in the prompt validate function and the later post-hoc check (the if block that currently re-tests projectId and returns BadArgsError) so both paths reference the same values; ensure you keep the existing validate behavior and error text when calling BadArgsError.make and in the validate closure.
725-727: Minor: opts type signature diverges from other handlers.Every other
handle*Clientusesopts: Record<string, unknown>;handleFirebaseClientusesOptsFromCommand<typeof authClientAddDef> & Record<string, unknown>. Sinceoptsis only accessed via string keys here (andOptsFromCommanddoesn't currently includeproject-idin the.option()surface per the existing design choice forauthClientAddDef), the added type doesn't buy anything. Consider aligning with the siblings for consistency, or push the richer type to all handlers at once.Based on learnings,
authClientAddDefintentionally uses.allowUnknownOption(true)and doesn't register provider-specific flags via.option(), soOptsFromCommand<typeof authClientAddDef>won't carryproject-idas a typed key anyway.🤖 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 725 - 727, The opts parameter type on handleFirebaseClient currently uses OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>, which diverges from the other handlers; change the signature to opts: Record<string, unknown> to match the siblings (or alternatively update all handle*Client handlers at once if you want the richer type everywhere). Update the function declaration for handleFirebaseClient (and any related references) so it accepts opts: Record<string, unknown>, keeping usage as string-key access consistent with authClientAddDef's allowUnknownOption behavior.
🤖 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 725-784: Add a new test file authClientAddFirebase.test.ts that
mirrors other provider tests and validates handleFirebaseClient behavior: write
tests for (1) missing --name and --project-id with --yes returning a
BadArgsError, (2) interactive prompts for clientName and project-id using the
existing test prompting utilities to simulate user input, (3) invalid project-id
format both when passed as a flag and when entered interactively (expecting
BadArgsError/validation message), and (4) the success case where addOAuthClient
is stubbed/mocked to capture its args and assert discoveryEndpoint equals
https://securetoken.google.com/<projectId>/.well-known/openid-configuration and
the CLI logs the created client info; use the same test helpers/mocks used by
other authClientAdd<Provider>.test.ts files and reference handleFirebaseClient,
firebaseProjectIdRegex, and addOAuthClient to locate the code under test.
---
Nitpick comments:
In `@client/packages/cli/src/commands/auth/client/add.ts`:
- Around line 725-784: Extract the Firebase project ID regex and its error
message into shared constants (e.g., FIREBASE_PROJECT_ID_REGEX and
FIREBASE_PROJECT_ID_ERROR) and use those constants inside handleFirebaseClient:
replace the inline regex and message used in the prompt validate function and
the later post-hoc check (the if block that currently re-tests projectId and
returns BadArgsError) so both paths reference the same values; ensure you keep
the existing validate behavior and error text when calling BadArgsError.make and
in the validate closure.
- Around line 725-727: The opts parameter type on handleFirebaseClient currently
uses OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>, which
diverges from the other handlers; change the signature to opts: Record<string,
unknown> to match the siblings (or alternatively update all handle*Client
handlers at once if you want the richer type everywhere). Update the function
declaration for handleFirebaseClient (and any related references) so it accepts
opts: Record<string, unknown>, keeping usage as string-key access consistent
with authClientAddDef's allowUnknownOption behavior.
In `@client/packages/cli/src/lib/oauth.ts`:
- Around line 227-231: The duplicate-name check currently done after prompting
(usedClientNames.has(clientName)) should be added into the interactive prompt's
validate function (the same place validateRequired is used) so the prompt
validator returns a user-facing error if the entered name exists; implement
validate to call usedClientNames.has and return a string like "The unique name
'X' is already in use." when true, while retaining the existing post-prompt
guard that yields BadArgsError.make for the --name flag path to keep
non-interactive behavior unchanged.
🪄 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: 1ee6bc6b-b0e4-4bac-932d-48cd34dca436
📒 Files selected for processing (10)
client/packages/cli/__tests__/authClientAddApple.test.tsclient/packages/cli/__tests__/authClientAddClerk.test.tsclient/packages/cli/__tests__/authClientAddGithub.test.tsclient/packages/cli/__tests__/authClientAddGoogle.test.tsclient/packages/cli/__tests__/authClientAddLinkedin.test.tsclient/packages/cli/__tests__/oauthMock.tsclient/packages/cli/src/commands/auth/client/add.tsclient/packages/cli/src/index.tsclient/packages/cli/src/lib/oauth.tsclient/packages/version/src/version.ts
✅ Files skipped from review due to trivial changes (2)
- client/packages/cli/src/index.ts
- client/packages/version/src/version.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- client/packages/cli/tests/authClientAddLinkedin.test.ts
- client/packages/cli/tests/authClientAddApple.test.ts
- client/packages/cli/tests/authClientAddClerk.test.ts
- client/packages/cli/tests/oauthMock.ts
| const handleFirebaseClient = Effect.fn(function* ( | ||
| opts: OptsFromCommand<typeof authClientAddDef> & Record<string, unknown>, | ||
| ) { | ||
| const { clientName, provider } = yield* getClientNameAndProvider( | ||
| 'firebase', | ||
| opts, | ||
| ); | ||
|
|
||
| const firebaseProjectIdRegex = /^[a-z][a-z0-9-]{4,28}[a-z0-9]$/; | ||
| const projectId = yield* optOrPrompt(opts['project-id'], { | ||
| simpleName: '--project-id', | ||
| required: true, | ||
| skipIf: false, | ||
| prompt: { | ||
| prompt: | ||
| 'Firebase project ID: (From Project Settings page on https://console.firebase.google.com/)', | ||
| placeholder: '', | ||
| modifyOutput: UI.modifiers.piped([ | ||
| UI.modifiers.topPadding, | ||
| UI.modifiers.dimOnComplete, | ||
| ]), | ||
| validate: (val) => { | ||
| if (!val) { | ||
| return 'Project ID is required'; | ||
| } | ||
| if (!firebaseProjectIdRegex.test(val)) { | ||
| return 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.'; | ||
| } | ||
| }, | ||
| }, | ||
| }); | ||
| // typeguard | ||
| if (!clientName || !projectId) { | ||
| return yield* BadArgsError.make({ | ||
| message: 'Missing required arguments', | ||
| }); | ||
| } | ||
| if (!firebaseProjectIdRegex.test(projectId)) { | ||
| return yield* BadArgsError.make({ | ||
| message: | ||
| 'Invalid Firebase project ID. It must be 6-30 characters, start with a lowercase letter, contain only lowercase letters, digits, and hyphens, and not end with a hyphen.', | ||
| }); | ||
| } | ||
| const response = yield* addOAuthClient({ | ||
| providerId: provider.id, | ||
| clientName, | ||
| discoveryEndpoint: `https://securetoken.google.com/${encodeURIComponent(projectId)}/.well-known/openid-configuration`, | ||
| }); | ||
|
|
||
| yield* Effect.log( | ||
| boxen( | ||
| [ | ||
| `Firebase OAuth client created: ${response.client.client_name}`, | ||
| `ID: ${response.client.id}`, | ||
| `Firebase Project ID: ${projectId}`, | ||
| ].join('\n'), | ||
| { dimBorder: true, padding: { right: 1, left: 1 } }, | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate any Firebase test file and inspect its coverage.
fd -t f -i 'firebase' client/packages/cli/__tests__ || true
rg -nP --type=ts -C2 '\b(firebase|project-id|securetoken)\b' client/packages/cli/__tests__ || trueRepository: instantdb/instant
Length of output: 43
🏁 Script executed:
# First, check the test directory structure
ls -la client/packages/cli/__tests__/ 2>/dev/null | head -20 || echo "Directory check failed"
# Find all test files matching the authClientAdd pattern
fd -t f 'authClientAdd.*test' client/packages/cli --hidden 2>/dev/null
# Also search more broadly for test files in the auth commands directory
find client/packages/cli -name '*test*' -o -name '*spec*' 2>/dev/null | grep -i auth | head -20Repository: instantdb/instant
Length of output: 1536
🏁 Script executed:
# Examine one existing test file to see patterns tested
head -60 client/packages/cli/__tests__/authClientAddGoogle.test.ts
# Verify the Firebase handler exists in the main file
rg -n 'handleFirebaseClient|const handleFirebase' client/packages/cli/src/commands/auth/client/add.ts | head -5Repository: instantdb/instant
Length of output: 1905
Add authClientAddFirebase.test.ts to match test coverage of other OAuth providers.
Firebase is the only OAuth provider lacking a dedicated test file. All other providers (Apple, Clerk, GitHub, Google, LinkedIn) have authClientAdd<Provider>.test.ts files. Given the PR description highlights Firebase flag validation and interactive/non-interactive behaviors, add tests covering: missing --name/--project-id with --yes, interactive prompts for each field, invalid project ID format (both via flag and prompt), and the success case asserting the discoveryEndpoint matches https://securetoken.google.com/<projectId>/.well-known/openid-configuration.
🤖 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 725 - 784,
Add a new test file authClientAddFirebase.test.ts that mirrors other provider
tests and validates handleFirebaseClient behavior: write tests for (1) missing
--name and --project-id with --yes returning a BadArgsError, (2) interactive
prompts for clientName and project-id using the existing test prompting
utilities to simulate user input, (3) invalid project-id format both when passed
as a flag and when entered interactively (expecting BadArgsError/validation
message), and (4) the success case where addOAuthClient is stubbed/mocked to
capture its args and assert discoveryEndpoint equals
https://securetoken.google.com/<projectId>/.well-known/openid-configuration and
the CLI logs the created client info; use the same test helpers/mocks used by
other authClientAdd<Provider>.test.ts files and reference handleFirebaseClient,
firebaseProjectIdRegex, and addOAuthClient to locate the code under test.
The fix: the refactor moved `getOrCreateProvider` / `findName` / `getClientNameAndProvider` into `oauth.ts`, but the test files mock `oauth.ts` as a whole — so the new internal helpers were missing from the mocks. I added `__tests__/oauthMock.ts` with `makeOAuthMock(...)` that wraps the three existing per-file stubs (`getAppsAuth`, `addOAuthProvider`, `addOAuthClient`) and fills in the three new helpers using the real `optOrPrompt` / `validateRequired`. Each test file now calls this wrapper inside its `vi.mock` factory.
drew-harris
force-pushed
the
drewh/firebase-oauth-cli
branch
from
bcd08a5 to
ae5ce69
Compare
drew-harris
changed the title
2 participants
Adds ability to add firebase oauth clients via cli.