Verify checkout rendering from the installed app - #8926
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 a Windows-installed payment checkout smoke flow for Stripe and Shepherd, including CLI configuration, app bootstrap, WebView telemetry, UI automation, artifact collection, and CI workflow controls. ChangesPayment checkout smoke flow
Radiance dependency update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant SmokeSuite
participant PaymentSmoke
participant LanternApp
participant WebViewLogs
Workflow->>SmokeSuite: enable installed payment smoke
SmokeSuite->>PaymentSmoke: run checkout cases
PaymentSmoke->>LanternApp: launch app as disposable user
LanternApp->>WebViewLogs: record checkout lifecycle events
PaymentSmoke->>WebViewLogs: validate checkout events
PaymentSmoke-->>Workflow: write and upload artifacts
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 an installed-app Windows smoke workflow to validate that WebView2 uses per-user storage and that Stripe/Shepherd checkout pages render successfully (without main-frame navigation errors), with diagnostics captured as artifacts.
Changes:
- Introduces a
PaymentCheckoutSmokeConfigCLI hook and wires it through app startup to auto-navigate into the checkout flow on Windows nightly builds. - Adds WebView instrumentation logs for “created / load_start / load_stop / navigation_error” events and document-length probing to detect “blank” checkout renders.
- Extends GitHub Actions Windows smoke suite with a new “payment checkout smoke” option and a dedicated UIAutomation PowerShell runner that uploads artifacts.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/core/smoke/payment_checkout_smoke_test.dart | Unit tests for PaymentCheckoutSmokeConfig.parse() argument validation and derived email behavior. |
| lib/main.dart | Accepts Dart entrypoint arguments, logs WebView2 diagnostics on Windows, parses/propagates checkout smoke config into the app. |
| lib/lantern_app.dart | On smoke enablement, bootstraps plans/provider state and routes directly to ChoosePaymentMethod to start the flow. |
| lib/features/auth/choose_payment_method.dart | Adds Semantics labels to make provider/button discoverable via Windows UIAutomation during smokes. |
| lib/core/widgets/app_webview.dart | Emits structured smoke logs around WebView lifecycle/loading/errors and captures document length. |
| lib/core/smoke/payment_checkout_smoke.dart | New config parser enforcing Windows+nightly constraint, provider allowlist, and UUID run-id validation. |
| go.mod | Updates github.com/getlantern/radiance dependency version. |
| go.sum | Updates checksums for the new radiance versions. |
| .github/workflows/build-windows.yml | Adds input + artifact upload for Windows payment checkout smoke artifacts. |
| .github/workflows/app-smoke-tests.yml | Plumbs new Windows payment checkout smoke toggle into the Windows build/smoke workflow call. |
| .github/scripts/windows_smoke_suite.ps1 | Adds orchestration switch and invocation of the new payment checkout smoke script. |
| .github/scripts/windows_payment_checkout_smoke.ps1 | New end-to-end installed-app UIAutomation script: disposable user, staging service, provider selection, WebView log assertions, and artifact capture. |
Comments suppressed due to low confidence (1)
lib/features/auth/choose_payment_method.dart:606
- The checkout
PrimaryButtonis wrapped inSemantics(excludeSemantics: true, label: 'payment-checkout-...'), which causes assistive technologies to announce the technical automation label rather than the visible button label ('Subscribe'/'Checkout'). This is an accessibility regression. Prefer preserving the button’s normal semantics and exposing a separate stable automation identifier that doesn’t override the spoken label (and only enable it during the smoke run if needed).
Semantics(
container: true,
excludeSemantics: true,
label: 'payment-checkout-${method.providers.name}',
button: true,
enabled: !isSubmitting,
onTap: isSubmitting ? null : () => onSubscribe.call(method),
child: PrimaryButton(
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| title: Semantics( | ||
| container: true, | ||
| excludeSemantics: true, | ||
| label: 'payment-provider-${method.providers.name}', | ||
| child: Row( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/scripts/windows_payment_checkout_smoke.ps1 (1)
271-279: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffTransient/intermediate WebView errors are treated as globally fatal.
This scans the entire accumulated log each poll and throws on any
navigation_error/document_erroranywhere. During redirect-heavy provider flows,app_webview.dartemitsnavigation_errorfor any main-frame error (incl.ERR_ABORTEDon redirect) anddocument_errorwhenevaluateJavascriptfails on an intermediate load — even when the final checkout page renders fine. This can cause false CI failures. Consider only failing when no successfulload_stopfor the expected host was captured, or scoping the fatal checks to the target host/final navigation.🤖 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 271 - 279, Update the polling logic around the accumulated $logText checks so transient navigation_error and document_error events do not immediately fail redirect-heavy flows. Track or detect a successful load_stop for the expected checkout host, and only throw the existing WebView errors when that final target navigation was not successfully reached; keep the checkout rejected/bootstrap_error failure handling unchanged.lib/features/auth/choose_payment_method.dart (1)
501-515: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAutomation hooks override the human-readable a11y labels. Both sites use
excludeSemantics: truewith a machinelabel, which drops descendant semantics so screen readers announcepayment-provider-<name>/payment-checkout-<name>instead of the localized provider name and theSubscribe/Checkoutbutton label — an accessibility regression introduced purely to give UI Automation aNamePropertyto match. PreferSemanticsProperties.identifier(exposed as the UIA AutomationId) for the automation hook and search byAutomationIdPropertyin the PowerShell, leaving the human-readable label intact.
lib/features/auth/choose_payment_method.dart#L501-L515: replace thelabel/excludeSemanticsoverride on the provider title with anidentifier, keeping the provider Text/logo semantics.lib/features/auth/choose_payment_method.dart#L599-L615: same for the checkout button so the localizedSubscribe/Checkoutlabel is still announced; updatewindows_payment_checkout_smoke.ps1Find-AutomationElementto queryAutomationIdProperty.🤖 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 501 - 515, Replace the provider title Semantics override around lib/features/auth/choose_payment_method.dart#L501-L515 with identifier set to the automation hook, removing excludeSemantics and the machine label so the provider Text and logo remain announced. Apply the same change to the checkout button at lib/features/auth/choose_payment_method.dart#L599-L615, preserving its localized Subscribe/Checkout label, and update windows_payment_checkout_smoke.ps1's Find-AutomationElement to query AutomationIdProperty instead of NameProperty.
🤖 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.
Nitpick comments:
In @.github/scripts/windows_payment_checkout_smoke.ps1:
- Around line 271-279: Update the polling logic around the accumulated $logText
checks so transient navigation_error and document_error events do not
immediately fail redirect-heavy flows. Track or detect a successful load_stop
for the expected checkout host, and only throw the existing WebView errors when
that final target navigation was not successfully reached; keep the checkout
rejected/bootstrap_error failure handling unchanged.
In `@lib/features/auth/choose_payment_method.dart`:
- Around line 501-515: Replace the provider title Semantics override around
lib/features/auth/choose_payment_method.dart#L501-L515 with identifier set to
the automation hook, removing excludeSemantics and the machine label so the
provider Text and logo remain announced. Apply the same change to the checkout
button at lib/features/auth/choose_payment_method.dart#L599-L615, preserving its
localized Subscribe/Checkout label, and update
windows_payment_checkout_smoke.ps1's Find-AutomationElement to query
AutomationIdProperty instead of NameProperty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2072b4ae-cbed-42d2-b247-4fc58d0177c3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.github/scripts/windows_payment_checkout_smoke.ps1.github/scripts/windows_smoke_suite.ps1.github/workflows/app-smoke-tests.yml.github/workflows/build-windows.ymlgo.modlib/core/smoke/payment_checkout_smoke.dartlib/core/widgets/app_webview.dartlib/features/auth/choose_payment_method.dartlib/lantern_app.dartlib/main.darttest/core/smoke/payment_checkout_smoke_test.dart
# Conflicts: # go.mod # go.sum
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
28-28: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix the current dependency failure before relying on this rollback.
With the selected Radiance revision pinned,
go test ./...fails becausegolang.getoutline.org/sdk/x/smartandgithub.com/Jigsaw-Code/outline-sdk/x/smartcannot calllookupCNAME, breaking the module graph. Resolve that dependency issue rather than rolling back Radiance without a working build.🤖 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 `@go.mod` at line 28, Resolve the dependency failure introduced by the pinned Radiance revision in go.mod: ensure the module graph provides compatible versions of golang.getoutline.org/sdk/x/smart and github.com/Jigsaw-Code/outline-sdk/x/smart so their lookupCNAME calls compile and go test ./... succeeds. Keep the Radiance pin only once the selected dependency versions build successfully.
🤖 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.
Outside diff comments:
In `@go.mod`:
- Line 28: Resolve the dependency failure introduced by the pinned Radiance
revision in go.mod: ensure the module graph provides compatible versions of
golang.getoutline.org/sdk/x/smart and github.com/Jigsaw-Code/outline-sdk/x/smart
so their lookupCNAME calls compile and go test ./... succeeds. Keep the Radiance
pin only once the selected dependency versions build successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd5fb566-31f6-48fc-b12a-3eb2f48fab16
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
lib/core/widgets/app_webview.dart:193
PAYMENT_WEBVIEW_SMOKEis emitted unconditionally for every AppWebView navigation. Since this is a smoke-only diagnostic signal, it should be gated (e.g., Windows + nightly) to avoid adding new info-level logging in normal runs and to keep log semantics accurate.
void _logSmokeEvent(String event, Uri? uri, {String detail = ''}) {
// Checkout paths and query strings can contain session tokens. The origin
// is enough to prove which provider loaded without putting them in CI logs.
final safeUri = uri == null
? '<none>'
: uri.replace(path: '', query: '', fragment: '').toString();
final suffix = detail.isEmpty ? '' : ' $detail';
appLogger.info(
'PAYMENT_WEBVIEW_SMOKE event=$event host=${uri?.host ?? '<none>'} '
'url=$safeUri$suffix',
);
}
lib/core/widgets/app_webview.dart:150
- This
document_errorlog line is part of the smoke diagnostics, but it currently bypasses_logSmokeEvent, so it won’t benefit from any gating/redaction logic there. Consider routing it through_logSmokeEvent(and avoid logging the full exception string if it could contain sensitive data).
} catch (error) {
appLogger.error(
'PAYMENT_WEBVIEW_SMOKE event=document_error '
'host=${uri?.host ?? '<none>'} error=$error',
);
}
| if (PlatformUtils.isWindows) { | ||
| appLogger.info( | ||
| 'WEBVIEW2_DIAGNOSTIC user_data_folder=' | ||
| '${Platform.environment['WEBVIEW2_USER_DATA_FOLDER'] ?? '<unset>'} ' | ||
| 'local_app_data=${Platform.environment['LOCALAPPDATA'] ?? '<unset>'}', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
lib/core/widgets/app_webview.dart:170
- onReceivedError stops the global WebView loading state and calls _handleCompletionUrl() even for subresource failures (isForMainFrame != true). That can hide the loading indicator while the main frame is still loading, and it risks treating a failing resource URL as a completion URL. Limit the stop/completion handling to main-frame errors only.
onReceivedError: (_, webResourceRequest, error) async {
appLogger.error("Received error: $error");
final uri = Uri.tryParse(webResourceRequest.url.toString());
_logSmokeEvent(
webResourceRequest.isForMainFrame == true
? 'navigation_error'
: 'resource_error',
uri,
detail: 'error_type=${error.type}',
);
ref.read(webViewLoadingProvider.notifier).stop();
await _handleCompletionUrl(uri);
},
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
lib/features/auth/choose_payment_method.dart:505
- Using excludeSemantics: true here hides the visible provider name/logo semantics and exposes only the technical label (e.g., "payment-provider-stripe") to assistive technologies. That label is useful for UI automation, but it should not replace the user-facing semantics.
title: Semantics(
container: true,
excludeSemantics: true,
label: 'payment-provider-${method.providers.name}',
child: Row(
lib/main.dart:50
- The WEBVIEW2_DIAGNOSTIC log currently prints the WEBVIEW2_USER_DATA_FOLDER environment variable, but the Windows payment checkout smoke script asserts this field equals the effective per-user WebView2 folder (LOCALAPPDATA\Lantern\WebView2 when the env var is unset). With the smoke harness explicitly clearing WEBVIEW2_USER_DATA_FOLDER, this log will be '' and the smoke will fail even when WebView2 storage is correct.
if (PlatformUtils.isWindows) {
appLogger.info(
'WEBVIEW2_DIAGNOSTIC user_data_folder='
'${Platform.environment['WEBVIEW2_USER_DATA_FOLDER'] ?? '<unset>'} '
'local_app_data=${Platform.environment['LOCALAPPDATA'] ?? '<unset>'}',
);
}
lib/core/widgets/app_webview.dart:145
- If evaluateJavascript returns a non-numeric value (but doesn't throw), documentLength becomes -1 without emitting PAYMENT_WEBVIEW_SMOKE event=document_error. The Windows smoke harness looks specifically for document_error (or a positive document_length) and may instead time out with a less-informative failure.
documentLength = rawLength is num
? rawLength.toInt()
: int.tryParse(rawLength.toString()) ?? -1;
} catch (error) {
lib/features/auth/choose_payment_method.dart:603
- excludeSemantics: true on the checkout button wrapper similarly replaces the button's accessible label with a technical identifier ("payment-checkout-…"). This makes screen reader output confusing and can hide the actual button text semantics.
Semantics(
container: true,
excludeSemantics: true,
label: 'payment-checkout-${method.providers.name}',
button: true,
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/scripts/windows_payment_checkout_smoke.ps1 (3)
162-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFail-fast bootstrap detection duplicates the rejection regex from
Wait-CheckoutDocument.The
PAYMENT_CHECKOUT_SMOKE event=(rejected|bootstrap_error)pattern is hardcoded here (line 182) and again inWait-CheckoutDocument(line 286). If the event name changes in one place, detection silently degrades in the other.♻️ Proposed fix: extract shared pattern
+$script:CheckoutRejectionPattern = 'PAYMENT_CHECKOUT_SMOKE event=(rejected|bootstrap_error)' + function Find-AutomationElement { ... if ($FailureLogPath -and (Test-Path $FailureLogPath)) { $failure = Select-String -Path $FailureLogPath ` - -Pattern 'PAYMENT_CHECKOUT_SMOKE event=(rejected|bootstrap_error)' | + -Pattern $script:CheckoutRejectionPattern | Select-Object -Last 1🤖 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 162 - 192, Extract the shared PAYMENT_CHECKOUT_SMOKE rejection/bootstrap-error regex into a single script-level pattern variable or constant, then update both Find-AutomationElement and Wait-CheckoutDocument to reuse it instead of hardcoding the pattern independently.
381-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
$profileto avoid shadowing the automatic$PROFILEvariable.Static analysis flags this twice (lines 381, 383):
$profilecollides with PowerShell's built-in$PROFILEautomatic variable (names are case-insensitive). No functional impact is evident today since$PROFILEisn't otherwise used in this script, but it's a known footgun for future edits in this function.🔧 Proposed rename
- $profile = $null + $userProfile = $null for ($i = 0; $i -lt 15; $i++) { - $profile = Get-CimInstance Win32_UserProfile -Filter "SID='$sid'" ` + $userProfile = Get-CimInstance Win32_UserProfile -Filter "SID='$sid'" ` -ErrorAction SilentlyContinue - if ($profile -and -not [string]::IsNullOrWhiteSpace($profile.LocalPath)) { + if ($userProfile -and -not [string]::IsNullOrWhiteSpace($userProfile.LocalPath)) { break } Start-Sleep -Seconds 1 } - if (-not $profile -or [string]::IsNullOrWhiteSpace($profile.LocalPath)) { + if (-not $userProfile -or [string]::IsNullOrWhiteSpace($userProfile.LocalPath)) { throw "Windows did not create a profile for disposable user $username" } - $profilePath = [Environment]::ExpandEnvironmentVariables($profile.LocalPath) + $profilePath = [Environment]::ExpandEnvironmentVariables($userProfile.LocalPath)🤖 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 381 - 393, Rename the local `$profile` variable used in the profile lookup and validation flow to a distinct name such as `$userProfile`, updating all references through the `Get-CimInstance` loop, null/path checks, and `$profilePath` assignment while preserving the existing behavior.Source: Linters/SAST tools
541-541: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
$flutterLog/$finalFlutterLogrecompute the same hardcoded path twice.Line 541 introduces
$flutterLoginside thetryblock; the unchangedfinallyblock (line 603) independently recomputes the identical path as$finalFlutterLog. Sincetry/finallyshare the function scope,$flutterLogis already visible infinallywhen execution reaches that point in the try block, but the separate variable stays needed as a safety net for failures that occur before line 541 executes. Consider hoisting a single path computation to the top ofInvoke-CheckoutCase(alongside$caseDirectory/$transcript) so both blocks reference the same variable without relying on execution order.Also applies to: 603-608
🤖 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 at line 541, Hoist the Flutter log path computation to the start of Invoke-CheckoutCase alongside $caseDirectory and $transcript, then reuse $flutterLog in both the try block and finally block. Remove the duplicate $finalFlutterLog computation while preserving cleanup when execution fails before the try-block assignment.
🤖 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.
Nitpick comments:
In @.github/scripts/windows_payment_checkout_smoke.ps1:
- Around line 162-192: Extract the shared PAYMENT_CHECKOUT_SMOKE
rejection/bootstrap-error regex into a single script-level pattern variable or
constant, then update both Find-AutomationElement and Wait-CheckoutDocument to
reuse it instead of hardcoding the pattern independently.
- Around line 381-393: Rename the local `$profile` variable used in the profile
lookup and validation flow to a distinct name such as `$userProfile`, updating
all references through the `Get-CimInstance` loop, null/path checks, and
`$profilePath` assignment while preserving the existing behavior.
- Line 541: Hoist the Flutter log path computation to the start of
Invoke-CheckoutCase alongside $caseDirectory and $transcript, then reuse
$flutterLog in both the try block and finally block. Remove the duplicate
$finalFlutterLog computation while preserving cleanup when execution fails
before the try-block assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 472cd754-6b2a-499d-8008-e10ab5d52b2e
📒 Files selected for processing (2)
.github/scripts/windows_payment_checkout_smoke.ps1lib/lantern_app.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/lantern_app.dart
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
lib/core/widgets/app_webview.dart:160
evaluateJavascriptto computedocumentLengthruns on everyonLoadStopacross all platforms/build types, even though the result is only used for Windows nightly smoke logging. This adds unnecessary per-navigation overhead and can introduce extra WebView failures/noise outside the smoke scenario. Gate the JS evaluation behind the same Windows-nightly condition used by_logSmokeEvent.
var documentLength = -1;
try {
final rawLength = await controller.evaluateJavascript(
source:
"(document.documentElement && document.documentElement.outerHTML || '').length",
.github/scripts/windows_payment_checkout_smoke.ps1:292
Wait-CheckoutDocumentonly checks that somePAYMENT_WEBVIEW_SMOKE event=createdappears in the log, not that the created event corresponds to the same host that produced the successfulload_stop. That can let the smoke pass even if the checkout WebView missed the creation marker (e.g., if another WebView was created earlier). Scope the check to the matching host.
if ($hostName -match $HostPattern -and $documentLength -gt 0) {
if ($lastLogText -notmatch 'PAYMENT_WEBVIEW_SMOKE event=created') {
throw "The checkout page loaded without a WebView creation marker"
}
lib/core/smoke/payment_checkout_smoke.dart:54
_singleArgumentbuilds a lazywhere(...)iterable and then readslength,isEmpty, andsingle, which can iterate the underlying list multiple times. Converting to a list once keeps the logic straightforward and avoids repeated scans.
final matches = arguments.where((argument) => argument.startsWith(prefix));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
.github/scripts/windows_payment_checkout_smoke.ps1:309
Wait-CheckoutDocumentcan return success even if the WebView already loggedPAYMENT_WEBVIEW_SMOKE event=navigation_errororevent=document_error(e.g., main-frame HTTP errors may still produce aload_stopwith non-empty HTML). This can mask real checkout failures; treat these error markers as fatal as soon as they appear, before accepting aload_stopmatch.
if ($lastLogText -match 'PAYMENT_CHECKOUT_SMOKE event=(rejected|bootstrap_error)') {
throw "Lantern could not prepare the requested checkout smoke"
}
foreach ($match in [regex]::Matches($lastLogText, $linePattern)) {
$hostName = $match.Groups[1].Value
lib/core/widgets/app_webview.dart:192
onReceivedHttpErrorlogs anavigation_errorsmoke event but does not stop the loading state or run the same completion handling used for other main-frame failures (onReceivedError). If a main-frame HTTP error preventsonLoadStopfrom firing, the loading indicator can remain stuck and the smoke harness may miss the failure.
onReceivedHttpError: (_, webResourceRequest, errorResponse) async {
if (webResourceRequest.isForMainFrame != true) return;
_logSmokeEvent(
'navigation_error',
Uri.tryParse(webResourceRequest.url.toString()),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/core/widgets/app_webview.dart:136
onLoadStopalways evaluates JavaScript to computedocumentLength, even when the Windows nightly smoke logging is disabled. This adds unnecessary overhead for every WebView load (and can still throw/slow down pages) outside the smoke scenario.
Consider short-circuiting early when not running the Windows nightly smoke so the JS evaluation and related logging work only runs when it can actually be used.
final uri = webUri == null ? null : Uri.tryParse(webUri.toString());
var documentLength = -1;
.github/scripts/windows_payment_checkout_smoke.ps1:588
- The transcript is started before generating the disposable user's password. PowerShell transcripts can capture executed lines, so this risks writing the plain-text password into the artifact logs.
Generate the password/credential before starting the transcript (and clear the plain-text variable) to avoid leaking it into logs.
Start-Transcript -Path $transcript -Force | Out-Null
$transcriptStarted = $true
$passwordText = "L@ntern!" + ([Guid]::NewGuid().ToString("N"))
$securePassword = ConvertTo-SecureString $passwordText -AsPlainText -Force
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/scripts/windows_payment_checkout_smoke.ps1:485
- Wait-CheckoutDocument can currently return success as soon as it sees a non-empty
load_stopdocument from the expected host, even if the log already containsPAYMENT_WEBVIEW_SMOKE event=navigation_error/document_error(e.g., a main-frame HTTP error that still produced an HTML error page). This can incorrectly treat a checkout navigation error as a passing run and contradicts the smoke goal of "render without navigation errors".
Consider failing fast inside the polling loop whenever a navigation/document error is observed in the current log snapshot, before accepting load_stop.
if ($lastLogText -match 'PAYMENT_CHECKOUT_SMOKE event=(rejected|bootstrap_error)') {
throw "Lantern could not prepare the requested checkout smoke"
}
foreach ($match in [regex]::Matches($lastLogText, $linePattern)) {
$hostName = $match.Groups[1].Value
$documentLength = [int]$match.Groups[3].Value
if ($hostName -match $HostPattern -and $documentLength -gt 0) {
if ($lastLogText -notmatch 'PAYMENT_WEBVIEW_SMOKE event=created') {
throw "The checkout page loaded without a WebView creation marker"
}
@{
host = $hostName
url = $match.Groups[2].Value
documentLength = $documentLength
} | ConvertTo-Json | Set-Content $ResultPath
return
}
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/scripts/windows_payment_checkout_smoke.ps1:744
- The orchestrator treats
process.json'sisAdminflag as an error, but that flag is computed fromIsInRole(Administrator)in the launcher and can indicate group membership rather than an elevated token. Given the smoke user is intentionally added to Administrators for service access, this can incorrectly fail otherwise-non-elevated runs. Consider downgrading this to a warning (or removing it) and rely on theprocessIsElevatedcheck instead.
$diagnostics = Get-Content $diagnosticsPath -Raw | ConvertFrom-Json
if ($diagnostics.isAdmin) {
throw "The installed app was launched with an administrator token"
}
.github/scripts/windows_payment_checkout_smoke.ps1:476
$isAdminis derived fromWindowsPrincipal.IsInRole(Administrator), which reflects Administrators-group membership (and can be true even when the process is running with a filtered/non-elevated UAC token). Since the script explicitly adds the disposable user to the Administrators group earlier, this check can cause false failures; the elevation check should rely on$processIsElevated(TokenElevation) instead.
This issue also appears on line 741 of the same file.
$identity = [Security.Principal.WindowsIdentity]::GetCurrent()
$principal = [Security.Principal.WindowsPrincipal]::new($identity)
$isAdmin = $principal.IsInRole(
[Security.Principal.WindowsBuiltInRole]::Administrator
)
$installWritePath = Join-Path (Split-Path $AppPath -Parent) (
"smoke-write-" + [Guid]::NewGuid().ToString("N")
)
$canWriteInstallDirectory = $false
try {
New-Item -ItemType File -Path $installWritePath -Force `
-ErrorAction Stop | Out-Null
$canWriteInstallDirectory = $true
} catch {
} finally {
Remove-Item $installWritePath -Force -ErrorAction SilentlyContinue
}
$localAppData = $env:LOCALAPPDATA
$externalUDF = [Environment]::GetEnvironmentVariable(
"WEBVIEW2_USER_DATA_FOLDER",
"Process"
)
$app = Start-Process -FilePath $AppPath -ArgumentList @(
"--payment-checkout-smoke=$CheckoutProvider",
"--payment-checkout-run-id=$CheckoutRunID"
) -PassThru
@{
processId = $app.Id
username = $identity.Name
isAdmin = $isAdmin
canWriteInstallDirectory = $canWriteInstallDirectory
externalWebView2UserDataFolder = $externalUDF
localAppData = $localAppData
profilePath = $UserProfilePath
} | ConvertTo-Json | Set-Content $ProcessDiagnosticsPath
$app = Wait-ProcessMainWindow -ProcessID $app.Id -TimeoutSeconds 90
$flutterViewHandle = Wait-FlutterViewHandle `
-ParentHandle $app.MainWindowHandle -TimeoutSeconds 30
$processIsElevated = [WindowsPaymentSmoke.NativeToken]::IsElevated($app.Id)
$diagnostics = Get-Content $ProcessDiagnosticsPath -Raw | ConvertFrom-Json
$diagnostics | Add-Member -NotePropertyName processIsElevated `
-NotePropertyValue $processIsElevated
$diagnostics | Add-Member -NotePropertyName mainWindowHandle `
-NotePropertyValue ([IntPtr]$app.MainWindowHandle).ToInt64()
$diagnostics | Add-Member -NotePropertyName flutterViewHandle `
-NotePropertyValue $flutterViewHandle.ToInt64()
$diagnostics | ConvertTo-Json | Set-Content $ProcessDiagnosticsPath
if ($isAdmin -or $processIsElevated) {
throw "The installed Lantern process has an elevated access token"
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
.github/scripts/windows_payment_checkout_smoke.ps1:554
- This re-reads
WEBVIEW2_USER_DATA_FOLDERafter it was cleared above, which overwrites the inherited value you want to record inprocess.json. After capturing the inherited value earlier, avoid reassigning$externalUDFhere.
$localAppData = $env:LOCALAPPDATA
$externalUDF = [Environment]::GetEnvironmentVariable(
"WEBVIEW2_USER_DATA_FOLDER",
"Process"
)
lib/core/widgets/app_webview.dart:192
onReceivedHttpErrorlogs a main-frame navigation error but never stops the loading notifier or invokes_handleCompletionUrl, which can leave the WebView in a stuck loading state and skip existing completion handling logic for failures.
onReceivedHttpError: (_, webResourceRequest, errorResponse) async {
if (webResourceRequest.isForMainFrame != true) return;
_logSmokeEvent(
'navigation_error',
Uri.tryParse(webResourceRequest.url.toString()),
.github/scripts/windows_payment_checkout_smoke.ps1:530
$externalUDFis read afterWEBVIEW2_USER_DATA_FOLDERhas already been cleared, so the diagnostics/validation around inherited overrides will always see an empty value. Capture the inherited value before clearing it.
This issue also appears on line 550 of the same file.
[Environment]::SetEnvironmentVariable(
"WEBVIEW2_USER_DATA_FOLDER",
$null,
[EnvironmentVariableTarget]::Process
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/core/widgets/app_webview.dart:205
_logSmokeEventintends to log only the origin, buturi.replace(path: '', query: '', fragment: '')keeps an empty query/fragment as?/#in the serialized URL. That’s noisy and can defeat strict log parsing/sanitization expectations; prefer clearing them withnullso they are omitted entirely.
// Checkout paths and query strings can contain session tokens. The origin
// is enough to prove which provider loaded without putting them in CI logs.
final safeUri = uri == null
? '<none>'
: uri.replace(path: '', query: '', fragment: '').toString();
final suffix = detail.isEmpty ? '' : ' $detail';
lib/features/auth/choose_payment_method.dart:613
- Wrapping
PrimaryButtoninSemantics(excludeSemantics: true, ...)strips the button’s built-in semantics (role/actions). This can make the control non-operable or not announced as a button by assistive tech. Keep the automation identifier/name, but explicitly mark it as a button and provide anonTapaction (or stop excluding child semantics).
Semantics(
identifier: 'payment-checkout-${method.providers.name}',
label: 'Continue with $providerName',
excludeSemantics: true,
child: PrimaryButton(
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
lib/core/widgets/app_webview.dart:137
evaluateJavascriptand document-length parsing run unconditionally on everyonLoadStop, even when smoke logging is disabled (non-Windows / non-nightly). This adds unnecessary overhead (and potential failures) to all in-app webviews; it should be gated to the same condition as_logSmokeEvent.
onLoadStop: (controller, webUri) async {
ref.read(webViewLoadingProvider.notifier).stop();
final uri = webUri == null ? null : Uri.tryParse(webUri.toString());
var documentLength = -1;
try {
lib/features/auth/choose_payment_method.dart:613
- Wrapping
PrimaryButtoninSemantics(excludeSemantics: true)removes the button's built-in semantics (role + activation action), which can break screen-reader interaction and makes the element harder to activate via accessibility APIs. If you need a stable label/identifier for automation, re-add button semantics + onTap at the wrapper level.
Semantics(
identifier: 'payment-checkout-${method.providers.name}',
label: 'Continue with $providerName',
excludeSemantics: true,
child: PrimaryButton(
label: method.providers.supportSubscription
? 'subscribe'.i18n
: 'checkout'.i18n,
enabled: !isSubmitting,
onPressed: () {
onSubscribe.call(method);
},
),
),
| final safeUri = uri == null | ||
| ? '<none>' | ||
| : uri.replace(path: '', query: '', fragment: '').toString(); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/core/widgets/app_webview.dart:136
onLoadStopalways callsevaluateJavascriptto compute document length, even when the smoke logger is disabled (non-Windows or non-nightly builds). This adds unnecessary work (and potential failure noise) to every in-app webview navigation outside the smoke flow.
Gate the document-length probe behind the same Windows-nightly condition and early-return after _handleCompletionUrl when the smoke mode is not active.
final uri = webUri == null ? null : Uri.tryParse(webUri.toString());
var documentLength = -1;
lib/features/auth/choose_payment_method.dart:613
- Wrapping
PrimaryButtonwithSemantics(excludeSemantics: true)removes the button's semantics (role/action) unless the wrapper supplies them. As written, assistive technologies may not treat this as an actionable button, and activation via accessibility actions can stop working.
If you need a stable MSAA name, keep the wrapper but add the button role + tap action (and only expose the wrapper semantics).
Semantics(
identifier: 'payment-checkout-${method.providers.name}',
label: 'Continue with $providerName',
excludeSemantics: true,
child: PrimaryButton(
Resolves https://github.com/getlantern/engineering/issues/3723
Install and launch Lantern as a Windows user, verify per-user WebView2 storage, and confirm Stripe and Shepherd checkout pages render without navigation errors. Upload complete diagnostics on every run
Summary by CodeRabbit