refactor(firebase): replace REST API clients with googleapis library#22
refactor(firebase): replace REST API clients with googleapis library#22pitzcarraldo wants to merge 4 commits intomainfrom
Conversation
- Add IAM API client for Service Account operations (list, create, download key) - Add GCP project API for listing available projects and adding Firebase - Extend OAuth scopes to include iam and cloud-platform permissions - Create Zod-based Service Account JSON validator with real-time feedback - Extend FirebaseDownloader with SA management methods - Add new wizard phases: no_projects, service_account_menu, paste_json - Support JSON paste with real-time validation and error display - Save service account keys to .clix/service-account.json
Replace direct Firebase Management and IAM REST API implementations with @googleapis/firebase and @googleapis/iam packages. This reduces code by 57% while maintaining full backward compatibility through existing public APIs. Key improvements: - Reduced firebase-api.ts from 366 to 223 lines (-39%) - Reduced iam-api.ts from 219 to 126 lines (-42%) - Removed duplicate pagination and token handling logic - Type safety improved with generated types from googleapis - Automatic token refresh via OAuth2Client - Cleaner error handling with googleapis's built-in support All 588 unit tests and 20 E2E tests pass.
WalkthroughThis PR removes Firebase and iOS setup commands, migrates the Firebase API to use googleapis libraries, and introduces a comprehensive installation preparation workflow system. It adds setup status tracking via project config, restructures Firebase credential handling, and enhances the Firebase wizard with new phases and service account management. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c184418fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| compactCommand, | ||
| agentCommand, | ||
| debugCommand, | ||
| firebaseCommand, | ||
| iosSetupCommand, | ||
| transferCommand, | ||
| resumeCommand, |
There was a problem hiding this comment.
/firebase and /ios-setup are no longer in BUILT_IN_COMMANDS, but command execution still gates on getCommand(...) before the switch in useCommandHandler, so both commands now resolve as unknown and their existing handlers become unreachable. This breaks the interactive setup paths that are still referenced elsewhere (including embedded skill guidance), so users can no longer invoke these setup flows from chat.
Useful? React with 👍 / 👎.
| const updatedContext: PreparationContext = { | ||
| ...context, | ||
| firebase: updatedFirebase, | ||
| missing: context.missing.filter((m) => !m.includes('Firebase')), |
There was a problem hiding this comment.
In handleFirebaseComplete, Firebase-related missing items are always removed even when result.completed is false, so skipping or failing Firebase setup can still produce a context that no longer reports missing Firebase prerequisites. Because this context is forwarded to install skill prompting, the agent can proceed without being told required Firebase config is absent, leading to incomplete or broken installation runs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 52-54: Update the invalid dependency versions in package.json:
change "@googleapis/cloudresourcemanager" from ^6.0.1 to ^3.0.1,
"@googleapis/firebase" from ^12.0.1 to ^10.0.1, and "@googleapis/iam" from
^36.0.0 to ^30.1.0 so they match published npm releases; leave
"google-auth-library" at ^10.5.0 as-is, then run npm install (or yarn) and a
quick build/test to verify no further dependency conflicts.
In `@src/commands/skill/preparation.ts`:
- Around line 159-169: The readiness check incorrectly treats projectType.target
=== 'unknown' as not needing Firebase/iOS setup; update the logic in the
preparation routine so that when projectType.target === 'unknown' it is treated
as needing setup (i.e., needed = true or explicitly block readiness) instead of
returning configured true. Modify the branch around the needed variable and the
blocks that set configured, androidConfigured, and iosConfigured (and the
similar checks referenced later) so unknown target forces configured:false (or
prevents ready:true) and flows through normal setup checks for Firebase and iOS.
In `@src/lib/services/firebase/downloader.ts`:
- Around line 417-427: The saveServiceAccountJson function currently writes the
service-account.json with default permissions; change it to enforce owner-only
read/write permissions like credentials.json does by creating the .clix dir as
before, writing the file, then calling fs.chmod(savePath, 0o600) (and
handle/await any errors) so the file is restricted; reference
saveServiceAccountJson and mimic the chmod pattern used in
src/lib/auth/credentials.ts for credentials.json.
In `@src/lib/services/firebase/oauth/config.ts`:
- Around line 157-168: Update the OAuth scopes array in the scopes property so
it uses least-privilege Cloud Resource Manager scope(s) instead of the broad
cloud-platform scope: replace 'https://www.googleapis.com/auth/cloud-platform'
in the scopes array with the more specific
'https://www.googleapis.com/auth/cloudplatformprojects' (or
'https://www.googleapis.com/auth/cloudplatformprojects.readonly' if only
listing/reading projects) while keeping the existing firebase and iam scopes;
locate this change in the scopes definition in config.ts.
In `@src/ui/components/FirebaseWizard.tsx`:
- Around line 208-215: The menu item for "setup-service-account" is incorrectly
gated only on Android; change the condition in the FirebaseWizard component so
the item is shown when OAuth is configured AND a project ID or valid platform
config exists (use getProjectIdFromResult(result) or check result.ios?.valid)
instead of relying solely on result.android?.valid; update the conditional that
calls items.push({ id: 'setup-service-account', ... }) to use
isOAuthConfigured() && (getProjectIdFromResult(result) != null ||
result.ios?.valid || result.android?.valid) so iOS-only projects with a project
ID will see the menu entry.
In `@src/ui/components/InstallPreparationUI.tsx`:
- Around line 418-423: The render branch for phase 'ios_setup' calls
handleIosSkip() directly which triggers setPhase during render; instead remove
that direct call and create a useEffect that watches the phase (or a derived
boolean), context, and handleIosSkip, and invokes handleIosSkip() inside the
effect when phase === 'ios_setup' and context is present; keep the component
returning <CheckingPhase /> from the render path and ensure the effect's
dependency array includes handleIosSkip and context to avoid stale closures.
package.json
Outdated
| "@googleapis/cloudresourcemanager": "^6.0.1", | ||
| "@googleapis/firebase": "^12.0.1", | ||
| "@googleapis/iam": "^36.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the package.json to see the context and google-auth-library version
head -60 package.json | tail -20Repository: clix-so/clix-cli
Length of output: 679
🌐 Web query:
Latest npm versions peer requirements @googleapis/firebase @googleapis/iam @googleapis/cloudresourcemanager google-auth-library compatibility
💡 Result:
Latest npm package versions (as of Feb 6, 2026)
@googleapis/firebase: 10.0.1 [1]@googleapis/iam: 30.1.0 [2]@googleapis/cloudresourcemanager: 3.0.1 [3]google-auth-library: 10.5.0 [4]
Peer requirements / compatibility (what to align)
The @googleapis/* API packages are generated from the googleapis Node client ecosystem, which (for current releases) is built to use:
google-auth-library: ^10.2.0 (i.e., any compatible 10.x, including 10.5.0) [5]googleapis-common: ^8.0.0 [5]
google-auth-library 10.x requires Node.js >= 18. [6]
So the practical compatibility target is:
- Node >= 18
- google-auth-library 10.x (10.5.0 is fine), satisfying ^10.2.0.
Sources: [1] [2] [3] [4] [5] [6]
Update googleapis package versions to available releases on npm.
The specified versions do not match current releases: @googleapis/firebase should be ^10.0.1 (not ^12.0.1), @googleapis/iam should be ^30.1.0 (not ^36.0.0), and @googleapis/cloudresourcemanager should be ^3.0.1 (not ^6.0.1). The current specification will cause npm installation to fail as these versions do not exist on npm. The google-auth-library ^10.5.0 requirement is compatible with the @googleapis packages, but only with corrected versions.
🤖 Prompt for AI Agents
In `@package.json` around lines 52 - 54, Update the invalid dependency versions in
package.json: change "@googleapis/cloudresourcemanager" from ^6.0.1 to ^3.0.1,
"@googleapis/firebase" from ^12.0.1 to ^10.0.1, and "@googleapis/iam" from
^36.0.0 to ^30.1.0 so they match published npm releases; leave
"google-auth-library" at ^10.5.0 as-is, then run npm install (or yarn) and a
quick build/test to verify no further dependency conflicts.
| // Determine if Firebase is needed based on project type | ||
| const needed = projectType.target !== 'unknown'; | ||
|
|
||
| if (!needed) { | ||
| return { | ||
| configured: true, | ||
| androidConfigured: true, | ||
| iosConfigured: true, | ||
| needed: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Unknown project targets can falsely mark preparation as ready.
Both Firebase and iOS status checks treat target === 'unknown' as “not needed,” which can produce ready = true even when setup is missing. Consider treating unknown as needing setup (or blocking readiness) to avoid false positives.
🛠️ Suggested behavior for unknown targets
- // Determine if Firebase is needed based on project type
- const needed = projectType.target !== 'unknown';
-
- if (!needed) {
- return {
- configured: true,
- androidConfigured: true,
- iosConfigured: true,
- needed: false,
- };
- }
+ if (projectType.target === 'unknown') {
+ return {
+ configured: false,
+ androidConfigured: false,
+ iosConfigured: false,
+ needed: true,
+ };
+ }- // iOS setup is only needed for iOS or cross-platform targets
- const needed = projectType.target === 'ios' || projectType.target === 'ios-android';
-
- if (!needed) {
- return {
- needed: false,
- entitlementsConfigured: true,
- nseConfigured: true,
- };
- }
+ if (projectType.target === 'unknown') {
+ return {
+ needed: true,
+ entitlementsConfigured: false,
+ nseConfigured: false,
+ };
+ }
+ const needed = projectType.target === 'ios' || projectType.target === 'ios-android';
+ if (!needed) {
+ return {
+ needed: false,
+ entitlementsConfigured: true,
+ nseConfigured: true,
+ };
+ }Also applies to: 224-233
🤖 Prompt for AI Agents
In `@src/commands/skill/preparation.ts` around lines 159 - 169, The readiness
check incorrectly treats projectType.target === 'unknown' as not needing
Firebase/iOS setup; update the logic in the preparation routine so that when
projectType.target === 'unknown' it is treated as needing setup (i.e., needed =
true or explicitly block readiness) instead of returning configured true. Modify
the branch around the needed variable and the blocks that set configured,
androidConfigured, and iosConfigured (and the similar checks referenced later)
so unknown target forces configured:false (or prevents ready:true) and flows
through normal setup checks for Firebase and iOS.
| async saveServiceAccountJson( | ||
| projectPath: string, | ||
| serviceAccountJson: ServiceAccountJson, | ||
| ): Promise<string> { | ||
| const clixDir = path.join(projectPath, '.clix'); | ||
| const savePath = path.join(clixDir, 'service-account.json'); | ||
|
|
||
| await fs.mkdir(clixDir, { recursive: true }); | ||
| await fs.writeFile(savePath, JSON.stringify(serviceAccountJson, null, 2), 'utf-8'); | ||
|
|
||
| return savePath; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check .gitignore for .clix exclusion
find . -name '.gitignore' -type f | head -10Repository: clix-so/clix-cli
Length of output: 72
🏁 Script executed:
# Search for .clix related entries in .gitignore files
rg -n '\.(clix|clix/)' --glob '.gitignore'Repository: clix-so/clix-cli
Length of output: 106
🏁 Script executed:
# Look for other credential/session storage patterns in the codebase
rg -n 'saveServiceAccountJson|credentials\.json|sessions/' src/lib --type ts --type tsx -A 3 -B 1Repository: clix-so/clix-cli
Length of output: 87
🏁 Script executed:
# Check if fs.mkdir or fs.writeFile with mode options are used elsewhere
rg -n 'fs\.(mkdir|writeFile).*mode' src/ --type ts --type tsxRepository: clix-so/clix-cli
Length of output: 87
🏁 Script executed:
# Search for fs.mkdir or fs.writeFile with mode across ts files
rg -n 'fs\.(mkdir|writeFile)' src/ -A 2 | head -50Repository: clix-so/clix-cli
Length of output: 2695
🏁 Script executed:
# Look at how credentials.json is being saved
rg -n 'credentials\.json' src/lib -A 3 -B 1 -g '*.ts'Repository: clix-so/clix-cli
Length of output: 1097
🏁 Script executed:
# Read the downloader.ts file to see the full context
cat -n src/lib/services/firebase/downloader.ts | sed -n '410,435p'Repository: clix-so/clix-cli
Length of output: 1073
🏁 Script executed:
# Read credentials.ts to see how it saves the credentials file
cat -n src/lib/auth/credentials.ts | head -150Repository: clix-so/clix-cli
Length of output: 5099
🏁 Script executed:
# Look for the actual save method in credentials.ts
rg -n 'saveCredentials|writeFile|mkdir' src/lib/auth/credentials.ts -A 4 -B 1Repository: clix-so/clix-cli
Length of output: 734
Harden service-account key storage permissions.
The private key is written with default permissions, creating a security risk on shared machines. The credentials.json file already uses restricted permissions (0o600) and chmod() enforcement in src/lib/auth/credentials.ts—apply the same pattern here.
🔐 Proposed hardening for key storage
- await fs.mkdir(clixDir, { recursive: true });
- await fs.writeFile(savePath, JSON.stringify(serviceAccountJson, null, 2), 'utf-8');
+ await fs.mkdir(clixDir, { recursive: true, mode: 0o700 });
+ await fs.writeFile(
+ savePath,
+ JSON.stringify(serviceAccountJson, null, 2),
+ { encoding: 'utf-8', mode: 0o600 },
+ );📝 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.
| async saveServiceAccountJson( | |
| projectPath: string, | |
| serviceAccountJson: ServiceAccountJson, | |
| ): Promise<string> { | |
| const clixDir = path.join(projectPath, '.clix'); | |
| const savePath = path.join(clixDir, 'service-account.json'); | |
| await fs.mkdir(clixDir, { recursive: true }); | |
| await fs.writeFile(savePath, JSON.stringify(serviceAccountJson, null, 2), 'utf-8'); | |
| return savePath; | |
| async saveServiceAccountJson( | |
| projectPath: string, | |
| serviceAccountJson: ServiceAccountJson, | |
| ): Promise<string> { | |
| const clixDir = path.join(projectPath, '.clix'); | |
| const savePath = path.join(clixDir, 'service-account.json'); | |
| await fs.mkdir(clixDir, { recursive: true, mode: 0o700 }); | |
| await fs.writeFile( | |
| savePath, | |
| JSON.stringify(serviceAccountJson, null, 2), | |
| { encoding: 'utf-8', mode: 0o600 }, | |
| ); | |
| return savePath; | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/services/firebase/downloader.ts` around lines 417 - 427, The
saveServiceAccountJson function currently writes the service-account.json with
default permissions; change it to enforce owner-only read/write permissions like
credentials.json does by creating the .clix dir as before, writing the file,
then calling fs.chmod(savePath, 0o600) (and handle/await any errors) so the file
is restricted; reference saveServiceAccountJson and mimic the chmod pattern used
in src/lib/auth/credentials.ts for credentials.json.
| /** | ||
| * OAuth scopes required for Firebase Management API. | ||
| * Using full firebase scope for listing projects/apps, downloading configs, and creating apps. | ||
| * | ||
| * - firebase: List projects/apps, download configs, create apps | ||
| * - iam: Service Account management (create, list, generate keys) | ||
| * - cloud-platform: GCP project access (list projects, add Firebase to existing projects) | ||
| */ | ||
| scopes: ['https://www.googleapis.com/auth/firebase'], | ||
| scopes: [ | ||
| 'https://www.googleapis.com/auth/firebase', | ||
| 'https://www.googleapis.com/auth/iam', | ||
| 'https://www.googleapis.com/auth/cloud-platform', | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file to understand what operations use these scopes
cat -n src/lib/services/firebase/oauth/config.tsRepository: clix-so/clix-cli
Length of output: 7806
🏁 Script executed:
# Search for how these scopes are used in the codebase
rg -n "GOOGLE_OAUTH_CONFIG" --type ts --type tsx -B 3 -A 3Repository: clix-so/clix-cli
Length of output: 87
🏁 Script executed:
# Look for Firebase Admin API calls and GCP project operations
rg -n "firebase|iam|cloud-platform|projects\.|apps\." src/lib/services/firebase/ --type ts --type tsx -A 2Repository: clix-so/clix-cli
Length of output: 87
🌐 Web query:
Google Cloud OAuth scopes Firebase Management API IAM service account GCP projects minimum required permissions
💡 Result:
OAuth scopes (Firebase Management API)
Firebase Management API methods accept either Firebase-specific scopes or the broader Cloud Platform scopes:
- Read/write:
https://www.googleapis.com/auth/firebaseorhttps://www.googleapis.com/auth/cloud-platform[1] - Read-only (for read methods like
projects.get/projects.list):https://www.googleapis.com/auth/firebase.readonlyorhttps://www.googleapis.com/auth/cloud-platform.read-only[2][3]
Minimum IAM permissions (example: projects.addFirebase)
To call projects.addFirebase (turn an existing GCP project into a Firebase project), the calling principal (user or service account) must have:
firebase.projects.updateresourcemanager.projects.getserviceusage.services.enableserviceusage.services.get[1][4]
Google notes that Project Editor and Project Owner include these permissions (but you can also build a custom role with exactly these). [1]
“Baseline” permissions commonly required for Firebase project access (custom roles)
Firebase’s IAM docs also list permissions that are required broadly to retrieve Firebase project info and run many Firebase workflows/CLI commands, including:
firebase.projects.get(and otherfirebase.*reads likefirebase.clients.get/list,firebase.links.list)resourcemanager.projects.get,resourcemanager.projects.getIamPolicy,resourcemanager.projects.list- various
serviceusage.*and API Keys reads/list/lookup [5]
If your automation also needs to grant roles / change IAM policy, you additionally need resourcemanager.projects.setIamPolicy (called out as required to manage role assignments). [6]
Sources: [1] Firebase projects.addFirebase (scopes + required permissions), [2] Firebase projects.list (scopes), [3] Firebase projects.get (scopes), [4] Firebase “workflow set up/manage project” (required permissions), [5] Firebase IAM permissions (required permissions tables), [6] Firebase IAM roles (permission needed to assign roles).
🏁 Script executed:
# Search for usage of the GOOGLE_OAUTH_CONFIG and related API calls
rg -n "GOOGLE_OAUTH_CONFIG\|firebase|firebaseapi\|cloudresourcemanager\|projects\.addFirebase" src/lib/services/firebase/ -A 2 -B 2Repository: clix-so/clix-cli
Length of output: 42
🏁 Script executed:
# Find where OAuth token is used and what APIs are called
rg -n "getOAuthCredentials\|oauth\|bearer\|authorization" src/lib/services/firebase/ --type ts -A 3Repository: clix-so/clix-cli
Length of output: 42
🏁 Script executed:
# Check for any Firebase Admin SDK or Google Cloud client usage
rg -n "firebase-admin|google-cloud|googleapis" . -A 2Repository: clix-so/clix-cli
Length of output: 4954
🌐 Web query:
Firebase Management API scopes narrower alternatives to cloud-platform GCP project operations minimum required scope
💡 Result:
For Firebase Management API calls, you usually don’t need the broad GCP scope https://www.googleapis.com/auth/cloud-platform—Firebase provides narrower scopes you can use instead.
Firebase Management API (firebase.googleapis.com) — narrower scopes
- Read Firebase project metadata (example:
projects.get): usehttps://www.googleapis.com/auth/firebase.readonly(narrowest) instead ofcloud-platform.read-only. [1] - Write / manage Firebase project (example:
projects.addFirebase,projects.patch): usehttps://www.googleapis.com/auth/firebase(narrowest write scope).projects.addFirebaseacceptsfirebaseorcloud-platform(not read-only). [2]
If you truly need “GCP project operations” (Cloud Resource Manager)
If your code is calling Cloud Resource Manager (cloudresourcemanager.googleapis.com) endpoints like projects.get / projects.list, the narrower alternatives to cloud-platform are:
- Read-only:
https://www.googleapis.com/auth/cloudplatformprojects.readonly(orcloud-platform.read-only) [3] - Read/write:
https://www.googleapis.com/auth/cloudplatformprojects(orcloud-platform) [3]
Practical “minimum required scope” rule of thumb
- If you only call Firebase Management API: request
firebase.readonly(read) orfirebase(write). [1][2] - Only request
cloudplatformprojects(.readonly)if you are directly calling Cloud Resource Manager project APIs. [3]
Sources: Firebase projects.get scopes [1], Firebase projects.addFirebase scopes [2], Cloud Resource Manager projects.get scopes (includes cloudplatformprojects*) [3].
Replace cloud-platform with narrower scopes aligned to actual API usage.
The cloud-platform scope grants access across all GCP services. Based on the code's use of separate Google API clients (@googleapis/firebase, @googleapis/iam, @googleapis/cloudresourcemanager), consider these narrower alternatives:
https://www.googleapis.com/auth/firebase: Write operations on Firebase Management API (create apps, download configs, add Firebase to projects)https://www.googleapis.com/auth/cloudplatformprojectsorcloudplatformprojects.readonly: Access to Cloud Resource Manager (list/get projects) instead of the broadcloud-platformscopehttps://www.googleapis.com/auth/iam: Already appropriately scoped for service account management
Replace the cloud-platform scope with the specific cloudplatformprojects scope to follow least-privilege principles.
🤖 Prompt for AI Agents
In `@src/lib/services/firebase/oauth/config.ts` around lines 157 - 168, Update the
OAuth scopes array in the scopes property so it uses least-privilege Cloud
Resource Manager scope(s) instead of the broad cloud-platform scope: replace
'https://www.googleapis.com/auth/cloud-platform' in the scopes array with the
more specific 'https://www.googleapis.com/auth/cloudplatformprojects' (or
'https://www.googleapis.com/auth/cloudplatformprojects.readonly' if only
listing/reading projects) while keeping the existing firebase and iam scopes;
locate this change in the scopes definition in config.ts.
Remove backward compatibility code for V1→V2 migration: - Unify ProjectConfigV1Schema and ProjectConfigV2Schema into single ProjectConfigSchema - Change version field from z.literal(2) to z.number() for flexibility - Remove isConfigV1(), isConfigV2(), migrateV1ToV2() functions - Keep ensureLatestVersion() framework for future migrations - Remove loadV2() method, use load() directly - Update CURRENT_PROJECT_CONFIG_VERSION to 1 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/config/project-config-manager.ts`:
- Around line 167-192: The updateSetup method currently merges a
Partial<SetupStatus> and saves without validating, which can produce an invalid
ProjectConfig; modify updateSetup to merge updates into config.setup and then
validate or normalize the resulting config before calling save: use an existing
config validation/normalization utility (or add one) to fill missing default
fields for SetupStatus (e.g., getDefaultSetup or schema validate) and run it
against the merged object, and if validation fails throw a ConfigError; ensure
you reference updateSetup, load, save, SetupStatus and ProjectConfig when
locating and changing the code.
🧹 Nitpick comments (1)
src/lib/config/project-config-schema.ts (1)
123-123: Constrain config version to a non‑negative integer.
Currentlyz.number()allows decimals (e.g., 1.5), which complicates migrations and version comparisons. Versions should be integers.♻️ Proposed fix
- version: z.number(), + version: z.number().int().nonnegative(),
| /** | ||
| * Update the setup status in config. | ||
| * Creates setup object if it doesn't exist. | ||
| * | ||
| * @param updates - Partial setup status to merge | ||
| */ | ||
| async updateSetup(updates: Partial<SetupStatus>): Promise<void> { | ||
| const config = await this.load(); | ||
| if (!config) { | ||
| throw new ConfigError( | ||
| 'Project config not found. Run "clix login" first.', | ||
| ERROR_CODES.PROJECT_CONFIG_NOT_FOUND, | ||
| this.configFilePath, | ||
| ); | ||
| } | ||
|
|
||
| const updatedConfig: ProjectConfig = { | ||
| ...config, | ||
| setup: { | ||
| ...config.setup, | ||
| ...updates, | ||
| }, | ||
| }; | ||
|
|
||
| await this.save(updatedConfig); | ||
| } |
There was a problem hiding this comment.
Prevent saving invalid partial setup objects.
updateSetup accepts Partial<SetupStatus>; if callers pass incomplete sub-objects (e.g., only projectId), the saved config can fail schema validation and later loads will break. Validate the merged config (or fill defaults) before saving.
🛠️ Suggested fix: validate before save
const updatedConfig: ProjectConfig = {
...config,
setup: {
...config.setup,
...updates,
},
};
-
- await this.save(updatedConfig);
+ const validated = safeValidateProjectConfig(updatedConfig);
+ if (!validated) {
+ throw new ConfigError(
+ 'Invalid project configuration',
+ ERROR_CODES.PROJECT_CONFIG_INVALID,
+ this.configFilePath,
+ );
+ }
+
+ await this.save(validated);🤖 Prompt for AI Agents
In `@src/lib/config/project-config-manager.ts` around lines 167 - 192, The
updateSetup method currently merges a Partial<SetupStatus> and saves without
validating, which can produce an invalid ProjectConfig; modify updateSetup to
merge updates into config.setup and then validate or normalize the resulting
config before calling save: use an existing config validation/normalization
utility (or add one) to fill missing default fields for SetupStatus (e.g.,
getDefaultSetup or schema validate) and run it against the merged object, and if
validation fails throw a ConfigError; ensure you reference updateSetup, load,
save, SetupStatus and ProjectConfig when locating and changing the code.
- Migrate Firebase API clients from REST to googleapis library - Remove IAM API (SA keys now downloaded from Firebase Console) - Add Sender Config API integration (check/register push notification config) - Extract FirebaseWizard PHASE_TRANSITIONS state machine with transition() validation - Add Organization/Projects concurrent fetching service - Add ProjectSelector search/filter functionality - Improve InstallPreparationUI with explicit action menus - Add OAuth debug logging infrastructure - Simplify OAuth scopes to firebase-only - Add exponential backoff retry logic for API calls
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/lib/debug/logger.ts`:
- Around line 201-226: The writeToFile method currently calls JSON.stringify on
optional data in both the file-write branch (building variable line) and the
stderr fallback, which can throw on circular or unserializable values; change
writeToFile (and any helper uses of this.namespace within it) to first attempt
JSON.stringify(data) inside a try/catch (or use a small safeStringify helper)
and on failure replace with a safe placeholder (e.g., "[unserializable data]" or
inspect fallback) so that neither the appendFileSync branch nor the
console.error fallback can throw — update the construction of line and the
console.error call to use the safe stringified value instead of calling
JSON.stringify directly.
In `@src/lib/services/firebase/api/firebase-api.ts`:
- Around line 91-97: The code currently type-asserts p.state to 'ACTIVE' |
'DELETED' which can let unexpected strings through; instead normalize p.state
explicitly before pushing into projects (e.g., compute const normalizedState =
p.state === 'DELETED' ? 'DELETED' : 'ACTIVE' and use that). Update the push that
builds the FirebaseProject (the projects.push block using p.state) to use this
normalized value, and apply the same change to the other occurrence (the similar
block around lines 273-277) so the FirebaseProject union is always respected.
In `@src/lib/services/firebase/oauth/auth-client.ts`:
- Around line 336-345: The current refresh error handler in the auth client
unconditionally rewraps any refreshAccessToken failure as "invalid_grant",
causing unnecessary re-auth flows; update the catch in the token refresh block
(where tokenStore.isExpired(tokens) is checked and refreshAccessToken is called)
to inspect the caught error (e.g., error.message or error.code) and only throw a
new Error prefixed with "invalid_grant: " when the underlying error actually
indicates "invalid_grant"; for other errors rethrow the original error (or throw
a generic refresh error without the invalid_grant prefix) so transient
network/IO errors are not misclassified.
In `@src/lib/services/organization-projects.ts`:
- Around line 22-27: The normalizeConcurrency function currently floors
fractional inputs which can produce 0 for values like 0.5; update
normalizeConcurrency to first validate the input, then Math.floor the value and
clamp the result to a minimum of 1 (and fall back to
DEFAULT_PROJECT_FETCH_CONCURRENCY when input is missing/invalid) so the returned
concurrency is always at least 1; reference the normalizeConcurrency function
and DEFAULT_PROJECT_FETCH_CONCURRENCY constant when making the change.
In `@src/lib/utils/oauth.ts`:
- Around line 261-283: The debug write currently persists full_url and
all_params which can leak secrets; before calling oauthLogger.writeToFile (from
within the OAuth error branch), sanitize the data by redacting sensitive
parameter values (e.g., code, state, access_token, refresh_token, client_secret,
refresh_token, id_token) and replace them with a presence flag or "[REDACTED]"
and reconstruct a sanitized full_url without secret values; then pass
sanitized_all_params and sanitized_full_url to oauthLogger.writeToFile (keep use
of findProjectRoot() and existing metadata) so only non-sensitive info or
presence indicators are persisted.
In `@src/ui/components/FirebaseWizard.tsx`:
- Around line 2087-2091: The setTimeout in the FirebaseWizard component
schedules setPhase(...) and onComplete(...) after 1500ms but doesn't clear the
timer, which can lead to state updates after unmount; capture the timeout id
when calling setTimeout (e.g., store it in a ref or local variable tied to the
component), and ensure you call clearTimeout(timeoutId) in a useEffect cleanup
or when the component is unmounted/cancelled so that the scheduled setPhase and
onComplete are not invoked on an unmounted component.
In `@src/ui/components/InstallPreparationUI.tsx`:
- Around line 62-67: getPostFirebasePhase currently only checks
ios.entitlementsConfigured and skips ios_setup if entitlements are present,
which ignores missing NSE; update getPostFirebasePhase (and the related
handleContinue logic) to require both ios.entitlementsConfigured and
ios.nseConfigured to be true before returning 'ready' when ios.needed is true so
that missing NSE keeps the flow in 'ios_setup'. Locate the PreparationContext
usage in getPostFirebasePhase and the function/method named handleContinue and
change their conditional from checking ios.entitlementsConfigured to checking
(ios.entitlementsConfigured && ios.nseConfigured), preserving the ios.needed
guard and returning 'ios_setup' otherwise.
🧹 Nitpick comments (5)
src/ui/components/ProjectSelector.tsx (2)
6-9: Avoid duplicatingOrgWithProjects.This type already exists in
src/lib/services/organization-projects.ts; redefining it here risks drift. Consider importing and re-exporting the shared type.♻️ Suggested refactor
-import type { Organization, Project } from '@/lib/api'; +import type { Organization, Project } from '@/lib/api'; +import type { OrgWithProjects } from '@/lib/services/organization-projects'; @@ -export interface OrgWithProjects { - org: Organization; - projects: Project[]; -} +export type { OrgWithProjects };
84-159: Align selector input handling withGenericSelector+useCancelInput.This is a selector component but handles ESC/Ctrl+C manually; the UI guidelines prefer the shared selector abstraction and cancellation hook.
As per coding guidelines, “Import
useCancelInputfrom@/ui/hooksfor ESC/Ctrl+C cancellation in new components; for selector-based components, extendGenericSelector.”src/lib/services/firebase/api/firebase-api.ts (1)
49-68: Consider replacing the class with a plain-object factory.The codebase guidelines prefer plain objects with interfaces over classes for TS modules.
As per coding guidelines, “Prefer plain objects with interfaces over classes; use ES module exports for encapsulation in TypeScript code.”
src/lib/api/internal-client.ts (1)
71-73: Token resolved once before retry loop may become stale during long retry sequences.The access token is resolved once at line 71, but if
maxRetriesis high and the server returns 5xx repeatedly, the token could expire during the retry attempts. Consider refreshing the token before each retry attempt, or at least after a 401 response within the loop.However, since the default
maxRetriesis 0 and the total retry window is short (150ms + 300ms + ... backoff), this is unlikely to be a practical issue for most use cases.src/ui/components/FirebaseWizard.tsx (1)
799-823: Clipboard reading lacks Windows support.The
readClipboardfunction supports macOS (pbpaste) and Linux (xclip/xsel) but returnsnullon Windows. If Windows users are expected, consider addingpowershell -command "Get-Clipboard"as a fallback.🪟 Optional: Add Windows clipboard support
if (platform === 'linux') { try { const { stdout } = await execFileAsync('xclip', ['-selection', 'clipboard', '-o']); return stdout; } catch { const { stdout } = await execFileAsync('xsel', ['--clipboard', '--output']); return stdout; } } + if (platform === 'win32') { + const { stdout } = await execFileAsync('powershell', ['-command', 'Get-Clipboard']); + return stdout; + } return null;
| writeToFile(message: string, data?: unknown, projectRoot?: string): void { | ||
| // Try multiple locations to ensure logging works | ||
| const locations = [ | ||
| projectRoot, | ||
| process.cwd(), | ||
| process.env.HOME ? join(process.env.HOME, '.clix') : null, | ||
| ].filter((loc): loc is string => loc !== null && loc !== undefined); | ||
|
|
||
| for (const root of locations) { | ||
| try { | ||
| const clixDir = root.endsWith('.clix') ? root : join(root, '.clix'); | ||
| const logFile = join(clixDir, 'debug.log'); | ||
|
|
||
| mkdirSync(clixDir, { recursive: true }); | ||
|
|
||
| const timestamp = new Date().toISOString(); | ||
| const line = `${timestamp} ${this.namespace} ${message}${data !== undefined ? ` ${JSON.stringify(data)}` : ''}\n`; | ||
| appendFileSync(logFile, line); | ||
| return; // Success, exit loop | ||
| } catch { | ||
| // Try next location | ||
| } | ||
| } | ||
| // All locations failed, log to stderr as last resort | ||
| console.error(`[${this.namespace}] ${message}`, data !== undefined ? JSON.stringify(data) : ''); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l src/lib/debug/logger.tsRepository: clix-so/clix-cli
Length of output: 87
🏁 Script executed:
# Read the writeToFile method around the specified lines (201-226)
sed -n '195,230p' src/lib/debug/logger.tsRepository: clix-so/clix-cli
Length of output: 1364
🏁 Script executed:
# Check if there are any other stringify calls in the file
rg 'JSON\.stringify|safeStringify' src/lib/debug/logger.ts -nRepository: clix-so/clix-cli
Length of output: 356
🏁 Script executed:
# Check if there are any circular data patterns or objects that might be passed as data
rg 'writeToFile.*\(' src/lib/debug/logger.ts -A 2 -B 2Repository: clix-so/clix-cli
Length of output: 306
Harden file logging against unserializable context
JSON.stringify can throw on circular data. While line 217 is protected by a try-catch that attempts fallback locations, the stderr fallback on line 225 is unprotected. If all file writes fail and the data is unserializable, JSON.stringify will throw an uncaught exception, crashing the logging mechanism entirely. Wrap JSON.stringify calls in safe error handling.
🔧 Proposed fix
writeToFile(message: string, data?: unknown, projectRoot?: string): void {
+ const safeStringify = (value: unknown): string => {
+ try {
+ return JSON.stringify(value);
+ } catch {
+ return '[unserializable]';
+ }
+ };
// Try multiple locations to ensure logging works
const locations = [
projectRoot,
process.cwd(),
process.env.HOME ? join(process.env.HOME, '.clix') : null,
].filter((loc): loc is string => loc !== null && loc !== undefined);
for (const root of locations) {
try {
const clixDir = root.endsWith('.clix') ? root : join(root, '.clix');
const logFile = join(clixDir, 'debug.log');
mkdirSync(clixDir, { recursive: true });
const timestamp = new Date().toISOString();
- const line = `${timestamp} ${this.namespace} ${message}${data !== undefined ? ` ${JSON.stringify(data)}` : ''}\n`;
+ const serialized = data !== undefined ? safeStringify(data) : '';
+ const line = `${timestamp} ${this.namespace} ${message}${
+ data !== undefined ? ` ${serialized}` : ''
+ }\n`;
appendFileSync(logFile, line);
return; // Success, exit loop
} catch {
// Try next location
}
}
// All locations failed, log to stderr as last resort
- console.error(`[${this.namespace}] ${message}`, data !== undefined ? JSON.stringify(data) : '');
+ const fallback = data !== undefined ? safeStringify(data) : '';
+ console.error(`[${this.namespace}] ${message}`, fallback);
}📝 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.
| writeToFile(message: string, data?: unknown, projectRoot?: string): void { | |
| // Try multiple locations to ensure logging works | |
| const locations = [ | |
| projectRoot, | |
| process.cwd(), | |
| process.env.HOME ? join(process.env.HOME, '.clix') : null, | |
| ].filter((loc): loc is string => loc !== null && loc !== undefined); | |
| for (const root of locations) { | |
| try { | |
| const clixDir = root.endsWith('.clix') ? root : join(root, '.clix'); | |
| const logFile = join(clixDir, 'debug.log'); | |
| mkdirSync(clixDir, { recursive: true }); | |
| const timestamp = new Date().toISOString(); | |
| const line = `${timestamp} ${this.namespace} ${message}${data !== undefined ? ` ${JSON.stringify(data)}` : ''}\n`; | |
| appendFileSync(logFile, line); | |
| return; // Success, exit loop | |
| } catch { | |
| // Try next location | |
| } | |
| } | |
| // All locations failed, log to stderr as last resort | |
| console.error(`[${this.namespace}] ${message}`, data !== undefined ? JSON.stringify(data) : ''); | |
| } | |
| writeToFile(message: string, data?: unknown, projectRoot?: string): void { | |
| const safeStringify = (value: unknown): string => { | |
| try { | |
| return JSON.stringify(value); | |
| } catch { | |
| return '[unserializable]'; | |
| } | |
| }; | |
| // Try multiple locations to ensure logging works | |
| const locations = [ | |
| projectRoot, | |
| process.cwd(), | |
| process.env.HOME ? join(process.env.HOME, '.clix') : null, | |
| ].filter((loc): loc is string => loc !== null && loc !== undefined); | |
| for (const root of locations) { | |
| try { | |
| const clixDir = root.endsWith('.clix') ? root : join(root, '.clix'); | |
| const logFile = join(clixDir, 'debug.log'); | |
| mkdirSync(clixDir, { recursive: true }); | |
| const timestamp = new Date().toISOString(); | |
| const serialized = data !== undefined ? safeStringify(data) : ''; | |
| const line = `${timestamp} ${this.namespace} ${message}${ | |
| data !== undefined ? ` ${serialized}` : '' | |
| }\n`; | |
| appendFileSync(logFile, line); | |
| return; // Success, exit loop | |
| } catch { | |
| // Try next location | |
| } | |
| } | |
| // All locations failed, log to stderr as last resort | |
| const fallback = data !== undefined ? safeStringify(data) : ''; | |
| console.error(`[${this.namespace}] ${message}`, fallback); | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/debug/logger.ts` around lines 201 - 226, The writeToFile method
currently calls JSON.stringify on optional data in both the file-write branch
(building variable line) and the stderr fallback, which can throw on circular or
unserializable values; change writeToFile (and any helper uses of this.namespace
within it) to first attempt JSON.stringify(data) inside a try/catch (or use a
small safeStringify helper) and on failure replace with a safe placeholder
(e.g., "[unserializable data]" or inspect fallback) so that neither the
appendFileSync branch nor the console.error fallback can throw — update the
construction of line and the console.error call to use the safe stringified
value instead of calling JSON.stringify directly.
| projects.push({ | ||
| name: p.name ?? '', | ||
| projectId: p.projectId ?? '', | ||
| projectNumber: p.projectNumber ?? '', | ||
| displayName: p.displayName ?? '', | ||
| state: (p.state as 'ACTIVE' | 'DELETED') ?? 'ACTIVE', | ||
| }); |
There was a problem hiding this comment.
Normalize Firebase project state instead of type-asserting.
The current cast doesn’t constrain values; unexpected strings can leak and violate the FirebaseProject contract. Normalize explicitly to the supported union.
🔧 Suggested fix
- state: (p.state as 'ACTIVE' | 'DELETED') ?? 'ACTIVE',
+ state: p.state === 'DELETED' ? 'DELETED' : 'ACTIVE',
@@
- state: (project.state as 'ACTIVE' | 'DELETED') ?? 'ACTIVE',
+ state: project.state === 'DELETED' ? 'DELETED' : 'ACTIVE',Also applies to: 273-277
🤖 Prompt for AI Agents
In `@src/lib/services/firebase/api/firebase-api.ts` around lines 91 - 97, The code
currently type-asserts p.state to 'ACTIVE' | 'DELETED' which can let unexpected
strings through; instead normalize p.state explicitly before pushing into
projects (e.g., compute const normalizedState = p.state === 'DELETED' ?
'DELETED' : 'ACTIVE' and use that). Update the push that builds the
FirebaseProject (the projects.push block using p.state) to use this normalized
value, and apply the same change to the other occurrence (the similar block
around lines 273-277) so the FirebaseProject union is always respected.
| // Check if token is expired and we have a refresh token | ||
| if (this.tokenStore.isExpired(tokens) && tokens.refresh_token) { | ||
| tokens = await this.refreshAccessToken(tokens.refresh_token); | ||
| try { | ||
| tokens = await this.refreshAccessToken(tokens.refresh_token); | ||
| } catch (error) { | ||
| // If refresh failed (e.g., invalid_grant), tokens are already cleared | ||
| // Re-throw with indication that re-authentication is needed | ||
| const message = error instanceof Error ? error.message : 'Token refresh failed'; | ||
| throw new Error(`invalid_grant: ${message}`); | ||
| } |
There was a problem hiding this comment.
Only prefix invalid_grant when that error actually occurs
Right now any refresh failure gets wrapped as invalid_grant, which can force unnecessary re-auth on transient errors. Consider only prefixing when the underlying error is actually invalid_grant.
🔧 Proposed fix
try {
tokens = await this.refreshAccessToken(tokens.refresh_token);
} catch (error) {
// If refresh failed (e.g., invalid_grant), tokens are already cleared
// Re-throw with indication that re-authentication is needed
const message = error instanceof Error ? error.message : 'Token refresh failed';
- throw new Error(`invalid_grant: ${message}`);
+ if (message.includes('invalid_grant')) {
+ throw new Error(`invalid_grant: ${message}`);
+ }
+ throw new Error(message);
}📝 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.
| // Check if token is expired and we have a refresh token | |
| if (this.tokenStore.isExpired(tokens) && tokens.refresh_token) { | |
| tokens = await this.refreshAccessToken(tokens.refresh_token); | |
| try { | |
| tokens = await this.refreshAccessToken(tokens.refresh_token); | |
| } catch (error) { | |
| // If refresh failed (e.g., invalid_grant), tokens are already cleared | |
| // Re-throw with indication that re-authentication is needed | |
| const message = error instanceof Error ? error.message : 'Token refresh failed'; | |
| throw new Error(`invalid_grant: ${message}`); | |
| } | |
| // Check if token is expired and we have a refresh token | |
| if (this.tokenStore.isExpired(tokens) && tokens.refresh_token) { | |
| try { | |
| tokens = await this.refreshAccessToken(tokens.refresh_token); | |
| } catch (error) { | |
| // If refresh failed (e.g., invalid_grant), tokens are already cleared | |
| // Re-throw with indication that re-authentication is needed | |
| const message = error instanceof Error ? error.message : 'Token refresh failed'; | |
| if (message.includes('invalid_grant')) { | |
| throw new Error(`invalid_grant: ${message}`); | |
| } | |
| throw new Error(message); | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/services/firebase/oauth/auth-client.ts` around lines 336 - 345, The
current refresh error handler in the auth client unconditionally rewraps any
refreshAccessToken failure as "invalid_grant", causing unnecessary re-auth
flows; update the catch in the token refresh block (where
tokenStore.isExpired(tokens) is checked and refreshAccessToken is called) to
inspect the caught error (e.g., error.message or error.code) and only throw a
new Error prefixed with "invalid_grant: " when the underlying error actually
indicates "invalid_grant"; for other errors rethrow the original error (or throw
a generic refresh error without the invalid_grant prefix) so transient
network/IO errors are not misclassified.
| function normalizeConcurrency(input?: number): number { | ||
| if (!input || !Number.isFinite(input) || input <= 0) { | ||
| return DEFAULT_PROJECT_FETCH_CONCURRENCY; | ||
| } | ||
| return Math.floor(input); | ||
| } |
There was a problem hiding this comment.
Clamp fractional concurrency to at least 1.
Values like 0.5 currently floor to 0, yielding zero workers and empty results. Clamp after flooring.
🛠️ Proposed fix
function normalizeConcurrency(input?: number): number {
- if (!input || !Number.isFinite(input) || input <= 0) {
- return DEFAULT_PROJECT_FETCH_CONCURRENCY;
- }
- return Math.floor(input);
+ const normalized = Math.floor(input ?? NaN);
+ if (!Number.isFinite(normalized) || normalized <= 0) {
+ return DEFAULT_PROJECT_FETCH_CONCURRENCY;
+ }
+ return normalized;
}📝 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.
| function normalizeConcurrency(input?: number): number { | |
| if (!input || !Number.isFinite(input) || input <= 0) { | |
| return DEFAULT_PROJECT_FETCH_CONCURRENCY; | |
| } | |
| return Math.floor(input); | |
| } | |
| function normalizeConcurrency(input?: number): number { | |
| const normalized = Math.floor(input ?? NaN); | |
| if (!Number.isFinite(normalized) || normalized <= 0) { | |
| return DEFAULT_PROJECT_FETCH_CONCURRENCY; | |
| } | |
| return normalized; | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/services/organization-projects.ts` around lines 22 - 27, The
normalizeConcurrency function currently floors fractional inputs which can
produce 0 for values like 0.5; update normalizeConcurrency to first validate the
input, then Math.floor the value and clamp the result to a minimum of 1 (and
fall back to DEFAULT_PROJECT_FETCH_CONCURRENCY when input is missing/invalid) so
the returned concurrency is always at least 1; reference the
normalizeConcurrency function and DEFAULT_PROJECT_FETCH_CONCURRENCY constant
when making the change.
| // Handle OAuth error | ||
| if (error) { | ||
| const errorMsg = errorDescription || error; | ||
| const errorMsg = errorDescription ? `${error}: ${errorDescription}` : error; | ||
|
|
||
| // Write debug info to .clix/debug.log | ||
| oauthLogger.writeToFile( | ||
| 'OAuth callback error', | ||
| { | ||
| type: 'oauth_callback_error', | ||
| error, | ||
| error_description: errorDescription, | ||
| full_url: req.url, | ||
| all_params: Object.fromEntries(url.searchParams.entries()), | ||
| }, | ||
| findProjectRoot(), | ||
| ); | ||
|
|
||
| res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }); | ||
| res.end(this.options.errorHtml(errorMsg)); | ||
| clearTimeout(timeout); | ||
| this.stop(); | ||
| reject(new Error(`OAuth error: ${errorMsg}`)); | ||
| reject(new Error(errorMsg)); | ||
| return; |
There was a problem hiding this comment.
Redact sensitive OAuth parameters before persisting to debug.log
writeToFile always writes, and full_url + all_params can include secrets like code and state. Please redact or log only presence flags before persisting.
🛡️ Proposed fix
const error = url.searchParams.get('error');
const errorDescription = url.searchParams.get('error_description');
+ const allParams = Object.fromEntries(url.searchParams.entries());
+ const redactedParams = { ...allParams };
+ if (redactedParams.code) redactedParams.code = '[redacted]';
+ if (redactedParams.state) redactedParams.state = '[redacted]';
+ const sanitizedUrl = (req.url ?? '')
+ .replace(/code=[^&]+/g, 'code=[redacted]')
+ .replace(/state=[^&]+/g, 'state=[redacted]');
// Handle OAuth error
if (error) {
const errorMsg = errorDescription ? `${error}: ${errorDescription}` : error;
// Write debug info to .clix/debug.log
oauthLogger.writeToFile(
'OAuth callback error',
{
type: 'oauth_callback_error',
error,
error_description: errorDescription,
- full_url: req.url,
- all_params: Object.fromEntries(url.searchParams.entries()),
+ full_url: sanitizedUrl,
+ all_params: redactedParams,
},
findProjectRoot(),
);🤖 Prompt for AI Agents
In `@src/lib/utils/oauth.ts` around lines 261 - 283, The debug write currently
persists full_url and all_params which can leak secrets; before calling
oauthLogger.writeToFile (from within the OAuth error branch), sanitize the data
by redacting sensitive parameter values (e.g., code, state, access_token,
refresh_token, client_secret, refresh_token, id_token) and replace them with a
presence flag or "[REDACTED]" and reconstruct a sanitized full_url without
secret values; then pass sanitized_all_params and sanitized_full_url to
oauthLogger.writeToFile (keep use of findProjectRoot() and existing metadata) so
only non-sensitive info or presence indicators are persisted.
| // Auto-complete after brief display | ||
| setTimeout(() => { | ||
| setPhase(transition('registering_sender_config', 'complete')); | ||
| onComplete({ completed: true, skipped: false, detection: result }); | ||
| }, 1500); |
There was a problem hiding this comment.
Missing cleanup for setTimeout may cause state update on unmounted component.
The setTimeout at line 2088 schedules a state update after 1500ms, but if the component unmounts before then (e.g., user cancels), this could cause a React warning about updating unmounted components.
🧹 Proposed fix with cleanup
useEffect(() => {
if (phase !== 'registering_sender_config' || !clixProjectId) return;
+ let timeoutId: NodeJS.Timeout | undefined;
+
const registerSenderConfig = async () => {
try {
// ... existing registration logic ...
setSenderConfigResult('success');
} catch {
setSenderConfigResult('failed');
}
// Auto-complete after brief display
- setTimeout(() => {
+ timeoutId = setTimeout(() => {
setPhase(transition('registering_sender_config', 'complete'));
onComplete({ completed: true, skipped: false, detection: result });
}, 1500);
};
registerSenderConfig();
+
+ return () => {
+ if (timeoutId) clearTimeout(timeoutId);
+ };
}, [phase, clixProjectId, projectPath, onComplete, result]);🤖 Prompt for AI Agents
In `@src/ui/components/FirebaseWizard.tsx` around lines 2087 - 2091, The
setTimeout in the FirebaseWizard component schedules setPhase(...) and
onComplete(...) after 1500ms but doesn't clear the timer, which can lead to
state updates after unmount; capture the timeout id when calling setTimeout
(e.g., store it in a ref or local variable tied to the component), and ensure
you call clearTimeout(timeoutId) in a useEffect cleanup or when the component is
unmounted/cancelled so that the scheduled setPhase and onComplete are not
invoked on an unmounted component.
| export function getPostFirebasePhase(context: PreparationContext): PreparationPhase { | ||
| if (context.ios.needed && !context.ios.entitlementsConfigured) { | ||
| return 'ios_setup'; | ||
| } | ||
| return 'ready'; | ||
| } |
There was a problem hiding this comment.
iOS phase transitions ignore missing NSE configuration
getPostFirebasePhase and handleContinue only check entitlementsConfigured. If NSE is missing but entitlements are present, the flow skips ios_setup and jumps to ready. Consider including nseConfigured in the checks.
🔧 Proposed fix
export function getPostFirebasePhase(context: PreparationContext): PreparationPhase {
- if (context.ios.needed && !context.ios.entitlementsConfigured) {
+ if (
+ context.ios.needed &&
+ (!context.ios.entitlementsConfigured || !context.ios.nseConfigured)
+ ) {
return 'ios_setup';
}
return 'ready';
}
@@
if (phase === 'firebase_check') {
if (!context.firebase.configured && context.firebase.needed) {
setPhase('firebase_setup');
- } else if (context.ios.needed && !context.ios.entitlementsConfigured) {
+ } else if (
+ context.ios.needed &&
+ (!context.ios.entitlementsConfigured || !context.ios.nseConfigured)
+ ) {
setPhase('ios_setup');
} else {
setPhase('ready');
}
} else if (phase === 'ios_check') {
- if (!context.ios.entitlementsConfigured && context.ios.needed) {
+ if (
+ context.ios.needed &&
+ (!context.ios.entitlementsConfigured || !context.ios.nseConfigured)
+ ) {
setPhase('ios_setup');
} else {
setPhase('ready');
}Also applies to: 404-420
🤖 Prompt for AI Agents
In `@src/ui/components/InstallPreparationUI.tsx` around lines 62 - 67,
getPostFirebasePhase currently only checks ios.entitlementsConfigured and skips
ios_setup if entitlements are present, which ignores missing NSE; update
getPostFirebasePhase (and the related handleContinue logic) to require both
ios.entitlementsConfigured and ios.nseConfigured to be true before returning
'ready' when ios.needed is true so that missing NSE keeps the flow in
'ios_setup'. Locate the PreparationContext usage in getPostFirebasePhase and the
function/method named handleContinue and change their conditional from checking
ios.entitlementsConfigured to checking (ios.entitlementsConfigured &&
ios.nseConfigured), preserving the ios.needed guard and returning 'ios_setup'
otherwise.
1 participant
Summary
Replace direct Firebase Management and IAM REST API implementations with @googleapis/firebase and @googleapis/iam packages, reducing code by 57% while maintaining full backward compatibility.
Details
This refactoring leverages the official googleapis library for Firebase and IAM APIs instead of maintaining custom REST clients. Benefits include:
Key metrics:
How to Validate
bun test(all 588 unit tests pass)bun run build && bun test tests/e2e/(all 20 E2E tests pass)bun run typecheck(no errors)bun run lint(no warnings)Try Firebase operations in CLI:
DEBUG=1 bun run dev→/firebasecommandRelated Issues
Related to #21 (project-local storage migration)
Summary by CodeRabbit
New Features
Bug Fixes
Chores