verify staging checkout converts the installed app to Pro - #8939
verify staging checkout converts the installed app to Pro#8939atavism wants to merge 16 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds desktop payment checkout and conversion smoke modes. It introduces bounded account polling, WebView JavaScript evaluation, staging E2E payment handling, reusable payment-test utilities, Windows smoke parameters, workflow selection, and artifact uploads. ChangesPayment smoke coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SmokeTest
participant PaymentRobot
participant AppWebViewObserver
participant WebViewController
participant AccountStatus
SmokeTest->>PaymentRobot: load plan and open payment route
PaymentRobot->>AppWebViewObserver: start checkout observation
AppWebViewObserver->>WebViewController: evaluate completion JavaScript
SmokeTest->>AccountStatus: poll account status
AccountStatus-->>SmokeTest: return Pro status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Pull request overview
Adds a Windows nightly “payment conversion” smoke path that completes a staging-only E2E checkout in the installed app, then verifies the callback, server/local Pro state, success UI, and run-scoped cleanup via CI artifacts/log markers.
Changes:
- Introduces a
boundedPollutility and migratescheckUserAccountStatusto poll until the server reportsuserLevel=pro(with structured logging for smoke validation). - Enables an
e2echeckout provider in Windows nightly smoke mode (including a staging-only provider injection when absent from normal plan responses). - Extends Windows GitHub Actions + PowerShell smoke harness to run the conversion flow end-to-end, capture artifacts, and invoke remote cleanup.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/core/utils/bounded_poll_test.dart | Adds unit coverage for boundedPoll success + stall/timeout behavior. |
| test/core/smoke/payment_checkout_smoke_test.dart | Verifies CLI parsing accepts the staging-only e2e provider. |
| lib/lantern_app.dart | Injects the e2e provider into desktop plans for smoke sessions when missing. |
| lib/features/auth/choose_payment_method.dart | Routes e2e provider through the desktop redirect flow; adds smoke logging markers. |
| lib/core/widgets/app_webview.dart | Logs purchase callback result for conversion smoke verification. |
| lib/core/utils/bounded_poll.dart | New polling helper that bounds each fetch by remaining overall timeout. |
| lib/core/smoke/payment_checkout_smoke.dart | Adds e2e to supported providers and exposes usesE2EProvider. |
| lib/core/common/common.dart | Reworks Pro status check to poll + update local user data on success. |
| lib/core/common/app_dialog.dart | Adds a semantics label to the Pro success logo for UI automation targeting. |
| .github/workflows/build-windows.yml | Adds workflow input + artifact upload for payment conversion smoke. |
| .github/workflows/app-smoke-tests.yml | Plumbs new conversion smoke toggle into the Windows smoke job. |
| .github/scripts/windows_smoke_suite.ps1 | Runs the new conversion smoke mode and uploads its artifacts. |
| .github/scripts/windows_payment_checkout_smoke.ps1 | Implements conversion assertions, captures screenshots/results, and calls staging cleanup endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3368f58 to
886043e
Compare
|
@atavism Is this ready for review? |
152415a to
80f2b89
Compare
3373297 to
fead90c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (10)
lib/lantern_app.dart (1)
118-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a machine-readable failure marker.
The bootstrap swallows every error and logs
event=bootstrap_error. The app then stays on the default route with no visible indication. If the Windows runner waits forevent=ready, it can only detect the failure through a timeout, which produces a slow and unclear result. Log an explicit terminal marker, or surface the error in the UI, so the runner can fail fast with the cause.🤖 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/lantern_app.dart` around lines 118 - 125, The bootstrap error path in the payment checkout smoke flow must emit an explicit terminal failure marker after logging the exception, such as a machine-readable event containing the error details, so the Windows runner can fail fast instead of waiting for event=ready. Update the catch block around the bootstrap logic while preserving the existing error and stack-trace logging.test/core/smoke/payment_checkout_smoke_test.dart (1)
48-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-Windows path.
The gate in
parserejects on!isWindows || buildType != 'nightly'. This test only exercises thebuildTypehalf. Add a case withisWindows: falseso a future change cannot drop the platform half of the gate silently.💚 Proposed additional test
test('rejects the hook outside a Windows nightly build', () { expect( () => PaymentCheckoutSmokeConfig.parse( const [ '--payment-checkout-smoke=stripe', '--payment-checkout-run-id=9a1632f8-5b33-4d6f-8a42-7a8a4f77d829', ], isWindows: true, buildType: 'production', ), throwsFormatException, ); + expect( + () => PaymentCheckoutSmokeConfig.parse( + const [ + '--payment-checkout-smoke=stripe', + '--payment-checkout-run-id=9a1632f8-5b33-4d6f-8a42-7a8a4f77d829', + ], + isWindows: false, + buildType: 'nightly', + ), + throwsFormatException, + ); });🤖 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 `@test/core/smoke/payment_checkout_smoke_test.dart` around lines 48 - 60, Add a separate assertion in the test covering PaymentCheckoutSmokeConfig.parse where isWindows is false and buildType is nightly, using the same hook arguments and expecting a FormatException. Keep the existing production-on-Windows case to retain coverage of the build-type condition.test/core/utils/bounded_poll_test.dart (1)
22-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the non-positive timeout guard.
boundedPollreturns null immediately whentimeout <= Duration.zero, and it never callsfetch. No test covers that branch. Add a small case so the guard stays intact.♻️ Proposed additional test
test('bounds a stalled fetch by the overall timeout', () async {Add after the existing tests:
test('returns null without fetching when the timeout is not positive', () async { var calls = 0; final result = await boundedPoll<String>( timeout: Duration.zero, fetch: (_) async { calls++; return 'pro'; }, ); expect(result, isNull); expect(calls, 0); });🤖 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 `@test/core/utils/bounded_poll_test.dart` around lines 22 - 35, Add a test alongside the existing boundedPoll tests covering a zero or otherwise non-positive timeout. Assert boundedPoll<String> returns null and the fetch callback is never invoked, preserving the guard behavior.lib/core/common/app_dialog.dart (1)
117-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe automation token is announced to screen-reader users.
labelhere does not replace the child semantics, becauseexcludeSemanticsis not set. Assistive technology readspayment-conversion-successin addition to the localized title and description of a production success dialog.
identifiercarries the same value to Windows UI Automation asAutomationIdwithout changing the spoken label. See the consolidated comment for the full change across the payment screens.🤖 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/common/app_dialog.dart` around lines 117 - 142, Update the Semantics widget in the payment-conversion-success dialog to replace the spoken label property with the identifier property, preserving the existing value and child semantics so screen readers announce only the localized title and description while Windows UI Automation retains the automation ID..github/workflows/app-smoke-tests.yml (1)
151-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider gating the conversion smoke behind its own opt-in input.
This line enables the payment-to-Pro conversion smoke on every run where
testsisallor empty. That run creates a disposable local Windows user, drives the installed app through a real staging checkout, and depends on cross-user UI Automation. See the comment on.github/scripts/windows_payment_checkout_smoke.ps1Lines 502-528.Until that path is proven on hosted runners, keep it opt-in so a failure does not turn every nightly run red.
♻️ Proposed change
- run_payment_conversion_smoke: ${{ (inputs.tests || 'all') == 'all' }} + run_payment_conversion_smoke: ${{ inputs.tests == 'payment_conversion' }}🤖 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 @.github/workflows/app-smoke-tests.yml at line 151, Update the run_payment_conversion_smoke condition so the payment-to-Pro conversion smoke runs only when its dedicated opt-in input is explicitly enabled, while preserving the existing tests selection behavior for other smoke tests. Define or reuse the workflow input symbol for this opt-in rather than enabling the conversion smoke whenever tests is all or empty.lib/features/auth/choose_payment_method.dart (2)
657-673: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
onTaphandler duplicates the button callback.
PrimaryButtonalready callsonSubscribeinonPressed. TheSemantics.onTaphandler adds a second path to the same callback.beginPaymentRedirectguards against a concurrent second run, so this is not a correctness defect, but the extra handler is redundant. Removing it also keeps a single owner of the tap action.Note:
excludeSemantics: truealso hides the localizedsubscribe/checkoutlabel from assistive technology. See the consolidated comment on this file.♻️ Proposed change
Semantics( container: true, - excludeSemantics: true, - label: 'payment-checkout-${method.providers.name}', + identifier: 'payment-checkout-${method.providers.name}', button: true, enabled: !isSubmitting, - onTap: isSubmitting ? null : () => onSubscribe.call(method), child: PrimaryButton(🤖 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/features/auth/choose_payment_method.dart` around lines 657 - 673, Remove the redundant Semantics.onTap handler and its isSubmitting conditional from the Semantics wrapping PrimaryButton; keep PrimaryButton.onPressed as the sole caller of onSubscribe.call(method), while preserving the existing enabled state and other semantics properties.
559-572: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Semantics.identifierfor automation names. Put the automation tokens onidentifierinstead ofSemantics.labelat the payment provider row, checkout button, and conversion-success dialog. Update the corresponding Windows smoke script UIA lookups to searchAutomationIdPropertyfor"payment-provider-$Provider","payment-checkout-$Provider", and"payment-conversion-success".🤖 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/features/auth/choose_payment_method.dart` around lines 559 - 572, Replace the automation tokens currently assigned through Semantics.label with Semantics.identifier for the payment-provider row in lib/features/auth/choose_payment_method.dart lines 559-572, the checkout button in lib/features/auth/choose_payment_method.dart lines 657-673, and the conversion-success dialog in lib/core/common/app_dialog.dart lines 117-142. Update the corresponding Windows smoke script UIA lookups to use AutomationIdProperty with "payment-provider-$Provider", "payment-checkout-$Provider", and "payment-conversion-success"..github/scripts/windows_payment_checkout_smoke.ps1 (3)
343-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
$profile; it shadows a PowerShell automatic variable.
$profileholds the path of the current profile script. Use a distinct name such as$userProfile.♻️ Proposed change
for ($i = 0; $i -lt 15; $i++) { - $profile = Get-CimInstance Win32_UserProfile -Filter "SID='$SID'" -ErrorAction SilentlyContinue - if (-not $profile) { return } - if (-not $profile.Loaded) { - Remove-CimInstance $profile + $userProfile = Get-CimInstance Win32_UserProfile -Filter "SID='$SID'" -ErrorAction SilentlyContinue + if (-not $userProfile) { return } + if (-not $userProfile.Loaded) { + Remove-CimInstance $userProfile return }🤖 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 @.github/scripts/windows_payment_checkout_smoke.ps1 around lines 343 - 353, Rename the local `$profile` variable in the disposable-profile cleanup loop to a distinct name such as `$userProfile`, and update its checks and `Remove-CimInstance` call consistently without changing the cleanup behavior.Source: Linters/SAST tools
262-301: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAny single main-frame error fails the whole case, even after a successful load.
Wait-CheckoutDocumentscans the complete log text on each iteration. Oneevent=navigation_errorline anywhere in the log throws, including an aborted navigation that the redirect chain supersedes and errors that occur after the checkout page already rendered. The same applies toWait-PaymentConversionat Line 312.Scope the error check to the target host, or evaluate the success match before the error match so a rendered checkout page is not discarded.
♻️ Proposed change
- if ($logText -match 'PAYMENT_WEBVIEW_SMOKE event=navigation_error') { - throw "The checkout WebView reported a main-frame navigation error" - } - if ($logText -match 'PAYMENT_WEBVIEW_SMOKE event=document_error') { - throw "The checkout WebView could not inspect the loaded document" - } if ($logText -match 'PAYMENT_CHECKOUT_SMOKE event=(rejected|bootstrap_error)') { throw "Lantern could not prepare the requested checkout smoke" } foreach ($match in [regex]::Matches($logText, $linePattern)) {Then re-check
navigation_erroranddocument_errorfor the expected host only after the success loop finds no match.🤖 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 @.github/scripts/windows_payment_checkout_smoke.ps1 around lines 262 - 301, Update Wait-CheckoutDocument and Wait-PaymentConversion so successful checkout/conversion matches are evaluated before failure markers, and only treat navigation_error or document_error events for the expected host as fatal. Preserve unrelated-host errors and stale errors after a rendered target page, allowing the successful result to return before any subsequent error check.
396-406: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe transcript starts before the credential is created; document the plaintext password.
Start-Transcriptat Line 396 is active while the script builds$passwordTextand callsConvertTo-SecureString -AsPlainText. The transcript is uploaded as a workflow artifact. PowerShell transcripts record commands and output rather than variable values, so the generated password should not appear, but the margin is thin. PSScriptAnalyzer also reportsPSAvoidUsingConvertToSecureStringWithPlainTextas an error here.Create the credential before
Start-Transcript, and add a short comment plus a scoped suppression so the analyzer error is intentional rather than ignored.🤖 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 @.github/scripts/windows_payment_checkout_smoke.ps1 around lines 396 - 406, Move the `$passwordText`, `$securePassword`, and `$credential` initialization before `Start-Transcript` in the surrounding setup flow, while preserving their existing values and usage. Add a brief comment documenting the intentional plaintext conversion and apply a narrowly scoped suppression for `PSAvoidUsingConvertToSecureStringWithPlainText`; keep transcript startup and subsequent local-user creation behavior unchanged.Source: Linters/SAST tools
🤖 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/windows_payment_checkout_smoke.ps1:
- Around line 111-137: Update Use-StagingService to explicitly stop and delete
the existing LanternSvc service before invoking InstalledDaemonPath install for
staging. Ensure the cleanup handles an absent service safely, then preserve the
existing installation, wait, and PathName validation flow.
In @.github/scripts/windows_smoke_suite.ps1:
- Around line 389-392: The payment checkout switch is incorrectly shared by
pre-installer and installed-app suites. In
.github/scripts/windows_smoke_suite.ps1 lines 389-392, add and use a distinct
RunInstalledPaymentCheckoutSmoke switch for the installer-gated block; in
.github/workflows/build-windows.yml lines 331-343, pass it only from the
installer smoke step while leaving the pre-installer step on
RunPaymentCheckoutSmoke.
- Around line 389-419: Remove the $LASTEXITCODE validation blocks after both
invocations of windows_payment_checkout_smoke.ps1 in the RunPaymentCheckoutSmoke
and RunPaymentConversionSmoke branches. Continue relying on the child script’s
propagated terminating exceptions for failure handling, while preserving the
existing arguments and step messages.
In `@lib/core/common/common.dart`:
- Around line 142-186: In checkUserAccountStatus, check that the BuildContext is
still mounted immediately before
ref.read(homeProvider.notifier).updateUserData(proUser). Return false without
reading or updating homeProvider when the context has been disposed, while
preserving the existing successful update path for mounted contexts.
In `@lib/core/smoke/payment_checkout_smoke.dart`:
- Around line 27-42: Change the argument-processing flow around
`_singleArgument` so the presence of payment-checkout-smoke prefixes is detected
without validating their values first; apply the Windows nightly gate before
calling `_singleArgument` for `_providerPrefix` and `_runIDPrefix`. Preserve the
null-mode behavior for absent prefixes, and ensure malformed or empty values
still throw on Windows nightly builds.
In `@lib/core/widgets/app_webview.dart`:
- Around line 210-235: Update the WebView load-completion flow around
_logSmokeEvent and _notifyPageLoaded so document inspection and the load_stop
smoke event run even when observer is null. Only call _notifyPageLoaded when an
observer exists, and guard the catch-path observer.onPageLoadFailed callback
similarly while retaining error logging and document_error smoke reporting.
In `@lib/features/auth/choose_payment_method.dart`:
- Around line 490-492: Update the account-status polling path after purchase
completion, near the existing success_ui logger in choose payment flow, to emit
both required conversion markers before success_ui: server_user with the attempt
number and userLevel=pro, followed by local_user with userLevel=pro. Preserve
the existing success_ui event and ensure the markers are emitted on the
successful pro-account path.
In `@lib/lantern_app.dart`:
- Around line 64-90: The synthetic E2E provider added in
_openPaymentCheckoutSmoke must also reach checkout state, not remain only in
matchingProviders. Persist the updated provider list through
PlansNotifier.updatePlans() or pass it explicitly into ChoosePaymentMethod,
ensuring PaymentCheckoutMethods receives the injected provider while preserving
normal provider handling.
In `@lib/main.dart`:
- Around line 44-50: Update the Windows diagnostic logging block in main to
avoid emitting LOCALAPPDATA or other user-identifying paths in app logs. Gate
the existing WEBVIEW2 diagnostic behind the nightly-build condition, or replace
path logging with only the smoke-test facts needed, such as whether each folder
is set and writable.
---
Nitpick comments:
In @.github/scripts/windows_payment_checkout_smoke.ps1:
- Around line 343-353: Rename the local `$profile` variable in the
disposable-profile cleanup loop to a distinct name such as `$userProfile`, and
update its checks and `Remove-CimInstance` call consistently without changing
the cleanup behavior.
- Around line 262-301: Update Wait-CheckoutDocument and Wait-PaymentConversion
so successful checkout/conversion matches are evaluated before failure markers,
and only treat navigation_error or document_error events for the expected host
as fatal. Preserve unrelated-host errors and stale errors after a rendered
target page, allowing the successful result to return before any subsequent
error check.
- Around line 396-406: Move the `$passwordText`, `$securePassword`, and
`$credential` initialization before `Start-Transcript` in the surrounding setup
flow, while preserving their existing values and usage. Add a brief comment
documenting the intentional plaintext conversion and apply a narrowly scoped
suppression for `PSAvoidUsingConvertToSecureStringWithPlainText`; keep
transcript startup and subsequent local-user creation behavior unchanged.
In @.github/workflows/app-smoke-tests.yml:
- Line 151: Update the run_payment_conversion_smoke condition so the
payment-to-Pro conversion smoke runs only when its dedicated opt-in input is
explicitly enabled, while preserving the existing tests selection behavior for
other smoke tests. Define or reuse the workflow input symbol for this opt-in
rather than enabling the conversion smoke whenever tests is all or empty.
In `@lib/core/common/app_dialog.dart`:
- Around line 117-142: Update the Semantics widget in the
payment-conversion-success dialog to replace the spoken label property with the
identifier property, preserving the existing value and child semantics so screen
readers announce only the localized title and description while Windows UI
Automation retains the automation ID.
In `@lib/features/auth/choose_payment_method.dart`:
- Around line 657-673: Remove the redundant Semantics.onTap handler and its
isSubmitting conditional from the Semantics wrapping PrimaryButton; keep
PrimaryButton.onPressed as the sole caller of onSubscribe.call(method), while
preserving the existing enabled state and other semantics properties.
- Around line 559-572: Replace the automation tokens currently assigned through
Semantics.label with Semantics.identifier for the payment-provider row in
lib/features/auth/choose_payment_method.dart lines 559-572, the checkout button
in lib/features/auth/choose_payment_method.dart lines 657-673, and the
conversion-success dialog in lib/core/common/app_dialog.dart lines 117-142.
Update the corresponding Windows smoke script UIA lookups to use
AutomationIdProperty with "payment-provider-$Provider",
"payment-checkout-$Provider", and "payment-conversion-success".
In `@lib/lantern_app.dart`:
- Around line 118-125: The bootstrap error path in the payment checkout smoke
flow must emit an explicit terminal failure marker after logging the exception,
such as a machine-readable event containing the error details, so the Windows
runner can fail fast instead of waiting for event=ready. Update the catch block
around the bootstrap logic while preserving the existing error and stack-trace
logging.
In `@test/core/smoke/payment_checkout_smoke_test.dart`:
- Around line 48-60: Add a separate assertion in the test covering
PaymentCheckoutSmokeConfig.parse where isWindows is false and buildType is
nightly, using the same hook arguments and expecting a FormatException. Keep the
existing production-on-Windows case to retain coverage of the build-type
condition.
In `@test/core/utils/bounded_poll_test.dart`:
- Around line 22-35: Add a test alongside the existing boundedPoll tests
covering a zero or otherwise non-positive timeout. Assert boundedPoll<String>
returns null and the fetch callback is never invoked, preserving the guard
behavior.
🪄 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: 86c98dca-5e7f-4a4b-980d-49c5b99871d9
📒 Files selected for processing (14)
.github/scripts/windows_payment_checkout_smoke.ps1.github/scripts/windows_smoke_suite.ps1.github/workflows/app-smoke-tests.yml.github/workflows/build-windows.ymllib/core/common/app_dialog.dartlib/core/common/common.dartlib/core/smoke/payment_checkout_smoke.dartlib/core/utils/bounded_poll.dartlib/core/widgets/app_webview.dartlib/features/auth/choose_payment_method.dartlib/lantern_app.dartlib/main.darttest/core/smoke/payment_checkout_smoke_test.darttest/core/utils/bounded_poll_test.dart
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
lib/core/widgets/app_webview.dart (1)
128-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the empty
onWebViewCreatedcallback.The callback body is empty. The controller is now taken from
onLoadStopand the error callbacks. Drop the parameter to keep the widget configuration minimal.♻️ Proposed cleanup
- onWebViewCreated: (controller) {},🤖 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` at line 128, Remove the empty onWebViewCreated callback from the widget configuration, leaving controller acquisition through onLoadStop and the error callbacks unchanged..github/scripts/windows_smoke_suite.ps1 (1)
147-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
$argsto avoid the automatic variable.PSScriptAnalyzer flags the assignment at Line 159.
$argsis an automatic variable inside a PowerShell function. The assignment works here, but it shadows the automatic variable and the warning stays in the pipeline output. Rename the local variable.♻️ Proposed rename
- $args = @( + $flutterArgs = @( "test", $Path, "-d", "windows", "--reporter=expanded", "--dart-define=DISABLE_SYSTEM_TRAY=true" ) - $args += "--dart-define=RADIANCE_ENV=$RadianceEnvironment" + $flutterArgs += "--dart-define=RADIANCE_ENV=$RadianceEnvironment" foreach ($define in $DartDefines) { - $args += "--dart-define=$define" + $flutterArgs += "--dart-define=$define" }Update the remaining
$argsreferences at Lines 162-171 in the same way.🤖 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 @.github/scripts/windows_smoke_suite.ps1 around lines 147 - 160, Rename the local `$args` collection in the smoke-test argument construction to a non-reserved name, and update all subsequent `+=` and command-invocation references consistently. Preserve the existing test path, Dart defines, and argument ordering.Source: Linters/SAST tools
lib/features/auth/choose_payment_method.dart (1)
193-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider gating the
e2eprovider case to non-production builds.The
e2ecase lives in production code. It runs whenever the backend returns a provider namede2e, not only under the integration test. A build-time or environment guard keeps the test-only path unreachable in release builds. The comment documents the intent, but it does not enforce 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 `@lib/features/auth/choose_payment_method.dart` around lines 193 - 207, Gate the `e2e` provider branch in the provider-selection flow so it is available only in non-production or explicitly enabled integration-test builds. Ensure release builds cannot enter the `paymentRedirectFlow` path for `e2e`, while preserving the existing desktop test behavior and normal handling for other providers..github/workflows/app-smoke-tests.yml (1)
152-153: 📐 Maintainability & Code Quality | 🔵 TrivialConfirm the nightly
alltier should run the conversion smoke.
contains(fromJSON('["all", "payment-smoke"]'), ...)enables the conversion smoke in the nightly run. Each run creates a staging account and converts it to Pro, and the step budget is 15 minutes. If the intent is on-demand coverage only, restrict conversion topayment-smoke.🤖 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 @.github/workflows/app-smoke-tests.yml around lines 152 - 153, Confirm the intended tier for run_payment_conversion_smoke in the workflow: either retain all in its contains list so nightly all runs execute conversion, or restrict the list to payment-smoke for on-demand coverage only. Update only that condition and leave run_payment_checkout_smoke unchanged.integration_test/payment/desktop_stripe_checkout_smoke_test.dart (1)
437-461: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_completionStartedblocks retries on later E2E page loads.The guard is set on the first page load from
_e2eHost. If that page is an interstitial or redirect that never renders a control labeledcomplete, the observer polls the wrong document for 30 s, setsfailure, and never inspects the following E2E page load. Track completion instead of "started", so a later load can retry.♻️ Proposed change
- if (uri.host != _e2eHost || _completionStarted) return; - _completionStarted = true; + if (uri.host != _e2eHost || completeClicked || _inProgress) return; + _inProgress = true; this.uri = uri; this.documentLength = documentLength; try { screenshot = await _waitForRenderedScreenshot(captureScreenshot); await _saveScreenshot(screenshot!); final deadline = DateTime.now().add(const Duration(seconds: 30)); while (DateTime.now().isBefore(deadline)) { final result = await evaluateJavaScript( _clickCompleteScript, ).timeout(const Duration(seconds: 5)); if (_isJavaScriptTrue(result)) { completeClicked = true; return; } await Future<void>.delayed(_screenshotPollInterval); } - failure = 'The staging E2E completion control was not found'; } catch (error) { failure = 'Unable to complete the staging checkout: $error'; + } finally { + _inProgress = false; }Then let the outer
_waitForat Lines 166-171 provide the timeout, and rename_completionStartedto_inProgress.🤖 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 437 - 461, Update the completion observer around `_completionStarted` to track only whether completion handling is currently in progress, renaming it to `_inProgress` and clearing it when the attempt finishes or fails. Do not permanently block subsequent `_e2eHost` page loads; allow each later load to retry until the outer `_waitFor` timeout, and remove the inner 30-second polling deadline and failure path that prematurely ends the retry flow.
🤖 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/windows_smoke_suite.ps1:
- Around line 355-367: Update the $RunPaymentConversionSmoke branch before
Invoke-FlutterSmokeTest to validate that $ServiceEnvironment equals staging;
fail fast with a clear error message when it does not, and only construct or run
the conversion smoke test after the validation passes.
In `@integration_test/payment/desktop_stripe_checkout_smoke_test.dart`:
- Around line 42-69: The testWidgets timeout around the desktop Stripe smoke
test is too short for the conversion flow’s combined waits. Increase the outer
timeout beyond the conversion budget so staging delays do not trigger the
generic timeout before _runPaymentConversionSmoke completes, while preserving
the existing render and conversion behavior.
---
Nitpick comments:
In @.github/scripts/windows_smoke_suite.ps1:
- Around line 147-160: Rename the local `$args` collection in the smoke-test
argument construction to a non-reserved name, and update all subsequent `+=` and
command-invocation references consistently. Preserve the existing test path,
Dart defines, and argument ordering.
In @.github/workflows/app-smoke-tests.yml:
- Around line 152-153: Confirm the intended tier for
run_payment_conversion_smoke in the workflow: either retain all in its contains
list so nightly all runs execute conversion, or restrict the list to
payment-smoke for on-demand coverage only. Update only that condition and leave
run_payment_checkout_smoke unchanged.
In `@integration_test/payment/desktop_stripe_checkout_smoke_test.dart`:
- Around line 437-461: Update the completion observer around
`_completionStarted` to track only whether completion handling is currently in
progress, renaming it to `_inProgress` and clearing it when the attempt finishes
or fails. Do not permanently block subsequent `_e2eHost` page loads; allow each
later load to retry until the outer `_waitFor` timeout, and remove the inner
30-second polling deadline and failure path that prematurely ends the retry
flow.
In `@lib/core/widgets/app_webview.dart`:
- Line 128: Remove the empty onWebViewCreated callback from the widget
configuration, leaving controller acquisition through onLoadStop and the error
callbacks unchanged.
In `@lib/features/auth/choose_payment_method.dart`:
- Around line 193-207: Gate the `e2e` provider branch in the provider-selection
flow so it is available only in non-production or explicitly enabled
integration-test builds. Ensure release builds cannot enter the
`paymentRedirectFlow` path for `e2e`, while preserving the existing desktop test
behavior and normal handling for other providers.
🪄 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: 9319af7b-c2f2-49ce-8836-1ad87c473aa1
📒 Files selected for processing (8)
.github/scripts/windows_smoke_suite.ps1.github/workflows/app-smoke-tests.yml.github/workflows/build-windows.ymlintegration_test/payment/desktop_stripe_checkout_smoke_test.dartlib/core/common/common.dartlib/core/widgets/app_webview.dartlib/features/auth/choose_payment_method.darttest/core/utils/bounded_poll_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/core/common/common.dart
- test/core/utils/bounded_poll_test.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/features/plans/provider/plans_notifier.dart`:
- Line 18: Update the refresh tracking around _pendingBackgroundRefresh,
build(), and waitForPendingRefresh() so every active background refresh is
awaited or coalesced rather than allowing a newer future to replace an older
one. Ensure refresh completion cannot overwrite plans after
waitForPendingRefresh() returns, and add a regression test covering invalidation
during an in-progress refresh.
🪄 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: 8bbbc9a7-84df-401b-932c-3a8a035e9bec
📒 Files selected for processing (2)
integration_test/payment/desktop_stripe_checkout_smoke_test.dartlib/features/plans/provider/plans_notifier.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_test/payment/desktop_stripe_checkout_smoke_test.dart
* harmonize desktop payment smoke tests * code review updates
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 29-41: Create LANTERN_DATA_DIR with mkdir -p after it is removed
and before touching .radiance_env, preserving the existing marker creation and
Flutter test flow.
🪄 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: 95c89c5e-98c3-4a96-aa0c-da6a6617f39d
📒 Files selected for processing (5)
.github/scripts/macos_payment_checkout_smoke.sh.github/workflows/build-windows.ymlintegration_test/payment/desktop_stripe_checkout_smoke_test.dartintegration_test/utils/app_robot.dartintegration_test/utils/payment_robot.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build-windows.yml
- integration_test/payment/desktop_stripe_checkout_smoke_test.dart
| rm -rf "$LANTERN_DATA_DIR" | ||
| mkdir -p "$LANTERN_LOG_DIR" | ||
| mkdir -p "$ARTIFACT_DIR" | ||
|
|
||
| # The debug test build reads this marker before initializing Radiance. | ||
| touch "$LANTERN_DATA_DIR/.radiance_env" | ||
|
|
||
| flutter test \ | ||
| "$TEST_PATH" \ | ||
| -d macos \ | ||
| --reporter=expanded \ | ||
| --dart-define=DISABLE_SYSTEM_TRAY=true \ | ||
| --dart-define=PAYMENT_SMOKE_SCREENSHOT_PATH="$SCREENSHOT_PATH" \ | ||
| --dart-define=RADIANCE_ENV=staging | ||
| --dart-define=PAYMENT_SMOKE_SCREENSHOT_PATH="$SCREENSHOT_PATH" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Create LANTERN_DATA_DIR before creating the marker.
Line 29 deletes LANTERN_DATA_DIR. Line 34 then runs touch in that deleted directory. This command fails unless another process recreates the directory. Create the directory before touch.
Proposed fix
rm -rf "$LANTERN_DATA_DIR"
+mkdir -p "$LANTERN_DATA_DIR"
mkdir -p "$LANTERN_LOG_DIR"
mkdir -p "$ARTIFACT_DIR"
# The debug test build reads this marker before initializing Radiance.
touch "$LANTERN_DATA_DIR/.radiance_env"📝 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.
| rm -rf "$LANTERN_DATA_DIR" | |
| mkdir -p "$LANTERN_LOG_DIR" | |
| mkdir -p "$ARTIFACT_DIR" | |
| # The debug test build reads this marker before initializing Radiance. | |
| touch "$LANTERN_DATA_DIR/.radiance_env" | |
| flutter test \ | |
| "$TEST_PATH" \ | |
| -d macos \ | |
| --reporter=expanded \ | |
| --dart-define=DISABLE_SYSTEM_TRAY=true \ | |
| --dart-define=PAYMENT_SMOKE_SCREENSHOT_PATH="$SCREENSHOT_PATH" \ | |
| --dart-define=RADIANCE_ENV=staging | |
| --dart-define=PAYMENT_SMOKE_SCREENSHOT_PATH="$SCREENSHOT_PATH" | |
| rm -rf "$LANTERN_DATA_DIR" | |
| mkdir -p "$LANTERN_DATA_DIR" | |
| mkdir -p "$LANTERN_LOG_DIR" | |
| mkdir -p "$ARTIFACT_DIR" | |
| # The debug test build reads this marker before initializing Radiance. | |
| touch "$LANTERN_DATA_DIR/.radiance_env" | |
| flutter test \ | |
| "$TEST_PATH" \ | |
| -d macos \ | |
| --reporter=expanded \ | |
| --dart-define=DISABLE_SYSTEM_TRAY=true \ | |
| --dart-define=PAYMENT_SMOKE_SCREENSHOT_PATH="$SCREENSHOT_PATH" |
🤖 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 @.github/scripts/macos_payment_checkout_smoke.sh around lines 29 - 41, Create
LANTERN_DATA_DIR with mkdir -p after it is removed and before touching
.radiance_env, preserving the existing marker creation and Flutter test flow.
Complete checkout through the staging-only E2E provider in the installed Windows app. Verify the normal payment callback, server and local Pro state, success UI, and run-scoped cleanup.
For https://github.com/getlantern/engineering/issues/3723
Summary by CodeRabbit
New Features
Bug Fixes