refactor(ui): migrate protected sheets to DashUIKit - #1075
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDashUIKit is pinned to a fixed revision. Marketplace, CoinJoin, Evonode withdrawal, internal transfer, and send confirmation sheets now use ChangesBottomSheet migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change centralizes protected sheet dismissal behavior while preserving callbacks and detents; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5 files. (1 skipped: 1 unsupported.) ✨ 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 |
|
@coderabbitai review 🤖 Posted autonomously by Codex on behalf of pasta. |
|
✅ Action performedReview finished.
|
b5273c4 to
bb34895
Compare
a0e4c38 to
9198db9
Compare
|
@coderabbitai review\n\n---\n🤖 Posted autonomously by Codex on behalf of pasta. |
|
|
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
The migration itself is wired correctly — I traced all five onClose: handlers and each one really does dismiss (confirmation = nil, showConfirm = false, showConfirmation = false, pendingShieldedRecovery = nil, showCoinJoinMoveFundsSheet = false), so there is no dead X. The problems are the pinned revision, and the fact that the newly-added X is wired to one handler for every phase.
The pin is wrong in both directions
e8d9243 is the first commit of DashUIKit#14's branch, not its merged result. Three commits from that same branch are excluded — b8f3159 (separate the close button from interactive dismissal), ab9eac1 (block the swipe on iOS 14), and 8097d16 (fix: keep the sheet's identity when dismissal flips).
So the PR is written against semantics that no longer exist upstream, and both states are broken:
Staying on e8d9243 means shipping without 8097d16. That commit exists because BottomSheetDismissalModifier branched on isEnabled inside a @ViewBuilder, producing _ConditionalContent; flipping isDismissalEnabled at runtime swaps branches and SwiftUI tears the subtree down and rebuilds it. Flipping that flag at runtime is exactly what this PR does, on five money-moving sheets, at the moment signing starts.
Moving the pin to master silently defeats the protection this PR is built to add. The merge changed the contract:
// e8d9243 — the pinned revision // master (0fe7fcd and later)
guard isEnabled else { return } if let onClose { onClose() }
if let onClose { onClose() } else if isDismissalEnabled { dismiss() }
else { dismiss() }and added isCloseButtonEnabled, with the button active when isCloseButtonEnabled && (isDismissalEnabled || hasCustomCloseAction). Every call site here passes onClose:, so hasCustomCloseAction is true and isCloseButtonEnabled defaults to true — the X stays tappable during signing, locking, proving and broadcasting, and fires onCancel. The PR body describes the pin as temporary ("while that dependency is under review"), but #14 is already merged, so lifting it is a one-line change that is unlikely to prompt a re-review of all five sites.
The fix is both halves together: track master, and pass isCloseButtonEnabled: !isInFlight at each site. Note master has moved again since — DashUIKit#16 merged as 83cf65a.
The X ignores the sheet's phase
onClose: takes a single handler, but these sheets have phases whose own buttons call something different. In .success and .submittedUnconfirmed the button calls onCompleted; the X calls onCancel. Verified concretely in SendScreen and EvonodeWithdrawalScreen — details inline.
Smaller
Package.resolved changes the requirement kind from branch to revision but leaves originHash at its previous value. If SwiftPM treats it as stale it re-resolves and rewrites the lock, so the intended pin can drift; a CI job resolving with automatic resolution disabled would fail on the mismatch instead.
All five migrated bodies still end in ButtonsGroup/DashButton with .padding(.bottom, 16), calibrated for the old safe-area-respecting VStack. BottomSheet(fillsHeight: true) applies edgesIgnoringSafeArea(.bottom) — which the base commit b8cf2aa9 in this very stack documents — so those buttons now sit in the home-indicator strip. I assessed that from the layout rather than on a device; worth a look on hardware.
🤖 Reviewed with Claude Code
| branch = master; | ||
| kind = branch; | ||
| kind = revision; | ||
| revision = e8d92434bfc28fbf933b896cd40a01dd61835b5f; |
There was a problem hiding this comment.
This is an intermediate commit of DashUIKit#14, not its merged result — b8f3159, ab9eac1 and 8097d16 are all on that branch and all excluded.
The one that matters most is 8097d16, whose message describes the exact failure this PR would ship:
BottomSheetDismissalModifierbranched onisEnabledinside a@ViewBuilder[…] FlippingisDismissalEnabledat runtime — the use the API is built for, locking the sheet while signing or broadcasting and unlocking it afterwards — swapped branches, and SwiftUI answers that by tearing the subtree down and building the other one from scratch.
And lifting the pin to master is not a safe no-op either: the merge made perform call onClose unconditionally and introduced isCloseButtonEnabled, so the X would become live during the protected phases (see the review body).
Please track master (83cf65a after DashUIKit#16) and add isCloseButtonEnabled: !isInFlight to each of the five call sites.
| title: NSLocalizedString("Confirm", comment: ""), | ||
| showBackButton: .constant(false), | ||
| isDismissalEnabled: .constant(!isInFlight), | ||
| onClose: onCancel |
There was a problem hiding this comment.
onClose is wired to onCancel for every phase, but the sheet's own buttons are not.
In .success the button is action: onCompleted (line 839) and in .submittedUnconfirmed it is onDone: onCompleted (line 739). The host defines them differently:
onCancel: { showConfirm = false }
onCompleted: { showConfirm = false; onSendCompleted() }So after a successful send, tapping the newly-added X closes the sheet without onSendCompleted() — the user lands back on the amount screen with the amount still filled in, which is an easy accidental second send. Route close per phase, or make the success phases hide the X.
| title: NSLocalizedString("Confirm withdrawal", comment: "Evonode withdrawal"), | ||
| showBackButton: .constant(false), | ||
| isDismissalEnabled: .constant(!(isInFlight || isUnconfirmed)), | ||
| onClose: onCancel |
There was a problem hiding this comment.
Same phase mismatch, and here the completion handler does more than dismiss:
onCancel: { showConfirmation = false }
onCompleted: { remaining in showConfirmation = false; onWithdrawn(remaining); dismiss() }Tapping the X on the success screen runs onCancel, so onWithdrawn(remaining) never fires and the withdrawal screen stays up showing the stale pre-withdrawal claimable balance. onUnconfirmedAcknowledged is skipped the same way.
| title: NSLocalizedString("Confirm", comment: ""), | ||
| showBackButton: .constant(false), | ||
| isDismissalEnabled: .constant(!isInFlight), | ||
| onClose: onCancel |
There was a problem hiding this comment.
Same shape: in .success / .submittedUnconfirmed the X runs onCancel and skips onCompleted(), so the transfer screen is never told the transfer finished.
Lower impact than the other two because onCancel here is { confirmation = nil } and the screen refreshes anyway — but the X makes it the obvious exit, where previously only a swipe could reach this state.
| } else { | ||
| DashUIKit.BottomSheet.selfSizing( | ||
| showBackButton: .constant(false), | ||
| isDismissalEnabled: .constant(!viewModel.isPerformingAction), |
There was a problem hiding this comment.
Concrete consequence of the missing 8097d16 on the pinned revision: tapping Register/Request sets isPerformingAction = true, which flips isDismissalEnabled and — at e8d9243 — swaps _ConditionalContent branches, so SwiftUI rebuilds RegisterNameSheet from scratch.
Its @State goes with it: precheck and voteState reset to nil, the .task re-fires contestPrecheck/contestState, and the contest cards blank back to loading exactly while the action is running — then rebuild again when it finishes.
The four transfer sheets take the same rebuild on every start and end of a transfer; this one is just the easiest to observe.
| title: NSLocalizedString("Move your mixed coins", comment: "CoinJoin"), | ||
| showBackButton: .constant(false), | ||
| isDismissalEnabled: .constant(!viewModel.isInFlight), | ||
| onClose: onDismiss |
There was a problem hiding this comment.
This is the only migrated sheet whose initial detent is .medium (HomeView.swift:502, [.medium, .large]).
The BottomSheet chrome is 82pt (18pt grabber + 64pt navigation bar) against roughly 51pt for the handle and title it replaces, and choiceBody is a plain non-scrolling VStack — amount, explainer, two destination cards, "Later". The extra ~31pt comes out of the bottom, so on a shorter device the "Later" button can clip, and unlike the .large hosts there is no scroll fallback.
Worth either an explicit detent that accounts for the chrome, or making the body scrollable — b8cf2aa9 in this stack took the scroll route for TransferTimingSheet for the same reason.
DashUIKit#14 is merged, so pin back to `branch = master` (resolved at `83cf65a`, which also carries #16) instead of `e8d9243`. That commit was the first of #14's branch and predates three of its own follow-ups — notably `8097d16`, which stops `BottomSheetDismissalModifier` from branching on `isEnabled` inside a `@ViewBuilder`. Without it, flipping `isDismissalEnabled` swaps `_ConditionalContent` branches and SwiftUI rebuilds the sheet's content from scratch — every time a transfer starts and ends. Tracking master changes the dismissal contract, so the call sites have to follow: - `perform` now calls `onClose` unconditionally, and the button is live when `isCloseButtonEnabled && (isDismissalEnabled || hasCustomCloseAction)`. Every sheet here passes `onClose`, so without `isCloseButtonEnabled` the X would stay tappable through signing, locking, proving and broadcasting. Each site now mirrors its own `isDismissalEnabled` condition. - `onClose` took a single handler while these sheets have phases whose own buttons call something else. Route close per phase: on success the send and internal-transfer sheets run `onCompleted` (which unwinds the flow) and the evonode sheet runs `onCompleted(remaining)` (which reports the new balance), rather than `onCancel`, which only closes the sheet. `UsernameMarketplaceScreen` passes no `onClose`, so its close button is already gated by `isDismissalEnabled` alone and needs no change. Also make `CoinJoinMoveFundsSheet`'s choice body scrollable: it is the only migrated sheet presented at `.medium`, and the 82pt of chrome came out of the bottom, where "Later" — the only exit that records the deferral — sits.
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
All findings from my earlier review are addressed in 4a627bda0, which I pushed to this branch.
- The pin is back to
branch = master, resolved at83cf65a(master tip, which also carries DashUIKit#16). That brings in8097d16, so flippingisDismissalEnabledno longer swaps_ConditionalContentbranches and rebuilds the sheet content — which also resolves theRegisterNameSheetteardown without any change at that call site. isCloseButtonEnabledadded at the five sites that passonClose, each mirroring its ownisDismissalEnabledcondition. Tracking master is only safe with this:performnow callsonCloseunconditionally, and the button is live whenisCloseButtonEnabled && (isDismissalEnabled || hasCustomCloseAction).- Close is routed per phase. On success the send and internal-transfer sheets run
onCompleted(which unwinds the flow) and the evonode sheet runsonCompleted(remaining)(which reports the new balance), instead ofonCancel, which only closed the sheet. CoinJoinMoveFundsSheet's choice body scrolls, so "Later" stays reachable inside the.mediumdetent.originHashis no longer a concern: restoringbranch = masterrestores the dependency declaration the hash covers, and it now matches #1073 and #1074 again.
UsernameMarketplaceScreen needed no change — it passes no onClose, so its close button is gated by isDismissalEnabled alone.
Verified with a full dashpay build resolving DashUIKit at master, then installed on an iPhone 17 Pro / iOS 26.5 simulator and exercised by the reviewer. The bottom-padding question I raised earlier — whether the migrated sheets' buttons land in the home-indicator strip — was checked on device and is fine, so no padding change was made.
One note for whoever merges: this branch, like #1073 and #1074, is 31 commits behind develop and does not compile as-is — ContestedNamesService.swift:83 and SwiftDashSDKHost.swift:941,958 still call dpnsActiveContests / loadFromPersistor without await, which the current SDK requires. A squash merge applies only this PR's diff, so the merged result is fine; it is local verification that needs develop merged in first. Worth knowing, because CI here runs no build check.
🤖 Reviewed with Claude Code
3 participants
Issue being fixed or feature implemented
Follow-up to #1073 and #1074, using the dismissal controls introduced by
dashpay/DashUIKit#14. Protected signing, locking, proving, and broadcast flows still owned
their grabber/title shell and used a content-level
interactiveDismissDisabledworkaround.That prevented swipe dismissal but could not coordinate DashUIKit's close affordance for
sheets already using the shared component.
This pull request is intentionally stacked on
refactor/dashuikit-sheet-content.What was done?
is under review.
move-funds, and evonode withdrawal confirmation to
DashUIKit.BottomSheet.isDismissalEnabled, disabling swipe and the shared closecontrol together during authorization, signing, locking, proving, submitting, and
broadcasting as appropriate.
dismissal modifiers.
and the shared close control together during an active operation.
interactive-dismissal protection.
SDKIdentityProfileSheetandWalletsScreenon native SwiftUI sheets; their remaininginteractiveDismissDisabledmodifiers are intentional.How Has This Been Tested?
xcodebuild -resolvePackageDependencies -workspace DashWallet.xcworkspace -scheme dashpaydashpayDebug builds for iPhone 17 Pro / iOS 26.5 Simulator at exact basebb34895b65769d5849b97aea56adb39dcf3c6982and exact head9198db97fe6f58f69efcfa5d7062d59530800463, using separate worktrees, separate DerivedData,normal Simulator signing, and the required local-only service plists.
org.dashfoundation.dashon clonesof the same shut-down wallet fixture.
phase. Accessibility reported the head's close control as disabled while protected.
e8d92434bfc28fbf933b896cd40a01dd61835b5f, and rangit diff --check.correctness, quality, architecture, and reliability passes approved the migration with no
findings.
the dismissal API.
Exact provenance, device identifiers, full-resolution originals, dimensions, and SHA-256
hashes: visual evidence tree
Idle confirmation
Protected locking phase
Breaking Changes
None. This wallet change depends on dashpay/DashUIKit#14 landing first.
Checklist:
is validated in Simulator; the shared dismissal behavior has focused tests in DashUIKit)
For repository code-owners and collaborators only
This pull request was created by Codex.