Open
Conversation
✅ Deploy Preview for cv-mobile-app-web ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis PR implements Google OAuth authentication support and updates build configurations. Key changes include: (1) Android SDK version updates (compileSdk to 36, targetSdkVersion to 36), (2) addition of Google Services Gradle plugin configuration, (3) implementation of full Google Sign-In flow in auth viewmodel with token handling and error management, (4) addition of GOOGLE_OAUTH_CLIENT_ID environment configuration, (5) conversion of GitHub OAuth redirect URI to a hard-coded value, (6) improved error handling and non-nullable return type in password reset API endpoint, and (7) Windows plugin registration for quill_native_bridge_windows. Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/API/users_api.dart (1)
41-41: 🛠️ Refactor suggestion | 🟠 MajorAbstract method return type is still nullable — update to match implementation.
The abstract declaration returns
Future<bool>?but the overriding implementation at line 212 returnsFuture<bool>. Update the abstract signature to be consistent.Proposed fix
- Future<bool>? sendResetPasswordInstructions(String email); + Future<bool> sendResetPasswordInstructions(String email);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/API/users_api.dart` at line 41, Update the abstract method signature for sendResetPasswordInstructions in the Users API interface so it returns a non-nullable Future<bool> (change from Future<bool>? to Future<bool>) to match the concrete implementation (the overriding method at/near line where sendResetPasswordInstructions is implemented); ensure the declaration in the UsersApi (or relevant abstract class/interface) uses Future<bool> and update any imports or null-safety annotations if necessary.
🧹 Nitpick comments (5)
android/app/build.gradle (2)
126-136: Excessive blank lines betweendependenciesblock andapply plugin.There are two blank lines (127–128) between the
dependenciesclosing brace and theconfigurations.allblock, and again betweenconfigurations.allandapply plugin(134–135). Trim to single blank lines for consistency with the rest of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/build.gradle` around lines 126 - 136, Remove the extra blank lines around the configurations.all block so spacing matches the rest of the file: collapse the two blank lines after the dependencies closing brace and the two blank lines between the configurations.all { ... } block and the apply plugin: 'com.google.gms.google-services' line to a single blank line each; locate the dependencies closing brace, the configurations.all / resolutionStrategy block, and the apply plugin: 'com.google.gms.google-services' declaration to make this change.
129-133: Resolution strategy forcingandroidx.browser:browser:1.8.0— add an explanatory comment.Dependency resolution overrides can mask underlying incompatibilities. A brief comment explaining why this force is needed (e.g., which dependency conflict it resolves) would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/build.gradle` around lines 129 - 133, Add a brief explanatory comment above the configurations.all { resolutionStrategy { force "androidx.browser:browser:1.8.0" } } block stating why this dependency is being forced (e.g., which transitive dependency or version conflict it resolves, any affected libraries or crash/behavior being prevented, and when it can be revisited), so future maintainers can understand the rationale; reference the configurations.all/resolutionStrategy/force entries when adding the comment.lib/viewmodels/authentication/auth_options_viewmodel.dart (1)
22-61: Remove commented-out code.The old
googleAuthimplementation is fully commented out. Since this is version-controlled, delete it rather than leaving 40 lines of dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/viewmodels/authentication/auth_options_viewmodel.dart` around lines 22 - 61, The file contains a large block of commented-out legacy code for the googleAuth method; remove the entire commented block (the commented-out googleAuth function and its internal references to GoogleSignIn, GOOGLE_OAUTH, ViewState, _userApi, and _storage) so the file no longer contains dead/commented code but keep any real references elsewhere intact; simply delete the commented section rather than uncommenting or modifying it.lib/services/API/users_api.dart (2)
242-254: Remove commented-out code outside the class.These 13 lines of commented-out old implementation sit outside the
HttpUsersApiclass (which closes at the}matching line 44). Dead code should be deleted — it's preserved in version control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/API/users_api.dart` around lines 242 - 254, Delete the 13 lines of commented-out implementation that appear after the closing brace of the HttpUsersApi class (the commented block starting with the try/catch that calls ApiUtils.post and ends with the final closing brace), since this dead code lives outside the class; remove the entire commented block so only the class definition remains.
231-234: Use constants for error messages instead of hardcoded strings.Lines 232 and 234 use inline strings (
"No account found with this email","Unauthorized request") whereas other catch clauses consistently useConstants.*values. This breaks the pattern and makes localization/maintenance harder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/API/users_api.dart` around lines 231 - 234, Replace the hardcoded error strings in the NotFoundException and UnauthorizedException catch blocks with the shared constants used elsewhere for error messages (so the Failure throws use Constants.* instead of inline strings); locate the catch clauses handling NotFoundException and UnauthorizedException in users_api.dart and update the Failure constructors to use the project's constants (e.g., Constants.noAccountFound / Constants.unauthorizedRequest or whatever constant names are used for those messages elsewhere) to match the existing pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/google-services.json`:
- Around line 8-36: Remove the extraneous client object whose
client_info.android_client_info.package_name is "com.company.mobileapp" from the
JSON "client" array (the whole object starting with that client_info block);
ensure you leave the other valid client entry for "org.circuitverse.mobile_app"
intact so the remaining "client" array only contains the correct app
configuration.
- Around line 1-76: The committed google-services.json exposes Firebase API keys
and OAuth client IDs (see keys under "api_key" -> "current_key" and entries
under "oauth_client"/"client_id") and must be removed from the repo; remove
android/app/google-services.json from the commit, add that path to .gitignore,
and replace it with a template file like google-services.json.example containing
the same top-level keys ("project_info", "client", "client_info",
"mobilesdk_app_id", "package_name") but with placeholder values; update
CI/deployment to inject the real google-services.json via secrets or secure
storage instead of committing it.
In `@l10n.yaml`:
- Line 5: The commented-out setting synthetic-package in l10n.yaml causes
Flutter to default to synthetic-package: true and generate localization files
into .dart_tool/flutter_gen/gen_l10n/, breaking existing imports such as
package:mobile_app/gen_l10n/app_localizations.dart across many files; restore
the active setting by re-enabling synthetic-package: false in l10n.yaml so the
generator respects output-dir and keeps generated files under
package:mobile_app/gen_l10n, ensuring imports in files like
forgot_password_view.dart continue to resolve.
In `@lib/config/environment_config.dart`:
- Around line 17-19: The GOOGLE_OAUTH_CLIENT_ID constant is hardcoded and
unused; either make it environment-driven like the other vars or remove/wire it
into GoogleSignIn. If you need the constant, replace its declaration with
String.fromEnvironment("GOOGLE_OAUTH_CLIENT_ID", defaultValue:
"<public-client-id-if-needed>") and then pass that symbol
(GOOGLE_OAUTH_CLIENT_ID) into the GoogleSignIn initializers in
auth_options_viewmodel.dart and cv_landing_viewmodel.dart (instead of relying
only on scopes), otherwise delete the GOOGLE_OAUTH_CLIENT_ID constant to avoid
dead code and remove any unused imports/usages.
In `@lib/services/API/users_api.dart`:
- Around line 205-212: Remove the duplicate `@override` annotation left by the
commented-out old method in users_api.dart: ensure only one `@override` directly
precedes the sendResetPasswordInstructions method declaration (remove the stray
`@override` above the commented block or delete the commented annotation), and
keep the active method signature "Future<bool>
sendResetPasswordInstructions(String email) async" with a single `@override` above
it.
- Around line 217-239: The catch-all is currently re-catching Failure (which
implements Exception) and masking its message; in the try block that awaits
ApiUtils.post and currently handles NotFoundException, UnauthorizedException,
FormatException and a generic on Exception, insert an explicit catch clause "on
Failure { rethrow; }" immediately before the existing "on Exception" handler so
any Failure thrown (e.g. from the jsonResponse checks that throw Failure(...))
is rethrown unchanged; update the catch order in the users_api.dart function
containing the ApiUtils.post call accordingly.
In `@lib/viewmodels/authentication/auth_options_viewmodel.dart`:
- Around line 63-121: The googleAuth function currently has incorrect top-level
indentation and a vague return type; change its signature from Future to
Future<void> and re-indent the entire method body so all statements (including
setStateFor(GOOGLE_OAUTH,...), the GoogleSignIn creation, try/catch blocks, and
calls to _userApi and _storage) are nested inside the googleAuth method block
(matching the style of githubAuth), keeping all references like GOOGLE_OAUTH,
ViewState, _storage, _userApi, and AuthType.GOOGLE in place and preserving the
existing try/on Failure/catch logic and early returns.
In `@windows/flutter/generated_plugin_registrant.cc`:
- Line 14: The generated registrant contains an incorrect include and
registration for quill_native_bridge_windows: replace the bogus include
"<quill_native_bridge_windows/none.h>" and the call to
noneRegisterWithRegistrar(registering the plugin key "none") with the correct
generated header and registration function that follow the package naming (use
the actual header and register function produced for
quill_native_bridge_windows), then regenerate the registrant by running flutter
pub get (or flutter clean && flutter pub get) and ensure pubspec.yaml references
the correct quill_native_bridge_windows version so the produced code uses the
proper header and register function names instead of "none".
---
Outside diff comments:
In `@lib/services/API/users_api.dart`:
- Line 41: Update the abstract method signature for
sendResetPasswordInstructions in the Users API interface so it returns a
non-nullable Future<bool> (change from Future<bool>? to Future<bool>) to match
the concrete implementation (the overriding method at/near line where
sendResetPasswordInstructions is implemented); ensure the declaration in the
UsersApi (or relevant abstract class/interface) uses Future<bool> and update any
imports or null-safety annotations if necessary.
---
Nitpick comments:
In `@android/app/build.gradle`:
- Around line 126-136: Remove the extra blank lines around the
configurations.all block so spacing matches the rest of the file: collapse the
two blank lines after the dependencies closing brace and the two blank lines
between the configurations.all { ... } block and the apply plugin:
'com.google.gms.google-services' line to a single blank line each; locate the
dependencies closing brace, the configurations.all / resolutionStrategy block,
and the apply plugin: 'com.google.gms.google-services' declaration to make this
change.
- Around line 129-133: Add a brief explanatory comment above the
configurations.all { resolutionStrategy { force "androidx.browser:browser:1.8.0"
} } block stating why this dependency is being forced (e.g., which transitive
dependency or version conflict it resolves, any affected libraries or
crash/behavior being prevented, and when it can be revisited), so future
maintainers can understand the rationale; reference the
configurations.all/resolutionStrategy/force entries when adding the comment.
In `@lib/services/API/users_api.dart`:
- Around line 242-254: Delete the 13 lines of commented-out implementation that
appear after the closing brace of the HttpUsersApi class (the commented block
starting with the try/catch that calls ApiUtils.post and ends with the final
closing brace), since this dead code lives outside the class; remove the entire
commented block so only the class definition remains.
- Around line 231-234: Replace the hardcoded error strings in the
NotFoundException and UnauthorizedException catch blocks with the shared
constants used elsewhere for error messages (so the Failure throws use
Constants.* instead of inline strings); locate the catch clauses handling
NotFoundException and UnauthorizedException in users_api.dart and update the
Failure constructors to use the project's constants (e.g.,
Constants.noAccountFound / Constants.unauthorizedRequest or whatever constant
names are used for those messages elsewhere) to match the existing pattern.
In `@lib/viewmodels/authentication/auth_options_viewmodel.dart`:
- Around line 22-61: The file contains a large block of commented-out legacy
code for the googleAuth method; remove the entire commented block (the
commented-out googleAuth function and its internal references to GoogleSignIn,
GOOGLE_OAUTH, ViewState, _userApi, and _storage) so the file no longer contains
dead/commented code but keep any real references elsewhere intact; simply delete
the commented section rather than uncommenting or modifying it.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/viewmodels/authentication/auth_options_viewmodel.dart (1)
63-121: Indentation andFuture<void>return type are still unresolved from the previous review.The method body (lines 64–121) remains indented at the class-member level instead of being nested inside
googleAuth, and the return type is still the rawFutureinstead ofFuture<void>. The local variable_googleSignInat line 64 should also drop the_prefix (reserved for private members in Dart, not locals).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/viewmodels/authentication/auth_options_viewmodel.dart` around lines 63 - 121, The googleAuth method should be corrected to use an explicit Future<void> return type, ensure the entire implementation is correctly indented and enclosed inside the googleAuth(...) method body, and rename the local GoogleSignIn variable from _googleSignIn to googleSignIn to avoid using a leading underscore for a local variable; locate the googleAuth declaration and change its signature to Future<void> googleAuth({bool isSignUp = false}) async { ... }, move all existing try/catch and logic inside that block, and replace references to _googleSignIn with googleSignIn (including signOut() and signIn() calls).
🧹 Nitpick comments (2)
.gitignore (2)
46-46: Minor: Missing leading/for consistency with sibling entries.Lines 43–45 all use a leading
/to anchor paths to the repo root. Whileandroid/app/google-services.json(containing an interior/) is functionally equivalent for this specific path, adding/is consistent with the rest of the block.🔧 Proposed fix
-android/app/google-services.json +/android/app/google-services.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 46, Update the .gitignore entry for android/app/google-services.json to match the sibling entries' style by adding a leading slash; locate the line containing "android/app/google-services.json" and change it to "/android/app/google-services.json" so paths are consistently anchored to the repository root.
46-46: Ensure CI/CD pipelines injectgoogle-services.jsonat build time.With this file now gitignored, any Android build (local or CI) will fail at the Google Services Gradle plugin step unless the file is provided out-of-band. Store the file contents as an encrypted CI secret and write it to
android/app/google-services.jsonas a pre-build step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 46, Since android/app/google-services.json is now gitignored, update CI to inject it at build time: store the google-services.json content as an encrypted CI secret, add a pre-build step in your pipeline to decode/write that secret to android/app/google-services.json (ensuring correct file permissions), and run that step before the Gradle/Google Services plugin runs so the Android build succeeds locally and in CI while keeping the file out of the repo.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignorelib/config/environment_config.dartlib/viewmodels/authentication/auth_options_viewmodel.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/config/environment_config.dart
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 46: The repo contains a committed sensitive file
android/app/google-services.json; add it to .gitignore as done is fine but you
must purge it from history and rotate credentials: run a history-rewrite tool
(e.g., BFG Repo Cleaner or git filter-repo targeting the path
android/app/google-services.json) to remove all historical commits containing
that file, then force-push the rewritten history and notify collaborators to
reclone; after purge, revoke and regenerate all OAuth client IDs and API keys
referenced in that google-services.json via the Google Cloud Console and
Firebase Console to ensure compromised credentials are rotated.
In `@lib/viewmodels/authentication/auth_options_viewmodel.dart`:
- Around line 72-74: The current flow calls _googleSignIn.signOut() before
attempting _googleSignIn.signIn(), which clears the session even if signIn()
returns null (user cancelled); change the flow to avoid signing out
preemptively: remove the pre-signIn _googleSignIn.signOut() call (or only call
signOut() after a successful signIn and explicit user request), instead call
_googleSignIn.signIn() directly and handle a null return (user cancelled)
without clearing the session, or if you truly need to force the account chooser
use signIn(forceCodeForRefreshToken: true) / the appropriate signIn parameter
rather than signing out first; update code paths that assumed signOut() already
ran to preserve silent re-auth and account sessions.
---
Duplicate comments:
In `@lib/viewmodels/authentication/auth_options_viewmodel.dart`:
- Around line 63-121: The googleAuth method should be corrected to use an
explicit Future<void> return type, ensure the entire implementation is correctly
indented and enclosed inside the googleAuth(...) method body, and rename the
local GoogleSignIn variable from _googleSignIn to googleSignIn to avoid using a
leading underscore for a local variable; locate the googleAuth declaration and
change its signature to Future<void> googleAuth({bool isSignUp = false}) async {
... }, move all existing try/catch and logic inside that block, and replace
references to _googleSignIn with googleSignIn (including signOut() and signIn()
calls).
---
Nitpick comments:
In @.gitignore:
- Line 46: Update the .gitignore entry for android/app/google-services.json to
match the sibling entries' style by adding a leading slash; locate the line
containing "android/app/google-services.json" and change it to
"/android/app/google-services.json" so paths are consistently anchored to the
repository root.
- Line 46: Since android/app/google-services.json is now gitignored, update CI
to inject it at build time: store the google-services.json content as an
encrypted CI secret, add a pre-build step in your pipeline to decode/write that
secret to android/app/google-services.json (ensuring correct file permissions),
and run that step before the Gradle/Google Services plugin runs so the Android
build succeeds locally and in CI while keeping the file out of the repo.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #499
Describe the changes you have made in this PR -
This PR fixes the Google OAuth login/signup flow which was failing after Google account selection.
The issue was caused by incorrect Firebase and Google OAuth configuration, including missing SHA-1 fingerprints, improper Android app linking in Firebase, and misconfigured google-services.json.
Changes include:
Proper Firebase Android app configuration
Correct SHA-1 fingerprint setup
Valid google-services.json integration
Fixed Google Sign-In Android configuration
Correct OAuth client setup
Ensured access token is received and processed correctly
Fixed backend authentication flow after OAuth
This resolves the issue where login/signup did not proceed after selecting a Google account.
1.mp4
Screenshots of the changes (If any) -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit
New Features
Bug Fixes
Chores