test(swift-sdk): add wallet UI smoke flow#3550
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request adds comprehensive UI testing infrastructure for the Swift example app by introducing a new CI workflow for automated UI smoke testing, adding accessibility identifiers to key UI elements across multiple views, and implementing a detailed end-to-end UI test case that covers wallet creation and deletion flows. A functional enhancement to wallet deletion ensures mnemonic cleanup from keychain storage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Review GateCommit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift (1)
532-555:⚠️ Potential issue | 🟡 MinorMnemonic may be deleted while the wallet row persists if
modelContext.save()fails.
try? modelContext.save()at line 545 silently discards errors. If the save fails (e.g., merge conflict, store error), thePersistentWalletrow stays in the database, but execution proceeds toWalletStorage().deleteMnemonic(for: walletId), leaving a wallet that can no longer reveal its seed phrase or be re-derived. Worse, the UI has already been dismissed viaonWalletDeleted(), so the user has no signal that anything went wrong.Consider keying mnemonic deletion off a successful save and surfacing failures to the user, e.g.:
🛡️ Proposed fix
private func deleteWallet() async { let walletId = wallet.walletId - // Dismiss views FIRST to prevent UI from accessing deleted - // relationships, then delete the `PersistentWallet` row. - // Cascade-delete rules on `accounts` / `identities` null out - // or cascade the children automatically. - await MainActor.run { - dismiss() - onWalletDeleted() - } - modelContext.delete(wallet) - try? modelContext.save() do { + try modelContext.save() try WalletStorage().deleteMnemonic(for: walletId) } catch { SDKLogger.error( - "Failed to delete wallet mnemonic from keychain: \(error.localizedDescription)" + "Failed to fully delete wallet: \(error.localizedDescription)" ) + await MainActor.run { + errorMessage = "Failed to delete wallet: \(error.localizedDescription)" + showError = true + } + return + } + await MainActor.run { + dismiss() + onWalletDeleted() } // TODO(platform-wallet): expose wallet removal on PlatformWalletManager // so the Rust side also drops the in-memory handle. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift` around lines 532 - 555, In deleteWallet(), don't silently ignore modelContext.save() — perform a do/try/catch around modelContext.save() and only call WalletStorage().deleteMnemonic(for: walletId) and onWalletDeleted()/dismiss() after the save succeeds; if save throws, surface the error to the UI (via an error callback, alert, or by not calling dismiss()/onWalletDeleted()) so the PersistentWallet row isn't left without its mnemonic, and log the failure; update references: deleteWallet(), modelContext.save(), WalletStorage().deleteMnemonic(for:), onWalletDeleted(), and dismiss() accordingly.
🧹 Nitpick comments (7)
.github/workflows/swift-example-app-ui-smoke.yml (3)
34-44: Overlapping cache step + redundantactions/cache/save@v4.
actions/cache@v4performs both restore and save (via the post-step). Pairing it with a separateactions/cache/save@v4for the same key risks redundant work and can race on cache-key uniqueness. Either:
- Use
actions/cache@v4alone (drop the explicit save step), or- Use
actions/cache/restore@v4+actions/cache/save@v4paired (don't mix the combined action with the separate save).Also, the cargo cache step at lines 34–40 has no
restore-keysfallback, so any change toCargo.lockproduces a 100% miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/swift-example-app-ui-smoke.yml around lines 34 - 44, The workflow currently uses the combined actions/cache@v4 step named "Cache cargo registry" alongside an explicit actions/cache/save@v4 which causes redundant saves and potential race conditions; remove the separate actions/cache/save@v4 step (or alternatively replace the combined action with a restore/save pair) so only one caching mechanism handles save/restore for the same key, and add a restore-keys fallback to the "Cache cargo registry" step (e.g., include a restore-keys entry alongside key) to avoid 100% misses when Cargo.lock changes; locate the steps by the step name "Cache cargo registry" and the explicit "actions/cache/save@v4" usage to make the change.
21-25:git clean -ffdxafteractions/checkout@v4 with: clean: trueis a heavy-handed reset that can wipe legitimate runner state.On a self-hosted runner,
git clean -ffdxdeletes all untracked & ignored files in the working tree — including any pre-existing local caches checked-out actions might rely on. Since the checkout step already requestedclean: true, this extra step provides little additional benefit at the cost of slower builds and lost caches. Recommend dropping it unless there is a specific reproduction of dirty state on this runner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/swift-example-app-ui-smoke.yml around lines 21 - 25, Remove the aggressive git clean invocation in the "Force clean working directory" step: delete the git clean -ffdx command (which can wipe legitimate runner state) and keep only the safer reset (git reset --hard HEAD) or remove the whole step if actions/checkout@v4 already uses clean: true; update the step titled "Force clean working directory" to omit git clean -ffdx and rely on the checkout clean option or a lighter reset.
117-143: Validate device type compatibility against the selected runtime.The script selects the newest iOS runtime, then independently selects an iPhone device type. The fallback logic at lines 130–131 accepts the first matching device without checking if it's supported by the chosen runtime—
simctl createmay fail at line 135–143 with unclear diagnostics. Additionally, the first fallback will match "iPhone 4s" if listed before newer models due to the linear walk without version sorting.Use
runtime["supportedDeviceTypes"](provided bysimctl list runtimes --json) to filter the device selection to only compatible types:♻️ Suggested change
supported_ids = runtime.fetch("supportedDeviceTypes", []).map { |dt| dt["identifier"] }.to_set candidates = device_types.select { |dt| supported_ids.empty? || supported_ids.include?(dt["identifier"]) } device = preferred_names .map { |name| candidates.find { |item| item["name"] == name } } .compact .first device ||= candidates.find { |item| item["name"].start_with?("iPhone") && !item["name"].include?("SE") } abort "No compatible iPhone simulator device type for runtime #{runtime["name"]}" unless device🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/swift-example-app-ui-smoke.yml around lines 117 - 143, The device selection currently uses device_types without ensuring compatibility with the chosen runtime (runtime), so simctl create can fail; update selection to first build a set of supported device identifiers from runtime.fetch("supportedDeviceTypes", []) (e.g. supported_ids) and filter device_types into candidates = device_types.select { |dt| supported_ids.empty? || supported_ids.include?(dt["identifier"]) }, then replace uses of device_types in the preferred_names mapping and fallbacks with candidates (and update the abort message to "No compatible iPhone simulator device type for runtime #{runtime["name"]}" to clarify the failure).packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/SwiftExampleAppUITests.swift (2)
217-227: Coordinate math hard-codes 5 tab bar items and ignores tab bar safe-area insets.
tapTabBarItem(index: 1, itemCount: 5, ...)is a coordinate-based fallback that breaks the moment a tab is added or removed (5 already matchesRootTabexactly, but it's a silent coupling). It also computes the tap point relative totabBar.frame, which on iPhones with a home indicator places the visual tab buttons above the bottom of the bar — the math still tends to land within the button, but it's fragile.Since the previous branches already try
accessibilityIdentifierandapp.tabBars.buttons["Wallets"], consider promoting one of those to "must succeed" rather than masking failures with a coordinate tap. If the dedicated identifier fromContentView.swiftis fixed to actually surface (see review there), this fallback can be removed entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/SwiftExampleAppUITests.swift` around lines 217 - 227, The fallback coordinate tap in tapTabBarItem should be removed and replaced by a deterministic tap on the tab bar button itself: stop hard-coding itemCount/coordinate math and instead locate the target via the tab button accessibility identifier or app.tabBars.buttons.element(boundBy: index), assert its existence (XCTAssertTrue) and call .tap(); if you expect identifiers from ContentView.swift, make tapping by accessibilityIdentifier the required path (promote app.tabBars.buttons["Wallets"] / the identifier lookup to the primary action) and remove the coordinate-based calculation so tests no longer depend on a 5-item layout or on fragile frame math that ignores safe-area insets.
285-302: PreferXCTNSPredicateExpectation/wait(for:timeout:)over hand-rolled RunLoop spinning.
wait(timeout:until:)polls every 100 ms viaRunLoop.current.run(until:), which is correct but reinvents what XCTest provides natively.XCTNSPredicateExpectationis more idiomatic, integrates with the test reporter (showing the predicate that timed out), and is awaitable onMainActor. Same goes forwaitForElementToBeEnabledandwaitForNonExistence— both can be expressed as predicate expectations onXCUIElement.exists/.isEnabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/SwiftExampleAppUITests.swift` around lines 285 - 302, The custom spinner wait(timeout:until:) (used by waitForElementToBeEnabled and waitForNonExistence) should be replaced with XCTest predicate expectations: create an NSPredicate for the target condition (e.g. NSPredicate(format: "exists == true") or NSPredicate(format: "isEnabled == true") or the inverse for non-existence), wrap the XCUIElement in an XCTNSPredicateExpectation, then call wait(for: [expectation], timeout:). Keep isSwitchOn(_:) as-is for reading switch value but stop using RunLoop polling—update waitForElementToBeEnabled, waitForNonExistence and any callers that use wait(timeout:until:) to use XCTNSPredicateExpectation and wait(for:timeout:) on the MainActor so XCTest integrates with test reporting and timeouts.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletsContentView.swift (1)
51-58: Wallet-row identifier built from a user-controlled label.
accessibilityIdentifier("wallets.walletRow.\(wallet.label)")is sourced from the user-editable wallet name, so:
- Two wallets with the same label collide on the same identifier (the test's
firstMatchwould silently target whichever row XCUITest picks first).- Renaming the wallet (via Wallet Info → Edit) breaks any test that captured a row before the rename.
- An empty
namefalls back to a hex fingerprint via the computedlabel, so the identifier becomes the hex string — fine functionally but couples the test to a UI detail.For the new smoke test it's adequate because the test always uses a fresh
UUID-suffixed name, but consider a stable-by-id identifier and exposing the label separately as the accessibility label, e.g.:♻️ Suggested change
ForEach(wallets) { wallet in NavigationLink { WalletDetailView(wallet: wallet) } label: { WalletRowView(wallet: wallet) } - .accessibilityIdentifier("wallets.walletRow.\(wallet.label)") + .accessibilityIdentifier("wallets.walletRow.\(wallet.walletId.toHexString())") }The UI test would then locate rows via the wallet id captured at creation time, or via a label-based query (
app.staticTexts[walletName]) for screen verification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletsContentView.swift` around lines 51 - 58, The accessibility identifier for each wallet row currently uses the user-editable wallet.label which can collide or break tests; update WalletsContentView so the NavigationLink/row sets a stable identifier using the wallet's unique id (e.g., "wallets.walletRow.\(wallet.id)") and move the human-readable wallet.label into the accessibilityLabel (or a separate staticText) so UI tests can reliably query rows by id while still asserting visible labels via accessibilityLabel or staticText; adjust references to WalletRowView and WalletDetailView to preserve existing behavior.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swift (1)
80-85: Move accessibility identifier to parent view for proper tab bar button identification.
.accessibilityIdentifier("rootTab.wallets")on theLabelinside.tabItem { ... }will not propagate to the tab bar button in XCUITest. SwiftUI's tab item renders a system tab bar button, and identifiers on inner elements are not surfaced as the button's identifier. The test inSwiftExampleAppUITests.swiftalready works around this with fallbacks (app.tabBars.buttons["Wallets"]and coordinate-based taps), but applying the identifier to the parent view makes the selector more reliable:♻️ Suggested change
WalletsTabView() + .accessibilityIdentifier("rootTab.wallets") .tabItem { Label("Wallets", systemImage: "wallet.pass") - .accessibilityIdentifier("rootTab.wallets") } .tag(RootTab.wallets)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swift` around lines 80 - 85, The accessibility identifier is currently applied to the Label inside the .tabItem (Label("Wallets", systemImage: "wallet.pass")), which doesn't propagate to the rendered tab bar button; move the .accessibilityIdentifier("rootTab.wallets") call from the Label and attach it to the parent view that defines the tab item (the WalletsTabView() instance that has .tabItem { ... } and .tag(RootTab.wallets)) so the identifier is set on the tab's parent and is discoverable by XCUITest as the tab bar button.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/swift-example-app-ui-smoke.yml:
- Around line 59-72: The "Install Protobuf if cache miss" step exposes
GITHUB_TOKEN via set -euxo pipefail and uses an unnecessary Authorization header
for public protobuf releases; remove the env: GITHUB_TOKEN block and drop the -H
"Authorization: token ${GITHUB_TOKEN}" from the curl invocation in that step (or
alternatively avoid tracing only that curl command by not using -x there) so the
token is not referenced or echoed by set -x; update the run block that sets
VERSION/OS/PROTOC_DIR and performs curl/unzip accordingly in the step.
- Around line 50-81: The cache paths use the workflow env context (${ { env.HOME
} }) which is empty on runners; update the "Restore cached Protobuf" (id:
cache-protoc) and "Save cached Protobuf" steps to use a runner-expanded home
path (e.g. replace `${{ env.HOME }}/.local/protoc-32.0/...` with
`~/.local/protoc-32.0/...` or an absolute
`/Users/runner/.local/protoc-32.0/...`) so the cache path matches where the
install step writes (the install step already uses "$HOME" shell expansion and
PROTOC_DIR); keep the same key and ensure both restore and save use the
identical corrected paths.
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- Around line 532-555: In deleteWallet(), don't silently ignore
modelContext.save() — perform a do/try/catch around modelContext.save() and only
call WalletStorage().deleteMnemonic(for: walletId) and
onWalletDeleted()/dismiss() after the save succeeds; if save throws, surface the
error to the UI (via an error callback, alert, or by not calling
dismiss()/onWalletDeleted()) so the PersistentWallet row isn't left without its
mnemonic, and log the failure; update references: deleteWallet(),
modelContext.save(), WalletStorage().deleteMnemonic(for:), onWalletDeleted(),
and dismiss() accordingly.
---
Nitpick comments:
In @.github/workflows/swift-example-app-ui-smoke.yml:
- Around line 34-44: The workflow currently uses the combined actions/cache@v4
step named "Cache cargo registry" alongside an explicit actions/cache/save@v4
which causes redundant saves and potential race conditions; remove the separate
actions/cache/save@v4 step (or alternatively replace the combined action with a
restore/save pair) so only one caching mechanism handles save/restore for the
same key, and add a restore-keys fallback to the "Cache cargo registry" step
(e.g., include a restore-keys entry alongside key) to avoid 100% misses when
Cargo.lock changes; locate the steps by the step name "Cache cargo registry" and
the explicit "actions/cache/save@v4" usage to make the change.
- Around line 21-25: Remove the aggressive git clean invocation in the "Force
clean working directory" step: delete the git clean -ffdx command (which can
wipe legitimate runner state) and keep only the safer reset (git reset --hard
HEAD) or remove the whole step if actions/checkout@v4 already uses clean: true;
update the step titled "Force clean working directory" to omit git clean -ffdx
and rely on the checkout clean option or a lighter reset.
- Around line 117-143: The device selection currently uses device_types without
ensuring compatibility with the chosen runtime (runtime), so simctl create can
fail; update selection to first build a set of supported device identifiers from
runtime.fetch("supportedDeviceTypes", []) (e.g. supported_ids) and filter
device_types into candidates = device_types.select { |dt| supported_ids.empty?
|| supported_ids.include?(dt["identifier"]) }, then replace uses of device_types
in the preferred_names mapping and fallbacks with candidates (and update the
abort message to "No compatible iPhone simulator device type for runtime
#{runtime["name"]}" to clarify the failure).
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swift`:
- Around line 80-85: The accessibility identifier is currently applied to the
Label inside the .tabItem (Label("Wallets", systemImage: "wallet.pass")), which
doesn't propagate to the rendered tab bar button; move the
.accessibilityIdentifier("rootTab.wallets") call from the Label and attach it to
the parent view that defines the tab item (the WalletsTabView() instance that
has .tabItem { ... } and .tag(RootTab.wallets)) so the identifier is set on the
tab's parent and is discoverable by XCUITest as the tab bar button.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletsContentView.swift`:
- Around line 51-58: The accessibility identifier for each wallet row currently
uses the user-editable wallet.label which can collide or break tests; update
WalletsContentView so the NavigationLink/row sets a stable identifier using the
wallet's unique id (e.g., "wallets.walletRow.\(wallet.id)") and move the
human-readable wallet.label into the accessibilityLabel (or a separate
staticText) so UI tests can reliably query rows by id while still asserting
visible labels via accessibilityLabel or staticText; adjust references to
WalletRowView and WalletDetailView to preserve existing behavior.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/SwiftExampleAppUITests.swift`:
- Around line 217-227: The fallback coordinate tap in tapTabBarItem should be
removed and replaced by a deterministic tap on the tab bar button itself: stop
hard-coding itemCount/coordinate math and instead locate the target via the tab
button accessibility identifier or app.tabBars.buttons.element(boundBy: index),
assert its existence (XCTAssertTrue) and call .tap(); if you expect identifiers
from ContentView.swift, make tapping by accessibilityIdentifier the required
path (promote app.tabBars.buttons["Wallets"] / the identifier lookup to the
primary action) and remove the coordinate-based calculation so tests no longer
depend on a 5-item layout or on fragile frame math that ignores safe-area
insets.
- Around line 285-302: The custom spinner wait(timeout:until:) (used by
waitForElementToBeEnabled and waitForNonExistence) should be replaced with
XCTest predicate expectations: create an NSPredicate for the target condition
(e.g. NSPredicate(format: "exists == true") or NSPredicate(format: "isEnabled ==
true") or the inverse for non-existence), wrap the XCUIElement in an
XCTNSPredicateExpectation, then call wait(for: [expectation], timeout:). Keep
isSwitchOn(_:) as-is for reading switch value but stop using RunLoop
polling—update waitForElementToBeEnabled, waitForNonExistence and any callers
that use wait(timeout:until:) to use XCTNSPredicateExpectation and
wait(for:timeout:) on the MainActor so XCTest integrates with test reporting and
timeouts.
🪄 Autofix (Beta)
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
Run ID: ea1f8652-c078-4185-8dca-955f9805898a
📒 Files selected for processing (8)
.github/workflows/swift-example-app-ui-smoke.ymlpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp.xcodeproj/xcshareddata/xcschemes/SwiftExampleApp.xcschemepackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SeedBackupView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletsContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/SwiftExampleAppUITests.swift
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "9132cf415c166c231bfc5bb5ad993f33d9059ce74e6408297d3069a419a8abc2"
)Xcode manual integration:
|
3 participants
Summary
Validation
Notes
Summary by CodeRabbit
Testing
Bug Fixes
Accessibility