Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude[agent] re-review |
|
@fadi-george The model is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it. You can try again without specifying a model (just If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: |
|
@claude review |
There was a problem hiding this comment.
Both previously flagged issues have been addressed — LGTM.
Extended reasoning...
Overview
This PR bumps the Flutter SDK to 5.5.0 and introduces Swift Package Manager (SPM) support. It touches pubspec.yaml, ios/onesignal_flutter.podspec, android/build.gradle, Package.swift, both native plugin files (Java and ObjC), lib/src/liveactivities.dart, the demo Podfile.lock, a test file, and the release CI workflow.
Prior Review Findings — Now Resolved
The previous review flagged two issues: (1) OneSignalLiveActivities was missing from Package.swift's target dependencies, which would have caused every SPM build to fail; and (2) s.version in the podspec and version in android/build.gradle were stale at 5.3.4 while pubspec.yaml moved to 5.5.0. Both are fully addressed in the current diff — Package.swift now lists all five required products, and all three version-bearing files are consistently at 5.5.0.
Security Risks
No security-sensitive code paths are modified. Changes are limited to version strings, SPM dependency declarations, a CI workflow improvement, and a minor deprecation-annotation formatting fix.
Remaining Nit
The only new issue flagged in this pass is a nit: the sed pattern used in create-release-pr.yml to update the podspec version hardcodes ten spaces between s.version and =. This could silently no-op if the podspec is ever reformatted, though it works correctly against the current file. This is a CI robustness concern for future releases, not a correctness issue for this release.
Level of Scrutiny
Version-bump release PRs with SPM support additions warrant moderate scrutiny. The critical SPM build-failure bug has been fixed, all version strings are consistent, and the mechanical changes (wrapper version strings, lock file checksum) look correct.
ios/onesignal_flutter/Package.swift
Outdated
| .product(name: "OneSignalInAppMessages", package: "OneSignal-XCFramework"), | ||
| .product(name: "OneSignalLocation", package: "OneSignal-XCFramework"), | ||
| .product(name: "OneSignalExtension", package: "OneSignal-XCFramework"), | ||
| .product(name: "OneSignalLiveActivities", package: "OneSignal-XCFramework"), |
There was a problem hiding this comment.
Removing,
I tested flutter build ios --no-codesign which did cause build errors WITH it
There was a problem hiding this comment.
All previously raised issues have been addressed — LGTM.
Extended reasoning...
Overview
This is a standard release PR bumping the Flutter SDK to 5.5.0 and shipping SPM support (introduced in #1131). Modified files span version strings across pubspec.yaml, android/build.gradle, ios/onesignal_flutter.podspec, OneSignalPlugin.java, OneSignalPlugin.m, the new ios/onesignal_flutter/Package.swift, and the create-release-pr.yml workflow. A minor deprecation message update in lib/src/liveactivities.dart and a corresponding test // ignore: annotation round out the changes.
Security Risks
No security-sensitive code is touched. Changes are version string updates, dependency declarations, and a CI workflow improvement. No auth, crypto, or permission logic is modified.
Level of Scrutiny
Release/version-bump PRs are low-risk and mechanical. The only non-trivial addition is the SPM Package.swift, which has now been corrected to include all required dependencies.
Resolution of Prior Feedback
All three issues flagged in previous review passes are resolved in the current diff: (1) OneSignalLiveActivities is now listed as a target dependency in Package.swift; (2) s.version in the podspec and version in build.gradle are both set to 5.5.0; (3) the sed pattern in create-release-pr.yml uses [[:space:]]* to match whitespace agnostically rather than hardcoding 10 spaces. Both inline comments are marked resolved.
| : () => vm.updateLiveActivity(), | ||
| ), | ||
| AppSpacing.gapBox, | ||
| SizedBox( | ||
| width: double.infinity, | ||
| child: OutlinedButton( | ||
| onPressed: activityEmpty ? null : () => vm.exitLiveActivity(), | ||
| style: OutlinedButton.styleFrom( | ||
| foregroundColor: AppColors.osPrimary, | ||
| side: const BorderSide(color: AppColors.osPrimary), | ||
| ), | ||
| child: const Text('STOP UPDATING LIVE ACTIVITY'), | ||
| ), | ||
| ), | ||
| AppSpacing.gapBox, | ||
| SizedBox( | ||
| width: double.infinity, | ||
| child: OutlinedButton( |
There was a problem hiding this comment.
🟡 The PR removes the STOP UPDATING LIVE ACTIVITY button from the UI but leaves behind dead code: exitLiveActivity() in app_viewmodel.dart (lines 504-507) and onesignal_repository.dart (lines 195-198) have no remaining callers. Additionally, onesignal_repository.dart calls OneSignal.LiveActivities.exitLiveActivity() -- the method marked @deprecated in this same PR -- without a suppress comment, producing a build warning when the demo app is compiled.
Extended reasoning...
What the bug is
This PR removes the STOP UPDATING LIVE ACTIVITY button from live_activities_section.dart (lines 82-87 in the diff). That button called vm.exitLiveActivity() in the viewmodel. However, the removal was not propagated down the calling chain: the exitLiveActivity() methods in both examples/demo/lib/viewmodels/app_viewmodel.dart (lines 504-507) and examples/demo/lib/repositories/onesignal_repository.dart (lines 195-198) were left intact with no remaining UI callers, making them dead code.
The specific code path that triggers it
Before this PR: UI button calls vm.exitLiveActivity(), which calls _repository.exitLiveActivity(), which calls OneSignal.LiveActivities.exitLiveActivity(activityId). After this PR, the UI button is removed but the entire chain below it remains. Searching the demo app lib directory for callers of vm.exitLiveActivity() returns only the method definition itself -- confirming it is unreachable dead code. There is a second distinct issue: onesignal_repository.dart line 197 calls OneSignal.LiveActivities.exitLiveActivity(activityId) -- the exact method marked @deprecated in this same PR -- without any suppress comment.
Why existing code does not prevent it
There is no static analysis rule or linting configuration in the demo app that flags unreachable instance methods. The Dart analyzer does not treat public instance methods as dead code even when no callers exist, so the compiler will not emit a warning about the orphaned viewmodel and repository methods. The missing suppress comment in onesignal_repository.dart is separately noteworthy because the test file at test/liveactivities_test.dart correctly added the suppress comment before its call to the same deprecated method, demonstrating the project is aware of the requirement -- but the demo repository was not updated consistently.
Impact
The impact is limited to the demo app example code and does not affect the core SDK. The dead code creates maintenance confusion, and building the demo app will emit a Dart deprecation warning at onesignal_repository.dart:197 because exitLiveActivity was marked @deprecated in this exact PR. While not a runtime failure, the warning is a direct inconsistency introduced by this PR and is avoidable.
How to fix
Remove exitLiveActivity() from both app_viewmodel.dart (lines 504-507) and onesignal_repository.dart (lines 195-198). If the methods are intentionally retained for possible future use, add the appropriate ignore comment to the call in onesignal_repository.dart to suppress the warning. The cleaner approach is full removal since the UI entry point is gone.
Step-by-step proof
- Before this PR, live_activities_section.dart contained a button with onPressed: () => vm.exitLiveActivity().
- This PR removes that button entirely (diff lines 82-94 in live_activities_section.dart).
- app_viewmodel.dart:504-507 still defines exitLiveActivity() calling _repository.exitLiveActivity() -- no other call site exists in the demo app.
- onesignal_repository.dart:195-198 still defines exitLiveActivity() calling OneSignal.LiveActivities.exitLiveActivity(activityId) -- no other call site exists.
- lib/src/liveactivities.dart now marks exitLiveActivity with @deprecated('Currently unsupported, avoid using this method.').
- Running dart analyze on the demo app will produce a deprecated_member_use warning at onesignal_repository.dart:197 because there is no suppress comment, unlike test/liveactivities_test.dart which correctly has the ignore comment.
| @Deprecated('Currently unsupported, avoid using this method.') | ||
| Future<void> exitLiveActivity(String activityId) async { |
There was a problem hiding this comment.
🟡 The @Deprecated annotation on exitLiveActivity uses a two-line format but its message (49 chars) fits on a single line within the 80-char limit, so dart format will collapse it and cause the CI format-check step to fail with a non-zero exit code. Fix: use @Deprecated('Currently unsupported, avoid using this method.') on a single line.
Extended reasoning...
What the bug is
In lib/src/liveactivities.dart lines 22–23, the newly added @Deprecated annotation is written in multi-line form:
@Deprecated(
'Currently unsupported, avoid using this method.')The deprecation message is 49 characters. The full single-line annotation with two leading spaces would be @Deprecated('Currently unsupported, avoid using this method.') — exactly 64 characters — well within dart format's default 80-character line limit.
The specific code path that triggers it
The CI workflow runs dart run rps format-check (defined in pubspec.yaml under scripts.format-check), which expands to:
dart format --set-exit-if-changed --output=none lib test
dart format normalizes single-argument function-like expressions (including annotations) to a single line whenever they fit within the line limit. Because the single-line form is only 64 characters, dart format would reformat lines 22–23 into one line. With --set-exit-if-changed, this causes the format-check step to exit non-zero, failing CI.
Why existing code does not prevent it
There is no pre-commit hook or editor integration enforcing dart format before changes are pushed. The commit history shows the root cause: commit 111e903 style: reformat deprecated annotation originally had a longer message that dart format kept multi-line, then commit ae0f975 docs: soften exitLiveActivity deprecation message shortened the message without collapsing the multi-line form to match what dart format would produce.
The contrast with the existing @Deprecated in lib/onesignal_flutter.dart (lines 73–74) is telling: that annotation's message is 123 characters long, so its multi-line form is correct and dart format leaves it as-is. The new annotation has a much shorter message that must be on one line.
Impact
Every CI run against this PR (or any branch that contains this file state) will fail the format-check step. This blocks merging through CI until the annotation is reformatted.
How to fix it
Change lines 22–23 to single-line:
@Deprecated('Currently unsupported, avoid using this method.')Step-by-step proof
lib/src/liveactivities.dartlines 22–23 contain the two-line@Deprecatedform.- CI triggers the
format-checkscript:dart format --set-exit-if-changed --output=none lib test. dart formatprocesseslib/src/liveactivities.dartand determines the annotation fits on one line (64 chars ≤ 80 chars).dart formatwould reformat the file — meaning the file content differs from what is on disk.- Because
--set-exit-if-changedis set and the file would change,dart formatexits with a non-zero status code. - The CI step fails, blocking the PR from passing its required checks.
4 participants
Channels: Current
🚀 New Features
Swift Package Manager (SPM) Support: Flutter's SPM integration is used automatically when enabled (the default in newer Flutter versions). No changes are needed for new projects.
Migrating from CocoaPods to SPM: Existing projects using CocoaPods can migrate by:
enable-swift-package-manager: falsefrompubspec.yaml(if present)Podfile,Podfile.lock, andPods/directoryios/Flutter/Debug.xcconfigandios/Flutter/Release.xcconfigflutter run, Flutter adds SPM integration automatically