Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrating qr_code_scanner to mobile_scanner in talawa mobile #2717

Conversation

MukalDadhwal
Copy link

@MukalDadhwal MukalDadhwal commented Jan 24, 2025

What kind of change does this PR introduce?

This PR aims to remove the deprecated qr_code_scanner and replace it with mobile_scanner which is the latest one. Also it fixes the build exception on android 14. Now the project supports the latest android 14 version which is also the highest version that can be supported by the project 3.22.3

Issue Number:

Fixes #2714

Did you add tests for your changes?

  • [ ✅] Tests are written for all changes made in this PR.
  • [ ✅] Test coverage meets or exceeds the current coverage (~90/95%).

Snapshots/Videos:

image

Summary

  • Successful migration from qr_code_scanner to mobile_scanner
  • Successful build for android 14 (api 34)
  • updated test for mobile_scanner
  • fixed faulty tests for splash_screen.dart which was producing a pendingtimer exception

Does this PR introduce a breaking change?

yes

Checklist for Repository Standards

  • [ ✅] Have you reviewed and implemented all applicable coderaabbitai review suggestions?
  • [ ✅] Have you ensured that the PR aligns with the repository’s contribution guidelines?

Other information

The PR also solves a flaky tests for test\widget_tests\pre_auth_screens\splash_screen_test.dart which was failing due a pending timer exception. The error was at the splash screen widget which was waiting for 750 milliseconds on the start screen and then navigating away. The behaviour is hard to test so I have introduced a isTesting flag which prevents the behaviour only while testing.

image

Have you read the contributing guide?

yes

Summary by CodeRabbit

  • New Features

    • Added a DevTools configuration file for Dart and Flutter DevTools
    • Updated QR code scanning functionality using the mobile_scanner package
  • Bug Fixes

    • Improved error handling in QR code scanning
    • Enhanced splash screen testing and initialization
  • Refactor

    • Replaced qr_code_scanner with mobile_scanner across multiple components
    • Updated dependency injection and locator setup
    • Converted some widgets to stateful components
  • Chores

    • Updated project dependencies
    • Cleaned up import statements and test configurations

Copy link
Contributor

coderabbitai bot commented Jan 24, 2025

Walkthrough

This pull request introduces a comprehensive migration from the qr_code_scanner package to the mobile_scanner package across multiple files in the Talawa mobile application. The changes involve updating import statements, replacing QR code scanning widgets and controllers, modifying test cases, and ensuring compatibility with the new scanning library. The migration aims to resolve dependency issues and improve QR code scanning functionality, particularly for newer Android versions.

Changes

File Change Summary
devtools_options.yaml New configuration file for DevTools settings
lib/locator.dart Added setupLocator() function for dependency injection
lib/splash_screen.dart Added isTesting parameter, updated dispose method
lib/view_model/pre_auth_view_models/select_organization_view_model.dart Updated import for mobile scanner
lib/view_model/pre_auth_view_models/set_url_view_model.dart Replaced QR code scanning implementation
lib/views/after_auth_screens/join_org_after_auth/join_organisation_after_auth.dart Converted to StatefulWidget, updated QR scanning
pubspec.yaml Removed qr_code_scanner, added mobile_scanner
test/* Updated test files to use mobile_scanner

Assessment against linked issues

Objective Addressed Explanation
Migrate from qr_code_scanner to mobile_scanner
Resolve Android 14 compatibility issues
Update related tests

Suggested reviewers

  • palisadoes
  • noman2002

Poem

🐰 A Rabbit's QR Quest 🔍

From old scanner to mobile's might,
Hopping through code with pure delight,
QR codes dance, no longer stuck,
With mobile scanner, we've got luck!
Compatibility leaps so high! 🚀


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9963228 and 0f8c436.

⛔ Files ignored due to path filters (1)
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • test/widget_tests/pre_auth_screens/set_url_page_test.dart (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Checking codebase
🔇 Additional comments (2)
test/widget_tests/pre_auth_screens/set_url_page_test.dart (2)

4-4: LGTM! Import statement added correctly.

The mobile_scanner package import has been added cleanly.


525-525: LGTM! Test expectation updated correctly.

The test has been properly updated to check for the new MobileScanner widget instead of the old QRView, aligning with the migration from qr_code_scanner to mobile_scanner.

Let's verify the test coverage for the QR scanner functionality:

✅ Verification successful

Test coverage for QR scanner functionality is comprehensive and complete

All MobileScanner implementations are thoroughly tested with both widget and view model tests, covering success and error scenarios. The migration from QRView to MobileScanner is complete with no remaining implementation dependencies.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if we have comprehensive test coverage for the mobile_scanner implementation
rg -l "MobileScanner" "test/" | grep "_test\.dart$"

Length of output: 273


Script:

#!/bin/bash
# Find all MobileScanner usages in main code
echo "=== MobileScanner usage in lib/ ==="
rg -l "MobileScanner" "lib/"

echo -e "\n=== Checking for any remaining QRView references ==="
rg -l "QRView" .

echo -e "\n=== Showing test implementations ==="
echo "--- set_url_page_test.dart ---"
rg "MobileScanner" "test/widget_tests/pre_auth_screens/set_url_page_test.dart" -B 2 -A 2

echo -e "\n--- set_url_view_model_test.dart ---"
rg "MobileScanner" "test/view_model_tests/pre_auth_view_models/set_url_view_model_test.dart" -B 2 -A 2

echo -e "\n--- join_organisation_after_auth_test.dart ---"
rg "MobileScanner" "test/views/after_auth_screens/join_org_after_auth_test/join_organisation_after_auth_test.dart" -B 2 -A 2

Length of output: 4919


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Our Pull Request Approval Process

Thanks for contributing!

Testing Your Code

Remember, your PRs won't be reviewed until these criteria are met:

  1. We don't merge PRs with poor code quality.
    1. Follow coding best practices such that CodeRabbit.ai approves your PR.
  2. We don't merge PRs with failed tests.
    1. When tests fail, click on the Details link to learn more.
    2. Write sufficient tests for your changes (CodeCov Patch Test). Your testing level must be better than the target threshold of the repository
    3. Tests may fail if you edit sensitive files. Ask to add the ignore-sensitive-files-pr label if the edits are necessary.
  3. We cannot merge PRs with conflicting files. These must be fixed.

Our policies make our code better.

Reviewers

Do not assign reviewers. Our Queue Monitors will review your PR and assign them.
When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

  1. comments in this PR or
  2. our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

  1. arbitrators of future discussions with other contributors about the validity of your changes
  2. point of contact for evaluating the validity of your work
  3. person who verifies matching issues by others that should be closed.
  4. person who gives general guidance in fixing your tests

Other

🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (12)
lib/views/after_auth_screens/join_org_after_auth/join_organisation_after_auth.dart (1)

125-172: Refine error handling and correct typo

  1. There is a minor typo on line 143: "unkonwn" should be "unknown".
  2. Provide actionable help text for permissionDenied and other errors to guide users to phone settings or next steps.
  3. Consider using a partially opaque barrierColor to ensure users realize a modal is open, which can improve UX and accessibility.
lib/view_model/pre_auth_view_models/set_url_view_model.dart (1)

244-281: Improve user feedback and fix spelling

  1. Correct the typo on line 262: "unkonwn" → "unknown".
  2. Customize error messages to guide users on granting camera permissions or handling unsupported devices.
  3. Verify that a fully transparent bottom sheet will not confuse the user. Consider a slight overlay for clarity.
🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes

lib/locator.dart (1)

Line range hint 104-189: Consider splitting service registrations for maintainability.
This function registers numerous singletons and factories all in one place. Over time, this approach can become unwieldy. Consider modularizing by grouping logically related services or view models (e.g., feed-related, user-related, event-related) into distinct setup functions or files to enhance clarity and maintainability.

test/views/after_auth_screens/join_org_after_auth_test/join_organisation_after_auth_test.dart (4)

78-78: Stopping the camera.
Mocking stop() with a future return is fine. Consider asserting that it's called if that matters for coverage.


104-105: Additional mock controller initialization.
Repeated pattern from earlier tests. Consider extracting a setup helper for consistent mock initialization.


117-117: Re-used stop mock.
As with the previous test, optional to verify call usage for thorough coverage.


144-145: MockMobileScannerController usage repeated.
Same pattern as above. Factor out if repeating across multiple tests.

test/view_model_tests/pre_auth_view_models/set_url_view_model_test.dart (2)

210-210: Commented-out code for unregistering Validator.
Revisiting whether this is needed could keep tests consistent and avoid confusion if re-registration is required.


212-212: Commented-out code for re-registering Validator.
Same note as above—ensure the approach to setting up Validator is consistent across tests.

test/widget_tests/pre_auth_screens/splash_screen_test.dart (2)

84-85: Commented-out stream controller.
Revisit if needed for further mocking. Otherwise, removing it fully might reduce clutter.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


90-91: Commented-out stream controller references.
Same note: clean up if not required to avoid confusion.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes

lib/splash_screen.dart (1)

215-216: Avoid plain print statements

Using a print statement to show isTesting status can clutter logs in production. Consider wrapping it in a debug check or removing it entirely if only needed for local troubleshooting.

-   print(widget.isTesting);
+   // debugPrint('isTesting: ${widget.isTesting}');
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a144878 and cfd7f5d.

⛔ Files ignored due to path filters (1)
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • devtools_options.yaml (1 hunks)
  • lib/locator.dart (1 hunks)
  • lib/models/chats/chat_list_tile_data_model.g.dart (2 hunks)
  • lib/services/graphql_config.dart (1 hunks)
  • lib/splash_screen.dart (3 hunks)
  • lib/view_model/pre_auth_view_models/select_organization_view_model.dart (1 hunks)
  • lib/view_model/pre_auth_view_models/set_url_view_model.dart (4 hunks)
  • lib/views/after_auth_screens/join_org_after_auth/join_organisation_after_auth.dart (4 hunks)
  • pubspec.yaml (1 hunks)
  • test/helpers/test_helpers.dart (2 hunks)
  • test/view_model_tests/pre_auth_view_models/set_url_view_model_test.dart (8 hunks)
  • test/views/after_auth_screens/join_org_after_auth_test/join_organisation_after_auth_test.dart (4 hunks)
  • test/widget_tests/pre_auth_screens/set_url_page_test.dart (2 hunks)
  • test/widget_tests/pre_auth_screens/splash_screen_test.dart (9 hunks)
✅ Files skipped from review due to trivial changes (3)
  • lib/view_model/pre_auth_view_models/select_organization_view_model.dart
  • devtools_options.yaml
  • lib/models/chats/chat_list_tile_data_model.g.dart
🧰 Additional context used
🪛 GitHub Actions: PR Workflow
lib/services/graphql_config.dart

[warning] File required formatting changes

pubspec.yaml

[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.

test/widget_tests/pre_auth_screens/splash_screen_test.dart

[warning] File required formatting changes

lib/view_model/pre_auth_view_models/set_url_view_model.dart

[warning] File required formatting changes

🔇 Additional comments (72)
lib/views/after_auth_screens/join_org_after_auth/join_organisation_after_auth.dart (7)

3-3: No meaningful change on this line


4-4: Migrating to MobileScanner

Switching to mobile_scanner is a solid choice for modern Android support and better camera handling.


17-17: Good move to a StatefulWidget

Transforming from a stateless to a stateful widget is appropriate to manage the scanner’s lifecycle properly.


24-27: Proper state initialization

createState() approach looks clean and follows recommended Flutter patterns.


29-31: Ensure thorough disposal of the scanner

While you do call controller.dispose() after a successful scan, consider handling the scenario where the user closes the bottom sheet without scanning. You might need to override the widget’s dispose() method to guarantee that resources are freed anytime the screen unmounts.


36-36: Maintaining existing logic

Invoking model.initialise(widget.orgId) on model readiness preserves existing functionality without issues.


201-228: Prevent null pointer exceptions and guard code parsing

  • Forcing displayValue! could throw an exception if the barcode is malformed or missing. Gracefully handle null cases.
  • Splitting the code on '?' may fail if the QR content doesn’t match the expected format. Use checks to avoid uncaught exceptions.
  • Avoid print statements in production; prefer logging or remove them entirely.
lib/view_model/pre_auth_view_models/set_url_view_model.dart (3)

3-3: Migration to MobileScanner

Upgrading from qr_code_scanner to mobile_scanner aligns with the project’s move to modern QR scanning solutions.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


300-329: Handle unexpected QR formats

  • If scanData.barcodes.first.displayValue is null, the forced unwrap can crash. Use a null-safe check.
  • The code expects a '?' separator. Validate the string before splitting to avoid runtime exceptions.
  • Disposing the controller on every successful scan might prevent subsequent scans; confirm this meets the desired UX flow.
🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


329-329: Address formatting issues

The pipeline indicates that formatting changes are required. Please run dart format . or the project’s chosen formatter to resolve style or indentation problems.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes

lib/services/graphql_config.dart (2)

20-21: Assess initialization of displayImgRoute

Allowing displayImgRoute to be null or ' ' is flexible but ensure subsequent usage checks for these fallback states.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


21-21: Fix formatting

The pipeline flags this file for formatting changes. Please run the required formatting steps to satisfy the pipeline’s checks.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes

test/views/after_auth_screens/join_org_after_auth_test/join_organisation_after_auth_test.dart (10)

7-8: Migration from qr_code_scanner to mobile_scanner.
These imports confirm the switch from qr_code_scanner to mobile_scanner. Looks good!


63-64: Mock controller initialization in test.
Initializing the mock MobileScannerController is straightforward. No issues spotted.


65-76: Mocking barcodes stream.
Providing a stream for barcodes is a good approach for testing. Ensure each test case covers valid and invalid data.


98-100: Explicitly starting the scanner.
Explicitly calling .start() is correct for validating scanning behavior.


106-116: Mock barcodes in second test.
No issues. Keep verifying different scenarios to maximize coverage.


138-140: Repeating scanner start.
Identical approach to ensure scanning is triggered. All good.


146-156: Stream with barcodes.
Logic remains consistent. Confirm the test includes edge cases or invalid data as well.


157-157: Mocking stop to return a future.
No problems. Keep an eye on whether verification is needed.


163-163: Testing exception from stop.
Verifying the exception path is an excellent addition to coverage.


170-172: Calling .start() again.
Reiterates scanning logic for a new scenario. Looks good.

test/view_model_tests/pre_auth_view_models/set_url_view_model_test.dart (21)

6-6: Successfully migrated from qr_code_scanner to mobile_scanner.
The import confirms the new package usage. All good.


236-236: Asserting the QR overlay.
Great check ensuring the expected widget is rendered when scanning.


247-248: Creating a mock controller.
Same pattern as in other tests creating a MobileScannerController. Good for consistency.


249-260: Simulating barcodes with Stream.fromIterable.
Provides a straightforward test scenario. Confirm coverage for invalid or edge-case data.


265-265: Starting scanner.
Verifies that scanning is triggered as intended.


271-271: Testing exception scenario with MobileScannerException.
Ensures coverage for an uninitialized controller. Good negative test.


280-283: Creating and mocking barcodes for exception scenario.
Standard approach to test error handling.


294-298: Mocking stop() to throw MobileScannerException.
Well done to validate error handling.


302-304: Forcing scanner start after exception.
Ensures coverage for the flow post-exception.


316-317: Controller for QrEmbeddedImageException scenario.
Ensures the test suite thoroughly checks different exception classes.


318-329: Providing barcodes.
Same approach as prior tests. Looks consistent.


330-330: Mocking stop() to throw QrEmbeddedImageException.
Valid coverage for embedded image error scenario.


335-337: Starting scanner after embedded image error.
Maintaining coverage for possible subsequent calls.


349-350: Mocking controller for QrUnsupportedVersionException.
Comprehensive test coverage for unique exception types.


351-361: Building barcodes for unsupported version test.
Mirrors the same stream-based approach.


362-363: Mock stop() to throw QrUnsupportedVersionException.
Consistency in negative testing ensures robust coverage.


368-370: Scanner start after unsupported version error.
All good, verifying the fallback logic.


382-383: Mocking controller for generic exception scenario.
Brings all exception categories under test.


384-394: Providing barcodes for the generic exception test.
Same approach as in other tests.


396-396: Throwing a generic Exception on stop.
Ensures no crashes if an unknown error occurs.


400-402: Triggering .start() after generic exception.
Final coverage pass for the entire error flow.

test/widget_tests/pre_auth_screens/splash_screen_test.dart (21)

6-7: Importing dart:async.
Ensuring stream manipulations remain feasible if re-enabled. Currently commented out usage is safe.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


51-51: New isTesting param.
Passing isTesting: true is a neat approach to avoid test flakiness.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


71-71: Removed async from main test function.
Dropping the async generally simplifies test flow if no top-level awaits are needed.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


129-141: Light mode test scenario for the splash screen.
Confirms correct widget rendering, color, and theme usage. No issues.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


144-155: Testing the splash logo in light mode.
Checks the desired size and existence. Looks solid.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


158-174: Testing the app name in light mode.
Verifies text color, font family, and font size. Thorough coverage.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


177-193: Testing 'from' text in light mode.
Ensuring the 'provider text' is visible and styled as intended.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


196-208: Testing the provider name "PALISADOES" in light mode.
Using expectLater is fine, ensuring correct text style.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


210-210: Dark mode tests begin.
All equivalences for the dark theme.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


213-224: Splash screen existence in dark mode.
Same checks as light mode. Implementation looks consistent.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


227-238: App logo renders in dark mode.
Again verifying dimension logic. All good.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


241-257: App name styling in dark mode.
Checks color, font, and size. Mirror of the light mode test.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


260-276: Testing 'from' text in dark mode.
Retains consistent typography.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


279-291: Provider name in dark mode.
Confirms color and font family. Looks fine.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


Line range hint 311-323: Stream-based test for URI.
Commented-out runAsync; removing it simplifies test flow. Confirmed that the stream is tested for updates.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


327-331: Handling PlatformException for initial link.
Proper negative testing ensures robust link handling.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


339-339: Asserting getInitialLink call.
Shows the function is invoked once. All good.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


343-347: Handling FormatException for initial link.
Another negative scenario ensuring stable link processing.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


355-355: Asserting getInitialLink with FormatException.
All correct.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


Line range hint 358-370: URI stream error test.
Valid negative coverage.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes


Line range hint 374-385: Stream subscription cleanup.
Test ensures no memory leaks on disposal. Great practice.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes

test/widget_tests/pre_auth_screens/set_url_page_test.dart (2)

6-7: Successful migration to mobile_scanner

Commenting out the old qr_code_scanner import and adding mobile_scanner aligns with the PR objective to replace the deprecated library. The approach is correct and consistent with the rest of the migration.


526-526: Verifies new MobileScanner usage in tests

This assertion confirms that the widget under test relies on the newly integrated MobileScanner rather than qr_code_scanner. The test reflects the updated functionality, ensuring continued coverage.

test/helpers/test_helpers.dart (3)

2-2: No functional change

This appears to be an empty or whitespace modification. No concerns.


8-8: Consistent import of mobile_scanner

Replacing the old scanner import with mobile_scanner is consistent with the migration strategy. The change is straightforward and poses no issues.


90-92: Mock specification correctly updated

Updating to MockSpec<MobileScannerController> ensures that all mock-based tests reference the new scanning library. This is a vital step to maintain test coverage for the updated features.

pubspec.yaml (1)

60-60: Confirm the new dependency version

Installing mobile_scanner: ^6.0.2 replaces qr_code_scanner. Please verify there's no leftover usage of qr_code_scanner and confirm this version is compatible with the project.

✅ Verification successful

Migration to mobile_scanner is properly implemented

The codebase has been successfully migrated to use mobile_scanner: ^6.0.2. The old qr_code_scanner references only remain in documentation and build artifacts which will be automatically updated. The Material icon Icons.qr_code_scanner usage is valid and unrelated to the library change.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that qr_code_scanner references are removed and the new library is used uniformly.
rg 'qr_code_scanner' -A 3

Length of output: 9730


Script:

#!/bin/bash
# Check the actual usage of mobile_scanner in the codebase
rg "mobile_scanner" -A 3

# Also check for any potential runtime dependencies
grep "mobile_scanner:" pubspec.yaml -A 5

Length of output: 4829

🧰 Tools
🪛 GitHub Actions: PR Workflow

[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.

lib/splash_screen.dart (2)

15-26: Added isTesting for simplified test behavior

Declaring and documenting isTesting in the constructor is a clean approach to bypass time-based or business logic during tests. This reduces flakiness and clarifies test intent.


250-252: Asynchronous disposal handled properly

Converting dispose() into an async method and awaiting _sub.cancel() ensures the subscription is fully cleaned up, reducing the risk of memory leaks.

Comment on lines +58 to +60
/// qrController.
final MobileScannerController controller = MobileScannerController();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Manage controller lifecycle

Dispose of the MobileScannerController when it’s no longer needed to avoid memory leaks. For instance, add a cleanup method to this ViewModel or handle disposal if the user exits before any scan completes.

🧰 Tools
🪛 GitHub Actions: PR Workflow

[warning] File required formatting changes

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
lib/view_model/pre_auth_view_models/set_url_view_model.dart (1)

58-60: ⚠️ Potential issue

Implement controller disposal

The MobileScannerController needs proper cleanup to prevent memory leaks.

Add a dispose method to clean up the controller:

+  @override
+  void dispose() {
+    controller.dispose();
+    super.dispose();
+  }
🧹 Nitpick comments (1)
lib/view_model/pre_auth_view_models/set_url_view_model.dart (1)

262-262: Fix typo in error message

"unkonwn" should be "unknown"

-                          errorMessage = 'An unkonwn error occurred';
+                          errorMessage = 'An unknown error occurred';
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cfd7f5d and 9963228.

📒 Files selected for processing (2)
  • lib/view_model/pre_auth_view_models/set_url_view_model.dart (4 hunks)
  • test/widget_tests/pre_auth_screens/splash_screen_test.dart (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/widget_tests/pre_auth_screens/splash_screen_test.dart
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Checking codebase
🔇 Additional comments (1)
lib/view_model/pre_auth_view_models/set_url_view_model.dart (1)

244-278: LGTM! Comprehensive error handling implementation.

The error handling is well-implemented with clear, user-friendly messages for different error scenarios.

Comment on lines +304 to +328
void _onQRViewCreated(BarcodeCapture scanData) {
if (scanData.raw != null && scanData.barcodes.isNotEmpty) {
final String code = scanData.barcodes.first.displayValue!;
try {
final List<String> data = code.split('?');
url.text = data[0];
final List<String> queries = data[1].split('&');
orgId = queries[0].split('=')[1];
Vibration.vibrate(duration: 100);
controller.stop();
controller.dispose();
final box = Hive.box('url');
box.put(urlKey, url.text);
box.put(imageUrlKey, "${url.text}/talawa/");
graphqlConfig.getOrgUrl();
Navigator.pop(navigationService.navigatorKey.currentContext!);
navigationService.pushScreen('/selectOrg', arguments: orgId);
} on Exception catch (e) {
debugPrint(e.toString());
navigationService.showTalawaErrorSnackBar(
"The Camera is not working",
MessageType.error,
);
}
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and URL validation

The current implementation has two issues:

  1. The error message "The Camera is not working" is misleading as the exception could be due to invalid QR code format.
  2. The URL parsing assumes a specific format without validation.

Consider this improved implementation:

   void _onQRViewCreated(BarcodeCapture scanData) {
     if (scanData.raw != null && scanData.barcodes.isNotEmpty) {
       final String code = scanData.barcodes.first.displayValue!;
       try {
+        // Validate QR code format
+        if (!code.contains('?') || !code.contains('=')) {
+          throw FormatException('Invalid QR code format');
+        }
         final List<String> data = code.split('?');
         url.text = data[0];
         final List<String> queries = data[1].split('&');
         orgId = queries[0].split('=')[1];
         Vibration.vibrate(duration: 100);
         controller.stop();
         controller.dispose();
         final box = Hive.box('url');
         box.put(urlKey, url.text);
         box.put(imageUrlKey, "${url.text}/talawa/");
         graphqlConfig.getOrgUrl();
         Navigator.pop(navigationService.navigatorKey.currentContext!);
         navigationService.pushScreen('/selectOrg', arguments: orgId);
-      } on Exception catch (e) {
+      } on FormatException catch (e) {
+        debugPrint(e.toString());
+        navigationService.showTalawaErrorSnackBar(
+          "Invalid QR code format. Please scan a valid organization QR code.",
+          MessageType.error,
+        );
+      } catch (e) {
         debugPrint(e.toString());
         navigationService.showTalawaErrorSnackBar(
-          "The Camera is not working",
+          "Failed to process QR code. Please try again.",
           MessageType.error,
         );
       }
     }
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void _onQRViewCreated(BarcodeCapture scanData) {
if (scanData.raw != null && scanData.barcodes.isNotEmpty) {
final String code = scanData.barcodes.first.displayValue!;
try {
final List<String> data = code.split('?');
url.text = data[0];
final List<String> queries = data[1].split('&');
orgId = queries[0].split('=')[1];
Vibration.vibrate(duration: 100);
controller.stop();
controller.dispose();
final box = Hive.box('url');
box.put(urlKey, url.text);
box.put(imageUrlKey, "${url.text}/talawa/");
graphqlConfig.getOrgUrl();
Navigator.pop(navigationService.navigatorKey.currentContext!);
navigationService.pushScreen('/selectOrg', arguments: orgId);
} on Exception catch (e) {
debugPrint(e.toString());
navigationService.showTalawaErrorSnackBar(
"The Camera is not working",
MessageType.error,
);
}
});
}
void _onQRViewCreated(BarcodeCapture scanData) {
if (scanData.raw != null && scanData.barcodes.isNotEmpty) {
final String code = scanData.barcodes.first.displayValue!;
try {
// Validate QR code format
if (!code.contains('?') || !code.contains('=')) {
throw FormatException('Invalid QR code format');
}
final List<String> data = code.split('?');
url.text = data[0];
final List<String> queries = data[1].split('&');
orgId = queries[0].split('=')[1];
Vibration.vibrate(duration: 100);
controller.stop();
controller.dispose();
final box = Hive.box('url');
box.put(urlKey, url.text);
box.put(imageUrlKey, "${url.text}/talawa/");
graphqlConfig.getOrgUrl();
Navigator.pop(navigationService.navigatorKey.currentContext!);
navigationService.pushScreen('/selectOrg', arguments: orgId);
} on FormatException catch (e) {
debugPrint(e.toString());
navigationService.showTalawaErrorSnackBar(
"Invalid QR code format. Please scan a valid organization QR code.",
MessageType.error,
);
} catch (e) {
debugPrint(e.toString());
navigationService.showTalawaErrorSnackBar(
"Failed to process QR code. Please try again.",
MessageType.error,
);
}
}

@palisadoes palisadoes added the ignore-sensitive-files-pr The contributor has a legitimate reason for editiing protected files label Jan 25, 2025
@MukalDadhwal
Copy link
Author

@palisadoes please close this one as I have created another PR which is different from develop-postgres

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-sensitive-files-pr The contributor has a legitimate reason for editiing protected files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants