Skip to content

Added the functionality for checking app update#521

Merged
M4dhav merged 22 commits intoAOSSIE-Org:devfrom
Mayank4352:dev
Sep 14, 2025
Merged

Added the functionality for checking app update#521
M4dhav merged 22 commits intoAOSSIE-Org:devfrom
Mayank4352:dev

Conversation

@Mayank4352
Copy link
Contributor

@Mayank4352 Mayank4352 commented Sep 1, 2025

Description

Added the functionality for checking app update on play store on launch and about page

Fixes #516

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested locally for the functionality of updated checking on launch

Feature.Test.mp4

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

Maintainer Checklist

Summary by CodeRabbit

  • New Features

    • In-app update check on About screen with tappable status block, loading indicator, and option to open app store.
    • Automatic update check after login when controller is present.
    • Added English and Hindi UI strings for update prompts, statuses, and outcomes.
    • New enums to represent update check and action results.
  • Chores

    • Added updater dependency and updated .gitignore to ignore pubspec.lock.
  • Tests

    • Unit tests covering update-check flows and store-launch outcomes.

@coderabbitai
Copy link

coderabbitai bot commented Sep 1, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Implements a real app update check using the Upgrader package: adds controller-driven update flow and enums, registers the controller at startup, triggers guarded checks on login, exposes a tappable About-screen check with reactive UI and localized messages, adds tests, adds dependency and ignores pubspec.lock.

Changes

Cohort / File(s) Summary
Update logic core (controller & enums)
lib/controllers/about_app_screen_controller.dart, lib/utils/enums/action_enum.dart, lib/utils/enums/update_enums.dart
Replaces placeholder update logic with an Upgrader-based flow. Adds Upgrader injection, isCheckingForUpdate, checkForUpdate(...) returning UpdateCheckResult, launchStoreForUpdate() returning UpdateActionResult, and new enums. Removes obsolete fake check.
UI: About screen update entrypoint
lib/views/screens/about_app_screen.dart
Adds a tappable update-check row with loading state and reactive UI (Obx), triggers controller check, and shows localized snackbars based on UpdateCheckResult. Replaces controller creation with Get.find.
App wiring
lib/main.dart, lib/controllers/auth_state_controller.dart
Registers AboutAppScreenController at startup and invokes a guarded checkForUpdate() in isUserLoggedIn() if the controller is registered.
Localization (keys & implementations)
lib/l10n/app_en.arb, lib/l10n/app_hi.arb, lib/l10n/app_localizations.dart, lib/l10n/app_localizations_en.dart, lib/l10n/app_localizations_hi.dart
Adds ~21 update-related localization keys and corresponding getters/overrides in English and Hindi for update UI and messages.
Dependencies & repo config
pubspec.yaml, .gitignore
Adds dependency upgrader: ^11.5.0 and adds pubspec.lock to .gitignore.
Tests (controller + mocks)
test/controllers/about_app_screen_controller_test.dart, test/controllers/about_app_screen_controller_test.mocks.dart
Adds unit tests for AboutAppScreenController flows and Mockito-generated mocks for Upgrader.
Project metadata
project_structure.txt
Updates structure overview to include added enums and tests.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant AAS as AboutAppScreen (UI)
  participant C as AboutAppScreenController
  participant Up as Upgrader
  participant Store as App Store

  U->>AAS: Tap "Check for updates"
  AAS->>C: checkForUpdate(isManualCheck: true)
  activate C
  C->>Up: initialize()
  Up-->>C: init result
  C->>Up: shouldDisplayUpgrade()
  Up-->>C: true/false
  alt Update available
    C-->>AAS: UpdateCheckResult.updateAvailable
    opt launchUpdateIfAvailable or showDialog
      C->>Up: show dialog / launch store
      Up->>Store: Open listing
      Store-->>U: Store page
    end
  else No update
    C-->>AAS: UpdateCheckResult.noUpdateAvailable
  else Platform unsupported
    C-->>AAS: UpdateCheckResult.platformNotSupported
  else Check failed
    C-->>AAS: UpdateCheckResult.checkFailed
  end
  deactivate C
  AAS->>U: Localized snackbar/message
Loading
sequenceDiagram
  autonumber
  participant App as App Startup
  participant ASC as AuthStateController
  participant C as AboutAppScreenController
  participant Up as Upgrader

  App->>ASC: isUserLoggedIn()
  ASC->>ASC: setUserProfileData()
  alt About controller registered
    ASC->>C: checkForUpdate()
    C->>Up: initialize() + shouldDisplayUpgrade()
    Up-->>C: result
    C-->>ASC: UpdateCheckResult
  else Not registered
    ASC-->>App: continue
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

I twitch my whiskers, sniff the update breeze,
A hop, a click—dialog pops with ease.
If new bits sparkle, off to the store I go,
If all is fresh, I wiggle and glow.
Carrots and code, both kept up-to-date! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning Most changes are directly related to the update‑check feature (Upgrader dependency, controller logic, About screen UI, enums, l10n, and tests), however the addition of "pubspec.lock" to .gitignore is unrelated to implementing update checks and appears out of scope for this feature. That .gitignore change alters repository lockfile handling and should be treated separately or justified. Revert the .gitignore entry that ignores pubspec.lock and keep lockfile handling consistent with project policy; if maintainers do want to change lockfile tracking, move that change into a separate PR with an explicit rationale and CI/maintainer sign‑off.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Added the functionality for checking app update" succinctly describes the primary change introduced by the PR: adding app update checking functionality (Upgrader integration, controller logic, and About‑screen trigger). It is concise and directly related to the changeset. The phrasing could be slightly improved for grammar ("checking for app updates" or "Add app update check") but the current title is acceptable and informative.
Linked Issues Check ✅ Passed The changes implement an Upgrader-based update check, add a manual About‑screen trigger, replace the previous dummy logic in the AboutAppScreenController, include localized strings and unit tests, and register/invoke the controller so update checks run in the signed-in flow; these items align with the objectives of issue #516. The code adds launchStoreForUpdate, result enums, and test coverage that verify the new behaviors. Based on the provided diffs, the implementation satisfies the linked issue's coding requirements for Play Store checks and a manual About‑screen action.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2025

🎉 Welcome @Mayank4352!
Thank you for your pull request! Our team will review it soon. 🔍

  • Please ensure your PR follows the contribution guidelines. ✅
  • All automated tests should pass before merging. 🔄
  • If this PR fixes an issue, link it in the description. 🔗

We appreciate your contribution! 🚀

@M4dhav M4dhav added enhancement New feature or request good first issue Good for newcomers labels Sep 1, 2025
@M4dhav M4dhav linked an issue Sep 1, 2025 that may be closed by this pull request
Copy link
Contributor

@M4dhav M4dhav left a comment

Choose a reason for hiding this comment

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

Please implement the suggested changes and also share a video of the functionality in progress

@Mayank4352 Mayank4352 requested a review from M4dhav September 6, 2025 12:59
Copy link
Contributor

@M4dhav M4dhav left a comment

Choose a reason for hiding this comment

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

Please complete these changes and solve merge conflicts

@Mayank4352 Mayank4352 requested a review from M4dhav September 7, 2025 12:23
Copy link
Contributor

@M4dhav M4dhav left a comment

Choose a reason for hiding this comment

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

Please make the requested changes

Comment on lines +36 to +38
Get.snackbar("Error", "Could not load package info");
log('Could not load package info: $e');
Copy link
Contributor

Choose a reason for hiding this comment

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

Showing errors via Snackbar is better than logging it, let us revert to that

@Mayank4352 Mayank4352 requested a review from M4dhav September 7, 2025 12:52
Copy link
Contributor

@M4dhav M4dhav left a comment

Choose a reason for hiding this comment

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

Implement changes

}

Future<void> _handleUpdateCheck() async {
aboutAppScreenController.isCheckingForUpdate.value = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

The internal functions also set the value for this variable already so it is redundant and unpredictable to set them at 2 place, please remove from one place

Copy link
Contributor

@M4dhav M4dhav left a comment

Choose a reason for hiding this comment

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

Please implement requested changes and complete the pending changes from previous reviews, I think everything else looks fine so this should be the last cycle of changes

Comment on lines +25 to +26
debugDisplayAlways: true,
debugDisplayOnce: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

set debug options to false so that update doesn't always show

@Mayank4352 Mayank4352 requested a review from M4dhav September 12, 2025 06:22
@M4dhav
Copy link
Contributor

M4dhav commented Sep 14, 2025

Please make changes related to the final 3 comments

@M4dhav
Copy link
Contributor

M4dhav commented Sep 14, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Sep 14, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@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: 5

♻️ Duplicate comments (7)
lib/utils/enums/update_enums.dart (1)

1-6: Add concise docs per value.

Clarifies control-flow and logging.

-enum UpdateCheckResult {
-  updateAvailable,
-  noUpdateAvailable,
-  checkFailed,
-  platformNotSupported,
-}
+/// Result of checking for an app update.
+enum UpdateCheckResult {
+  /// A newer version is available in the store.
+  updateAvailable,
+  /// Already on the latest version.
+  noUpdateAvailable,
+  /// Network/HTTP or parsing failed.
+  checkFailed,
+  /// Platform does not support this check (guard UI accordingly).
+  platformNotSupported,
+}
lib/main.dart (1)

57-58: Register controller as permanent and before first use.

Good fix placing the registration before it’s accessed elsewhere. Make it permanent so the singleton survives route changes.

Apply this diff:

-  Get.put(AboutAppScreenController());
+  Get.put(AboutAppScreenController(), permanent: true);
lib/views/screens/about_app_screen.dart (2)

181-227: Use the app’s standard loader; drop external loading_indicator dep.

Per earlier guidance, prefer the app-wide loader (see Explore). A minimal swap to CircularProgressIndicator also works here.

Apply this diff:

-                        aboutAppScreenController.isCheckingForUpdate.value
-                            ? SizedBox(
-                                width: UiSizes.width_25,
-                                height: UiSizes.height_26,
-                                child: LoadingIndicator(
-                                  indicatorType: Indicator.ballPulse,
-                                  colors: [
-                                    Theme.of(context).colorScheme.primary,
-                                  ],
-                                  strokeWidth: 2,
-                                ),
-                              )
+                        aboutAppScreenController.isCheckingForUpdate.value
+                            ? SizedBox(
+                                width: UiSizes.width_25,
+                                height: UiSizes.height_26,
+                                child: CircularProgressIndicator(
+                                  strokeWidth: 2,
+                                  valueColor: AlwaysStoppedAnimation(
+                                    Theme.of(context).colorScheme.primary,
+                                  ),
+                                ),
+                              )
                             : Icon(

And remove the unused import:

-import 'package:loading_indicator/loading_indicator.dart';

260-294: Don’t toggle isCheckingForUpdate in the view; controller already owns it.

Setting it in both places risks races and incorrect UI states. Also prefer using the local BuildContext over Get.context!.

Apply this diff:

-  Future<void> _handleUpdateCheck() async {
-    aboutAppScreenController.isCheckingForUpdate.value = true;
+  Future<void> _handleUpdateCheck(BuildContext context) async {
     try {
-      final result = await aboutAppScreenController.checkForUpdate(
-        isManualCheck: true,
-      );
+      final result = await aboutAppScreenController.checkForUpdate(
+        isManualCheck: true,
+        showDialog: true, // lets Upgrader present its platform dialog
+      );
       switch (result) {
         case UpdateCheckResult.noUpdateAvailable:
-          customSnackbar(
-            AppLocalizations.of(Get.context!)!.upToDateTitle,
-            AppLocalizations.of(Get.context!)!.upToDateMessage,
+          customSnackbar(
+            AppLocalizations.of(context)!.upToDateTitle,
+            AppLocalizations.of(context)!.upToDateMessage,
             LogType.success,
           );
           break;
         case UpdateCheckResult.updateAvailable:
-          break;
+          // Optional: provide an immediate CTA if dialog suppressed by policy
+          // final action = await aboutAppScreenController.launchStoreForUpdate();
+          break;
         case UpdateCheckResult.platformNotSupported:
-          customSnackbar(
-            AppLocalizations.of(Get.context!)!.platformNotSupported,
-            AppLocalizations.of(Get.context!)!.platformNotSupportedMessage,
+          customSnackbar(
+            AppLocalizations.of(context)!.platformNotSupported,
+            AppLocalizations.of(context)!.platformNotSupportedMessage,
             LogType.warning,
           );
           break;
         case UpdateCheckResult.checkFailed:
-          customSnackbar(
-            AppLocalizations.of(Get.context!)!.updateCheckFailed,
-            AppLocalizations.of(Get.context!)!.updateCheckFailedMessage,
+          customSnackbar(
+            AppLocalizations.of(context)!.updateCheckFailed,
+            AppLocalizations.of(context)!.updateCheckFailedMessage,
             LogType.error,
           );
           break;
       }
-    } finally {
-      aboutAppScreenController.isCheckingForUpdate.value = false;
-    }
+    } finally {}
   }

And wire the tap to pass context:

-                  onTap: () => _handleUpdateCheck(),
+                  onTap: () => _handleUpdateCheck(context),
lib/controllers/about_app_screen_controller.dart (3)

43-43: Surface errors via Snackbar (per previous feedback).

Prior review requested using Snackbars instead of logs for user‑facing errors.

Apply this diff:

-      log('Could not load package info: $e');
+      if (Get.context != null) {
+        Get.snackbar(
+          AppLocalizations.of(Get.context!)?.error ?? 'Error',
+          AppLocalizations.of(Get.context!)?.errorLoadPackageInfo ??
+              'Could not load package info',
+          snackPosition: SnackPosition.BOTTOM,
+        );
+      } else {
+        log('Could not load package info: $e');
+      }

Add import if missing:

import 'package:resonate/l10n/app_localizations.dart';

88-91: Use Snackbar for update‑check failures too.

Match UX guidance from earlier review.

Example (adjust as needed with i18n):

-      log('Update check error: $e');
-      return UpdateCheckResult.checkFailed;
+      if (Get.context != null) {
+        Get.snackbar(
+          AppLocalizations.of(Get.context!)?.updateCheckFailed ?? 'Update check failed',
+          AppLocalizations.of(Get.context!)?.updateCheckFailedMessage ??
+              'Could not check for updates. Please try again later.',
+          snackPosition: SnackPosition.BOTTOM,
+        );
+      } else {
+        log('Update check error: $e');
+      }
+      return UpdateCheckResult.checkFailed;

6-9: Enums location: please move to models/enums as requested earlier.

Imports still point to utils/enums/....

If moving now is risky, add a TODO and track a follow‑up issue; otherwise relocate the files and update imports across the app.

🧹 Nitpick comments (11)
lib/utils/enums/action_enum.dart (1)

1-1: Document enum semantics (failed vs error).

Add brief docs so callers consistently interpret outcomes.

+/// Outcome of launching the store for an update.
+/// - success: store opened.
+/// - userDenied: user dismissed.
+/// - failed: could not launch intent/URL.
+/// - error: unexpected exception.
 enum UpdateActionResult { success, userDenied, failed, error }
lib/l10n/app_en.arb (1)

591-611: Minor copy tweak: “Check for Updates”.

Small UX polish.

-    "checkForUpdates": "Check Updates",
+    "checkForUpdates": "Check for Updates",

Also consider deprecating older keys (updateAvailable, newVersionAvailable) if the new update*Title/Message pair replaces them, to reduce translator load.

lib/l10n/app_hi.arb (1)

592-612: Minor Hindi wording improvements.

More natural/standard terms.

-    "checkForUpdates": "अपडेट चेक करें",
+    "checkForUpdates": "अपडेट जाँचें",
-    "updateError": "अपडेट एरर",
+    "updateError": "अपडेट त्रुटि",
-    "platformNotSupported": "प्लेटफॉर्म सपोर्टेड नहीं",
+    "platformNotSupported": "प्लेटफ़ॉर्म समर्थित नहीं है",
lib/controllers/auth_state_controller.dart (1)

237-239: Don’t surface dialogs during login-time auto-check; pass showDialog: false.

The silent, launch-time update check should not interrupt navigation. Call the controller with showDialog: false to avoid presenting UI during login.

Apply this diff:

-      if (Get.isRegistered<AboutAppScreenController>()) {
-        Get.find<AboutAppScreenController>().checkForUpdate();
-      }
+      if (Get.isRegistered<AboutAppScreenController>()) {
+        Get.find<AboutAppScreenController>().checkForUpdate(showDialog: false);
+      }
test/controllers/about_app_screen_controller_test.dart (1)

77-85: Also covers error path—nice. Consider asserting isCheckingForUpdate transitions.

Add a test that triggers a delayed initialize, call checkForUpdate without awaiting, assert isCheckingForUpdate flips to true, then back to false.

Apply this addition:

+    test('isCheckingForUpdate toggles during async check', () async {
+      when(mockUpgrader.initialize()).thenAnswer((_) async {
+        await Future<void>.delayed(const Duration(milliseconds: 10));
+        return true;
+      });
+      when(mockUpgrader.shouldDisplayUpgrade()).thenReturn(false);
+
+      // Start without awaiting
+      // ignore: unawaited_futures
+      controller.checkForUpdate(clearSettings: false);
+      expect(controller.isCheckingForUpdate.value, true);
+      await Future<void>.delayed(const Duration(milliseconds: 20));
+      expect(controller.isCheckingForUpdate.value, false);
+    });
lib/views/screens/about_app_screen.dart (3)

72-78: Bind to Rx values, not the Rx object.

Using the Rx directly can render Instance of 'RxString' instead of the value.

Apply this diff:

-                                    "${aboutAppScreenController.appVersion} | ${aboutAppScreenController.appBuildNumber} | Stable",
+                                    "${aboutAppScreenController.appVersion.value} | ${aboutAppScreenController.appBuildNumber.value} | Stable",

3-3: Remove commented import.

Apply this diff:

-// import 'package:resonate/l10n/app_localizations.dart';

166-174: Optional: Implement “Rate” action.

Hook this to Play Store listing for better UX; otherwise hide the action.

lib/controllers/about_app_screen_controller.dart (1)

51-56: Default clearSettings to false; remove unused isManualCheck.

Clearing settings on every check will keep re-prompting. Also, isManualCheck is not used.

Apply this diff:

-  Future<UpdateCheckResult> checkForUpdate({
-    bool launchUpdateIfAvailable = false,
-    bool isManualCheck = false,
-    bool clearSettings = true,
-    bool showDialog = true,
-  }) async {
+  Future<UpdateCheckResult> checkForUpdate({
+    bool launchUpdateIfAvailable = false,
+    bool clearSettings = false,
+    bool showDialog = true,
+  }) async {
lib/l10n/app_localizations_hi.dart (2)

1159-1162: Minor wording nit: localize “Play Store” consistently.

Consider “प्ले स्टोर” instead of “Play Store” for full Hindi localization, or keep the brand in English consistently across all strings.


1185-1190: Tone consistency nit.

Elsewhere this file mixes informal “तुम/तुम्हारी” and formal “आप”. These new entries use formal tone; choose one consistently (recommend formal for system prompts).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e01eaec and d6e3864.

📒 Files selected for processing (16)
  • .gitignore (1 hunks)
  • lib/controllers/about_app_screen_controller.dart (2 hunks)
  • lib/controllers/auth_state_controller.dart (2 hunks)
  • lib/l10n/app_en.arb (1 hunks)
  • lib/l10n/app_hi.arb (1 hunks)
  • lib/l10n/app_localizations.dart (1 hunks)
  • lib/l10n/app_localizations_en.dart (1 hunks)
  • lib/l10n/app_localizations_hi.dart (1 hunks)
  • lib/main.dart (2 hunks)
  • lib/utils/enums/action_enum.dart (1 hunks)
  • lib/utils/enums/update_enums.dart (1 hunks)
  • lib/views/screens/about_app_screen.dart (5 hunks)
  • project_structure.txt (4 hunks)
  • pubspec.yaml (1 hunks)
  • test/controllers/about_app_screen_controller_test.dart (1 hunks)
  • test/controllers/about_app_screen_controller_test.mocks.dart (1 hunks)
🔇 Additional comments (11)
lib/l10n/app_localizations_en.dart (1)

1130-1196: EN mappings added correctly.

Strings match ARB; no issues spotted.

lib/l10n/app_localizations.dart (1)

2091-2210: Confirm Hindi localizations implement new getters

Script returned no output; verify lib/l10n/app_localizations_hi.dart overrides all 20 new getters: checkForUpdates, updateNow, updateLater, updateSuccessful, updateSuccessfulMessage, updateCancelled, updateCancelledMessage, updateFailed, updateFailedMessage, updateError, updateErrorMessage, platformNotSupported, platformNotSupportedMessage, updateCheckFailed, updateCheckFailedMessage, upToDateTitle, upToDateMessage, updateAvailableTitle, updateAvailableMessage, updateFeaturesImprovement.

pubspec.yaml (1)

63-63: Upgrader added — confirm Dart SDK ≥ 3.5; latest stable is 11.5.1.

  • Latest stable: 11.5.1 — your ^11.5.0 will resolve to 11.5.1.
  • upgrader 11.x requires Dart 3.5+ — ensure your pubspec environment sdk lower bound is >=3.5 or pin to an older upgrader major.
  • Notable breaking changes to watch for when crossing majors: 9.0.0 (UI moved to UpgradeAlert/UpgradeCard), 10.0.0 (UpgraderState rework + Appcast removed) — audit any Appcast/state usages.

File: pubspec.yaml (line 63)

  upgrader: ^11.5.0
project_structure.txt (2)

309-320: Enum additions look good and scoped correctly.


394-396: Tests added in the right location; structure updated accordingly.

test/controllers/about_app_screen_controller_test.dart (2)

29-45: Solid default-state and toggle tests.


46-75: Great: verifies no-update and update-available branches with mocks.

test/controllers/about_app_screen_controller_test.mocks.dart (1)

19-32: Generated mock relies on Upgrader’s internal src types; keep an eye on brittleness.

Upgrading upgrader may break these imports. Pin the dependency or wrap Upgrader behind a thin app interface and mock that instead.

Would you like a small adapter interface to decouple tests from upgrader’s internals?

lib/controllers/about_app_screen_controller.dart (2)

65-83: Verify dialog usage: UpgradeAlert inside Get.dialog may double‑wrap.

UpgradeAlert typically manages its own dialog when present in the widget tree. Rendering it inside Get.dialog can lead to no content or nested dialogs depending on version.

Please run a quick manual check:

  • Trigger an update-available state.
  • Confirm exactly one dialog appears and it contains the Upgrader UI.
  • If it double shows or appears empty, prefer placing UpgradeAlert(upgrader: upgrader, child: ...) in the screen tree and, for manual checks, just navigate to the screen instead of wrapping with Get.dialog.

100-103: Static Android Play Store URL: LGTM.

Hard‑coding the package ID is correct post‑publish and matches gradle config.

lib/l10n/app_localizations_hi.dart (1)

1135-1201: Approve — i18n additions verified.
All getters from the original comment are declared in lib/l10n/app_localizations.dart and implemented in lib/l10n/app_localizations_en.dart and lib/l10n/app_localizations_hi.dart.

.gitignore Outdated
.pub-cache/
.pub/
/build/
pubspec.lock
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Don’t ignore pubspec.lock for apps.

For application repos, committing pubspec.lock ensures reproducible builds across CI/dev. Recommend removing this ignore.

Apply this diff:

-pubspec.lock
📝 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
pubspec.lock
🤖 Prompt for AI Agents
In .gitignore around line 37, the file currently ignores "pubspec.lock" which
should not be ignored for application repositories; remove the "pubspec.lock"
entry from .gitignore so the lockfile can be tracked, then add and commit
pubspec.lock to the repo (git add pubspec.lock && git commit -m "Track
pubspec.lock for reproducible builds").

@M4dhav
Copy link
Contributor

M4dhav commented Sep 14, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Sep 14, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@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: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/controllers/about_app_screen_controller.dart (1)

37-41: Don’t access AppLocalizations from onInit; move to onReady (context may be null).

Get.context can be null during onInit, causing a crash when resolving AppLocalizations and showing a snackbar. Call _loadPackageInfo from onReady instead.

Apply:

-  @override
-  void onInit() {
-    super.onInit();
-    _loadPackageInfo();
-  }
+  @override
+  void onReady() {
+    super.onReady();
+    _loadPackageInfo();
+  }

Optionally guard the snackbar:

-      customSnackbar(
+      if (Get.context != null) {
+        customSnackbar(
           AppLocalizations.of(Get.context!)!.updateCheckFailed,
           AppLocalizations.of(Get.context!)!.updateCheckFailedMessage,
           LogType.error,
-      );
+        );
+      }

Also applies to: 49-53

♻️ Duplicate comments (3)
lib/controllers/about_app_screen_controller.dart (3)

75-93: Avoid wrapping UpgradeAlert in Get.dialog; let the widget display its own alert.

UpgradeAlert is itself responsible for showing the alert prompt; wrapping it in a dialog can create nested dialogs and bypass built‑in flags like barrierDismissible, shouldPopScope, etc. Prefer placing UpgradeAlert in the widget tree (e.g., in MaterialApp.builder or the About screen) and drive behavior via Upgrader. (pub.dev)

Example (About screen):

@override
Widget build(BuildContext context) {
  return UpgradeAlert(
    upgrader: controller.upgrader,
    barrierDismissible: true,
    shouldPopScope: () => true, // allow Android back to dismiss
    child: AboutScreenContent(),
  );
}

106-129: Return notSupported on iOS/other; avoid parsing blank URLs and prefer market:// on Android.

Blank URL on iOS leads to a failed state; better to explicitly signal not supported and avoid Uri.parse('') calls. Also try the market scheme first on Android for a smoother UX, then fall back to https. (github.com)

   Future<UpdateActionResult> launchStoreForUpdate() async {
     try {
-      String storeUrl;
-      if (Platform.isAndroid) {
-        storeUrl =
-            'https://play.google.com/store/apps/details?id=com.resonate.resonate';
-      } else if (Platform.isIOS) {
-        // The App Store URL of app
-        storeUrl = '';
-      } else {
-        return UpdateActionResult.error;
-      }
-      final uri = Uri.parse(storeUrl);
-      if (await canLaunchUrl(uri)) {
-        await launchUrl(uri, mode: LaunchMode.externalApplication);
-        return UpdateActionResult.success;
-      } else {
-        return UpdateActionResult.failed;
-      }
+      const packageName = 'com.resonate.resonate';
+      if (Platform.isAndroid) {
+        // Try Play Store app first.
+        final marketUri = Uri.parse('market://details?id=$packageName');
+        if (await canLaunchUrl(marketUri)) {
+          await launchUrl(marketUri, mode: LaunchMode.externalApplication);
+          return UpdateActionResult.success;
+        }
+        // Fallback to web Play Store.
+        final webUri = Uri.parse(
+          'https://play.google.com/store/apps/details?id=$packageName',
+        );
+        if (await canLaunchUrl(webUri)) {
+          await launchUrl(webUri, mode: LaunchMode.externalApplication);
+          return UpdateActionResult.success;
+        }
+        return UpdateActionResult.failed;
+      }
+      // iOS and other platforms not yet supported.
+      return UpdateActionResult.notSupported;
     } catch (e) {
       log('Update error: $e');
       return UpdateActionResult.error;
     }
   }

Optionally hoist the packageName into a top‑level const.


61-71: Don’t clear Upgrader settings by default; gate with kDebugMode.

Always clearing saved settings defeats suppression (Ignore/Later) and will re-prompt users. Default to not clearing and only clear when explicitly requested or in debug. (github.com)

   Future<UpdateCheckResult> checkForUpdate({
     bool launchUpdateIfAvailable = false,
     bool isManualCheck = false,
-    bool clearSettings = true,
+    bool clearSettings = false,
     bool showDialog = true,
   }) async {
     isCheckingForUpdate.value = true;
     try {
-      if (clearSettings) {
+      if (clearSettings || kDebugMode) {
         Upgrader.clearSavedSettings();
       }
🧹 Nitpick comments (3)
lib/controllers/about_app_screen_controller.dart (3)

4-4: Scope the foundation import.

Prefer importing only what you use to reduce namespace pollution.

-import 'package:flutter/foundation.dart';
+import 'package:flutter/foundation.dart' show kDebugMode;

23-35: Align debugDisplayOnce with kDebugMode for consistency.

Minor: consider kDebugMode for debugDisplayOnce so one‑off prompts are also enabled only in debug.

-            debugDisplayOnce: false, 
+            debugDisplayOnce: kDebugMode,

61-66: Guard against concurrent re‑entry.

If checkForUpdate can be triggered rapidly (launch + About), early‑out when a check is already running.

   Future<UpdateCheckResult> checkForUpdate({
@@
   }) async {
-    isCheckingForUpdate.value = true;
+    if (isCheckingForUpdate.value) return UpdateCheckResult.noUpdateAvailable;
+    isCheckingForUpdate.value = true;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6e3864 and 6bbeece.

📒 Files selected for processing (2)
  • lib/controllers/about_app_screen_controller.dart (2 hunks)
  • lib/views/screens/about_app_screen.dart (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/views/screens/about_app_screen.dart
🔇 Additional comments (3)
lib/controllers/about_app_screen_controller.dart (3)

29-35: Gated Upgrader debug flags correctly.

Using kDebugMode for debug flags and a shorter alert cadence in debug is appropriate.


109-112: No action required — Android applicationId matches package name.
android/app/build.gradle.kts sets applicationId = "com.resonate.resonate", which matches the Play Store URL in lib/controllers/about_app_screen_controller.dart (id=com.resonate.resonate).


61-66: Incorrect — manual-check feedback is already implemented in the view.

AboutAppScreenController doesn't use isManualCheck, but lib/views/screens/about_app_screen.dart::_handleUpdateCheck calls checkForUpdate(isManualCheck: true) and already shows the up‑to‑date, platform‑not‑supported and failure snackbars (upToDateTitle/upToDateMessage, platformNotSupported*, updateCheckFailed*). No controller changes required.

Likely an incorrect or invalid review comment.

@M4dhav
Copy link
Contributor

M4dhav commented Sep 14, 2025

Please implement the last change, coderabbit's recently suggested changes are unnecessary

@M4dhav
Copy link
Contributor

M4dhav commented Sep 14, 2025

Good work on the PR @Mayank4352 , thank you for the contribution

@M4dhav M4dhav merged commit 7dcf1c7 into AOSSIE-Org:dev Sep 14, 2025
2 checks passed
@github-actions
Copy link
Contributor

PR Closed - Thank You, @Mayank4352!

  • If this PR was merged: Congratulations! Your contribution is now part of the project. 🚀
  • If this PR was closed without merging: Don’t worry! You can always improve it and submit again. 💪

We appreciate your effort and look forward to more contributions from you! 🤝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement App Update check functionality

2 participants