Fix: Remove flutter_facebook_auth dependency #429
Conversation
WalkthroughRemoves Facebook auth/config across app and workflows, updates CI/CD artifact handling, raises iOS minimum to 12, adjusts CocoaPods/Xcode project settings, adds Windows platform build files, tweaks tests and Android build parsing, and refactors contributors API to parse list data without JSON encoding. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
android/app/build.gradle (1)
28-35: Fix dart-defines parsing: Flutter passes Base64-encoded entries, not URL-encoded. Current logic will mis-parse.Flutter injects
project.property('dart-defines')as a comma-separated list of Base64-encodedKEY=VALUEpairs. UsingURLDecoder.decode(...)and a naivesplit('=')can produce wrong keys/values or throw on values containing=. Decode Base64 and split once.Apply this diff:
-if (project.hasProperty('dart-defines')) { - environmentVariables = environmentVariables + project.property('dart-defines') - .split(',') - .collectEntries { entry -> - def pair = URLDecoder.decode(entry).split('=') - [(pair.first()): pair.last()] - } -} +if (project.hasProperty('dart-defines')) { + def dartDefinesMap = project.property('dart-defines') + .split(',') + .collectEntries { entry -> + // Each entry is Base64-encoded "KEY=VALUE" + def decoded = new String(entry.decodeBase64(), 'UTF-8') + int idx = decoded.indexOf('=') + if (idx < 0) return [:] // skip malformed entries + def key = decoded.substring(0, idx) + def value = decoded.substring(idx + 1) + [(key): value] + } + environmentVariables += dartDefinesMap +}If this map is not consumed anywhere (see next comment), prefer removing the whole block.
.github/workflows/ci.yml (2)
201-206: This artifact download will fail: no producer for “android-apk-${{ github.run_number }}” in this workflow.There’s no prior step uploading an artifact with that name. This will 404 and break the job. Remove it or guard it behind a condition, or align the name with the producer job.
Apply this diff to remove the step:
- - name: Download APK Artifact - uses: actions/download-artifact@v5 - with: - name: android-apk-${{ github.run_number }} - path: build/app/outputs/apk/release/If you actually need this, point it to the “Preview APK” artifact name (
preview-apk-pr-...) and restrict to PR runs.
41-45: CI Workflow: Flutter version pin “3.32” is invalid and will break the buildPinning
flutter-version: 3.32on the stable channel causes the action to fail—there is no stable release matching 3.32 (verified against the official Flutter releases JSON). You must update the workflow to use a valid version or omit the pin entirely:• .github/workflows/ci.yml (lines 41–45)
– Remove theflutter-version: 3.32setting to default to the latest stable.
– Or replace it with a concrete, published Flutter release (for example,3.10.6or whichever is current).If you manage Flutter versions via FVM, you can also source the version from
.fvm/fvm_config.json.
🧹 Nitpick comments (3)
.gitignore (1)
21-23: Update the explanatory comment to match the new behaviorThe comment still says the line is “commented out by default,” but Line 24 now actively ignores .vscode. This can confuse contributors.
Apply this diff to clarify intent:
-# The .vscode folder contains launch configuration and tasks you configure in -# VS Code which you may wish to be included in version control, so this line -# is commented out by default. +# The .vscode folder contains local VS Code settings and launch tasks. +# We choose to exclude it from version control to avoid committing per-dev state. +# If shared workspace files are desired later, consider whitelisting specific files via negation rules.lib/enums/auth_type.dart (1)
5-9: Harden string→enum parsing for backward compatibility.If any user device still holds
'facebook'in local storage, strict lookups viaEnumValuesmay return null and crash if not guarded. Consider adding a safe parser (optional):// Top-level helper (optional) AuthType? tryParseAuthType(String raw) { if (raw.isEmpty) return null; return authTypeValues.map[raw.toLowerCase()]; }Then ensure callers handle null (fallback to EMAIL or show an error).
I can wire this into the points where auth type is read from storage and add a one-time migration.
android/app/build.gradle (1)
25-26: environmentVariables is currently unused; consider removing the entire block to reduce build logic surface area.Given the manifest/resValue injections were removed, this parsed map isn’t referenced. Dead code in Gradle can confuse future maintainers and slow troubleshooting. If you don’t plan to use it, delete the initialization and parsing.
Minimal removal:
-// Initialize environmentVariables as an empty map -def environmentVariables = [:] - -if (project.hasProperty('dart-defines')) { - environmentVariables = environmentVariables + project.property('dart-defines') - .split(',') - .collectEntries { entry -> - def pair = URLDecoder.decode(entry).split('=') - [(pair.first()): pair.last()] - } -} +// (environmentVariables removed — no longer used)If you intend to use it later, keep the previous fix (Base64 decode) instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/cd.yml(0 hunks).github/workflows/ci.yml(2 hunks).gitignore(1 hunks)android/app/build.gradle(2 hunks)android/app/src/main/AndroidManifest.xml(0 hunks)ios/Runner/Info.plist(0 hunks)lib/config/environment_config.dart(0 hunks)lib/enums/auth_type.dart(1 hunks)lib/ui/views/authentication/components/auth_options_view.dart(0 hunks)lib/viewmodels/authentication/auth_options_viewmodel.dart(0 hunks)lib/viewmodels/cv_landing_viewmodel.dart(0 hunks)pubspec.yaml(0 hunks)
💤 Files with no reviewable changes (8)
- lib/config/environment_config.dart
- android/app/src/main/AndroidManifest.xml
- lib/ui/views/authentication/components/auth_options_view.dart
- lib/viewmodels/cv_landing_viewmodel.dart
- .github/workflows/cd.yml
- pubspec.yaml
- lib/viewmodels/authentication/auth_options_viewmodel.dart
- ios/Runner/Info.plist
⏰ 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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
🔇 Additional comments (3)
.gitignore (2)
24-24: Ignoring .vscode is fine to keep local editor state out of VCSThis reduces noise from per-developer settings and launch configs. No functional risk relative to the PR scope.
24-24: No tracked.vscode/files—.gitignoreis effectiveVerified that there are no tracked files under
.vscode/, so the entry in.gitignoreis already functioning as intended. No further action needed..github/workflows/ci.yml (1)
196-200: Coveralls finalize inputs look good.Adding
with: github-tokenandparallel-finished: truematches the earlier parallel uploads.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/ui_tests/groups/update_assignment_view_test.dart (1)
132-134: Force-casting to mocks is brittle; add a sanity check (and it relies on the fix above)These casts will fail if a real service slipped through (see setUp issue). After unconditionally re-registering mocks, consider asserting the type before casting to make failures obvious and actionable.
Apply this diff:
- var dialogService = locator<DialogService>() as MockDialogService; - var updateAssignmentViewModel = - locator<UpdateAssignmentViewModel>() as MockUpdateAssignmentViewModel; + final ds = locator<DialogService>(); + final uavm = locator<UpdateAssignmentViewModel>(); + expect(ds, isA<MockDialogService>()); + expect(uavm, isA<MockUpdateAssignmentViewModel>()); + final dialogService = ds as MockDialogService; + final updateAssignmentViewModel = uavm as MockUpdateAssignmentViewModel;Also, pulling from our prior learning for this repo: if DialogService.popDialog() returns a Future, prefer stubbing it with thenAnswer((_) async {}) instead of thenReturn(null) to preserve the async contract. Otherwise, keep it as-is if popDialog is truly synchronous.
Run the following to confirm registration in setupLocator and the signature of popDialog:
#!/bin/bash # Show where setupLocator is defined and what it registers. rg -n -C3 -g '!**/build/**' -P '\bsetupLocator\s*\(' # Look for DialogService and UpdateAssignmentViewModel registrations. rg -n -C2 -g '!**/build/**' -P 'register\w*<\s*DialogService\s*>\(|register\w*<\s*UpdateAssignmentViewModel\s*>\(' # Inspect DialogService.popDialog signature. rg -n -C3 -g '!**/build/**' -P 'class\s+DialogService\b|abstract\s+class\s+DialogService\b' rg -n -C2 -g '!**/build/**' -P '\bpopDialog\s*\('
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/ui_tests/groups/update_assignment_view_test.dart(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T13:11:01.586Z
Learnt from: JatsuAkaYashvant
PR: CircuitVerse/mobile-app#381
File: test/ui_tests/groups/my_groups_view_test.dart:136-148
Timestamp: 2025-06-17T13:11:01.586Z
Learning: When mocking async methods in Flutter/Dart tests, always use `thenAnswer((_) async {})` or `thenAnswer((_) => Future.value())` instead of `thenReturn(null)` to maintain the async contract and prevent runtime errors when production code awaits the result.
Applied to files:
test/ui_tests/groups/update_assignment_view_test.dart
⏰ 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). (3)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (windows-latest)
Pull Request Test Coverage Report for Build 17296171504Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
219-223: Create Release step in CI is unsafe and likely to fail — gate to tags, grant permissions, and/or move to CD.This was flagged previously and remains unresolved. It still:
- Runs on every CI event (push/PR) — risk of unintended releases.
- Lacks job-level permissions (top-level is contents: read).
- Attaches no assets (files not specified), so releases would be empty.
Minimal hardening if you keep it here:
- Gate the step to tags.
- Add job-level permissions: contents: write.
- If you intend to attach the debug APK, point files to the downloaded artifact (note: shipping debug artifacts from CI is atypical — prefer moving release creation to CD and producing signed release builds there).
- name: Create Release uses: softprops/action-gh-release@v2 + if: startsWith(github.ref, 'refs/tags/') with: token: ${{ secrets.GITHUB_TOKEN }} + files: build/app/outputs/flutter-apk/app-debug.apkAdd job-level permissions under the coverage-finish job (outside this hunk):
coverage-finish: name: Finalize Coverage needs: [test] runs-on: ubuntu-latest permissions: contents: writePreferably, delete this step from CI and handle releases in .github/workflows/cd.yml with a proper signed release build.
🧹 Nitpick comments (5)
.github/workflows/ci.yml (5)
200-200: Unnecessary dependency on build-validation for Coveralls finalize.Finalizing Coveralls only needs the parallel test uploads to complete. Depending on build-validation slows coverage reporting and can block it on unrelated build failures.
- coverage-finish: + coverage-finish: name: Finalize Coverage - needs: [test, build-validation] + needs: [test]
213-213: Trailing whitespace breaks YAML lint.Remove the trailing spaces after the download path.
- path: build/app/outputs/flutter-apk/ + path: build/app/outputs/flutter-apk/
272-272: Hardcoded APK path is consistent with the uploaded artifact.The path matches your upload location. Optional: add a lightweight else branch to comment that the artifact wasn’t found to help debuggability when builds fail.
- if (fs.existsSync(path)) { + if (fs.existsSync(path)) { ... - } + } else { + github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body: "⚠️ Preview APK artifact was not found. Check the Build Validation job for failures." + }); + }
175-175: Trailing spaces flagged by yamllint.Clean up the trailing spaces at this comment line.
- # iOS Build Validation + # iOS Build Validation
291-291: Add a newline at end of file.Fixes yamllint new-line-at-end-of-file complaint and keeps diffs cleaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/ci.yml(4 hunks)ios/Podfile(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 175-175: trailing spaces
(trailing-spaces)
[error] 213-213: trailing spaces
(trailing-spaces)
[error] 291-291: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (3)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (windows-latest)
🔇 Additional comments (3)
.github/workflows/ci.yml (3)
167-174: New APK artifact upload step looks good and matches Flutter’s default debug output path.Naming the artifact with the run number helps disambiguate downloads across runs; 7-day retention is reasonable for PR review loops.
205-208: Coveralls finalize inputs are correct.Using github-token and parallel-finished: true matches the parallel upload pattern used in the test job.
260-265: Preview APK now reuses the artifact from Build Validation — good reuse and faster PR feedback.This keeps the APK generation in one place and avoids duplicate builds in PR workflows.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
219-223: Create Release step is still unsafe and likely to fail: missing tag gating, job permissions, and no assets wired in.This mirrors a prior review. Add tag gating, grant job permissions, and attach the downloaded APK; otherwise this will run on every CI event, fail on PRs, or create unintended releases.
Apply:
- name: Create Release uses: softprops/action-gh-release@v2 + if: startsWith(github.ref, 'refs/tags/') with: - token: ${{ secrets.GITHUB_TOKEN }} + token: ${{ secrets.GITHUB_TOKEN }} + files: build/app/outputs/flutter-apk/app-debug.apk + fail_on_unmatched_files: trueAlso add job-level permissions (outside this step, under the coverage-finish job):
# in coverage-finish job permissions: contents: writeOptional: move release creation to a dedicated CD workflow and remove it from CI to avoid cross‑concerns.
🧹 Nitpick comments (7)
.github/workflows/ci.yml (7)
167-174: Harden artifact upload: fail fast if APK is missing and keep artifact naming explicit.Add if-no-files-found to catch path drift and consider a more explicit name if you’ll add more variants later.
Apply:
- name: Upload Android APK if: matrix.build-type == 'android' uses: actions/upload-artifact@v4 with: - name: android-apk + name: android-apk path: build/app/outputs/flutter-apk/app-debug.apk + if-no-files-found: error retention-days: 7
200-200: Unnecessary coupling of coverage finalization to builds increases flakiness.Finalizing Coveralls does not depend on building iOS/Windows/Android; coupling it to build-validation means a red iOS build will block coverage finishing.
Consider limiting needs to just the test job:
- needs: [test, build-validation] + needs: [test]
212-213: Ensure artifact download fails loudly if missing.This prevents silently creating empty releases/comments downstream when the build didn’t produce an APK.
with: name: android-apk path: build/app/outputs/flutter-apk/ + if-no-files-found: error
227-227: Preview job dependency slows feedback; it waits for iOS/Windows unnecessarily.preview-apk needs only the Android artifact but depends on the entire build-validation matrix, delaying PR feedback.
Split Android into a dedicated job (e.g., build-android-debug) that uploads the APK, then set:
- needs: [test, build-validation] + needs: [test, build-android-debug]I can draft this job split if you want.
260-265: Artifact retrieval in Preview job: OK but add safety.The name/path align with the upload step. Add if-no-files-found to fail fast when the upstream build didn’t produce the artifact.
- name: Download Android APK from Build Validation uses: actions/download-artifact@v5 with: name: android-apk path: build/app/outputs/flutter-apk/ + if-no-files-found: error
1-247: Verify pinned Flutter version value or rely on the stable channel.The action is configured with flutter-version: 3.32 in multiple jobs. If 3.32 is not a valid tag, all jobs will fail early.
Two safer options:
- Omit flutter-version to install latest stable for the specified channel.
- Or pin via a version file (e.g., .fvm/fvm_config.json) and set flutter-version-file in subosito/flutter-action.
Example (apply in each job using Flutter):
- with: - channel: "stable" - flutter-version: 3.32 + with: + channel: "stable" + # flutter-version omitted; or use: + # flutter-version-file: .fvm/fvm_config.json
291-291: Add a newline at EOF to satisfy linters and avoid patch churn.- } + } +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 291-291: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (3)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
205-208: Coveralls finalize wiring looks correct.parallel-finished is set and the token is passed only once from Ubuntu, matching the matrix upload behavior earlier.
272-272: Good: PR comment references the downloaded path consistently.Path matches the artifact location; should work as-is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/utils_tests/image_test_utils.dart (1)
33-34: Good call mirroring real HttpClient defaults; consider Mockito null-safe closure syntax.
- Stubbing
followRedirectstotrueandmaxConnectionsPerHostto6avoids NiceMock default values (false/0) that diverge from the real HttpClient defaults. This reduces test flakiness. Nice touch.- Optional: With Mockito’s null-safety, prefer the closure form to future-proof stubbing and quiet analyzers.
Apply this minimal change:
- when(request.followRedirects).thenReturn(true); - when(client.maxConnectionsPerHost).thenReturn(6); + when(() => request.followRedirects).thenReturn(true); + when(() => client.maxConnectionsPerHost).thenReturn(6);Notes:
- If the production code assigns these later (e.g.,
request.followRedirects = false), the getter stub will still returntrue. If you need to assert the assignment behavior instead of forcing a value, use averify(() => request.followRedirects = true)in the test rather than stubbing the getter.- Thanks for using
thenAnswerfor async methods elsewhere in this helper; that aligns with our prior learning to preserve async contracts in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/utils_tests/image_test_utils.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T13:11:01.586Z
Learnt from: JatsuAkaYashvant
PR: CircuitVerse/mobile-app#381
File: test/ui_tests/groups/my_groups_view_test.dart:136-148
Timestamp: 2025-06-17T13:11:01.586Z
Learning: When mocking async methods in Flutter/Dart tests, always use `thenAnswer((_) async {})` or `thenAnswer((_) => Future.value())` instead of `thenReturn(null)` to maintain the async contract and prevent runtime errors when production code awaits the result.
Applied to files:
test/utils_tests/image_test_utils.dart
⏰ 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). (3)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
223-227: Create Release runs on every CI event, lacks write permissions, and attaches no assets—restrict to tags, grant permissions, and attach the APK.This repeats an issue already raised: without gating and
contents: write, the step is prone to unintended runs or failures; currently nofiles:are passed, so releases will be empty.Minimal hardening:
coverage-finish: name: Finalize Coverage needs: [test, build-validation] runs-on: ubuntu-latest + permissions: + contents: write @@ - - name: Create Release - uses: softprops/action-gh-release@v2 - with: - token: ${{ secrets.GITHUB_TOKEN }} + - name: Create Release + if: startsWith(github.ref, 'refs/tags/') && github.event_name == 'push' + uses: softprops/action-gh-release@v2 + with: + token: ${{ secrets.GITHUB_TOKEN }} + files: | + build/app/outputs/flutter-apk/app-debug.apk + fail_on_unmatched_files: false + generate_release_notes: truePreferably, move release creation to a dedicated CD workflow triggered by tags and delete it from CI.
🧹 Nitpick comments (6)
.github/workflows/ci.yml (6)
167-174: Good addition: persist the debug APK artifact from Build Validation.Uploading
build/app/outputs/flutter-apk/app-debug.apkasandroid-apklooks correct and aligns with the later download steps.Optional: if you expect multiple Android variants later, consider including a brief label in the artifact name (e.g.,
android-apk-debug) to avoid collisions when you add more build types.
176-179: Speed up iOS builds by replacingpod repo updatewithpod install --repo-update.Running
pod repo updateupfront is slow and redundant; CocoaPods can refresh specs only when needed during install.Apply this diff:
- - name: Update CocoaPods specs repo (iOS) - if: matrix.build-type == 'ios' - run: pod repo update @@ - cd ios && pod install && cd .. + cd ios && pod install --repo-update && cd ..
216-218: Guard artifact download to relevant events only.This step will fail the job if the artifact is missing (e.g., on non-Android-only runs or future workflow edits). Since the artifact is only needed when you actually create a release, gate this to tag pushes (or push events).
- - name: Download APK Artifact - uses: actions/download-artifact@v5 + - name: Download APK Artifact + if: startsWith(github.ref, 'refs/tags/') && github.event_name == 'push' + uses: actions/download-artifact@v5 with: name: android-apk path: build/app/outputs/flutter-apk/
219-221: Remove unused “Get Current Date” step.
steps.date.outputs.dateis not used elsewhere in the job; keep the workflow lean.- - name: Get Current Date - id: date - run: echo "date=$(date +'%Y-%m-%d')" >> $GITHUB_OUTPUT
156-174: Heads-up: secrets on forked PRs.
GH_OAUTH_CLIENT_ID/SECRETare consumed during Android build. On PRs from forks, GitHub does not pass repository secrets, which can lead to empty--dart-definevalues and brittle builds.Two options:
- Gate Android build to non-fork PRs (like your preview job), or
- Inject these defines only when secrets are set.
Example:
- - name: Build APK Debug (Android) + - name: Build APK Debug (Android) if: matrix.build-type == 'android' env: GH_OAUTH_CLIENT_ID: ${{ secrets.GH_OAUTH_CLIENT_ID }} GH_OAUTH_CLIENT_SECRET: ${{ secrets.GH_OAUTH_CLIENT_SECRET }} run: | - flutter build apk --debug \ - --dart-define=GITHUB_OAUTH_CLIENT_ID=$GH_OAUTH_CLIENT_ID \ - --dart-define=GITHUB_OAUTH_CLIENT_SECRET=$GH_OAUTH_CLIENT_SECRET + EXTRA_DEFINES="" + if [ -n "${GH_OAUTH_CLIENT_ID}" ] && [ -n "${GH_OAUTH_CLIENT_SECRET}" ]; then + EXTRA_DEFINES="--dart-define=GITHUB_OAUTH_CLIENT_ID=$GH_OAUTH_CLIENT_ID \ + --dart-define=GITHUB_OAUTH_CLIENT_SECRET=$GH_OAUTH_CLIENT_SECRET" + fi + flutter build apk --debug $EXTRA_DEFINES
295-295: Add a trailing newline at EOF to satisfy YAML lint.- } + } +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 295-295: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (4)
.github/workflows/ci.yml (4)
204-212: Coveralls finalize wiring looks right.Depending on both
testandbuild-validationprevents premature finalization. Includingparallel-finished: trueandgithub-tokenis the correct configuration for matrix/report aggregation.
231-232: Preview job depends on build-validation: good.Tying preview to
build-validationensures the downloadable APK comes from the validated build rather than rebuilding. This is the right direction and reduces duplicate work.
264-269: Artifact handoff for preview comments is correct; consider fork-safety.Downloading
android-apkintobuild/app/outputs/flutter-apk/matches the upload path. The job is already gated to non-fork PRs—good. If you later allow forks, you’ll need to avoid secrets in the producing job or switch toworkflow_runon the base repo.If you want, I can open a follow-up PR to make the Android build secrets optional for forked PRs (e.g., conditional
--dart-defineinjection only when secrets are present).
276-276: Path reference matches the artifact layout.
build/app/outputs/flutter-apk/app-debug.apkis the correct Flutter debug APK location. The PR comment generator should work as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/utils_tests/image_test_utils.dart (1)
47-49: Type youranymatchers for openUrl to avoid inference pitfalls
HttpClient.openUrlexpects(String, Uri). Untypedanycan trip null-safety inference or analyzer hints in generated mocks. Prefer typed matchers.Apply this diff:
- when( - client.openUrl(any, any), - ).thenAnswer((_) => Future<HttpClientRequest>.value(request)); + when( + client.openUrl(any<String>(), any<Uri>()), + ).thenAnswer((_) async => request)Additionally, for consistency you can make the same typed tweak to
getUrl(outside the changed lines):- ).thenAnswer((_) => Future<HttpClientRequest>.value(request)); + ).thenAnswer((_) async => request);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/utils_tests/image_test_utils.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T13:11:01.586Z
Learnt from: JatsuAkaYashvant
PR: CircuitVerse/mobile-app#381
File: test/ui_tests/groups/my_groups_view_test.dart:136-148
Timestamp: 2025-06-17T13:11:01.586Z
Learning: When mocking async methods in Flutter/Dart tests, always use `thenAnswer((_) async {})` or `thenAnswer((_) => Future.value())` instead of `thenReturn(null)` to maintain the async contract and prevent runtime errors when production code awaits the result.
Applied to files:
test/utils_tests/image_test_utils.dart
⏰ 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). (3)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
236-240: Create Release step still unsafe and will likely fail; gate by tag, add job permissions, and attach the APKThis mirrors a prior review. The job has only
contents: readat top-level and no job-level override; creating a release requirescontents: write. It also runs on every CI event and attaches no assets. Add tag gating, permissions, and pass the APK file.Apply this diff to the step:
- name: Create Release uses: softprops/action-gh-release@v2 + if: startsWith(github.ref, 'refs/tags/') with: token: ${{ secrets.GITHUB_TOKEN }} + files: build/app/outputs/flutter-apk/app-debug.apk + fail_on_unmatched_files: trueAnd add job-level permissions for
coverage-finish(outside this hunk):coverage-finish: name: Finalize Coverage needs: [test, build-validation] runs-on: ubuntu-latest permissions: contents: writeIf releases are not intended in CI, remove this step entirely and move release logic to CD.
🧹 Nitpick comments (6)
.github/workflows/ci.yml (6)
167-174: Upload artifact: add safety net for missing APK and keep artifact smallLooks good. Add
if-no-files-found: errorto fail fast if the APK path changes, and optionally enable zstd compression to reduce artifact size.Apply this diff:
- name: Upload Android APK if: matrix.build-type == 'android' uses: actions/upload-artifact@v4 with: name: android-apk path: build/app/outputs/flutter-apk/app-debug.apk + if-no-files-found: error + compression-level: 6 retention-days: 7
175-193: CocoaPods steps: fail fast and simplify to a single repo update during installCurrent flow hides install errors (
|| true) and runs a separatepod repo updatewhich increases time. Prefer failing early and usingpod install --repo-updatein one step.Apply this diff to streamline:
- # iOS Build Validation - - name: Ensure CocoaPods (iOS) - if: matrix.build-type == 'ios' - run: sudo gem install cocoapods --no-document || true - - - name: Update CocoaPods Specs Repo (iOS) - if: matrix.build-type == 'ios' - run: | - cd ios - pod repo update - cd .. - - - name: Install CocoaPods dependencies (iOS) - if: matrix.build-type == 'ios' - run: | - cd ios - pod install - cd .. + # iOS Build Validation + - name: Ensure CocoaPods (iOS) + if: matrix.build-type == 'ios' + run: sudo gem install cocoapods --no-document + + - name: Install CocoaPods dependencies (iOS) + if: matrix.build-type == 'ios' + run: | + cd ios + pod install --repo-update + cd ..
226-231: Artifact download: add guard for missing artifactConsider failing fast if the artifact name/path is wrong, and ensure the directory exists.
Apply this diff:
- name: Download APK Artifact uses: actions/download-artifact@v5 with: name: android-apk path: build/app/outputs/flutter-apk/ + merge-multiple: false + - name: Verify APK presence + run: test -f build/app/outputs/flutter-apk/app-debug.apk
277-282: Preview: add guardrails to artifact downloadMirror the safety checks used in coverage job to avoid silent “no files” scenarios.
Apply this diff:
- name: Download Android APK from Build Validation uses: actions/download-artifact@v5 with: name: android-apk path: build/app/outputs/flutter-apk/ + merge-multiple: false + - name: Verify APK presence (preview) + run: test -f build/app/outputs/flutter-apk/app-debug.apk
308-308: Add a trailing newlineYAML lints flag missing newline at EOF.
Apply this diff:
- } + } +
44-45: Update Flutter version pin to a valid stable release or omit it to float on stableThe workflow is currently pinned to
3.32, which exists (3.32.0–3.32.7) but is no longer the latest on the stable channel. As of mid-August 2025, Flutter 3.35.0 is the current stable release; consider either removing the explicitflutter-versionto always grab the newest stable or pinning to the full patch version for reproducibility.
(community.chocolatey.org, docs.flutter.dev)Affected locations:
- .github/workflows/ci.yml lines 44–45
- .github/workflows/ci.yml lines 125–126
- .github/workflows/ci.yml lines 263–264
Suggested diff:
flutter-version: 3.32becomes:
channel: "stable" - flutter-version: 3.32 + # Use latest stable; pin to full version when ready + # flutter-version: "3.35.0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 308-308: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (3)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (windows-latest)
🔇 Additional comments (4)
.github/workflows/ci.yml (4)
217-217: Good: gate coverage finish on tests and buildsMaking
coverage-finishdepend on bothtestandbuild-validationis correct so Coveralls finalize waits for all parallel jobs.
222-225: Coveralls finalize inputs look correctUsing
github-tokenandparallel-finished: truealigns with Coveralls’ parallel workflow requirements.
241-245: Preview job now waits for builds — goodDepending on both
testandbuild-validationavoids racing PR comments before artifacts are ready.
289-289: Path matches uploaded artifact
build/app/outputs/flutter-apk/app-debug.apkaligns with the upload/download steps, so the PR comment will find the file.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/utils_tests/image_test_utils.dart (1)
33-43: Remove Mockito setter stubs; they misuse matchers and can break under null-safetyStubbing
voidsetters withanyis unnecessary and brittle.anyis a matcher, not a boolean; evaluatingrequest.<setter> = anyat stub time can trip the analyzer and is invalid under NNBD. Keep the getter stubs; drop the setter stubs. If you need to assert assignments, useverify(...)in tests.Apply this diff:
when(request.followRedirects).thenReturn(true); - when(request.followRedirects = any).thenReturn(null); when(request.persistentConnection).thenReturn(true); - when(request.persistentConnection = any).thenReturn(null); when(request.bufferOutput).thenReturn(true); - when(request.bufferOutput = any).thenReturn(null); when(client.maxConnectionsPerHost).thenReturn(6);
🧹 Nitpick comments (2)
test/utils_tests/image_test_utils.dart (2)
47-49: Use typed matchers for null-safety onopenUrlWith NNBD, untyped
anycan be inferred asNulland upset the analyzer. Prefer typed matchers to make intent explicit.Apply this diff:
- when( - client.openUrl(any, any), - ).thenAnswer((_) => Future<HttpClientRequest>.value(request)); + when( + client.openUrl(any<String>(), any<Uri>()), + ).thenAnswer((_) => Future<HttpClientRequest>.value(request));Additionally (outside the changed hunk), consider typing the existing
getUrlstub similarly:when(client.getUrl(any<Uri>())).thenAnswer((_) => Future<HttpClientRequest>.value(request));
59-66: Also stubheaders.value('content-type')for callers that read a single headerYou’ve covered
forEach, which is great. Some codepaths read a single header directly; stubbingvalueimproves robustness.Apply this diff:
when(response.headers).thenReturn(headers); when(headers.forEach(any)).thenAnswer((invocation) { final callback = invocation.positionalArguments[0] as void Function(String, List<String>); callback('content-type', ['image/png']); }); + when(headers.value(HttpHeaders.contentTypeHeader)).thenReturn('image/png');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/utils_tests/image_test_utils.dart(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T13:11:01.586Z
Learnt from: JatsuAkaYashvant
PR: CircuitVerse/mobile-app#381
File: test/ui_tests/groups/my_groups_view_test.dart:136-148
Timestamp: 2025-06-17T13:11:01.586Z
Learning: When mocking async methods in Flutter/Dart tests, always use `thenAnswer((_) async {})` or `thenAnswer((_) => Future.value())` instead of `thenReturn(null)` to maintain the async contract and prevent runtime errors when production code awaits the result.
Applied to files:
test/utils_tests/image_test_utils.dart
⏰ 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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
lib/services/API/contributors_api.dart (5)
9-10: Make the Future return type non-nullable.The method always completes with a value or throws. Returning
Future<List<...>>?is misleading and complicates callers with unnecessary nullability.Apply:
- Future<List<CircuitVerseContributor>>? fetchContributors(); + Future<List<CircuitVerseContributor>> fetchContributors(); @@ - Future<List<CircuitVerseContributor>>? fetchContributors() async { + Future<List<CircuitVerseContributor>> fetchContributors() async {Also applies to: 16-16
13-13: Preferfinaltyped headers and use Accept for GET (GitHub recommended headers).For GET requests,
Content-Typeis unnecessary. Using GitHub’s recommended Accept/API-Version headers improves compatibility and clarity.Apply:
- var headers = {'Content-Type': 'application/json'}; + final Map<String, String> headers = const { + 'Accept': 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', + };
17-19: Consider fetching more contributors and clarifying URI construction.Default GitHub pagination is 30 items/page. If you want more, request up to 100 per page. Also, consider
finalfor immutability.Minimal change to get more items:
- var _url = - 'https://api.github.com/repos/CircuitVerse/CircuitVerse/contributors'; + final _url = + 'https://api.github.com/repos/CircuitVerse/CircuitVerse/contributors?per_page=100';If
ApiUtils.getaccepts aUri, this is more idiomatic:- var _url = - 'https://api.github.com/repos/CircuitVerse/CircuitVerse/contributors'; + final uri = Uri.https( + 'api.github.com', + '/repos/CircuitVerse/CircuitVerse/contributors', + {'per_page': '100'}, + );(Remember to pass
uritoApiUtils.getif you adopt the second option.)
17-22: Minor nits: preferfinallocals and drop leading underscores for locals.Local variables don’t need privacy markers; using
finalcommunicates intent.Apply (if you keep the String URL variant):
- var _url = - 'https://api.github.com/repos/CircuitVerse/CircuitVerse/contributors?per_page=100'; + final url = + 'https://api.github.com/repos/CircuitVerse/CircuitVerse/contributors?per_page=100'; @@ - var _jsonResponse = await ApiUtils.get(_url, headers: headers); - return circuitVerseContributorsFromJson(json.encode(_jsonResponse)); + final jsonResponse = await ApiUtils.get(url, headers: headers); + return circuitVerseContributorsFromJson(json.encode(jsonResponse));
21-22: API GET return type yields dynamic JSON—re-encoding is requiredThe
ApiUtils.getmethod returns a decoded JSON object (aListorMap), not a raw string, so passing_jsonResponsethroughjson.encodeto produce a JSON string is both intentional and necessary forcircuitVerseContributorsFromJson, which expects aStringinput.Optional cleanup:
- You could refactor
circuitVerseContributorsFromJson(inlib/models/cv_contributors.dart) to accept adynamicJSON object directly and eliminate the extrajson.encodestep here, but it isn’t required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/services/API/contributors_api.dart(1 hunks)
⏰ 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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (2)
lib/services/API/contributors_api.dart (2)
22-22: Switch tojson.encodeis a no-op and fine.Functionally equivalent to
jsonEncode; safe change given the current usage.
20-27: Differentiate failure modes (rate limits, 4xx/5xx) for better UX.Right now all non-parse exceptions become
GENERIC_FAILURE. Consider surfacing GitHub rate-limit errors (403 with header cues), 404/500, and network timeouts distinctly so the UI can respond appropriately.If
ApiUtils.getexposes status codes or custom exceptions, handle them here; otherwise, consider enhancingApiUtils.getto map common HTTP failures to typed errors (e.g.,Failure.rateLimited,Failure.network,Failure.server).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/models/cv_contributors.dart (3)
10-15: Tighten typing and add a defensive cast for safety.Current code force-casts each element and will throw a
TypeErrorwith a less-informative message if any element isn’t aMap<String, dynamic>. Consider asserting the shape in debug and usingMap<String, dynamic>.fromto normalize the map type.Apply this diff:
-List<CircuitVerseContributor> circuitVerseContributorsFromList( - List<dynamic> data, -) => - data - .map((x) => CircuitVerseContributor.fromJson(x as Map<String, dynamic>)) - .toList(); +/// Converts a decoded JSON array (List of Map<String, dynamic>) into models. +List<CircuitVerseContributor> circuitVerseContributorsFromList( + List<dynamic> data, +) { + assert( + data.every((e) => e is Map), + 'Expected a List of Map<String, dynamic> but got: ${data.runtimeType}', + ); + return data + .map((e) => CircuitVerseContributor.fromJson( + Map<String, dynamic>.from(e as Map), + )) + .toList(); +}
10-15: Add a unit test for the new helper.Recommend a small test that feeds a decoded JSON list and verifies:
- correct length,
- a couple of fields mapped (e.g.,
id,htmlUrl),- throws or asserts on bad element types (optional).
I can draft a test using a minimal fixture if you’d like.
83-85: Enum name collides with Dart’s built-inType. Consider renaming.While unchanged in this PR, having
enum Typeis confusing alongside Dart’sType. Renaming toContributorTypeimproves clarity and avoids accidental shadowing in imports.If acceptable, I can propose a follow-up PR that renames the enum, updates the field type, and adjusts
typeValuesaccordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/models/cv_contributors.dart(1 hunks)lib/services/API/contributors_api.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/services/API/contributors_api.dart
⏰ 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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (2)
lib/models/cv_contributors.dart (2)
10-15: Good addition: avoids unnecessary encode/decode hop.Adding
circuitVerseContributorsFromListlets callers pass a decoded List directly, removing a redundantjsonEncodestep and cutting allocations. Nice.
10-15: All contributors API call sites updated; no dart:convert usage remains
- In lib/services/API/contributors_api.dart, the response is now passed through
circuitVerseContributorsFromList(_jsonResponse as List<dynamic>)instead of manual JSON decoding.- There are no
import 'dart:convert'statements orjsonEncodecalls in contributors_api.dart.- No other files reference the old
circuitVerseContributorsFromJsonhelper beyond the model itself.All checks pass—removal of extra encoding and switch to the new helper are complete.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ios/Podfile (1)
37-41: Enforce iOS 12.0 for all Pods to avoid “deployment target” mismatch warnings.Extend the existing post_install to set IPHONEOS_DEPLOYMENT_TARGET uniformly. This prevents CocoaPods from emitting warnings or silently overriding targets when Runner.xcodeproj (or individual Pods) disagree with the Podfile.
Apply this diff:
post_install do |installer| installer.pods_project.targets.each do |target| flutter_additional_ios_build_settings(target) + target.build_configurations.each do |config| + config.build_settings['IPHONEOS_DEPLOYMENT_TARGET'] = '12.0' + end end end
🧹 Nitpick comments (1)
ios/Podfile (1)
31-32: Re-evaluate use_frameworks!/use_modular_headers! post-Facebook removal.If no remaining pods require dynamic frameworks or modular headers, consider:
- Prefer static linkage for faster startup and smaller app size.
- Drop use_modular_headers! unless a specific pod needs it.
Two options:
Option A (preferred if no hard requirement for dynamic frameworks):
- use_frameworks! + use_frameworks! :linkage => :staticOption B (if nothing needs frameworks/modular headers at all):
- use_frameworks! - use_modular_headers! + # use_frameworks! # Uncomment only if a pod explicitly requires dynamic frameworks + # use_modular_headers! # Uncomment only if a pod explicitly requires modular headersPlease verify remaining plugins (e.g., Firebase, GoogleSignIn, etc.) for linkage requirements before changing this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ios/Podfile(1 hunks)
⏰ 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). (3)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
🔇 Additional comments (1)
ios/Podfile (1)
2-2: iOS minimum bumped to 12.0 — LGTM.This aligns with current Flutter iOS baselines and helps drop legacy SDK code paths.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
ios/Runner.xcodeproj/project.pbxproj(3 hunks)
⏰ 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). (3)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (windows-latest)
🔇 Additional comments (4)
ios/Runner.xcodeproj/project.pbxproj (4)
150-151: Good addition: [CP] Copy Pods Resources phase included in target.This is needed to bundle pod resources (incl. privacy bundles). Placement after “[CP] Embed Pods Frameworks” is fine.
223-242: Copy Pods Resources script looks correct.
- Uses the standard Pods-Runner-resources.sh.
- showEnvVarsInLog=0 avoids leaking secrets in CI logs.
285-310: FB SDK frameworks removed from embed list — aligned with PR intent.FBAEMKit/FBSDK* entries are gone; replacement/renamed frameworks for other plugins look consistent.
Also applies to: 311-335
223-242: All clear — no lingering Facebook configuration detectedI ran searches across Dart imports, iOS Info.plist entries, and Podfile(s). The only match was an empty
LSApplicationQueriesSchemesarray (a default key, not containing any Facebook schemes). Noflutter_facebook_auth,FBSDK*,FacebookAppID, URL schemes likefbapi…, or Facebook pods remain. No further cleanup is required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
236-240: Create Release step still unsafe: missing tag gating, write permissions, and assets.This will either fail (no
contents: write) or create unintended releases on PRs/branches, and no files are attached despite downloading the APK. Same concern as earlier feedback.Apply the minimal fix below, or move release creation to a dedicated CD workflow:
coverage-finish: name: Finalize Coverage - needs: [test, build-validation] + needs: [test, build-validation] + permissions: + contents: write runs-on: ubuntu-latest steps: - name: Finalize Coveralls Reporting uses: coverallsapp/github-action@v2 with: github-token: ${{ secrets.GITHUB_TOKEN }} parallel-finished: true - name: Download APK Artifact uses: actions/download-artifact@v5 with: name: android-apk path: build/app/outputs/flutter-apk/ - name: Get Current Date id: date run: echo "date=$(date +'%Y-%m-%d')" >> $GITHUB_OUTPUT - name: Create Release uses: softprops/action-gh-release@v2 + if: startsWith(github.ref, 'refs/tags/') with: - token: ${{ secrets.GITHUB_TOKEN }} + token: ${{ secrets.GITHUB_TOKEN }} + files: | + build/app/outputs/flutter-apk/app-debug.apk + fail_on_unmatched_files: trueNote: If you don’t intend to ship debug artifacts, remove the release step entirely from CI and keep it in CD.
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
167-174: Harden artifact upload: fail fast if APK is missing.Add
if-no-files-found: errorso CI fails when the APK path is wrong or build breaks, instead of silently succeeding with no artifact.- name: Upload Android APK if: matrix.build-type == 'android' uses: actions/upload-artifact@v4 with: name: android-apk path: build/app/outputs/flutter-apk/app-debug.apk + if-no-files-found: error retention-days: 7
175-193: Consolidate CocoaPods steps; avoid redundant repo update and sudo gem install.Runner images already include CocoaPods. Prefer a single
pod install --repo-updateand drop the explicit repo update and gem install.- - name: Ensure CocoaPods (iOS) - if: matrix.build-type == 'ios' - run: sudo gem install cocoapods --no-document || true - - - name: Update CocoaPods Specs Repo (iOS) - if: matrix.build-type == 'ios' - run: | - cd ios - pod repo update - cd .. - - - name: Install CocoaPods dependencies (iOS) + - name: Install CocoaPods dependencies (iOS) if: matrix.build-type == 'ios' run: | cd ios - pod install + pod install --repo-update cd ..
308-308: Add a trailing newline.Fixes the YAML lint “no new line at end of file” warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 308-308: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (3)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (windows-latest)
🔇 Additional comments (4)
.github/workflows/ci.yml (4)
217-217: LGTM: coverage finalize waits for tests and builds.Depending on your tolerance for flaky runs, consider a follow-up to allow finalization on failures (separate job/workflow), but this wiring is fine.
222-225: LGTM: correct Coveralls finalize inputs.
github-tokenandparallel-finished: trueare set properly.
229-230: LGTM: artifact name/path alignment.Matches the upload step and the Flutter default debug APK location.
277-282: LGTM: reusing built artifact for preview reduces redundant work.This keeps the preview job fast and consistent with validation output.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
240-244: Create Release step remains unsafe: gate on tags, add permissions, and attach assets.This mirrors a prior review; please fix or move to CD. Also ensure files are attached.
coverage-finish: name: Finalize Coverage needs: [test, build-validation] runs-on: ubuntu-latest + permissions: + contents: write steps: - name: Finalize Coveralls Reporting uses: coverallsapp/github-action@v2 with: github-token: ${{ secrets.GITHUB_TOKEN }} parallel-finished: true @@ - - name: Create Release - uses: softprops/action-gh-release@v2 - with: - token: ${{ secrets.GITHUB_TOKEN }} + - name: Create Release + if: startsWith(github.ref, 'refs/tags/') + uses: softprops/action-gh-release@v2 + with: + token: ${{ secrets.GITHUB_TOKEN }} + files: | + build/app/outputs/flutter-apk/app-debug.apkOr remove this step from CI entirely and handle releases in cd.yml.
🧹 Nitpick comments (4)
.github/workflows/ci.yml (4)
167-174: Harden artifact upload: fail fast if APK is missing.Add if-no-files-found: error so CI fails when the APK path changes or build breaks.
- name: Upload Android APK if: matrix.build-type == 'android' uses: actions/upload-artifact@v4 with: name: android-apk path: build/app/outputs/flutter-apk/app-debug.apk + if-no-files-found: error retention-days: 7
175-197: Streamline CocoaPods steps to cut time and flakiness.Skip explicit
pod repo updateand fold into install; prefer install idempotency.- - name: Ensure CocoaPods (iOS) - if: matrix.build-type == 'ios' - run: sudo gem install cocoapods --no-document || true - - - name: Update CocoaPods Specs Repo (iOS) - if: matrix.build-type == 'ios' - run: | - cd ios - pod repo update - cd .. - - - name: Install CocoaPods dependencies (iOS) + - name: Install CocoaPods dependencies (iOS) if: matrix.build-type == 'ios' run: | cd ios - pod install + pod install --repo-update cd ..
257-279: Trim unnecessary setup in preview job.Since you only download an artifact and post a comment, Java/Flutter/Gradle setup is not needed and slows CI.
- - name: Setup Java - uses: actions/setup-java@v4 - with: - distribution: "temurin" - java-version: "17" - - - name: Setup Flutter - uses: subosito/flutter-action@v2 - with: - channel: "stable" - flutter-version: 3.32 - - - name: Cache Gradle - uses: actions/cache@v4 - with: - path: | - ~/.gradle/caches - ~/.gradle/wrapper - android/.gradle - key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle*', '**/gradle-wrapper.properties') }} - restore-keys: | - ${{ runner.os }}-gradle- + # No build here; we just download the APK and comment
312-312: Add a trailing newline.Fixes YAML lint error: “no new line character at the end of file.”
- } + } +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(5 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 312-312: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
🔇 Additional comments (8)
.github/workflows/ci.yml (8)
104-107: Good call pinning to macos-14 for iOS builds.This improves reproducibility across Xcode toolchain updates.
204-206: iOS simulator build looks good.No codesign + simulator target is the right combo for CI validation.
221-221: Good dependency wiring.Finalizing coverage after both test and build-validation prevents premature completion.
226-229: Coveralls finalize config LGTM.
parallel-finished: truepaired with the earlier parallel upload is correct.
233-235: Artifact download path/name alignment LGTM.Matches the upload step and downstream preview usage.
248-249: Preview now depends on build-validation: good.This guarantees the APK exists before commenting.
281-286: Artifact download for preview is consistent.Matches the upload name/path; good reuse across jobs.
293-311: PR comment logic LGTM.Checks file existence, computes size, and links to run artifacts without exposing secrets.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
237-241: Create Release still unsafe: ungated, missing job permissions, and no assets attached.Same issue as previously flagged; please gate on tags, grant permissions at job level, and attach the APK.
coverage-finish: name: Finalize Coverage needs: [test, build-validation] runs-on: ubuntu-latest + permissions: + contents: write @@ - - name: Create Release - uses: softprops/action-gh-release@v2 - with: - token: ${{ secrets.GITHUB_TOKEN }} + - name: Create Release + if: startsWith(github.ref, 'refs/tags/') + uses: softprops/action-gh-release@v2 + with: + token: ${{ secrets.GITHUB_TOKEN }} + files: | + build/app/outputs/flutter-apk/app-debug.apkPreferably, move release creation to CD and delete it from CI.
🧹 Nitpick comments (6)
.github/workflows/ci.yml (6)
104-106: Good pin to macos-14; also pin Xcode for reproducible iOS builds.Without a fixed Xcode, runner image updates can break builds.
- name: Setup Flutter uses: subosito/flutter-action@v2 with: channel: "stable" flutter-version: 3.32 + + - name: Select Xcode (iOS) + if: matrix.build-type == 'ios' + uses: maxim-lobanov/setup-xcode@v1 + with: + xcode-version: '15.4'
175-193: Speed up CocoaPods by avoiding a separatepod repo update.Use
pod install --repo-updateonce; it’s faster and simpler.- - name: Update CocoaPods Specs Repo (iOS) - if: matrix.build-type == 'ios' - run: | - cd ios - pod repo update - cd .. - - name: Install CocoaPods dependencies (iOS) if: matrix.build-type == 'ios' run: | cd ios - pod install + pod install --repo-update cd ..
198-207: Confirm Flutter 3.32 exists on the action; consider omitting version as fallback.If 3.32 isn’t available yet,
subosito/flutter-actionwill fail. Either verify availability or rely onchannel: stableonly.
242-246: This job no longer builds; rename and drop toolchain setup to save minutes.- preview-apk: - name: Build Preview APK + preview-apk: + name: Preview APK (download) @@ - needs: [test, build-validation] + needs: [test, build-validation]Additionally remove Setup Java/Flutter/Gradle steps in this job since no build occurs.
290-309: Avoid duplicate PR comments; upsert instead of append.Search existing comments for the header and update if found.
- script: | + script: | const fs = require('fs'); const path = 'build/app/outputs/flutter-apk/app-debug.apk'; @@ - github.rest.issues.createComment({ - issue_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - body: comment - }); + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + per_page: 100 + }); + const existing = comments.find(c => c.body && c.body.startsWith('### 📱 Preview APK Ready!')); + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body: comment + }); + } else { + await github.rest.issues.createComment({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + body: comment + }); + } }
309-309: Add a trailing newline at EOF.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(5 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 309-309: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (4)
.github/workflows/ci.yml (4)
167-174: APK artifact upload looks correct and consistent with later downloads.
214-214: Windows build command/env look correct for PowerShell.
227-233: Download path/name matches the uploaded artifact.
278-283: Artifact download for preview matches the producer job.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (23)
windows/runner/runner.exe.manifest (2)
4-12: Manifest looks correct for PerMonitorV2; consider optional WindowsSettings tweaks and verify embedding.
- Optional: add longPathAware to reduce MAX_PATH issues; confirm Runner.rc embeds this manifest.
Apply if desired:
<application xmlns="urn:schemas-microsoft-com:asm.v3"> <windowsSettings> <dpiAwareness xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">PerMonitorV2</dpiAwareness> + <longPathAware xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">true</longPathAware> </windowsSettings> </application>
15-15: PerMonitorV2 + EnableNonClientDpiScaling duplication.The code calls EnableNonClientDpiScaling (Per-Monitor V1 helper) while the manifest opts into PerMonitorV2. It’s harmless, but you can drop the dynamic call for clarity if you prefer.
windows/.gitignore (1)
9-17: Add common Windows build/artifact ignores.Helps keep repo clean for VS/CMake and ARM64 builds.
Apply:
# Visual Studio build-related files. x64/ x86/ +arm64/ +ARM/ +ARM64/ + +# Visual Studio workspace +.vs/ + +# CMake build dirs and files +build/ +out/ +CMakeFiles/ +CMakeCache.txt +cmake-build-*/ +compile_commands.jsonwindows/runner/win32_window.h (3)
15-26: Prefer signed ints for coordinates/sizes.Negative positions aren’t representable with unsigned. Consider
intto align with Win32 types (LONG).Apply:
- struct Point { - unsigned int x; - unsigned int y; - Point(unsigned int x, unsigned int y) : x(x), y(y) {} + struct Point { + int x; + int y; + Point(int x, int y) : x(x), y(y) {} }; ... - struct Size { - unsigned int width; - unsigned int height; - Size(unsigned int width, unsigned int height) + struct Size { + int width; + int height; + Size(int width, int height) : width(width), height(height) {} };
67-71: Comment mentions CreateAndShow but API only exposes Create/Show.Update the comment to avoid confusion.
Apply:
- // Called when CreateAndShow is called, allowing subclass window-related + // Called when Create is called, allowing subclass window-related
6-8: Header includes could be trimmed.
<functional>isn’t used here. Consider removing to reduce header bloat.-#include <functional> -#include <memory> +#include <memory>windows/runner/win32_window.cpp (5)
221-222: Use hwnd in DefWindowProc to avoid nullptr risk post-WM_DESTROY.Minor robustness/readability improvement.
Apply:
- return DefWindowProc(window_handle_, message, wparam, lparam); + return DefWindowProc(hwnd, message, wparam, lparam);
224-234: Guard Destroy to prevent double OnDestroy when called from WM_DESTROY.Currently WM_DESTROY sets
window_handle_ = nullptrthen callsDestroy(), which callsOnDestroy()again. Add an early-exit guard.Apply:
void Win32Window::Destroy() { - OnDestroy(); - - if (window_handle_) { + if (!window_handle_) { + return; // Already destroyed or in WM_DESTROY path. + } + OnDestroy(); + if (window_handle_) { DestroyWindow(window_handle_); window_handle_ = nullptr; } if (g_active_window_count == 0) { WindowClassRegistrar::GetInstance()->UnregisterWindowClass(); } }
41-55: PerMonitorV2 makes EnableNonClientDpiScaling optional.Safe to keep, but you can remove this dynamic load for a leaner path.
210-219: Update theme also on WM_SETTINGCHANGE (ImmersiveColorSet).Catching only
WM_DWMCOLORIZATIONCOLORCHANGEDmight miss app theme toggles on some builds.Apply:
case WM_DWMCOLORIZATIONCOLORCHANGED: UpdateTheme(hwnd); return 0; + case WM_SETTINGCHANGE: + UpdateTheme(hwnd); + return 0;
90-107: Optional: handle RegisterClass failure for diagnostics.Check the return of
RegisterClassand log/GetLastError on failure.windows/runner/flutter_window.h (1)
16-16: Preferoverrideon the destructor for clarity.Minor style improvement to signal it overrides the base virtual dtor.
Apply this diff:
- virtual ~FlutterWindow(); + ~FlutterWindow() override;windows/runner/main.cpp (2)
8-10: Silence unused parameter warnings inwWinMain.Optional, but avoids MSVC warnings.
Apply this diff just after the function signature:
int APIENTRY wWinMain(_In_ HINSTANCE instance, _In_opt_ HINSTANCE prev, _In_ wchar_t *command_line, _In_ int show_command) { + UNREFERENCED_PARAMETER(prev); + UNREFERENCED_PARAMETER(command_line);
16-18: Optional: checkCoInitializeExresult.Not critical, but logging/handling a failure can aid debugging on some setups.
windows/runner/utils.cpp (3)
48-65: Avoid unsigned underflow and simplify UTF-16→UTF-8 conversion.Current math can underflow on error and is harder to read. Use a two-call pattern without subtracting 1.
- unsigned int target_length = ::WideCharToMultiByte( - CP_UTF8, WC_ERR_INVALID_CHARS, utf16_string, - -1, nullptr, 0, nullptr, nullptr) - -1; // remove the trailing null character - int input_length = (int)wcslen(utf16_string); - std::string utf8_string; - if (target_length == 0 || target_length > utf8_string.max_size()) { - return utf8_string; - } - utf8_string.resize(target_length); - int converted_length = ::WideCharToMultiByte( - CP_UTF8, WC_ERR_INVALID_CHARS, utf16_string, - input_length, utf8_string.data(), target_length, nullptr, nullptr); - if (converted_length == 0) { - return std::string(); - } - return utf8_string; + const int in_len = static_cast<int>(wcslen(utf16_string)); + const int required = ::WideCharToMultiByte( + CP_UTF8, WC_ERR_INVALID_CHARS, utf16_string, in_len, + nullptr, 0, nullptr, nullptr); + if (required <= 0) { + return std::string(); + } + std::string utf8_string(required, '\0'); + const int written = ::WideCharToMultiByte( + CP_UTF8, WC_ERR_INVALID_CHARS, utf16_string, in_len, + utf8_string.data(), required, nullptr, nullptr); + if (written <= 0) { + return std::string(); + } + return utf8_string;
13-20: Make freopen_s checks explicit.Use explicit comparisons for clarity.
- if (freopen_s(&unused, "CONOUT$", "w", stdout)) { + if (freopen_s(&unused, "CONOUT$", "w", stdout) != 0) { _dup2(_fileno(stdout), 1); } - if (freopen_s(&unused, "CONOUT$", "w", stderr)) { + if (freopen_s(&unused, "CONOUT$", "w", stderr) != 0) { _dup2(_fileno(stdout), 2); }
32-37: Micro-opt: reserve argv capacity.Avoids reallocations for typical cases.
- std::vector<std::string> command_line_arguments; + std::vector<std::string> command_line_arguments; + command_line_arguments.reserve(argc > 0 ? argc - 1 : 0);windows/runner/flutter_window.cpp (2)
64-68: Guard against nullptr before reloading fonts.WM_FONTCHANGE could arrive after teardown; add a null check.
- case WM_FONTCHANGE: - flutter_controller_->engine()->ReloadSystemFonts(); + case WM_FONTCHANGE: + if (flutter_controller_) { + flutter_controller_->engine()->ReloadSystemFonts(); + } break;
30-33: Capture only what you need.Prefer [this] over [&] to avoid unintended captures.
- flutter_controller_->engine()->SetNextFrameCallback([&]() { + flutter_controller_->engine()->SetNextFrameCallback([this]() { this->Show(); });windows/runner/CMakeLists.txt (2)
19-21: Match function name casing for consistency.CMake is case-insensitive, but mirroring the declared name reads cleaner.
-apply_standard_settings(${BINARY_NAME}) +APPLY_STANDARD_SETTINGS(${BINARY_NAME})
37-38: Narrow include scope.Use CMAKE_CURRENT_SOURCE_DIR to avoid unintentionally exposing higher-level includes.
-target_include_directories(${BINARY_NAME} PRIVATE "${CMAKE_SOURCE_DIR}") +target_include_directories(${BINARY_NAME} PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}")windows/flutter/CMakeLists.txt (2)
64-68: Guard visibility preset on non-MSVC and coalesce property assignments.
CXX_VISIBILITY_PRESETis ignored by MSVC. Minor cleanup: set properties in one call and guard the visibility preset to non-MSVC.Apply this diff:
-set_target_properties(flutter_wrapper_plugin PROPERTIES - POSITION_INDEPENDENT_CODE ON) -set_target_properties(flutter_wrapper_plugin PROPERTIES - CXX_VISIBILITY_PRESET hidden) +if(NOT MSVC) + set(_vis "CXX_VISIBILITY_PRESET hidden") +endif() +set_target_properties(flutter_wrapper_plugin PROPERTIES + POSITION_INDEPENDENT_CODE ON + ${_vis} +) @@ -apply_standard_settings(flutter_wrapper_app) +apply_standard_settings(flutter_wrapper_app) +if(NOT MSVC) + set_target_properties(flutter_wrapper_app PROPERTIES CXX_VISIBILITY_PRESET hidden) +endif()Also applies to: 79-81
1-3: Scope check: Adding Windows desktop build files in a PR about removing Facebook auth.If intentional, note this broadens the PR scope and could complicate review/CI. Consider splitting or clearly documenting the Windows enablement in the PR description and updating CI to build Windows.
I can help split commits or add minimal Windows CI to prevent breakage. Do you want a follow-up PR template for that?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
windows/runner/resources/app_icon.icois excluded by!**/*.ico
📒 Files selected for processing (18)
.metadata(1 hunks)windows/.gitignore(1 hunks)windows/CMakeLists.txt(1 hunks)windows/flutter/CMakeLists.txt(1 hunks)windows/flutter/generated_plugin_registrant.cc(1 hunks)windows/flutter/generated_plugin_registrant.h(1 hunks)windows/flutter/generated_plugins.cmake(1 hunks)windows/runner/CMakeLists.txt(1 hunks)windows/runner/Runner.rc(1 hunks)windows/runner/flutter_window.cpp(1 hunks)windows/runner/flutter_window.h(1 hunks)windows/runner/main.cpp(1 hunks)windows/runner/resource.h(1 hunks)windows/runner/runner.exe.manifest(1 hunks)windows/runner/utils.cpp(1 hunks)windows/runner/utils.h(1 hunks)windows/runner/win32_window.cpp(1 hunks)windows/runner/win32_window.h(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- windows/runner/resource.h
- windows/runner/Runner.rc
🧰 Additional context used
🧬 Code graph analysis (7)
windows/flutter/generated_plugin_registrant.h (1)
windows/flutter/generated_plugin_registrant.cc (2)
RegisterPlugins(18-35)RegisterPlugins(18-18)
windows/runner/utils.h (1)
windows/runner/utils.cpp (6)
CreateAndAttachConsole(10-22)CreateAndAttachConsole(10-10)Utf8FromUtf16(44-65)Utf8FromUtf16(44-44)GetCommandLineArguments(24-42)GetCommandLineArguments(24-24)
windows/runner/main.cpp (1)
windows/runner/utils.cpp (4)
CreateAndAttachConsole(10-22)CreateAndAttachConsole(10-10)GetCommandLineArguments(24-42)GetCommandLineArguments(24-24)
windows/runner/flutter_window.cpp (3)
windows/runner/flutter_window.h (1)
FlutterWindow(12-31)windows/runner/win32_window.cpp (10)
OnCreate(266-269)OnCreate(266-266)GetClientArea(252-256)GetClientArea(252-252)SetChildContent(241-250)SetChildContent(241-241)OnDestroy(271-273)OnDestroy(271-271)MessageHandler(176-222)MessageHandler(177-180)windows/flutter/generated_plugin_registrant.cc (2)
RegisterPlugins(18-35)RegisterPlugins(18-18)
windows/runner/flutter_window.h (3)
windows/runner/flutter_window.cpp (8)
FlutterWindow(7-8)FlutterWindow(10-10)OnCreate(12-40)OnCreate(12-12)OnDestroy(42-48)OnDestroy(42-42)MessageHandler(50-71)MessageHandler(51-53)windows/runner/win32_window.cpp (8)
Win32Window(114-116)Win32Window(118-121)OnCreate(266-269)OnCreate(266-266)OnDestroy(271-273)OnDestroy(271-271)MessageHandler(176-222)MessageHandler(177-180)windows/runner/main.cpp (1)
window(27-27)
windows/runner/win32_window.h (1)
windows/runner/win32_window.cpp (2)
Win32Window(114-116)Win32Window(118-121)
windows/runner/win32_window.cpp (1)
windows/runner/flutter_window.cpp (6)
OnCreate(12-40)OnCreate(12-12)MessageHandler(50-71)MessageHandler(51-53)OnDestroy(42-48)OnDestroy(42-42)
⏰ 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). (3)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
🔇 Additional comments (17)
.metadata (2)
7-9: Channel value looks non-standard.
channel: "[user-branch]"isn’t a typical Flutter channel (stable/beta/dev). Ensure this file was generated by the Flutter tool on a standard channel to avoid tooling edge cases.
18-20: Windows added; was web intentionally removed from migrate metadata?Confirm that dropping the web platform entry was intended, since the PR goal is removing flutter_facebook_auth, not platforms.
windows/runner/win32_window.cpp (1)
98-104: Check resource IDI_APP_ICON presence.Ensure
resource.hdefinesIDI_APP_ICONand the .rc includes the icon, otherwiseLoadIconreturns null.windows/flutter/generated_plugin_registrant.h (1)
7-15: LGTM — generated registrant header.Matches Flutter’s template; nothing actionable here.
windows/flutter/generated_plugin_registrant.cc (1)
18-35: LGTM — Windows plugins registered; no facebook auth present.The list excludes flutter_facebook_auth, aligning with the PR goal.
windows/runner/utils.h (1)
7-17: LGTM — clear, minimal API surface.Signatures are appropriate; header-only declarations keep Windows deps out of the interface.
windows/runner/flutter_window.h (1)
12-31: LGTM — window wrapper is idiomatic.Constructor/overrides align with the Win32 runner pattern.
windows/runner/main.cpp (1)
8-43: Scope check: confirm Windows runner addition is intended in a “remove facebook_auth” PR.These Windows runner files look unrelated to the dependency removal. If intentional, ignore; otherwise consider splitting to keep PRs focused.
windows/CMakeLists.txt (2)
41-46: Confirm exception policy across plugins.You’re compiling with /EHsc but define _HAS_EXCEPTIONS=0, which disables exceptions in the STL. Verify all Windows plugins and runner code don’t rely on exceptions; otherwise remove that define.
1-109: No Facebook remnants detected – ready to merge.All automated scans confirm that the Facebook SDK and related configuration have been fully removed:
- No occurrences of
flutter_facebook_auth,FacebookAppID,FBSDK, or similar patterns in the source or config files.pubspec.yamlno longer references theflutter_facebook_authdependency.- AndroidManifest.xml (debug/profile/main) contains no Facebook entries.
- Info.plist (AppFrameworkInfo.plist and Runner/Info.plist) contains no Facebook keys or identifiers.
windows/flutter/generated_plugins.cmake (2)
5-14: Plugin list and wiring look consistent with the registrant.Names align with generated_plugin_registrant.cc. No action.
21-26: Sanity-check plugin target names during a Windows build.Generated files usually match, but if you hit link errors, check that each plugin’s CMake defines ${plugin}_plugin as the target.
windows/flutter/CMakeLists.txt (5)
18-41: Flutter interface target and linkage look standard.Matches the upstream Flutter Windows template: header publication, INTERFACE target, and import-lib linkage via "flutter_windows.dll.lib".
21-26: Remove dead variable concern:PROJECT_BUILD_DIRandAOT_LIBRARYare used upstreamI verified that both parent-scope variables are consumed in
windows/CMakeLists.txt:
- Line 91:
set(NATIVE_ASSETS_DIR "${PROJECT_BUILD_DIR}native_assets/windows/")- Lines 102–103:
install(DIRECTORY "${PROJECT_BUILD_DIR}/${FLUTTER_ASSET_DIR_NAME}" DESTINATION "${INSTALL_BUNDLE_DATA_DIR}" COMPONENT Runtime)- Lines 106–108:
install(FILES "${AOT_LIBRARY}" DESTINATION "${INSTALL_BUNDLE_DATA_DIR}" CONFIGURATIONS Profile;Release COMPONENT Runtime)Since both
PROJECT_BUILD_DIRandAOT_LIBRARYare actively used by the parent build/install logic, they should remain as-is.
97-101: Ensure thetool_backend.batpath matches your Flutter SDKI attempted the suggested quick-check script, but
flutterisn’t installed in this environment, so I couldn’t verify it directly. In the Flutter 3.32.x lineage (e.g. 3.32.6), the batch launcher is indeed at<FLUTTER_ROOT>/packages/flutter_tools/bin/tool_backend.bat(fossies.org). However, this location has shifted in the past. Please confirm in your CI/dev environments:
- Run
flutter --versionto see which Flutter release you’re on.- Check for the file:
test -f "$(flutter sdk-path)/packages/flutter_tools/bin/tool_backend.bat" \ && echo "OK: tool_backend.bat present" \ || echo "ERROR: tool_backend.bat missing"- If it’s missing or in a different location, update lines 97–101 in
windows/flutter/CMakeLists.txtto point at the correct path for your SDK.
1-110: No remainingflutter_facebook_authreferences detected
Ran comprehensive regex searches across pubspec, Dart imports, AndroidManifest, resource files, and iOS Info.plist. No occurrences offlutter_facebook_auth,FacebookAuth,fb<APP_ID>,facebook_app_id, or other Facebook-related configuration were found.
58-85: apply_standard_settings is defined before flutter is added; no changes neededThe function APPLY_STANDARD_SETTINGS (is case-insensitive) is defined in windows/CMakeLists.txt lines 13–20, and that file calls add_subdirectory(${FLUTTER_MANAGED_DIR}) immediately afterward, so the calls in windows/flutter/CMakeLists.txt will resolve correctly. No further action required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
243-256: Auto-tagging on every branch push is risky; switch to tag-triggered releases.This creates and pushes a tag on each push to master/main/develop, can collide on reruns (v${{ github.run_number }}) and mixes CI with CD. Prefer running releases only when a tag is pushed.
Apply:
- - name: Checkout Repository (for tagging) - uses: actions/checkout@v4 - with: - fetch-depth: 0 - ref: ${{ github.sha }} - - - name: Create Release Tag - if: github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/main' || github.ref == 'refs/heads/develop') - run: | - git config --global user.email "actions@github.com" - git config --global user.name "GitHub Actions" - git tag -a v${{ github.run_number }} -m "Release v${{ github.run_number }}" - git push origin v${{ github.run_number }}And gate the release step on tag refs (see next comment).
258-264: Gate releases to tags and simplify token usage.Run only on tag pushes; no need to pass
tokenexplicitly unless using a PAT.Apply:
- - name: Create Release - if: github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/main' || github.ref == 'refs/heads/develop') + - name: Create Release + if: startsWith(github.ref, 'refs/tags/') uses: softprops/action-gh-release@v2 with: - token: ${{ secrets.GITHUB_TOKEN }} - tag_name: v${{ github.run_number }} + # softprops uses GITHUB_TOKEN by default + # tag_name comes from the pushed ref files: build/app/outputs/flutter-apk/app-debug.apkOptionally move this to a dedicated CD workflow triggered by
push: tags: ['v*'].
🧹 Nitpick comments (7)
.github/workflows/ci.yml (7)
169-176: Artifact upload: fail fast if APK is missing.Ensure the job fails when the APK isn’t produced.
Apply:
- name: Upload Android APK if: matrix.build-type == 'android' uses: actions/upload-artifact@v4 with: name: android-apk path: build/app/outputs/flutter-apk/app-debug.apk + if-no-files-found: error retention-days: 7
178-199: iOS CocoaPods steps are heavy; gate them and avoid full repo updates.macos-14 images already include CocoaPods. Avoid reinstalling and global
pod repo updateunless necessary.Apply:
- - name: Ensure CocoaPods (iOS) - if: matrix.build-type == 'ios' - run: sudo gem install cocoapods --no-document || true + - name: Ensure CocoaPods (iOS) + if: matrix.build-type == 'ios' + run: | + if ! command -v pod >/dev/null 2>&1; then + sudo gem install cocoapods --no-document + fi - - name: Update CocoaPods Specs Repo (iOS) - if: matrix.build-type == 'ios' - run: | - cd ios - pod repo update - cd .. + # Prefer CDN; only update specs if install complains. + # You can drop this step entirely if not needed. - - name: Install CocoaPods dependencies (iOS) + - name: Install CocoaPods dependencies (iOS) if: matrix.build-type == 'ios' run: | cd ios - pod install + pod install cd ..
216-216: Windows build: verify flag and shell.Ensure
--debugis supported forflutter build windowswith your Flutter version and that the step runs under PowerShell (needed for$env:expansion). Explicitly set the shell to avoid surprises on runner changes.Apply:
- - name: Build Windows Debug + - name: Build Windows Debug if: matrix.build-type == 'windows' env: GH_OAUTH_CLIENT_ID: ${{ secrets.GH_OAUTH_CLIENT_ID }} GH_OAUTH_CLIENT_SECRET: ${{ secrets.GH_OAUTH_CLIENT_SECRET }} - run: flutter build windows --debug --dart-define=GITHUB_OAUTH_CLIENT_ID=$env:GH_OAUTH_CLIENT_ID --dart-define=GITHUB_OAUTH_CLIENT_SECRET=$env:GH_OAUTH_CLIENT_SECRET + shell: pwsh + run: > + flutter build windows --debug + --dart-define=GITHUB_OAUTH_CLIENT_ID=$env:GH_OAUTH_CLIENT_ID + --dart-define=GITHUB_OAUTH_CLIENT_SECRET=$env:GH_OAUTH_CLIENT_SECRET
220-231: Finalize Coverage: good permissions; ensure finish runs even on matrix flake.Add
if: always()so Coveralls finalize runs regardless of upstream job states.Apply:
coverage-finish: name: Finalize Coverage needs: [test, build-validation] runs-on: ubuntu-latest permissions: contents: write steps: - name: Finalize Coveralls Reporting + if: always() uses: coverallsapp/github-action@v2 with: github-token: ${{ secrets.GITHUB_TOKEN }} parallel-finished: true coverage-reporter-version: v0.6.15 fail-on-error: false
236-238: Artifact path update looks consistent; fix trailing whitespace.YAMLlint flags trailing spaces on Line 238.
Apply:
- path: build/app/outputs/flutter-apk/ + path: build/app/outputs/flutter-apk/
239-242: Remove unused “Get Current Date” step.Output
dateisn’t used. Drop the step to simplify the job.Apply:
- - name: Get Current Date - id: date - run: echo "date=$(date +'%Y-%m-%d')" >> $GITHUB_OUTPUT
313-314: Use await when posting the PR comment.Ensure the API call is awaited; otherwise errors can be swallowed.
Apply:
- github.rest.issues.createComment({ + await github.rest.issues.createComment({ issue_number: context.issue.number, owner: context.repo.owner, repo: context.repo.repo, body: comment });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 238-238: trailing spaces
(trailing-spaces)
⏰ 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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (5)
.github/workflows/ci.yml (5)
95-97: Coveralls pinning and soft-fail look good.Version is pinned and failures won’t break CI. Keep this aligned with the finalize step below.
106-106: Pinning to macos-14 is sensible.Stable runner target; reduces surprises from macos-latest drift.
206-206: Confirm simulator build path on Apple Silicon.
flutter build ios --debug --no-codesign --simulatorshould emit an arm64 Simulator build. Verify it succeeds on macos-14 and that Pods resolved foriphonesimulator. If flaking, consider addingxcode-select -p && xcrun xcodebuild -versionbefore the build for diagnostics.
268-268: Preview job dependency: good.Depends on tested and validated builds; ensures APK artifact exists before commenting.
301-306: Reusing the APK artifact in Preview is correct.Names/paths match the upload step.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ios/Runner.xcodeproj/project.pbxproj (1)
209-270: Remove duplicate keyboard-visibility frameworks (avoid duplicate symbols).Both flutter_keyboard_visibility and flutter_keyboard_visibility_temp_fork are embedded. Keep only one (prefer the fork if that’s the intent). Manual pbx edits are fragile—ideally drop the unused plugin from pubspec and re-pod-install; as a minimal fix, remove the legacy framework from inputs/outputs:
- "${BUILT_PRODUCTS_DIR}/flutter_keyboard_visibility/flutter_keyboard_visibility.framework", + /* removed: flutter_keyboard_visibility.framework */ - "${TARGET_BUILD_DIR}/${FRAMEWORKS_FOLDER_PATH}/flutter_keyboard_visibility.framework", + /* removed: flutter_keyboard_visibility.framework */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
ios/Runner.xcodeproj/project.pbxproj(11 hunks)ios/Runner/Info.plist(0 hunks)windows/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (1)
- ios/Runner/Info.plist
🚧 Files skipped from review as they are similar to previous changes (1)
- windows/CMakeLists.txt
⏰ 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). (3)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (5)
ios/Runner.xcodeproj/project.pbxproj (5)
11-11: Pods_Runner.framework rewire looks correct.Single fileRef and one build-phase entry; no dangling references observed.
Also applies to: 49-49, 57-58, 92-92, 127-134
36-41: Pods xcconfig references are properly grouped.Debug/Release/Profile xcconfigs under Pods group look consistent.
Also applies to: 67-70
142-151: Build phases sequence is sane for Flutter+CocoaPods.
- Check Pods Manifest, Flutter build, Thin Binary, Embed Frameworks, Copy Resources ordering is standard.
Also applies to: 271-284, 327-340
313-321: Copy Pods Resources: privacy bundles are correctly declared.google_sign_in_ios and permission_handler_apple privacy bundles wired to outputs; good.
209-270: Sanity check passed: no Facebook artifacts remain — searches for flutter_facebook_auth, FBSDK symbols, Facebook plist keys, Podfile and pubspec entries returned no hits; Info.plist URL schemes are generic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ios/Flutter/AppFrameworkInfo.plist (1)
23-24: Communicate the platform drop.Add a CHANGELOG/README note: “iOS minimum raised to 12.0.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ios/Flutter/AppFrameworkInfo.plist(1 hunks)
⏰ 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). (3)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
🔇 Additional comments (2)
ios/Flutter/AppFrameworkInfo.plist (2)
23-24: Bump to iOS 12.0 looks good.Aligned with the PR’s iOS cleanup and expected Podfile/project settings.
23-24: All iOS deployment targets consistently set to 12.0. Podfile, Xcode project, and AppFrameworkInfo.plist all specify 12.0.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/ui_tests/about/about_view_test.dart (1)
99-107: Apply the same interface binding in the success-path testBind the mock as ContributorsApi and auto-restore; current code registers under the mock type.
- var _mockContributorsApi = MockContributorsApi(); - locator.registerSingleton(_mockContributorsApi); - when(_mockContributorsApi.fetchContributors()).thenAnswer( + final original = locator<ContributorsApi>(); + if (locator.isRegistered<ContributorsApi>()) { + locator.unregister<ContributorsApi>(); + } + final mock = MockContributorsApi(); + locator.registerSingleton<ContributorsApi>(mock); + addTearDown(() { + locator.unregister<ContributorsApi>(); + locator.registerSingleton<ContributorsApi>(original); + }); + when(mock.fetchContributors()).thenAnswer( (_) => Future.value( mockContributors .map((e) => CircuitVerseContributor.fromJson(e)) .toList(), ), );Additional import (if not already added for the previous test):
import 'package:mobile_app/services/API/contributors_api.dart';
🧹 Nitpick comments (1)
test/ui_tests/about/about_view_test.dart (1)
23-39: Stabilize pumpUntilFound: avoid wall-clock dependency and fail test explicitlyUse Stopwatch instead of DateTime.now and fail(...) for clearer test failures.
Future<void> pumpUntilFound( WidgetTester tester, Finder finder, { Duration timeout = const Duration(seconds: 10), }) async { - bool found = false; - final end = DateTime.now().add(timeout); - - while (!found && DateTime.now().isBefore(end)) { - await tester.pump(const Duration(milliseconds: 100)); - found = tester.any(finder); - } - - if (!found) { - throw Exception('Widget not found within timeout'); - } + final sw = Stopwatch()..start(); + while (sw.elapsed < timeout) { + await tester.pump(const Duration(milliseconds: 100)); + if (tester.any(finder)) return; + } + fail('Widget not found within $timeout: $finder'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/ui_tests/about/about_view_test.dart(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T13:11:01.586Z
Learnt from: JatsuAkaYashvant
PR: CircuitVerse/mobile-app#381
File: test/ui_tests/groups/my_groups_view_test.dart:136-148
Timestamp: 2025-06-17T13:11:01.586Z
Learning: When mocking async methods in Flutter/Dart tests, always use `thenAnswer((_) async {})` or `thenAnswer((_) => Future.value())` instead of `thenReturn(null)` to maintain the async contract and prevent runtime errors when production code awaits the result.
Applied to files:
test/ui_tests/about/about_view_test.dart
⏰ 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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
🔇 Additional comments (1)
test/ui_tests/about/about_view_test.dart (1)
87-87: Good use of the wait helper for deterministic UI syncUsing pumpUntilFound avoids brittle fixed delays.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
243-256: Do not auto-create tags on every push; gate releases by tags.Creating tags from CI on each branch push is risky and noisy; this should be driven by annotated tags (or a dedicated CD flow).
- - name: Checkout Repository (for tagging) - uses: actions/checkout@v4 - with: - fetch-depth: 0 - ref: ${{ github.sha }} - - - name: Create Release Tag - if: github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/main' || github.ref == 'refs/heads/develop') - run: | - git config --global user.email "actions@github.com" - git config --global user.name "GitHub Actions" - git tag -a v${{ github.run_number }} -m "Release v${{ github.run_number }}" - git push origin v${{ github.run_number }}Move tagging out of CI, or rely on manually pushed tags.
258-264: Gate release creation on tags and avoid releasing debug APKs by default.Create releases only when a tag is pushed; keep the coverage job unconditional but gate this step on success.
- - name: Create Release - if: github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/main' || github.ref == 'refs/heads/develop') + - name: Create Release + if: success() && startsWith(github.ref, 'refs/tags/') uses: softprops/action-gh-release@v2 with: - token: ${{ secrets.GITHUB_TOKEN }} - tag_name: v${{ github.run_number }} - files: build/app/outputs/flutter-apk/app-debug.apk + token: ${{ secrets.GITHUB_TOKEN }} + generate_release_notes: true + prerelease: true + files: build/app/outputs/flutter-apk/app-debug.apkIf you intend nightly debug drops, move this to a separate workflow (scheduled/manual) instead of CI.
🧹 Nitpick comments (4)
.github/workflows/ci.yml (4)
86-86: Stabilize tests by randomizing order (optional).Consider adding a seed to surface order-dependencies.
- run: flutter test --coverage + run: flutter test --coverage --test-randomize-ordering-seed=random
169-176: Fail fast if APK is missing and harden artifact upload.Ensure the job fails when the expected file isn’t produced.
- name: Upload Android APK if: matrix.build-type == 'android' uses: actions/upload-artifact@v4 with: name: android-apk path: build/app/outputs/flutter-apk/app-debug.apk + if-no-files-found: error retention-days: 7
178-195: Streamline CocoaPods steps; avoid installing pods globally.macos-14 images already ship with CocoaPods. Prefer a single
pod install --repo-updateand dropsudo gem installand the separatepod repo update.- - name: Ensure CocoaPods (iOS) - if: matrix.build-type == 'ios' - run: sudo gem install cocoapods --no-document || true - - - name: Update CocoaPods Specs Repo (iOS) - if: matrix.build-type == 'ios' - run: | - cd ios - pod repo update - cd .. - - - name: Install CocoaPods dependencies (iOS) - if: matrix.build-type == 'ios' - run: | - cd ios - pod install - cd .. + - name: Install CocoaPods dependencies (iOS) + if: matrix.build-type == 'ios' + run: | + cd ios + pod install --repo-update + cd ..
301-306: Harden artifact download in preview job.Fail clearly if the artifact isn’t present.
- name: Download Android APK from Build Validation uses: actions/download-artifact@v5 with: name: android-apk path: build/app/outputs/flutter-apk/ + if-no-files-found: error
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/ci.yml(5 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 238-238: trailing spaces
(trailing-spaces)
⏰ 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). (3)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
- GitHub Check: Test & Analyze (windows-latest)
🔇 Additional comments (7)
.github/workflows/ci.yml (7)
95-96: Good: pinned Coveralls client and non-fatal mode.Pinning
coverage-reporter-versionimproves reproducibility;fail-on-error: falseprevents CI flakes due to Coveralls.
106-106: Good: macOS runner pinned to macos-14.This avoids surprise Xcode image rollouts from
macos-latest.
196-199: OK: listing simulators aids diagnostics.This helps debug Xcode/Sim availability on runners.
206-206: Looks good for simulator build validation.
--debug --no-codesign --simulatoris appropriate for CI sanity checks.
227-232: Good: pinned Coveralls client and non-fatal finalize.Matches the parallel upload; correct inputs.
268-269: Good: preview depends on build artifacts rather than rebuilding.This shortens PR latency and de-duplicates work.
313-314: OK: path aligns with uploaded artifact.
app-debug.apkmatches the upload step and downstream release/preview references.
3 participants
Fixes
Describe the changes you have made in this PR -
flutter_facebook_authpackage from the projectflutter_facebook_authin code and configurationScreenshots of the changes (If any) -
N/A
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.
Summary by CodeRabbit