Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a complete Firebase authentication provider implementation to the browser SDK, including error handling infrastructure, type definitions for Firebase configuration and signature operations, core provider logic for user authentication and path derivation, and utility functions for transaction encoding. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser SDK
participant Firebase as Firebase Auth
participant Provider as Google/Apple
participant App as Client App
User->>Browser: FirebaseProvider.login("google")
Browser->>Firebase: Initialize Firebase with config
Browser->>Firebase: Create GoogleAuthProvider
Browser->>Firebase: Trigger signInWithPopup()
Firebase->>Provider: Redirect to provider login
Provider->>User: Login UI
User-->>Provider: Authenticate
Provider-->>Firebase: Auth token
Firebase-->>Browser: Update auth state
Browser->>Browser: Store currentUser
Browser-->>App: login() resolves
App->>Browser: FirebaseProvider.getPath()
Browser->>Firebase: Get current user
Browser->>Firebase: Get ID token
Firebase-->>Browser: JWT token
Browser->>Browser: Decode token, extract 'sub'
Browser-->>App: Return path (issuerUrl + sub)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/sdks/browser/src/providers/firebase/firebase.errors.ts`:
- Around line 3-8: The FirebaseProviderError currently only uses the error code
as the message string; add a public readonly property (e.g., code:
FirebaseProviderErrorCodes) to the FirebaseProviderError class and assign the
incoming code to that property in the constructor while keeping the message via
super(code.toString()); ensure the class still sets this.name =
"FirebaseProviderError" so consumers can programmatically check error.code
instead of parsing the message.
In `@packages/sdks/browser/src/providers/firebase/firebase.provider.ts`:
- Around line 60-63: The JSDoc for the Google sign-in function is missing a
`@param` for providerType; update the comment above the sign-in function (the
JSDoc block that starts "Sign in to the client using Google.") to include a
`@param` tag for providerType (e.g., `@param` {string} providerType - the provider
identifier such as "google") so the doc and type checks pass; ensure the tag
name matches the function parameter exactly.
- Around line 56-58: isLoggedIn() currently checks the asynchronously-populated
this.currentUser causing a race; change it to read this.auth.currentUser
directly (return Boolean(this.auth.currentUser)) or modify the class to
expose/await an authReady promise resolved on the first onAuthStateChanged
callback and have isLoggedIn await that before returning this.currentUser.
Update the isLoggedIn method (and initialize authReady where onAuthStateChanged
is registered) so callers get the correct persisted session state.
- Around line 104-106: The code throws FirebaseProviderError with
FirebaseProviderErrorCodes.USER_NOT_LOGGED_IN when the JWT is missing a sub,
which is misleading; add a new error code (e.g., INVALID_TOKEN or MALFORMED_JWT)
to firebase.error-codes.ts and replace the thrown
FirebaseProviderError(FirebaseProviderErrorCodes.USER_NOT_LOGGED_IN) in the
check for sub with
FirebaseProviderError(FirebaseProviderErrorCodes.INVALID_TOKEN) (or the chosen
name) so the error accurately reflects a malformed/invalid token while keeping
the same FirebaseProviderError type.
In `@packages/sdks/browser/src/providers/firebase/firebase.types.ts`:
- Line 29: The file ends without a trailing newline causing CI to fail; open the
file containing the ProviderType declaration (the export type ProviderType =
"google" | "apple"; line) and add a single newline character at the end of the
file (ensure the file ends with a newline/line break).
🧹 Nitpick comments (6)
packages/sdks/browser/src/providers/firebase/utils/index.ts (2)
13-18: PreferArray.from()or spread for Uint8Array to number[] conversion.The
reducewithpushpattern is verbose.Array.from()or spread operator achieves the same result more idiomatically and efficiently.♻️ Proposed refactor
export function encodeTransaction(transaction: Transaction): number[] { - return encodeTransactionNear(transaction).reduce((acc: number[], curr: number) => { - acc.push(curr); - return acc; - }, [] as number[]); + return Array.from(encodeTransactionNear(transaction)); }
25-30: Same refactor applies toencodeDelegateAction.♻️ Proposed refactor
export function encodeDelegateAction(delegateAction: DelegateAction): number[] { - return encodeDelegateActionNear(delegateAction).reduce((acc: number[], curr: number) => { - acc.push(curr); - return acc; - }, [] as number[]); + return Array.from(encodeDelegateActionNear(delegateAction)); }packages/sdks/browser/package.json (1)
31-31: Consider upgrading to the latest Firebase SDK version.The Firebase SDK dependency is correctly added to support the new Firebase provider functionality. However, a newer major version (12.2.1) is currently available. Consider updating to
"firebase": "^12.2.1"to benefit from the latest features and improvements.packages/sdks/browser/src/providers/firebase/firebase.provider.ts (3)
1-20: Avoid mixing Firebase modular and compat APIs.The code imports from both the modular API (
firebase/app,firebase/auth) and the compat API (firebase/compat/app). This unnecessarily increases bundle size and is not recommended. TheAuthProvidertype can be obtained from the modular API.♻️ Proposed fix
import { initializeApp, type FirebaseApp } from "firebase/app"; import { getAuth, signInWithPopup, GoogleAuthProvider, OAuthProvider, type Auth, type User, + type AuthProvider, } from "firebase/auth"; import { - FirebaseProviderOptions, FirebaseRequestDelegateActionSignatureOptions, FirebaseRequestTransactionSignatureOptions, - ProviderType + FirebaseProviderOptions, + FirebaseRequestDelegateActionSignatureOptions, + FirebaseRequestTransactionSignatureOptions, + ProviderType, } from "./firebase.types"; import { decodeJwt } from "jose"; import { SignatureRequest } from "../../signers"; import { FirebaseProviderError } from "./firebase.errors"; import { FirebaseProviderErrorCodes } from "./firebase.error-codes"; import { IFastAuthProvider } from "../../client/providers/fast-auth.provider"; -import firebase from "firebase/compat/app"; -import AuthProvider = firebase.auth.AuthProvider;
64-67: Unnecessary null check onthis.auth.
this.authis always assigned synchronously in the constructor viagetAuth(this.app), so it can never be null/undefined at this point. This check is dead code. The same applies to the check inlogout().♻️ Proposed fix
async login(providerType: ProviderType): Promise<void> { - if (!this.auth) { - throw new FirebaseProviderError(FirebaseProviderErrorCodes.FIREBASE_NOT_INITIALIZED); - } let provider: AuthProvider;
110-137: Stub methods with TODO comments.These stub implementations are noted for future JWT-based issuer backend integration. Ensure these are tracked for follow-up implementation.
Would you like me to open GitHub issues to track the implementation of these methods?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/sdks/browser/package.jsonpackages/sdks/browser/src/providers/firebase/firebase.error-codes.tspackages/sdks/browser/src/providers/firebase/firebase.errors.tspackages/sdks/browser/src/providers/firebase/firebase.provider.tspackages/sdks/browser/src/providers/firebase/firebase.types.tspackages/sdks/browser/src/providers/firebase/index.tspackages/sdks/browser/src/providers/firebase/utils/index.tspackages/sdks/browser/src/providers/index.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-04-07T12:08:24.795Z
Learnt from: obnol
Repo: Peersyst/near-mobile PR: 154
File: packages/frontend/auth/package.json:0-0
Timestamp: 2025-04-07T12:08:24.795Z
Learning: The BiometricsService is correctly re-exported in /packages/frontend/auth/src/data-access/services/index.ts with the line `export * from "./biometrics.service";`, validating the export path in package.json.
Applied to files:
packages/sdks/browser/src/providers/firebase/index.tspackages/sdks/browser/src/providers/index.ts
📚 Learning: 2025-09-10T13:23:58.048Z
Learnt from: AgustinMJ
Repo: Peersyst/near-mobile PR: 571
File: packages/shared/api/src/index.ts:33-33
Timestamp: 2025-09-10T13:23:58.048Z
Learning: The packages/shared/api package exports source files directly (./src/index.ts) rather than compiled output because it's auto-generated by openapi-typescript-codegen and this export strategy is intentional for this shared package.
Applied to files:
packages/sdks/browser/src/providers/firebase/index.tspackages/sdks/browser/src/providers/index.ts
📚 Learning: 2025-04-17T02:49:42.537Z
Learnt from: JordiParraCrespo
Repo: Peersyst/near-mobile PR: 190
File: packages/frontend/price-service/package.json:18-25
Timestamp: 2025-04-17T02:49:42.537Z
Learning: In the frontend/price-service package, it's important to export the data-access layer modules (like price-service and errors) in the package.json exports configuration to enable other packages to use them when needed.
Applied to files:
packages/sdks/browser/src/providers/firebase/index.tspackages/sdks/browser/src/providers/index.ts
📚 Learning: 2025-08-29T16:22:11.387Z
Learnt from: AgustinMJ
Repo: Peersyst/near-mobile PR: 553
File: apps/mobile/src/modules/token/screens/npro-pre-staking-token-details/screens/npro-pre-staking-token-details-general/containers/npro-pre-staking-token-details-actions/npro-pre-staking-token-details-item/npro-pre-staking-token-details-item.tsx:3-3
Timestamp: 2025-08-29T16:22:11.387Z
Learning: In the NEAR mobile app codebase, the team prefers to use the React namespace (e.g., React.ReactNode) instead of standalone imports (e.g., ReactNode) from the react package.
Applied to files:
packages/sdks/browser/package.json
🧬 Code graph analysis (1)
packages/sdks/browser/src/providers/firebase/firebase.provider.ts (2)
packages/sdks/browser/src/providers/firebase/firebase.types.ts (4)
FirebaseProviderOptions(4-13)ProviderType(29-29)FirebaseRequestTransactionSignatureOptions(21-23)FirebaseRequestDelegateActionSignatureOptions(25-27)packages/sdks/browser/src/providers/firebase/firebase.errors.ts (1)
FirebaseProviderError(3-8)
🪛 GitHub Actions: Pull Request
packages/sdks/browser/src/providers/firebase/firebase.provider.ts
[error] 2-9: prettier/prettier: Replace multi-line import list with a single line: 'getAuth, signInWithPopup, GoogleAuthProvider, OAuthProvider, type Auth, type User' at lines 2:9.
[error] 11-11: prettier/prettier: Replace 'FirebaseRequestDelegateActionSignatureOptions,' with a multi-line broken form as shown in the lint output.
[error] 12-12: prettier/prettier: Insert missing comma at line 12.
[error] 60-60: jsdoc/require-param: Missing JSDoc @param "providerType" declaration.
[error] ESLint/Prettier formatting issues detected in 1 or more files during lint:packages run. Fix formatting issues or run with --fix.
packages/sdks/browser/src/providers/firebase/firebase.types.ts
[error] 29-29: prettier/prettier: Insert a line break before the closing of the type or apply proper formatting at 29:47.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-packages / Build
- GitHub Check: integration-contracts / integration
🔇 Additional comments (6)
packages/sdks/browser/src/providers/index.ts (1)
2-2: LGTM!The Firebase provider is correctly re-exported alongside Auth0, maintaining consistent barrel export pattern.
packages/sdks/browser/src/providers/firebase/index.ts (1)
1-2: The current exports are correct and follow the established pattern.The firebase provider correctly exports only
firebase.providerandfirebase.types, matching the identical pattern used by the auth0 provider. Thefirebase.errors.ts,firebase.error-codes.ts, andutilsmodules are internal implementation details not intended for the public API. Error handling and encoding utilities are consumed internally by the provider and should not be exposed through the barrel export.Likely an incorrect or invalid review comment.
packages/sdks/browser/src/providers/firebase/firebase.error-codes.ts (1)
1-5: LGTM!Clean enum definition with clear, descriptive error codes that map directly to their string values.
packages/sdks/browser/src/providers/firebase/firebase.types.ts (1)
1-13: LGTM!Well-structured type definitions with appropriate imports and clear field names.
packages/sdks/browser/src/providers/firebase/firebase.provider.ts (2)
30-50: LGTM!The constructor properly initializes the Firebase app, authentication providers, and sets up the auth state listener.
86-92: LGTM aside from the unnecessary null check.The logout logic is correct. The null check on
this.authis unnecessary (same as inlogin()), but otherwise the implementation is sound.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
[TA-XXXX]: Title
Changes 🛠️
app/package
Summary by CodeRabbit
New Features
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.