Repository navigation
Conversation
n13
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewer: GPT-6.1 Sol
Reviewed 7ad419b883c387bb9bcff46b7d5998a56be9e1cb against d2b5d53b02b7955a8fd428a9c5e117a0e16b1026.
-
[P1] Move progress visibility updates out of widget lifecycle methods. swap_progress_screen.dart:50–59 synchronously modifies a provider in
initState. Opening the progress screen throws “Tried to modify a provider while the widget tree was building”; the focused existing suites fail 30 tests at this call. There is also a separate exit failure:ref.readindisposethrows “Using ref when a widget is about to or has been unmounted is unsafe”, leaving visibility true. I reproduced that on the actual route after isolating the entry mutation. Capture the notifier while mounted and perform visibility changes at a safe time outside construction/disposal, with navigation tests covering both entry and exit. -
[P1] Restore a mobile lockfile compatible with the pinned Flutter SDK. pubspec.lock:864–871 downgrades SDK-pinned dependencies, including
intl 0.20.3 → 0.20.2, while.fvmrcremains Flutter 3.47.7 and Melos enforces lockfiles. This is already blocking GitHub CI's Bootstrap Melos step: it fails specifically inmobile-appwith “Unable to satisfy pubspec.yaml using pubspec.lock” and reports seven dependencies that would change. Revert the unrelated downgrades or regenerate this lockfile with the pinned SDK, then rerun bootstrap. -
[P2] Implement reception of Android swap push updates. The new Android registration path registers for the
type=swap_live_activitydata messages described by the new contract, but no message handler consumes them.firebaseMessagingBackgroundHandleronly logs, the foreground listener treats messages as transaction notifications, and the native card is only wired through MainActivity's method channel and the stale/dismiss receivers. A probe delivering a swap completion message to the actual background handler produces no native card call. Thus server updates cannot refresh the card after the app stops polling. Add a background-capable receiver and share payload handling with the foreground path so both update the matching, non-dismissed swap card. -
[P2] Make the Android 16 notification eligible for Live Updates. SwapLiveNotifications.kt:156–170 always installs custom RemoteViews and only adds
ProgressStyleon API 36. The manifest never requestsPOST_PROMOTED_NOTIFICATIONS, and the builder never requests promoted ongoing presentation. Android's eligibility requirements require both and prohibit custom content views. Consequently even supported Android 16 devices only get an ordinary notification, without the promised Live Update/status-bar chip. Build an eligible standard progress notification for supported/authorized devices and use the custom notification as the fallback. -
[P2] Provide the promised notification fallback on iOS. swap_live_activity_provider.dart:112–116 permanently disables updates on
MissingPluginExceptionwithout showing any notification. The app supports iOS 15+, but AppDelegate only registers this bridge on iOS 16.2+, so iOS 15/16.0/16.1 users receive no swap card. On newer iOS, disabled Live Activities also fail throughActivity.requestwithout a notification fallback. Select the presentation using availability/authorization and route unsupported or disabled Live Activities to a normal notification with the same swap tap behavior.
Validation: flutter test --no-pub test/unit/swap_live_activity_test.dart test/unit/deep_link_service_test.dart test/unit/swap_progress_test.dart test/screens/swap_flow_test.dart test/screens/home_intent_gating_test.dart --reporter expanded completed with 64 passed, 30 failed. Two temporary probes confirmed the disposal failure and ignored background push. Swift simulator type-checking passed for both the widget and bridge; plist/project lint, non-mutating workspace formatting, and git diff --check passed. Local Flutter is 3.44.4, so full workspace bootstrap was limited by the SDK mismatch; analysis was stopped at the required 10-second cap. Full native app builds, device behavior, and live Senoti/APNs/FCM delivery were not validated. GitHub CI independently fails bootstrap on the pinned SDK as described above.
n13
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewer: GPT-6.1 Sol
Reviewed d77f47cbeca2af9af62ac16e8ae223da77d674ab against d2b5d53b02b7955a8fd428a9c5e117a0e16b1026, including the fixes following the earlier review. Three correctness issues remain:
-
[P2] Keep Senoti registration from blocking card updates and removal. swap_live_activity_provider.dart:123–131 awaits
_registerinside the serialized_tailqueue. The registration request has no timeout. If Senoti accepts a connection but does not answer, every later sync waits behind it, including stopping tracking or clearing the swap. A controller probe displayed the card, held registration pending, set the snapshot's swap to null and requested another sync: nativeendremained at 0 calls, and ran only after registration was released. Unregistration also occupies this queue. Separate remote registration work from local presentation, bound the HTTP requests, and guard late completions against the current swap so a failed notification backend cannot freeze or retain the card. -
[P2] Reject older push updates before replacing the displayed status. swap_live_activity_message.dart:47–54 validates only the swap key and dismissal; the native receiver's equivalent validator does the same. FCM does not guarantee delivery order. Replaying a
swappedupdate atupdatedAt=2000, followed by a delayedsettlingupdate atupdatedAt=1000, throughdeliverSwapLiveActivityPushreplaces the completed card with settling andholdsProcess=true. On a background-only delivery path this can leave a completed swap looking unfinished and restart its service. Persist and atomically compare an update revision/timestamp, with protection against terminal-state regression, in both reception paths. -
[P2] Apply Dart-delivered pushes against the current native card atomically. swap_live_activity_message.dart:80–82 makes separate
cardand unconditionalshowcalls. The UI or dismissal receiver can clear, dismiss or replace the card between those calls, particularly while the background Flutter engine handles a push. A method-channel probe returned the matching snapshot and removed the stored card before the reply was consumed: the subsequentshowrecreated the removed card. Nativeshowdoes not revalidate its key/dismissal state. Route Dart push delivery through the existing nativeapplyoperation, and make that shared operation enforce current-card, dismissal and ordering checks immediately before writing. Cover clear/dismiss/new-swap interleavings as well as ordinary foreground/background delivery.
Validation: 117 focused Dart tests passed, covering live activity decisions/controller behavior, push handling, deep links, progress visibility, swap progress/flows and home intent gating. Three additional temporary regression probes reproduced the findings above. Both Swift widget and bridge type-checks passed (bridge checked with an iOS 15 deployment target); workspace formatting, plist/project lint and git diff --check passed. GitHub Analyze and Dependency cooldown checks are successful at the reviewed head. Local Flutter is 3.44.4 versus the pinned 3.47.7: locked bootstrap was blocked by that mismatch, so focused tests used locally resolved dependencies. Workspace analysis was stopped at the required 10-second cap and remains incomplete. Android builds/device behavior, full iOS app builds and live Senoti/APNs/FCM delivery were not validated.
n13
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewer: GPT-6.1 Sol
Reviewed c8551d84e7a6f994bd76c3bc332db0449ecc6813 against d2b5d53b02b7955a8fd428a9c5e117a0e16b1026, including the fixes following the previous review.
- [P2] Protect polling updates from overwriting a completed push. swap_live_activity_provider.dart:174 still sends polling updates through unconditional
show. The new revision/terminal checks protect push delivery throughreplace/commitPush, but the nativeshowbranch bypasses them. Push delivery also leavesactiveSwapProviderunchanged. Consequently, if a completion push arrives while the app still holds a processing order, subsequent failed status polls retain that order and, after the 45-second refresh interval, replace the completed card with “Settling” andholdsProcess=true. This can restart the Android foreground service for an already completed swap. A temporary probe using the actual controller, method-channel client and push handler with mocked native storage reproducedshow:settling → replace:swapped → show:settlingafter applyingwithPollError('HTTP 503')at +46 seconds. Reconcile accepted pushes with the tracked state or enforce the same terminal/revision rules for same-swap polling writes at the native write boundary, while still allowing explicit creation of a new swap. Check the equivalent unconditional iOSactivity.updatepath too, and add coverage combining a completion push with failed/delayed polls.
Validation: 127 focused Dart tests passed across live activity/controller behavior, push delivery, deep links, progress visibility, swap progress/flows and home intent gating. The additional regression probe above failed as described. Swift simulator type-checks passed for the widget and bridge, including the bridge at an iOS 15 deployment target; non-mutating workspace formatting and plist/project lint passed, as did git diff --check.
GitHub Analyze currently fails on the new use_null_aware_elements lint at swap_live_activity_provider.dart:225:44; use the nullable set element syntax there and rerun CI. Dependency cooldown passed. Local Flutter is 3.44.4 versus the pinned 3.47.7, so locked bootstrap was blocked and focused tests used locally resolved dependencies. Workspace analysis was stopped at the required 10-second cap and remains incomplete. Android native tests/builds could not run because no Java runtime is installed; full native app builds, device behavior and live Senoti/APNs/FCM delivery were not validated.
n13
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewer: GPT-6.1 Sol
Reviewed adbbe355f4cad862efb1ca572b6b0a382b2062fb against d2b5d53b02b7955a8fd428a9c5e117a0e16b1026, including the full diff and previous review findings. The latest change guards same-swap polling writes against overwriting a completed push on Android and iOS; its five new Dart regression cases pass.
-
[P2] Honor the persisted dismissal when restoring the Android controller. SwapLiveNotifications.kt:187–196 reads the stored card but passes only its key/revision to the local-write guard, omitting
dismissedKey. The Dart polling guard also ignores dismissal. If the user dismisses the card while no UI listener is attached, native code persists the dismissed key and queues a dismissal signal. On reattachment that signal can arrive before the new controller has a_shownKey, so the controller discards its identity. Its next sync then restores the same dismissed notification and can restart its foreground service. A temporary probe using the actual controller and method-channel client, with mocked persisted native storage and a pending dismissal event, reproducedshow:settlingdespitedismissedKey == key. Enforce same-key dismissal at the nativeshowboundary under the existing lock and in the Dart guard, preserving the ability to start a different swap; carry the dismissed swap key in events or restore it before presentation. Add a regression covering listener/controller recreation after dismissal. -
[P2] Fix the new lint that fails the required analysis check. swap_live_activity_provider.dart:225 still uses
if (_registeredToken != null) _registeredToken!in a set literal. The current-head GitHub Analyze job fails on exactly thisuse_null_aware_elementsdiagnostic at 225:44; the CI test steps are consequently skipped. Use the nullable collection element, e.g.<String>{..._attempted, ?_registeredToken}, and rerun CI.
Validation: 132 focused Dart tests passed across live activity/controller behavior, push delivery, deep links, progress visibility, swap progress/flows, and home intent gating. The additional dismissal regression probe failed as described above. The iOS SwapLiveActivity extension built successfully for the simulator with signing disabled; both widget and bridge Swift type-checks passed, including the bridge at an iOS 15 deployment target. Non-mutating workspace formatting, plist/project lint, and git diff --check passed. Dependency cooldown passed on GitHub.
Local Flutter is 3.44.4 versus the pinned 3.47.7, so locked bootstrap was blocked and focused tests used locally resolved dependencies. Workspace analysis was stopped at the required 10-second cap and remains incomplete; the CI analysis failure above is independent evidence from the pinned SDK. Android native tests/builds could not run because no Java runtime is installed. Full app builds, physical-device behavior, and live Senoti/APNs/FCM delivery were not validated.
…into beast/swap-progress
n13
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewer: GPT-6.1 Sol
Reviewed 73ff8167d705f7ba97b0809fb3f273fc8bc184bd against 413ffe82a715d2f0380b95f8edeb1e569aace801, including the full PR diff and fixes following the previous review. The Android dismissal fix now checks the persisted dismissed key in both the Dart polling guard and the native write guard, and dismissal events carry their swap key. The focused regression tests pass.
- [P2] Fix the new lint that fails CI. swap_live_activity_test.dart:298 still calls
SwapLiveDismissed()withoutconstafter this revision makes its constructor constant. The current-head GitHub Analyze job fails on exactlyprefer_const_constructorsat 298:17; CI's subsequent test steps are skipped. Change this tocard.emit(const SwapLiveDismissed());and rerun CI. This is the only confirmed blocker remaining in this review.
Validation: 138 focused Dart tests passed across live activity/controller behavior, push handling, deep links, progress visibility, swap progress/flows, and home intent gating. The iOS SwapLiveActivity extension built successfully for the simulator with signing disabled. Swift type-checks passed for the widget and bridge, including the bridge with an iOS 15 deployment target. Non-mutating workspace formatting, plist/project lint, and git diff --check passed. GitHub Dependency cooldown passed.
Local Flutter is 3.44.4 versus the pinned 3.47.7, so locked bootstrap was blocked by the SDK mismatch and focused tests used locally resolved dependencies. Workspace analysis was stopped at the required 10-second cap and remains incomplete; the CI failure above is independent evidence from the pinned SDK. Android native tests/builds could not run because no Java runtime is installed. Full native app builds, physical-device behavior, and live Senoti/APNs/FCM delivery were not validated.
The no-arg dismissal in the live activity test is the prefer_const_constructors failure that stops CI before tests. Co-authored-by: Cursor <cursoragent@cursor.com>
n13
left a comment
There was a problem hiding this comment.
Verdict: Approve
Reviewer: GPT-6.1 Sol
Reviewed 19fbeabcae68d98cfc276fd4596debcfc8d6c2a2 against 413ffe82a715d2f0380b95f8edeb1e569aace801, including the full PR diff and the fixes following the previous reviews. No blocking findings remain.
The current head fixes the last reported prefer_const_constructors failure. The focused regressions pass for progress-screen entry/exit, notification fallback, non-blocking registration, push ordering, polling after completion, and restoring the controller after dismissal. Inspection also confirms the Android native write boundary checks the persisted dismissal and revision, and the iOS polling update guards terminal state.
Validation:
- 138 focused Dart tests passed with
flutter test --no-pub test/unit/swap_live_activity_test.dart test/unit/swap_live_activity_push_message_test.dart test/unit/deep_link_service_test.dart test/unit/swap_progress_test.dart test/screens/swap_progress_visibility_test.dart test/screens/swap_flow_test.dart test/screens/home_intent_gating_test.dart --reporter expanded. - The
SwapLiveActivityXcode target built successfully for the iOS simulator with signing disabled. The bridge and shared attributes also passed Swift type-checking with an iOS 15 deployment target. - Non-mutating workspace formatting, plist/project lint, and
git diff --checkpassed. - GitHub CI passed locked bootstrap, formatting, analysis, and the mobile, SDK, and cold-wallet test suites at this exact head. Dependency cooldown also passed.
Local Flutter is 3.44.4 versus the pinned 3.47.7, so local locked bootstrap was blocked by that mismatch and focused tests used locally resolved dependencies. Workspace analysis was stopped at the required 10-second cap and remains incomplete locally. Android native tests/builds could not run because no Java runtime is installed. Full native app builds, physical-device behavior, and live Senoti/APNs/FCM delivery were not validated.
Adding live activities to iOS and Android when supported, otherwise fallback to notification card.