Macos smoke test update - #8960
Closed
jigar-f wants to merge 7 commits into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe PR replaces observer-based payment WebView tracking with Riverpod events and robot-driven checkout actions. It updates VPN smoke tests with macOS extension handling, screenshots, activation polling, and diagnostics. It also moves Radiance initialization to app launch and selects staging with Payment checkout flow
VPN smoke-test readiness
Radiance environment and startup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SmokeTest
participant PaymentRobot
participant ChoosePaymentMethod
participant AppWebView
participant WebViewPageEventProvider
SmokeTest->>PaymentRobot: load and select plan
PaymentRobot->>ChoosePaymentMethod: open payment methods
PaymentRobot->>ChoosePaymentMethod: start Stripe checkout
ChoosePaymentMethod->>AppWebView: open checkout URL
AppWebView->>WebViewPageEventProvider: report page event
WebViewPageEventProvider-->>SmokeTest: provide loaded or failed event
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates macOS smoke-test infrastructure and app startup wiring to make desktop smokes more reliable, including moving Radiance setup earlier into the native macOS lifecycle and adding better in-test observability of native WebView navigation.
Changes:
- Move macOS Radiance setup from a Flutter method-channel call into
AppDelegateand select stage/prod via a filesystem marker. - Replace per-WebView observer callbacks with a Riverpod provider that emits page-load events for smoke tests.
- Refactor desktop smoke tests toward “robot” helpers, improve macOS system-extension handling, and upload additional CI diagnostics/artifacts.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| macos/Shared/FilePath.swift | Add .radiance_env marker-based selection of Radiance environment (stage/prod). |
| macos/Runner/Handlers/MethodHandler.swift | Remove setupRadiance method-channel entrypoint and related state. |
| macos/Runner/AppDelegate.swift | Initialize Radiance during macOS app launch (native side). |
| lib/lantern/lantern_platform_service.dart | Stop invoking setupRadiance over the method channel on macOS. |
| lib/features/auth/choose_payment_method.dart | Remove WebView observer plumbing from payment flow UI. |
| lib/core/widgets/app_webview.dart | Introduce webViewPageEventProvider and emit load success/failure events. |
| lib/core/utils/url_utils.dart | Remove observer parameter from openWebview routing. |
| lib/core/router/router.gr.dart | Remove observer arguments from generated routes/args. |
| integration_test/vpn/vpn_smoke_helpers.dart | Dismiss macOS system-extension screen overlay while waiting for VPN controls. |
| integration_test/vpn/macos_connect_smoke_test.dart | Increase timeout and proactively request system-extension activation in CI. |
| integration_test/vpn/connect_smoke_harness.dart | Align desktop harness with robot-based structure and add screenshots/diagnostics. |
| integration_test/utils/payment_robot.dart | New robot to drive plan selection and payment-method UI for smokes. |
| integration_test/utils/app_robot.dart | Add desktop support, screenshots, and macOS extension-screen dismissal. |
| integration_test/payment/desktop_stripe_checkout_smoke_test.dart | Switch to robots and provider-based WebView event tracking for Stripe checkout. |
| .github/workflows/build-windows.yml | Upload Windows payment smoke diagnostics as an artifact. |
| .github/scripts/macos_smoke_suite.sh | Capture extra system-extension diagnostics; attempt enabling dev mode before connect smoke. |
| .github/scripts/macos_payment_checkout_smoke.sh | CI guard + .radiance_env marker; simplify Dart defines for payment smoke. |
Suppressed comments (1)
macos/Runner/AppDelegate.swift:113
- setupRadiance() runs synchronously on the main thread during applicationDidFinishLaunching. MobileSetupRadiance can take seconds (as logged) and will block app startup and window responsiveness. Consider running the setup inside a Task (as iOS does) to avoid blocking the launch path.
/// Calls API handler setup
private func setupRadiance() {
let startupTime = Date()
let opts = UtilsOpts()
opts.dataDir = FilePath.dataDirectory.relativePath
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
lib/core/widgets/app_webview.dart (2)
29-40: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDocument the lifetime constraint on
captureScreenshotmore strongly, or capture eagerly.
captureScreenshotcloses over theInAppWebViewController. The closure stays reachable through the provider state after the page navigates away or the WebView disposes. A late call then either returns null or throws a platform error. The comment states the constraint, but the event outlives the page.Consumers must call it inside the listener callback. The current smoke test does this. If you want to remove the hazard, capture the bytes in
_reportPageLoadedand storeUint8List?in the event instead of a closure. That costs a screenshot on every main-frame load, so keep the closure if that cost matters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/core/widgets/app_webview.dart` around lines 29 - 40, Strengthen the lifetime documentation for WebViewPageLoaded.captureScreenshot to explicitly require consumers to invoke it only within the listener callback while the page’s WebView remains active. Keep the closure-based API and do not add eager screenshot capture.
15-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOne global event outlives the WebView that produced it.
webViewPageEventProviderstores a single event with no instance scope and no reset, so both the event value and the controller closure it carries stay reachable after the page and the WebView are gone.
lib/core/widgets/app_webview.dart#L15-L52: add aclear()method toWebViewPageEventsand call it when_InnerWebViewStatestarts a load and when it disposes, so a later session cannot read the previous page's event.lib/core/widgets/app_webview.dart#L29-L40: either require consumers to callcaptureScreenshotinside the listener callback, or store the capturedUint8List?inWebViewPageLoadedinstead of a closure over theInAppWebViewController.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/core/widgets/app_webview.dart` around lines 15 - 52, Update lib/core/widgets/app_webview.dart lines 15-52 by adding clear() to WebViewPageEvents and invoking it from _InnerWebViewState when a load starts and during disposal, preventing stale events from later sessions. Also update WebViewPageLoaded at lib/core/widgets/app_webview.dart lines 29-40 to store captured Uint8List? data rather than a controller-capturing closure, or enforce that captureScreenshot is invoked only within the listener callback.integration_test/payment/desktop_stripe_checkout_smoke_test.dart (2)
100-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_waitForCheckoutreports a misleading message on a clean timeout.If no event ever arrives,
lastFailureis null and the test fails with "Stripe Checkout did not load a non-empty document". That text suggests an empty document, not a timeout. State the timeout explicitly.♻️ Proposed message
- fail(checkout.failureMessage); + fail('Stripe Checkout did not finish within 3 minutes. ' + '${checkout.failureMessage}');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_test/payment/desktop_stripe_checkout_smoke_test.dart` around lines 100 - 110, Update _waitForCheckout so a deadline reached without checkout.finished reports an explicit timeout message instead of using checkout.failureMessage when no failure was recorded. Preserve the existing failure message for checkout failures that do provide one.
54-61: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSerialize
checkout.addcalls; the fire-and-forget listener can interleave.
addis asynchronous and awaits_waitForRenderedScreenshot.unawaitedstarts a new call for every event, so calls overlap. Two effects follow:
- Two
WebViewPageLoadedevents for the Stripe host both pass thescreenshot == nullcheck before theawait, so the screenshot capture runs twice.urianddocumentLengthare assigned after theawait. A main-frame failure that arrives during an in-flight capture sets_terminalFailure, and the in-flight handler then setsurito the Stripe host._waitForCheckoutreturns, and Line 71 fails the test even though the page loaded.Chain the futures so events apply in arrival order.
♻️ Proposed serialization
final checkout = _StripeCheckoutTracker(); final pageEventSubscription = payment.container.listen( webViewPageEventProvider, (_, event) { - if (event != null) unawaited(checkout.add(event)); + if (event != null) checkout.enqueue(event); }, );In
_StripeCheckoutTracker:Future<void> _pending = Future<void>.value(); /// Applies events in arrival order; captures never overlap. void enqueue(WebViewPageEvent event) { _pending = _pending.then((_) => add(event)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_test/payment/desktop_stripe_checkout_smoke_test.dart` around lines 54 - 61, Serialize WebViewPageEvent processing in the payment listener by routing events through an ordered queue on _StripeCheckoutTracker, such as an enqueue method backed by a chained Future, instead of calling add with unawaited directly. Ensure each event waits for the previous add operation to finish, while preserving teardown and arrival order.integration_test/utils/payment_robot.dart (3)
57-61: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
orElsethrows a confusing error whenplans.plansis empty.If
plans.plansis empty,plans.plans.firstinsideorElsethrowsBad state: No element. The message does not name the robot step. The smoke test asserts non-empty plans before calling this method, so the path is currently unreachable. Add an explicit failure so future callers get a clear reason.🛡️ Proposed guard
Plan selectBestValuePlan(PlansData plans) { + if (plans.plans.isEmpty) { + fail('Cannot select a plan: the backend returned no plans'); + } final plan = plans.plans.firstWhere( (plan) => plan.bestValue, orElse: () => plans.plans.first, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_test/utils/payment_robot.dart` around lines 57 - 61, Update selectBestValuePlan to explicitly validate that plans.plans is non-empty before firstWhere or its fallback executes, and fail with a clear message identifying the robot step when the collection is empty. Preserve the existing bestValue selection and first-plan fallback for non-empty plans.
29-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe cache only helps if
containeris first read while home is mounted.The doc comment says the container "outlives route changes". That holds only after the first read.
openPaymentMethodscallsappRouter.replaceAll, which unmounts the home screen. If a test callsopenPaymentMethodsbefore any other robot method, the firstcontainerread then fails with "home screen is not mounted".The current smoke test reads
containerthroughloadPlansfirst, so it passes today. Resolve the container eagerly to make the order irrelevant.♻️ Proposed eager resolution
- Future<void> openPaymentMethods({ + /// Resolves and caches the container while home is still mounted. + void primeContainer() => container; + + Future<void> openPaymentMethods({ required String email, required AuthFlow authFlow, }) { + primeContainer(); return appRouter.replaceAll([ ChoosePaymentMethod(email: email, authFlow: authFlow), ]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_test/utils/payment_robot.dart` around lines 29 - 44, Resolve and cache the ProviderContainer during robot initialization, while the home screen is mounted, rather than deferring the first lookup until the container getter is accessed. Update the initialization path and the container getter so openPaymentMethods and other route-changing methods can safely use the cached container regardless of call order.
104-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
uuidfor the generated UUID.
package:uuidis available transitively, so_newUuid()can useconst Uuid().v4()instead of generating and formatting UUID v4 bytes locally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_test/utils/payment_robot.dart` around lines 104 - 115, Update _newUuid() to return a UUID generated via const Uuid().v4(), add the necessary uuid import, and remove the local Random-based byte generation and formatting logic.integration_test/utils/app_robot.dart (2)
295-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
dismissMacOSExtensionScreenIfShownduplicatesdismissOnboardingIfShown.Both methods follow the same shape: return false when the screen is absent, poll up to 5 seconds for a hit-testable target, tap it, then wait for the screen to disappear. Extract one private helper that takes the screen finder, the candidate tap targets, and the log labels. Both public methods then delegate to it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_test/utils/app_robot.dart` around lines 295 - 331, Extract the shared dismissal flow from dismissMacOSExtensionScreenIfShown and dismissOnboardingIfShown into one private helper accepting the screen finder, candidate tap targets, and log labels. Preserve each method’s existing return behavior, 5-second polling, tap/pump sequence, disappearance wait, timeout reason, and logging by delegating both public methods to the helper.
40-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
nameflows straight into a file path.
captureScreenshotbuildsFile('${dir.path}/$name.png'). Anamethat contains/or..writes outside the screenshots directory. All current callers pass literals, so this is not a live defect. Sanitize the name to keep future callers safe.🛡️ Proposed sanitization
- final file = File('${dir.path}/$name.png'); + final safeName = name.replaceAll(RegExp(r'[^A-Za-z0-9._-]'), '_'); + final file = File('${dir.path}/$safeName.png');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration_test/utils/app_robot.dart` around lines 40 - 68, Sanitize the name used by captureScreenshot before constructing the screenshot File path, removing path separators and preventing traversal segments such as “..”. Use the sanitized value consistently for the output filename and related log messages while preserving the existing screenshot capture behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/macos_payment_checkout_smoke.sh:
- Around line 33-34: Update the setup around the .radiance_env marker so staging
selection cannot persist unintentionally after the smoke test; replace the
persistent marker approach with a process-scoped signal, or implement a
stale-marker policy that detects and removes markers left by interrupted runs
while preserving staging selection for the current launch.
- Around line 20-24: Update the CI guard in the macOS payment checkout smoke
script to continue only when CI has the expected affirmative value, rejecting
false, zero, and arbitrary non-empty values before any destructive cleanup runs.
Preserve the existing refusal message and exit behavior for invalid CI values.
In @.github/scripts/macos_smoke_suite.sh:
- Around line 268-285: Update enable_system_extension_developer_mode so its
failure warning no longer states or implies that disabling SIP is required;
retain the sudo systemextensionsctl developer on attempt and report only the
relevant permission or command failure without instructing operators to disable
SIP.
In @.github/workflows/build-windows.yml:
- Around line 198-206: The “Upload Windows payment smoke diagnostics” step must
not upload the full C:\Users\Public\Lantern\logs directory, which contains
sensitive raw logs. Change its artifact path to the intentionally redacted
payment smoke diagnostics output, preserving the existing payment-smoke
condition, artifact name, and retention settings.
In `@integration_test/payment/desktop_stripe_checkout_smoke_test.dart`:
- Around line 196-227: The WebView failure handling in add currently makes
non-cancellation failures from any host terminal. Restrict _terminalFailure
assignment to failures from _stripeHost or the initiating Lantern host, while
preserving cancellation handling and lastFailure recording for all events;
identify and reuse the existing Lantern host symbol rather than introducing a
new host value.
In `@macos/Runner/AppDelegate.swift`:
- Around line 47-48: Add a native waitForRadiance method case to the macOS
method handler used by setupRadiance(), returning a successful readiness result
before startup invokes it; alternatively remove the Dart
service.waitForRadiance() call if Radiance readiness is intentionally handled
elsewhere, while ensuring unknown methods retain their existing behavior.
- Around line 123-132: Update setupRadiance() to return the MobileSetupRadiance
success status, including NSError failures, and make its caller handle a false
result by preventing method-channel registration or failing startup. Ensure
startup cannot continue as if Radiance initialized successfully after either the
error or !success path.
---
Nitpick comments:
In `@integration_test/payment/desktop_stripe_checkout_smoke_test.dart`:
- Around line 100-110: Update _waitForCheckout so a deadline reached without
checkout.finished reports an explicit timeout message instead of using
checkout.failureMessage when no failure was recorded. Preserve the existing
failure message for checkout failures that do provide one.
- Around line 54-61: Serialize WebViewPageEvent processing in the payment
listener by routing events through an ordered queue on _StripeCheckoutTracker,
such as an enqueue method backed by a chained Future, instead of calling add
with unawaited directly. Ensure each event waits for the previous add operation
to finish, while preserving teardown and arrival order.
In `@integration_test/utils/app_robot.dart`:
- Around line 295-331: Extract the shared dismissal flow from
dismissMacOSExtensionScreenIfShown and dismissOnboardingIfShown into one private
helper accepting the screen finder, candidate tap targets, and log labels.
Preserve each method’s existing return behavior, 5-second polling, tap/pump
sequence, disappearance wait, timeout reason, and logging by delegating both
public methods to the helper.
- Around line 40-68: Sanitize the name used by captureScreenshot before
constructing the screenshot File path, removing path separators and preventing
traversal segments such as “..”. Use the sanitized value consistently for the
output filename and related log messages while preserving the existing
screenshot capture behavior.
In `@integration_test/utils/payment_robot.dart`:
- Around line 57-61: Update selectBestValuePlan to explicitly validate that
plans.plans is non-empty before firstWhere or its fallback executes, and fail
with a clear message identifying the robot step when the collection is empty.
Preserve the existing bestValue selection and first-plan fallback for non-empty
plans.
- Around line 29-44: Resolve and cache the ProviderContainer during robot
initialization, while the home screen is mounted, rather than deferring the
first lookup until the container getter is accessed. Update the initialization
path and the container getter so openPaymentMethods and other route-changing
methods can safely use the cached container regardless of call order.
- Around line 104-115: Update _newUuid() to return a UUID generated via const
Uuid().v4(), add the necessary uuid import, and remove the local Random-based
byte generation and formatting logic.
In `@lib/core/widgets/app_webview.dart`:
- Around line 29-40: Strengthen the lifetime documentation for
WebViewPageLoaded.captureScreenshot to explicitly require consumers to invoke it
only within the listener callback while the page’s WebView remains active. Keep
the closure-based API and do not add eager screenshot capture.
- Around line 15-52: Update lib/core/widgets/app_webview.dart lines 15-52 by
adding clear() to WebViewPageEvents and invoking it from _InnerWebViewState when
a load starts and during disposal, preventing stale events from later sessions.
Also update WebViewPageLoaded at lib/core/widgets/app_webview.dart lines 29-40
to store captured Uint8List? data rather than a controller-capturing closure, or
enforce that captureScreenshot is invoked only within the listener callback.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 75d99a88-aa31-4318-90ef-5e4636565a91
📒 Files selected for processing (17)
.github/scripts/macos_payment_checkout_smoke.sh.github/scripts/macos_smoke_suite.sh.github/workflows/build-windows.ymlintegration_test/payment/desktop_stripe_checkout_smoke_test.dartintegration_test/utils/app_robot.dartintegration_test/utils/payment_robot.dartintegration_test/vpn/connect_smoke_harness.dartintegration_test/vpn/macos_connect_smoke_test.dartintegration_test/vpn/vpn_smoke_helpers.dartlib/core/router/router.gr.dartlib/core/utils/url_utils.dartlib/core/widgets/app_webview.dartlib/features/auth/choose_payment_method.dartlib/lantern/lantern_platform_service.dartmacos/Runner/AppDelegate.swiftmacos/Runner/Handlers/MethodHandler.swiftmacos/Shared/FilePath.swift
💤 Files with no reviewable changes (3)
- lib/lantern/lantern_platform_service.dart
- macos/Runner/Handlers/MethodHandler.swift
- lib/features/auth/choose_payment_method.dart
Contributor
Author
|
Overlapping with a ton of stuff. Going to create a focus PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant improvements to the macOS and Windows payment smoke test infrastructure and refactors the payment Stripe Checkout integration test for better reliability and maintainability. The changes enhance test safety, diagnostics, and code structure, especially for CI environments. The most important changes are grouped below:
macOS Payment Smoke Test Hardening & Diagnostics:
macos_payment_checkout_smoke.shscript now refuses to run outside CI to prevent accidental data loss, adds a marker file to select the staging environment, and removes an unused Dart define.macos_smoke_suite.shscript adds extra diagnostic commands (csrutil status,systemextensionsctl developer) and introduces a function to enable system extension developer mode, improving reliability for Flutter connect smoke tests. [1] [2] [3]Windows Payment Smoke Diagnostics:
Stripe Checkout Integration Test Refactor:
desktop_stripe_checkout_smoke_test.darttest is refactored to use newAppRobotandPaymentRobothelpers, removing direct provider manipulation and observer classes in favor of a stateful event tracker (_StripeCheckoutTracker). This improves clarity and robustness of test flow and error handling. [1] [2] [3] [4]Test Utility Enhancements:
AppRobotclass (inapp_robot.dart) gains screenshot capture for diagnostics, a method to dismiss the macOS system extension screen, and improved tap helpers. These utilities streamline test writing and debugging across platforms. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Test Flow and Error Handling Improvements:
These changes collectively make the payment smoke tests safer, more maintainable, and easier to debug in CI environments.
Summary by CodeRabbit
Improvements
Bug Fixes
Testing