feat(explore-dash): refactor confirmation and details sheet flow#769
feat(explore-dash): refactor confirmation and details sheet flow#769romchornyi wants to merge 15 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds multi-quantity PiggyCards ordering, a gift-card details polling view-model + barcode support, many new/refactored SwiftUI components, asset/color/localization updates, DB test-data insertion, and sheet/detent presentation changes. ChangesGift Card Purchase & Details Flow
Sequence Diagram(s)sequenceDiagram
participant UI as User Interface
participant VM as DashSpendPayViewModel
participant Repo as PiggyCardsRepository
participant API as PiggyCards API
participant DB as Local Database
UI->>VM: present selectedQuantities & confirm
VM->>Repo: orderGiftCards(lineItems)
Repo->>API: POST orders (multiple)
API-->>Repo: order response
Repo->>VM: processOrderResponse(summaryDiscount)
VM->>DB: save GiftCard with note (order metadata)
DB-->>VM: persisted
VM-->>UI: purchase success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
romchornyi
changed the title
romchornyi
force-pushed
the
mo-896-add-quantity-confirmation-card-details
branch
from
e95d40c to
81e7afb
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift (1)
303-321:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMore empty conditional bodies (likely stripped debug logging).
Lines 304-306 and 318-321 contain
if let ... { }andif case .statusCode { if let ... { } }with empty bodies. These produce zero behavior but still encode/serialize JSON and decode response data on the failure path, paying that cost for nothing. Either delete these blocks or wrap a realDSLogger.log("…")call inside#if DEBUG.🧹 Proposed cleanup
- // Log the JSON request for debugging - if let jsonData = try? JSONEncoder().encode(orderRequest), - let jsonString = String(data: jsonData, encoding: .utf8) { - } - do { let orderResponse: PiggyCardsOrderResponse = try await PiggyCardsAPI.shared.request(.createOrder(orderRequest)) … } catch let error as HTTPClientError { - // Log the raw error response for debugging - if case .statusCode(let response) = error { - if let errorString = String(data: response.data, encoding: .utf8) { - } - } throw try parseError(from: error, context: "create order")As per coding guidelines: Wrap debug print statements in
#ifDEBUG blocks or use conditional compilation to prevent performance impact from excessive logging.🤖 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 `@DashWallet/Sources/Models/Explore` Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift around lines 303 - 321, Remove the no-op conditional blocks that decode/encode JSON with empty bodies or convert error response data to strings; either delete the if-let blocks around encoding orderRequest (the empty block after JSONEncoder().encode(orderRequest) / String(...)) and the nested if-let that converts response.data inside the HTTPClientError .statusCode case, or replace them with real debug logging wrapped in conditional compilation (e.g., inside `#if` DEBUG use DSLogger.log(...) referencing orderRequest and the response.data string). Ensure you keep the core flow that awaits PiggyCardsAPI.shared.request(.createOrder(orderRequest)) and the call to processOrderResponse, and reference the HTTPClientError handling branch (case .statusCode(let response)) when adding any debug log so you only incur the cost in DEBUG builds.
🟡 Minor comments (11)
DashWallet/Sources/Models/Explore Dash/Infrastructure/Database Connection/ExploreDatabaseConnection.swift-234-295 (1)
234-295:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape or bind these SQL values before interpolation.
These statements interpolate merchant names, URLs, ids, and city/state values directly into SQL. One apostrophe in future fixture data will break the whole transaction and leave none of the test merchants inserted. Please switch to bound parameters or at least escape SQL string literals in one place before composing the queries.
Suggested hardening
private func addPiggyCardsTestMerchants(db: Connection) { do { - let allIds = piggyCardsTestMerchants.map { "'\($0.merchantId)'" }.joined(separator: ", ") + func sqlLiteral(_ value: String) -> String { + "'\(value.replacingOccurrences(of: "'", with: "''"))'" + } + + let allIds = piggyCardsTestMerchants.map { sqlLiteral($0.merchantId) }.joined(separator: ", ") try db.transaction { // Drop FTS triggers to allow merchant table modifications try db.run("DROP TRIGGER IF EXISTS room_fts_content_sync_merchant_fts_AFTER_INSERT") @@ // Insert all test merchants for m in piggyCardsTestMerchants { - let territory = "'\(m.territory ?? "")'" - let city = "'\(m.city ?? "")'" + let territory = sqlLiteral(m.territory ?? "") + let city = sqlLiteral(m.city ?? "") try db.run(""" INSERT INTO merchant ( @@ ) VALUES ( - '\(m.merchantId)', '\(m.name)', 'PiggyCards', '\(m.sourceId)', - '\(m.logo)', 1, 'gift card', \(m.merchantSavings), - '\(m.merchantDenomType)', 'online', 'online', - \(territory), \(city), '\(m.website)', + \(sqlLiteral(m.merchantId)), \(sqlLiteral(m.name)), 'PiggyCards', \(sqlLiteral(m.sourceId)), + \(sqlLiteral(m.logo)), 1, 'gift card', \(m.merchantSavings), + \(sqlLiteral(m.merchantDenomType)), 'online', 'online', + \(territory), \(city), \(sqlLiteral(m.website)), datetime('now'), datetime('now') ) """) @@ ) VALUES ( - '\(m.merchantId)', 'PiggyCards', '\(m.sourceId)', - \(m.providerSavings), '\(m.providerDenomType)', 1, 'online' + \(sqlLiteral(m.merchantId)), 'PiggyCards', \(sqlLiteral(m.sourceId)), + \(m.providerSavings), \(sqlLiteral(m.providerDenomType)), 1, 'online' ) """) - try db.run("INSERT INTO merchant_fts(docid, name) VALUES (\(rowId), '\(m.name)')") + try db.run("INSERT INTO merchant_fts(docid, name) VALUES (\(rowId), \(sqlLiteral(m.name)))") }🤖 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 `@DashWallet/Sources/Models/Explore` Dash/Infrastructure/Database Connection/ExploreDatabaseConnection.swift around lines 234 - 295, The SQL in addPiggyCardsTestMerchants interpolates raw strings (merchantId, name, city, territory, website, etc.) which can break or allow injection; change to use parameterized/bound queries instead of string interpolation: build DELETE/INSERT statements with placeholders and bind arrays of values (or execute per-merchant deletes/inserts) — e.g., replace the IN (...) string interpolation for allIds with a parameterized IN clause or loop and bind each merchantId, and convert the INSERTs into prepared statements that bind m.merchantId, m.name, m.sourceId, m.logo, m.merchantSavings, m.merchantDenomType, m.website, m.city, m.territory, m.providerSavings, m.providerDenomType, etc.; also bind the docid for merchant_fts instead of interpolating rowId. Ensure you use db.run/sqlite prepared statement APIs that accept parameters and remove any single-quote string concatenation.DashWallet/en.lproj/Localizable.strings-2968-2970 (1)
2968-2970:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle singular/plural for card count text
Line 2969 will produce incorrect text for
1("1 cards"). Please use a pluralized string (e.g.,.stringsdict) or at least separate singular/plural keys.🤖 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 `@DashWallet/en.lproj/Localizable.strings` around lines 2968 - 2970, The existing localization key "Merchant has only %d cards available" will render "1 cards" for singular; update localization to support pluralization by replacing this single string with a localized plural entry (use an .stringsdict) or at minimum add two keys (e.g., "Merchant has only %d card available" and "Merchant has only %d cards available") and switch selection in code where this message is used to choose singular vs plural based on the count; ensure the original key referenced in code (the "Merchant has only %d cards available" key) is updated to the new pluralized key names or that code is changed to fetch the .stringsdict plural form.DashWallet/Sources/UI/Main/MainTabbarController.swift-81-81 (1)
81-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
paymentIsOpenedflag is set but never read — double-presentation is not guardedThe flag is updated faithfully in
showPaymentsControllerandclosePayments, but there is no early-return inshowPaymentsControllerwhenpaymentIsOpened == true. Rapid double-taps on the payment tab (which bypassesshouldSelecttwice before the modal is fully presented) can trigger two concurrentpresentcalls onselectedViewController?.topController(). UIKit will emit a runtime warning and silently drop the second presentation, but the flag was clearly intended to prevent this.🔒 Proposed guard
func showPaymentsController(withActivePage pageIndex: PaymentsViewControllerState) { + guard !paymentIsOpened else { return } paymentIsOpened = trueAlso applies to: 390-413
🤖 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 `@DashWallet/Sources/UI/Main/MainTabbarController.swift` at line 81, Add an early-return guard in showPaymentsController to check the paymentIsOpened Bool before attempting to present to prevent double presentation; specifically, in the showPaymentsController function, if paymentIsOpened == true return immediately, otherwise set paymentIsOpened = true right before calling selectedViewController?.topController().present(...), and ensure closePayments sets paymentIsOpened = false when the payment modal is dismissed; reference the paymentIsOpened flag, showPaymentsController and closePayments and the presentation target selectedViewController?.topController() when applying the change.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendDenominationRow.swift-23-23 (1)
23-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove redundant
= nilfrom optional property (SwiftLintimplicit_optional_initialization)SwiftLint warns on explicit
= nilinitialisation of an optional property.🔧 Fix
- var inventoryLimit: Int? = nil + var inventoryLimit: Int?🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendDenominationRow.swift at line 23, The declaration var inventoryLimit: Int? = nil is redundantly initializing an optional to nil; remove the explicit "= nil" so the property is declared as var inventoryLimit: Int? (update the DashSpendDenominationRow optional property accordingly to satisfy SwiftLint's implicit_optional_initialization rule).DashWallet/Sources/UI/SwiftUI Components/DashStepper.swift-21-36 (1)
21-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwiftLint
empty_counterrors will break CI — renamecounttoquantitySwiftLint's
empty_countrule fires oncount > 0at lines 33 and 36 because the binding is namedcount. These are reported as[Error], which typically gates CI. The variable represents a quantity, not a collection count, soquantityis the semantically correct name and immediately eliminates the false positive without a suppression comment.♻️ Rename `count` → `quantity` throughout the file
struct DashStepper: View { - `@Binding` var count: Int + `@Binding` var quantity: Int var maxCount: Int? private var plusEnabled: Bool { guard let max = maxCount else { return true } - return count < max + return quantity < max } var body: some View { HStack(spacing: 6) { stepperButton( systemImage: "minus", - enabled: count > 0, + enabled: quantity > 0, accessibilityLabel: NSLocalizedString("Decrease quantity", comment: "DashSpend") ) { - if count > 0 { count -= 1 } + if quantity > 0 { quantity -= 1 } } Text("\(count)") + Text("\(quantity)") stepperButton( systemImage: "plus", enabled: plusEnabled, accessibilityLabel: NSLocalizedString("Increase quantity", comment: "DashSpend") ) { - count += 1 + quantity += 1 } } } // ... - .accessibilityValue(Text("\(count)")) + .accessibilityValue(Text("\(quantity)"))The call site in
DashSpendDenominationRowwould change to:-DashStepper(count: $count, maxCount: inventoryLimit) +DashStepper(quantity: $count, maxCount: inventoryLimit)🤖 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 `@DashWallet/Sources/UI/SwiftUI` Components/DashStepper.swift around lines 21 - 36, Rename the Binding and all uses from count to quantity to avoid the SwiftLint empty_count false positive: change the property declaration `@Binding` var count: Int to `@Binding` var quantity: Int, update maxCount comparison logic in the computed property plusEnabled (return quantity < max), update all checks and mutations inside body (enabled: quantity > 0, if quantity > 0 { quantity -= 1 }, and any increments where used), and update any callers such as DashSpendDenominationRow to pass and read the renamed binding (quantity) accordingly.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendPayIntro.swift-86-92 (1)
86-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMask string accessibility / consistency nit.
Text("***********")is a fixed-length 11-asterisk string. It will be read by VoiceOver as "asterisk" repeated, and visually it doesn't track the actual amount width. Two small improvements:
- give the masked text an
.accessibilityLabel(NSLocalizedString("Balance hidden", comment: ""))so VoiceOver announces the state, and- consider using a single redacted view (
DashAmount(...).redacted(reason: .placeholder)together with the fiat) so the masked layout matches the unmasked layout.🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendPayIntro.swift around lines 86 - 92, Replace the fixed literal mask Text("***********") with an accessible, layout-consistent placeholder: when balanceHidden is true, render the same DashAmount(amount:…, font: .subheadline, showDirection: false) and the fiat view but apply .redacted(reason: .placeholder) to those views so layout matches the unmasked state, and add .accessibilityLabel(NSLocalizedString("Balance hidden", comment: "")) to the masked container (or the DashAmount when redacted) so VoiceOver reads a meaningful message; update the conditional branch that currently uses Text("***********") to use these changes (refer to balanceHidden, DashAmount(...) and formattedFiatText(...)).DashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendAmountView.swift-25-25 (1)
25-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded
"<symbol> <amount>"order ignores locale formatting.Several locales/currencies place the symbol after the amount (e.g.,
42,50 €,42.50 kr) and use locale-specific spacing/grouping. Manually composing"\(currencySymbol) \(amount)"here (after stripping the symbol from the formatted string upstream) loses that information. If this view is reused for non‑USD locales, consider either:
- passing a fully pre-formatted localized currency string and rendering it as-is, or
- using
Decimal.formatted(.currency(code:))/ aNumberFormatterwithcurrencystyle inside the view.🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendAmountView.swift at line 25, DashSpendAmountView currently composes currency display by concatenating currencySymbol and amount with Text("\(currencySymbol) \(amount)"), which ignores locale-specific symbol position and formatting; update the view to either accept a fully preformatted localized currency string (e.g., pass formattedAmount and render it directly) or perform locale-aware formatting inside the view using the numeric value with Decimal.formatted(.currency(code: currencyCode)) or a NumberFormatter configured with .currency style; modify the view API and replace Text("\(currencySymbol) \(amount)") accordingly, referencing DashSpendAmountView, currencySymbol, amount, and any currencyCode/amount properties to locate and change the code.DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift-230-241 (1)
230-241:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDead empty
if/else ifbranches inside the merchant-selection failure path.When
selectGiftCardreturnsnil, the loop at lines 234-239 iteratesgiftCardsand runs emptyif normalizedType == "fixed"/else if normalizedType == "range"branches before throwing. Both branches are no-ops, so this whole block can be deleted, or the original diagnostic logging restored behind#if DEBUG. As written it just adds cyclomatic complexity (the function already trips SwiftLint'scyclomatic_complexityat 17) without any runtime effect.🧹 Proposed cleanup
guard let selectedCard = PiggyCardsCache.shared.selectGiftCard( from: giftCards, forAmount: lineItem.denomination ) else { - for card in giftCards { - let normalizedType = card.priceType.lowercased() - if normalizedType == "fixed" { - } else if normalizedType == "range" { - } - } throw DashSpendError.invalidAmount }As per coding guidelines: Wrap debug print statements in
#ifDEBUG blocks or use conditional compilation to prevent performance impact from excessive logging.🤖 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 `@DashWallet/Sources/Models/Explore` Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift around lines 230 - 241, The empty conditional branches inside the failure path after attempting to get selectedCard (the guard using PiggyCardsCache.shared.selectGiftCard with giftCards and lineItem.denomination) should be removed or turned into debug-only diagnostics: delete the for card in giftCards { let normalizedType = card.priceType.lowercased(); if normalizedType == "fixed" { } else if normalizedType == "range" { } } block to eliminate dead code and reduce cyclomatic complexity, or replace its body with a debug-only log (wrap prints in `#if` DEBUG) that records card.priceType or other diagnostic info before throwing DashSpendError.invalidAmount.DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift-222-223 (1)
222-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDead empty loop body — drop it or restore the log behind
#if DEBUG.
for (index, card) in giftCards.enumerated() { }does nothing; this looks like a stripped debug block. Per the repo guideline,Wrap debug print statements in#ifDEBUG blocks or use conditional compilation to prevent performance impact from excessive logging., either delete the loop entirely or restore the intendedDSLogger/#if DEBUG.🧹 Proposed cleanup
- for (index, card) in giftCards.enumerated() { - } - // Step 2: Select the appropriate gift cards for each line itemAs per coding guidelines: Wrap debug print statements in
#ifDEBUG blocks or use conditional compilation to prevent performance impact from excessive logging.🤖 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 `@DashWallet/Sources/Models/Explore` Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift around lines 222 - 223, The empty loop for (index, card) in giftCards.enumerated() {} in PiggyCardsRepository is dead code; either remove the entire loop or restore the debug logging inside a conditional compilation block—e.g., move the intended DSLogger/print call back into the loop and wrap it with `#if` DEBUG / `#endif`; update the method that iterates giftCards so there are no no-op loops left behind.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift-133-137 (1)
133-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the CTA localization key typo.
"Conttinue"will either render the typo directly or miss the existing"Continue"localization entry, so the keyboard action button can drift from the rest of the flow.Suggested fix
NumericKeyboardView( value: $input, showDecimalSeparator: true, - actionButtonText: NSLocalizedString("Conttinue", comment: ""), + actionButtonText: NSLocalizedString("Continue", comment: "DashSpend"), actionEnabled: actionEnabled && !showLimits && hasValidLimits, inProgress: inProgress, actionHandler: onAction )🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift around lines 133 - 137, Replace the misspelled localization key passed to NumericKeyboardView's actionButtonText parameter: change NSLocalizedString("Conttinue", comment: "") to use the correct key NSLocalizedString("Continue", comment: "") (or the exact existing localization key used elsewhere) so the CTA matches other screens; update the string key wherever the actionButtonText is constructed for the DashSpendSinglePanel and run localization checks to ensure the "Continue" entry exists in Localizable.strings.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsInfoCard.swift-168-171 (1)
168-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessibility label to the copy button.
This control is icon-only, so assistive technologies won't announce what gets copied here. The card number and PIN rows end up exposing two indistinguishable buttons.
Suggested fix
Button(action: onCopy) { Icon(name: .custom("icon_copy_outline", maxHeight: 14)) .tint(.primaryText) } + .accessibilityLabel(Text(title)) + .accessibilityHint(Text(NSLocalizedString("Copies this value to the clipboard", comment: "DashSpend")))🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsInfoCard.swift around lines 168 - 171, The copy Button in GiftCardDetailsInfoCard (the Button using action: onCopy and Icon(name: .custom("icon_copy_outline"...))) is icon-only and needs an accessibility label; update that Button to include a clear .accessibilityLabel (e.g. "Copy card number" for the card number row and "Copy PIN" for the PIN row) so VoiceOver can distinguish the two copy controls—if the component is reused, accept or compute a descriptive label (prop or local variable) and apply it to the Button that wraps the Icon.
🧹 Nitpick comments (9)
DashWallet/Sources/Models/Explore Dash/Infrastructure/Database Connection/ExploreDatabaseConnection.swift (1)
132-329: ⚡ Quick winGate this helper behind
PIGGYCARDS_ENABLEDtoo.This new path is PiggyCards-specific end-to-end, but it currently compiles for every
DEBUG/Testflightbuild. Please wrap the whole block with the PiggyCards feature flag as well so non-PiggyCards variants do not carry provider-specific test data logic.As per coding guidelines: "Use conditional compilation
#ifPIGGYCARDS_ENABLED for PiggyCards provider support in Swift code."🤖 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 `@DashWallet/Sources/Models/Explore` Dash/Infrastructure/Database Connection/ExploreDatabaseConnection.swift around lines 132 - 329, The PiggyCards-specific test merchant helpers (addTestMerchantAfterSync, insertTestMerchant, piggyCardsTestMerchants, addPiggyCardsTestMerchants) are only gated by DEBUG/Testflight and must also be compiled only when PIGGYCARDS_ENABLED is set; wrap the entire block that defines addTestMerchantAfterSync, insertTestMerchant, the PiggyCardsTestMerchant struct, piggyCardsTestMerchants array, and addPiggyCardsTestMerchants function with a conditional compilation check `#if` PIGGYCARDS_ENABLED (in addition to the existing DEBUG/Testflight check) so the provider-specific code is excluded for builds without PiggyCards support.DashWallet/Sources/UI/SwiftUI Components/DashStepper.swift (1)
55-65: 💤 Low valueMisleading parameter name
systemImage— actually loads from the asset catalog
Image(systemImage)callsImage(_ name: String), which looks up the asset catalog. The namesystemImagestrongly impliesImage(systemName:)(SF Symbols). Since customplus/minusassets were added to the catalog, the behavior is correct, but the parameter name will mislead future maintainers into thinking SF Symbol names are expected.♻️ Rename to `imageName`
- private func stepperButton( - systemImage: String, + private func stepperButton( + imageName: String, enabled: Bool, accessibilityLabel: String, action: `@escaping` () -> Void ) -> some View { - Image(systemImage) + Image(imageName)Update the two call sites accordingly (
"minus"and"plus").🤖 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 `@DashWallet/Sources/UI/SwiftUI` Components/DashStepper.swift around lines 55 - 65, Rename the misleading parameter systemImage in the stepperButton function to imageName (function: stepperButton(systemImage:enabled:accessibilityLabel:action:)) and update its two call sites that pass "minus" and "plus" to use the new name; keep the Image(initializer) usage as-is (Image(imageName)) since these are asset-catalog images rather than SF Symbols. Ensure the function signature and all references (including any internal uses like Image(...) and accessibilityLabel arguments) are updated to the new identifier.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendPayIntro.swift (2)
19-20: 💤 Low valueSort imports (lint).
SwiftLint warns that
import SDWebImageSwiftUIis out of order; placeSDWebImageSwiftUIbeforeSwiftUI(alphabetical) to clear the warning.🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendPayIntro.swift around lines 19 - 20, The import ordering is triggering a SwiftLint warning because imports must be alphabetical; swap the two import lines so that "import SDWebImageSwiftUI" appears before "import SwiftUI" (i.e., reorder the top-level imports in DashSpendPayIntro.swift) to satisfy the linter.
94-96: 💤 Low valueUse the explicit
label:form (lint).SwiftLint flags this as
multiple_closures_with_trailing_closure. The explicit form also reads better when theactionclosure does non-trivial work like wrapping inwithAnimation.♻️ Proposed fix
- Button(action: { withAnimation(.easeInOut(duration: 0.2)) { balanceHidden.toggle() } }) { - eyeIcon - } + Button { + withAnimation(.easeInOut(duration: 0.2)) { balanceHidden.toggle() } + } label: { + eyeIcon + }🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendPayIntro.swift around lines 94 - 96, Replace the trailing-closure Button usage with the explicit label: form to satisfy lint and improve clarity: keep the action closure that toggles balanceHidden inside withAnimation (the existing Button(action: { withAnimation(.easeInOut(duration: 0.2)) { balanceHidden.toggle() } }) usage) but pass the view for the button content via the label: parameter (using eyeIcon as the label). This changes the call to use the Button(action: { ... }, label: { eyeIcon }) initializer so SwiftLint no longer flags multiple_closures_with_trailing_closure and the action/label intent is explicit.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardPurchaseSelectionSheet.swift (2)
73-101: 💤 Low valueUse the
Button { } label: { }form (lint).SwiftLint flags line 75 with
multiple_closures_with_trailing_closure. Switching to the explicitlabel:form silences the warning and reads more clearly with this much label content.♻️ Proposed fix
- Button(action: { - onSelectCard(index) - }) { + Button { + onSelectCard(index) + } label: { HStack(spacing: 20) {🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardPurchaseSelectionSheet.swift around lines 73 - 101, Replace the current Button call that uses two trailing closures (Button(action: { onSelectCard(index) }) { ... }) with the explicit label form to satisfy SwiftLint: use the initializer Button { onSelectCard(index) } label: { /* preserve the HStack content including Icon(...) and Text(card.formattedPrice) */ }, keeping the same modifiers (.buttonStyle(.plain), .contentShape(Rectangle()), .frame(height: 42)) and references to onSelectCard, Icon(name:), card.formattedPrice, and index so behavior and layout remain identical while removing the multiple_closures_with_trailing_closure lint warning.
72-72: ⚡ Quick winPrefer a stable identity over
\.offsetinForEach.
Array(cards.enumerated())keyed by\.offsetties identity to position. If thecardsarray is ever reordered, partially refreshed, or appended-to mid-render, SwiftUI will reuse rows incorrectly and may show stale animations or selection state.GiftCardDetailsCardItemalready exposesid(see preview values), so prefer something like:♻️ Proposed fix
- ForEach(Array(cards.enumerated()), id: \.offset) { index, card in + ForEach(Array(cards.enumerated()), id: \.element.id) { index, card in🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardPurchaseSelectionSheet.swift at line 72, The ForEach is using Array(cards.enumerated()) with id: \.offset which ties identity to array position; change the iteration to use the card's stable identifier (e.g., use cards directly in ForEach and set id: \.id or rely on the card conforming to Identifiable) so rows keep stable identities — update the ForEach line that currently references Array(cards.enumerated()) and id: \.offset to iterate over cards and use card.id (GiftCardDetailsCardItem / GiftCardPurchaseSelectionSheet ForEach) to ensure stable identities.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsMerchantHeader.swift (1)
42-50: 💤 Low valueDouble
.frameis a confusing way to express padding around the badge.Lines 46–47 apply two consecutive frames (
20×20then23×23) to size the icon and then leave room for the circular background. This works but obscures intent. A clearer expression is sizing the icon once and using.padding(1.5)(or a fixed frame with.background(Circle()...)alone) so the next reader doesn't have to re-derive the geometry.🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsMerchantHeader.swift around lines 42 - 50, Replace the confusing double .frame usage in GiftCardDetailsMerchantHeader (inside the if merchantIcon != nil block) by sizing the Image once (e.g., .frame(width: 20, height: 20) on the Image) and then use .padding(1.5) (or a single larger frame) before applying .background(Circle().fill(Color.secondaryBackground)) and .offset(x:2,y:2); this keeps the intent clear by explicitly giving the icon size and the surrounding badge padding rather than chaining two frames.DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift (1)
207-328: ⚖️ Poor tradeoffConsider splitting
orderGiftCardsto lower its cyclomatic complexity.After deleting the dead-log blocks above, this method still does input validation, cache lookup, per-line-item card selection, basket summary computation, request building, request firing, and multi-arm error mapping — hitting SwiftLint's
cyclomatic_complexity: 17. ExtractingselectOrders(forLineItems:in:)andbuildOrderRequest(orders:email:)helpers would bring the body back under the threshold and make the multi-line-item flow easier to test.🤖 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 `@DashWallet/Sources/Models/Explore` Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift around lines 207 - 328, The orderGiftCards method is too complex; extract the per-line-item selection and the request-building into helpers to reduce cyclomatic complexity. Create a function selectOrders(forLineItems: [OrderLineItem], in giftCards: [PiggyCard]) -> (orders: [PiggyCardsOrder], basketItems: [SelectedBasketItem]) that encapsulates the loop that validates quantities, calls PiggyCardsCache.selectGiftCard, handles fixed/range type checks and throws DashSpendError.invalidAmount as needed; and create buildOrderRequest(orders: [PiggyCardsOrder], recipientEmail: String) -> PiggyCardsOrderRequest that builds PiggyCardsUserMetadata, PiggyCardsUser and the PiggyCardsOrderRequest (preserving the Android hardcoded values). Remove the dead log blocks from orderGiftCards and have orderGiftCards call these two helpers, compute summaryDiscountPercentage from returned basketItems, then continue with the network call and existing error handling unchanged.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsPoweredBySection.swift (1)
29-39: ⚡ Quick winUse typed
GiftCardProviderenum instead of hardcoded provider strings.The file currently branches on
provider == "PiggyCards", butGiftCardProviderenum exists with properly defined cases (.ctx,.piggyCards) and computed properties (displayName,logoName). RefactoringproviderfromString?toGiftCardProvider?would eliminate fragile string matching. Note: this requires updating the type throughout the view hierarchy (GiftCardDetailsViewModel→GiftCardDetailsView→GiftCardPurchaseSelectionSheet→GiftCardDetailsPoweredBySection), so plan for medium effort across multiple files. The enum's built-in properties would also simplify logo selection logic.🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsPoweredBySection.swift around lines 29 - 39, Replace fragile string matching by changing provider from String? to the typed GiftCardProvider? and propagate that type change through GiftCardDetailsViewModel, GiftCardDetailsView, GiftCardPurchaseSelectionSheet, and GiftCardDetailsPoweredBySection; update constructors/initializers and stored properties to accept GiftCardProvider? and adjust call sites accordingly; inside GiftCardDetailsPoweredBySection use provider?.logoName and provider?.displayName (or switch on GiftCardProvider cases .ctx/.piggyCards) to choose the Image and frame parameters instead of comparing provider == "PiggyCards", ensuring you use the enum’s computed properties for logo selection and sizing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d01118b-0536-4079-aee5-6683af9319d3
⛔ Files ignored due to path filters (15)
DashWallet/Resources/AppAssets.xcassets/eye_closed-icon.imageset/eye_closed-icon@x1.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/eye_closed-icon.imageset/eye_closed-icon@x2.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/eye_closed-icon.imageset/eye_closed-icon@x3.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/eye_opened-icon.imageset/eye_opened-icon@x1.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/eye_opened-icon.imageset/eye_opened-icon@x2.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/eye_opened-icon.imageset/eye_opened-icon@x3.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/icon-gift_card-piggy_cards.imageset/icon-gift_card-piggy_cards@x1.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/icon-gift_card-piggy_cards.imageset/icon-gift_card-piggy_cards@x2.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/icon-gift_card-piggy_cards.imageset/icon-gift_card-piggy_cards@x3.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/minus.imageset/minus-icon@x1.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/minus.imageset/minus-icon@x2.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/minus.imageset/minus-icon@x3.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/plus.imageset/plus-icon@x1.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/plus.imageset/plus-icon@x2.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/plus.imageset/plus-icon@x3.pngis excluded by!**/*.png
📒 Files selected for processing (44)
DashWallet.xcodeproj/project.pbxprojDashWallet/Resources/AppAssets.xcassets/eye_closed-icon.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/eye_opened-icon.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/icon-gift_card-piggy_cards.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/minus.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/plus.imageset/Contents.jsonDashWallet/Sources/Models/Explore Dash/Infrastructure/Database Connection/ExploreDatabaseConnection.swiftDashWallet/Sources/Models/Explore Dash/Model/DashSpend/PiggyCardsConstants.swiftDashWallet/Sources/Models/Explore Dash/Model/Entites/CTXSpendModels.swiftDashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swiftDashWallet/Sources/UI/Explore Dash/Merchants & ATMs/Details/Views/POIDetailsViewModel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendAmountView.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendDenominationRow.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendFixedContent.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendFlexibleContent.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendMultiplePanel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendPayIntro.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendSinglePanel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendConfirmationDialog.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayScreen.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsHowToUseSection.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsInfoCard.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsMerchantHeader.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsPoweredBySection.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardPurchaseSelectionSheet.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/GiftCardDetailsView.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/GiftCardDetailsViewModel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetailsView.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetailsViewModel.swiftDashWallet/Sources/UI/Home/HomeViewController.swiftDashWallet/Sources/UI/Home/Views/HomeView.swiftDashWallet/Sources/UI/Main/MainTabbarController.swiftDashWallet/Sources/UI/SwiftUI Components/BottomSheet.swiftDashWallet/Sources/UI/SwiftUI Components/Button.swiftDashWallet/Sources/UI/SwiftUI Components/Color+DWStyle.swiftDashWallet/Sources/UI/SwiftUI Components/DashStepper.swiftDashWallet/Sources/UI/SwiftUI Components/NumericKeyboardView.swiftDashWallet/Sources/UI/SwiftUI Components/SegmentedControl.swiftDashWallet/en.lproj/Localizable.stringsShared/Resources/SharedAssets.xcassets/Colors/Black/Black1000Alpha40.colorset/Contents.jsonShared/Resources/SharedAssets.xcassets/Colors/Black/Black1000Alpha5.colorset/Contents.jsonShared/Resources/SharedAssets.xcassets/Colors/Black/Black1000Alpha8.colorset/Contents.jsonShared/Resources/SharedAssets.xcassets/Colors/Gray/Gray300Alpha20.colorset/Contents.json
💤 Files with no reviewable changes (2)
- DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetailsViewModel.swift
- DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetailsView.swift
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift (1)
308-317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply
.rounded()before casting Double to UInt64 to prevent floating-point precision loss in satoshi conversion.
giftCardInfo.amountisDouble. When multiplied by 100,000,000 and cast directly toUInt64, binary rounding artifacts (e.g.,0.123456represented as0.12345599…) can silently lose one satoshi from the payment amount. This file already applies.rounded()before similar type casts (line 739). Use.rounded()here to ensure the on-chain amount matches the quoted DASH price.🛡️ Proposed fix
// Convert DASH amount to satoshis (1 DASH = 100,000,000 satoshis) - let dashAmountInSatoshis = UInt64(giftCardInfo.amount * 100_000_000) + let dashAmountInSatoshis = UInt64((giftCardInfo.amount * 100_000_000).rounded())🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/DashSpendPayViewModel.swift around lines 308 - 317, The calculation of dashAmountInSatoshis should apply .rounded() before casting to UInt64 to avoid floating-point precision loss: change the expression that creates dashAmountInSatoshis from UInt64(giftCardInfo.amount * 100_000_000) to use giftCardInfo.amount.rounded() (i.e., UInt64(giftCardInfo.amount.rounded() * 100_000_000)) so the satoshi amount passed into sendCoinsService.sendCoins(address:amount:) matches the quoted DASH price; update the assignment where dashAmountInSatoshis is defined in DashSpendPayViewModel to use .rounded() on giftCardInfo.amount.
♻️ Duplicate comments (1)
DashWallet/Sources/UI/Home/Views/HomeView.swift (1)
552-557:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
onChange(selectedCardIndex)may overwrite the explicitshowBackButton = truefromonSelectCard.When the user picks a card at line 519-523, three state mutations happen in the same render:
selectedCardIndex = index,showBackButton = true, andprefersLargeDetent = true. After the state batch commits,onChange(of: selectedCardIndex)fires and reassignsshowBackButton = shouldShowBackButton, which evaluates tofalsebecause line 626-628 still only returnstxDetailRoute != nil. The same happens ononAppear(line 546) when re-entering with a previously selected card. The flow currently works only ifGiftCardDetailsViewre-asserts the back button viaonShowBackButton(true)on itsonAppear; otherwise the back button briefly disappears (or fully disappears) and the multi-card "return to picker" branch inhandleBackNavigationbecomes unreachable.The cleanest fix is the one suggested in the prior review — make
shouldShowBackButtonaware of the selection state. This was marked as addressed in commit 80f81bd, but the predicate at line 626-628 still only checkstxDetailRoute.🛡️ Suggested fix
private var shouldShowBackButton: Bool { - txDetailRoute != nil + txDetailRoute != nil || (selectedCardIndex != nil && viewModel.uiState.cards.count > 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 `@DashWallet/Sources/UI/Home/Views/HomeView.swift` around lines 552 - 557, The onChange handler for selectedCardIndex overwrites an explicit showBackButton set by onSelectCard because shouldShowBackButton currently only checks txDetailRoute; update the predicate used by shouldShowBackButton (the function or computed Bool referenced at its declaration and used in the onChange block) to also return true when selectedCardIndex != nil (or when a card is selected), so that onChange(of: selectedCardIndex) doing showBackButton = shouldShowBackButton preserves the intent; locate and modify the shouldShowBackButton computed property / function and any callers (e.g., in onChange, onAppear) to include selectedCardIndex != nil in its logic so the back button remains visible after selection.
🧹 Nitpick comments (5)
DashWallet/Sources/Models/Explore Dash/Services/ExploreDatabaseSyncManager.swift (1)
109-109: ⚡ Quick winAdd emoji log prefix for DSLogger consistency
Line 109 should use the repo’s emoji-prefixed logging format to keep Explore logs filterable.
Suggested change
- DSLogger.log("ExploreDash: local explore.db missing, forcing cloud database download") + DSLogger.log("💾 ExploreDash: local explore.db missing, forcing cloud database download")As per coding guidelines: "Use emoji debug markers for easy log filtering: 🎯 🌐 💾 🎨 🔍 📍 🗺️ (e.g., DSLogger.log("🔍 Message:"))."
🤖 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 `@DashWallet/Sources/Models/Explore` Dash/Services/ExploreDatabaseSyncManager.swift at line 109, Update the log call in ExploreDatabaseSyncManager so it uses the repo's emoji-prefixed format; replace the existing DSLogger.log("ExploreDash: local explore.db missing, forcing cloud database download") invocation with a message that includes the emoji prefix (e.g., "🔍 ExploreDash: local explore.db missing, forcing cloud database download") so logs remain filterable and consistent with DSLogger.log usage across the project.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendPayIntro.swift (1)
94-96: 💤 Low valueAvoid trailing closure when passing both
action:andlabel:.SwiftLint flags
multiple_closures_with_trailing_closurehere. Switching to the explicitButton { } label: { }form is the idiomatic fix and silences the warning.♻️ Proposed refactor
- Button(action: { withAnimation(.easeInOut(duration: 0.2)) { balanceHidden.toggle() } }) { - eyeIcon - } + Button { + withAnimation(.easeInOut(duration: 0.2)) { balanceHidden.toggle() } + } label: { + eyeIcon + }🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendPayIntro.swift around lines 94 - 96, Replace the trailing-closure Button usage to the explicit SwiftUI syntax to satisfy SwiftLint: change the Button call that currently uses Button(action: { withAnimation(.easeInOut(duration: 0.2)) { balanceHidden.toggle() } }) { eyeIcon } to the idiomatic form using a leading action closure and a labeled label closure (i.e., Button { withAnimation(.easeInOut(duration: 0.2)) { balanceHidden.toggle() } } label: { eyeIcon }), updating the instance in DashSpendPayIntro (the Button referencing balanceHidden and eyeIcon) so SwiftLint no longer reports multiple_closures_with_trailing_closure.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendMultiplePanel.swift (1)
54-63: ⚡ Quick winUse the new
Black1000Alpha8design token instead of a hardcoded RGB literal.This PR adds
Black1000Alpha8as a design token. Replacing the inlineColor(red: 10/255, green: 11/255, blue: 13/255).opacity(0.7)with the token keeps the chip consistent with the design system and resolves the SwiftLintoperator_usage_whitespacewarnings on this line.🎨 Proposed refactor
if let warning = inventoryWarning { Text(warning) .font(.caption) .foregroundColor(.white) .padding(.horizontal, 12) .padding(.vertical, 8) - .background(Color(red: 10/255, green: 11/255, blue: 13/255).opacity(0.7)) + .background(Color.black1000Alpha8) .cornerRadius(10) .offset(y: 20) }🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendMultiplePanel.swift around lines 54 - 63, Replace the hardcoded background color in the inventory warning chip with the new design token Black1000Alpha8: locate the conditional that renders Text(warning) in DashSpendMultiplePanel (the inventoryWarning block) and swap Color(red: 10/255, green: 11/255, blue: 13/255).opacity(0.7) for the project design token (e.g. use the token API available in your codebase such as Color.DesignTokens.Black1000Alpha8 or Color("Black1000Alpha8") depending on how tokens are exposed) so the chip uses Black1000Alpha8 and resolves the SwiftLint operator_usage_whitespace warnings.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardPurchaseSelectionSheet.swift (2)
78-84: 💤 Low valueIndex badges only render for 3+ cards — duplicate-priced 2-card case has no disambiguator.
cards.count > 2hides the numeric badge whenever there are exactly two cards. If both denominations shareformattedPrice(a plausible result of two providers/sources for the same amount), the rows become visually identical and the user cannot tell which is which until selection. Considercards.count > 1(always show a 1/2 badge when more than one card), or add another distinguishing field (provider/source/sourceId) to the row.♻️ Suggested adjustment
- if cards.count > 2 { + if cards.count > 1 { Text("\(index + 1)") .font(.caption1.weight(.medium)) .padding(.horizontal, 4) .background(Color.gray50) .clipShape(.rect(cornerRadius: 5)) }🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardPurchaseSelectionSheet.swift around lines 78 - 84, The numeric index badge is hidden by the condition cards.count > 2 which leaves two-item lists ambiguous when both cards share the same formattedPrice; update the condition to cards.count > 1 so the Text("\(index + 1)") badge is shown whenever there is more than one card, or alternatively augment the row to display an additional distinguishing field (e.g., provider/source or sourceId) alongside formattedPrice; locate the badge rendering in GiftCardPurchaseSelectionSheet.swift (the block containing cards.count > 2 and Text("\(index + 1)")) and replace the condition or add the extra identifier display so duplicate-priced two-card rows are disambiguated.
86-91: ⚖️ Poor tradeoffMagic string
"PiggyCards"repeated across views.
provideris aString?and"PiggyCards"literals are used in this file and inGiftCardDetailsPoweredBySection/HomeView. As multi-provider support grows, a typed enum (or static constant on the existing provider model) would prevent typos and make refactors straightforward.🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardPurchaseSelectionSheet.swift around lines 86 - 91, The code is using the magic string "PiggyCards" to test provider (provider: String?), which is error-prone; introduce a typed Provider enum (e.g., enum GiftCardProvider { case piggyCards, other(...) } or a static constant on the provider model) and replace string comparisons with enum checks: add an initializer or mapper from String? to GiftCardProvider? (preserving nil) and update usages in GiftCardPurchaseSelectionSheet (Icon selection), GiftCardDetailsPoweredBySection, and HomeView to compare against GiftCardProvider.piggyCards (or the static constant) instead of the literal "PiggyCards". Ensure the Icon selection uses the enum value (providerEnum == .piggyCards) and keep backward-compatible mapping for existing provider strings.
🤖 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 `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift:
- Line 136: Fix the typo in the DashSpendSinglePanel localization lookup by
replacing the misspelled key "Conttinue" with "Continue" where actionButtonText
is set (look for actionButtonText: NSLocalizedString("Conttinue", comment: "")
in DashSpendSinglePanel.swift) so it matches the other panels (e.g.,
DashSpendMultiplePanel) and avoids a duplicate/orphan localization entry.
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/DashSpendPayViewModel.swift:
- Around line 444-451: The denominations mapping drops decimal-formatted strings
because it uses Int($0); replace the compactMap closure in the newDenominations
assignment so it calls parseWholeDollarDenomination($0) instead (e.g.,
merchantInfo.denominations.compactMap { parseWholeDollarDenomination($0) }) to
correctly parse values like "5.00" or "10.0" and produce whole-dollar Ints; the
helper function parseWholeDollarDenomination(_:) is defined in this file and
should be used to populate self.denominations.
---
Outside diff comments:
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/DashSpendPayViewModel.swift:
- Around line 308-317: The calculation of dashAmountInSatoshis should apply
.rounded() before casting to UInt64 to avoid floating-point precision loss:
change the expression that creates dashAmountInSatoshis from
UInt64(giftCardInfo.amount * 100_000_000) to use giftCardInfo.amount.rounded()
(i.e., UInt64(giftCardInfo.amount.rounded() * 100_000_000)) so the satoshi
amount passed into sendCoinsService.sendCoins(address:amount:) matches the
quoted DASH price; update the assignment where dashAmountInSatoshis is defined
in DashSpendPayViewModel to use .rounded() on giftCardInfo.amount.
---
Duplicate comments:
In `@DashWallet/Sources/UI/Home/Views/HomeView.swift`:
- Around line 552-557: The onChange handler for selectedCardIndex overwrites an
explicit showBackButton set by onSelectCard because shouldShowBackButton
currently only checks txDetailRoute; update the predicate used by
shouldShowBackButton (the function or computed Bool referenced at its
declaration and used in the onChange block) to also return true when
selectedCardIndex != nil (or when a card is selected), so that onChange(of:
selectedCardIndex) doing showBackButton = shouldShowBackButton preserves the
intent; locate and modify the shouldShowBackButton computed property / function
and any callers (e.g., in onChange, onAppear) to include selectedCardIndex !=
nil in its logic so the back button remains visible after selection.
---
Nitpick comments:
In `@DashWallet/Sources/Models/Explore`
Dash/Services/ExploreDatabaseSyncManager.swift:
- Line 109: Update the log call in ExploreDatabaseSyncManager so it uses the
repo's emoji-prefixed format; replace the existing DSLogger.log("ExploreDash:
local explore.db missing, forcing cloud database download") invocation with a
message that includes the emoji prefix (e.g., "🔍 ExploreDash: local explore.db
missing, forcing cloud database download") so logs remain filterable and
consistent with DSLogger.log usage across the project.
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/Components/DashSpendMultiplePanel.swift:
- Around line 54-63: Replace the hardcoded background color in the inventory
warning chip with the new design token Black1000Alpha8: locate the conditional
that renders Text(warning) in DashSpendMultiplePanel (the inventoryWarning
block) and swap Color(red: 10/255, green: 11/255, blue: 13/255).opacity(0.7) for
the project design token (e.g. use the token API available in your codebase such
as Color.DesignTokens.Black1000Alpha8 or Color("Black1000Alpha8") depending on
how tokens are exposed) so the chip uses Black1000Alpha8 and resolves the
SwiftLint operator_usage_whitespace warnings.
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/Components/DashSpendPayIntro.swift:
- Around line 94-96: Replace the trailing-closure Button usage to the explicit
SwiftUI syntax to satisfy SwiftLint: change the Button call that currently uses
Button(action: { withAnimation(.easeInOut(duration: 0.2)) {
balanceHidden.toggle() } }) { eyeIcon } to the idiomatic form using a leading
action closure and a labeled label closure (i.e., Button {
withAnimation(.easeInOut(duration: 0.2)) { balanceHidden.toggle() } } label: {
eyeIcon }), updating the instance in DashSpendPayIntro (the Button referencing
balanceHidden and eyeIcon) so SwiftLint no longer reports
multiple_closures_with_trailing_closure.
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardPurchaseSelectionSheet.swift:
- Around line 78-84: The numeric index badge is hidden by the condition
cards.count > 2 which leaves two-item lists ambiguous when both cards share the
same formattedPrice; update the condition to cards.count > 1 so the
Text("\(index + 1)") badge is shown whenever there is more than one card, or
alternatively augment the row to display an additional distinguishing field
(e.g., provider/source or sourceId) alongside formattedPrice; locate the badge
rendering in GiftCardPurchaseSelectionSheet.swift (the block containing
cards.count > 2 and Text("\(index + 1)")) and replace the condition or add the
extra identifier display so duplicate-priced two-card rows are disambiguated.
- Around line 86-91: The code is using the magic string "PiggyCards" to test
provider (provider: String?), which is error-prone; introduce a typed Provider
enum (e.g., enum GiftCardProvider { case piggyCards, other(...) } or a static
constant on the provider model) and replace string comparisons with enum checks:
add an initializer or mapper from String? to GiftCardProvider? (preserving nil)
and update usages in GiftCardPurchaseSelectionSheet (Icon selection),
GiftCardDetailsPoweredBySection, and HomeView to compare against
GiftCardProvider.piggyCards (or the static constant) instead of the literal
"PiggyCards". Ensure the Icon selection uses the enum value (providerEnum ==
.piggyCards) and keep backward-compatible mapping for existing provider strings.
🪄 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: 9cb76500-9eab-4cf8-9230-73d45d0d889d
📒 Files selected for processing (13)
DashWallet.xcodeproj/project.pbxprojDashWallet/Sources/Models/Explore Dash/Services/ExploreDatabaseSyncManager.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendFixedContent.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendFlexibleContent.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendMultiplePanel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendPayIntro.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendSinglePanel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsMerchantHeader.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardPurchaseSelectionSheet.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/GiftCardDetailsViewModel.swiftDashWallet/Sources/UI/Home/Views/HomeView.swiftDashWallet/Sources/UI/SwiftUI Components/NumericKeyboardView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsMerchantHeader.swift
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift (1)
309-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse rounding when converting PiggyCards payment amount from DASH to satoshis.
GiftCardInfo.amountis aDouble. When you computeUInt64(giftCardInfo.amount * 100_000_000), the multiplication produces a Double result that is then truncated toward zero on the integer cast. For example,0.29becomes28_999_999.999...and truncates to28_999_999satoshis, underpaying by one satoshi—enough to fail the transaction.Round the result before casting to
UInt64, or useDecimalthroughout this conversion path to preserve precision.🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/DashSpendPayViewModel.swift around lines 309 - 316, The conversion multiplying GiftCardInfo.amount (a Double) by 100_000_000 and casting directly to UInt64 truncates fractional satoshis and can underpay (e.g., 0.29 → 28_999_999); update the calculation that sets dashAmountInSatoshis so it rounds the Double result before casting (e.g., use round(...) or construct via Decimal and NSDecimalNumber/Decimal rounding) to ensure exact satoshi amounts, then pass that rounded UInt64 into sendCoinsService.sendCoins(address: giftCardInfo.paymentAddress, amount: dashAmountInSatoshis) so PIN authorization flow remains unchanged.
🧹 Nitpick comments (5)
DashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift (4)
64-64: ⚡ Quick winMove
@ViewBuilderattributes to their own lines.SwiftLint flags these
@ViewBuilderattributes as they should be on separate lines per the project's style configuration.🎨 Proposed fix
+ `@ViewBuilder` private var costMessageView: some View { - `@ViewBuilder` private var costMessageView: some View { + `@ViewBuilder` private var limitsView: some View { - `@ViewBuilder` private var limitsView: some View {Also applies to: 76-76
🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift at line 64, SwiftLint wants the `@ViewBuilder` attributes on their own lines for the view properties; locate the property declarations for costMessageView and the other similar property (the one starting around line 76) and move the `@ViewBuilder` attribute so it sits on its own line immediately above the property declaration (e.g., place "@ViewBuilder" on a separate line before "private var costMessageView: some View" and do the same for the other annotated view property) to satisfy the style rule.
136-136: ⚡ Quick winAdd descriptive comment to localization string.
The localization comment is empty. Add a descriptive comment like
"DashSpend"to help translators understand the context, consistent with other localized strings in this PR (e.g., line 102).📝 Proposed fix
- actionButtonText: NSLocalizedString("Continue", comment: ""), + actionButtonText: NSLocalizedString("Continue", comment: "DashSpend"),🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift at line 136, The NSLocalizedString call used for actionButtonText in DashSpendSinglePanel.swift has an empty comment; update the comment argument to a descriptive context (e.g., "DashSpend") so translators understand the usage. Locate the actionButtonText: NSLocalizedString("Continue", comment: "") and replace the empty comment with a meaningful string matching other entries in this PR (for example "DashSpend") to keep comments consistent.
25-25: ⚡ Quick winRemove commented-out code.
Commented-out code should be removed before merging. If this padding was intentionally removed, delete the line. If it needs to be reconsidered later, document the reason in a TODO comment or track it in an issue.
🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift at line 25, In DashSpendSinglePanel.swift remove the leftover commented-out layout line "// .padding(.bottom, 30)" from the view body; if the padding change was intentional and needs tracking, replace the commented line with a concise TODO comment referencing the decision or link to an issue ID instead of leaving commented code (locate the commented line near the DashSpendSinglePanel view declaration and update accordingly).
214-214: ⚡ Quick winRemove redundant
nilinitialization.SwiftLint flags this optional initialization. Optionals are implicitly
nilin Swift, so the explicit= nilis redundant.🎨 Proposed fix
- `@State` private var selected: Int? = nil + `@State` private var selected: Int?🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift at line 214, The `@State` variable selected is explicitly initialized to nil which is redundant; update the declaration of selected (the `@State` private var selected: Int? property in DashSpendSinglePanel) to remove the "= nil" so it becomes simply `@State` private var selected: Int? to satisfy SwiftLint and rely on Swift's implicit nil initialization for optionals.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift (1)
414-613: 🏗️ Heavy liftSplit
updateMerchantInfo()into provider-specific normalization helpers.This method is now doing fetch orchestration, DTO parsing, PiggyCards denomination reconciliation, and UI-state publication in one place, which is why it already trips the complexity gate. Extract the CTX and PiggyCards normalization into helper/service methods that return a single normalized state object, then apply that state on the main actor here. That will make this flow much easier to reason about and much safer to change. As per coding guidelines, "Use service layer with dedicated services for major functionality (CoinJoin, networking, database)."
🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/DashSpendPayViewModel.swift around lines 414 - 613, Refactor updateMerchantInfo() by extracting provider-specific normalization into two helper methods (e.g., normalizeCTXMerchant(merchantInfo:)->DashSpendNormalizedState and normalizePiggyCardsState(giftCards:sourceId:)->DashSpendNormalizedState) so the method only orchestrates fetches and applies a single normalized state on the MainActor; move all DTO parsing and reconciliation logic currently using ctxSpendRepository.getMerchant, PiggyCardsRepository.getGiftCards, parseWholeDollarDenomination, providerConfiguredIsFixed, denomDiscounts, denomInventory, rangeMin/rangeMax, etc. into those helpers (they should compute isFixedDenomination, minimumAmount, maximumAmount, denominations, savingsFraction, denominationDiscounts and denominationInventory), then call await MainActor.run once to set self.* from the returned DashSpendNormalizedState and finish with the existing isLoading/objectWillChange/checkAmountForErrors 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 `@DashWallet/Sources/Models/Transactions/SendCoinsService.swift`:
- Around line 99-107: In payWithDashUrl, authentication is performed before
validating the payment URL/request, causing unnecessary biometric/PIN prompts
for malformed links; move the URL parsing and payment request validation (the
guard for URL(string: paymentUrlString) and the subsequent payment request
creation/validation code around lines 115–117) to occur before calling
authenticateForPayment(), and only call authenticateForPayment() when the
paymentUrl and payment request are successfully validated; update any related
error throws (e.g., DashSpendError.paymentProcessingError("Invalid payment
URL")) to be returned prior to invoking authenticateForPayment().
- Around line 28-36: authenticateForPayment() currently returns a Bool and
collapses "failed auth" and "user cancelled" into false, causing callers to
always treat failures as "Authentication cancelled"; change
authenticateForPayment (and the similar calls around the other occurrences) to
surface the distinct outcome by returning an enum (e.g., AuthenticationOutcome {
case success, cancelled, failed }) or by throwing specific errors (e.g.,
AuthenticationCancelledError vs AuthenticationFailedError) instead of Bool,
update the continuation to resume with the appropriate enum/call throw for
DSAuthenticationManager.sharedInstance()'s authenticatedOrSuccess and cancelled
values, and then update the callers in this file to handle/propagate the
distinct cases (throwing AuthenticationCancelledError only when cancelled is
true, and a different error when auth failed).
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift:
- Line 143: The .background(ignoresSafeAreaEdges: .top) modifier usage in
DashSpendSinglePanel is invalid because it omits the required content parameter;
update the modifier on the view inside the DashSpendSinglePanel (where
.background(ignoresSafeAreaEdges: .top) is applied) to either call
.ignoresSafeArea(edges: .top) to simply ignore the top safe area, or supply a
background view/color as the first parameter to
.background(_:ignoresSafeAreaEdges:) if you intend to set a background; adjust
the modifier placement on the view so the corrected modifier replaces the broken
.background call.
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/DashSpendPayViewModel.swift:
- Around line 93-96: updateTotalAmount(_:) only sets amount, leaving
savingsFraction stale and costMessage wrong for mixed-denomination baskets;
modify updateTotalAmount(_:) to accept or access the current selected
basket/quantities (e.g., selectedQuantities or basket) and recompute the
checkout total and savingsFraction from that basket before updating amount and
calling checkAmountForErrors(); in practice either add a parameter like
updateTotalAmount(_ total: Decimal, selectedQuantities: [Denom: Int]) or call an
existing helper (implement computeCheckoutTotal(from:),
computeSavingsFraction(for:)) inside updateTotalAmount(_:), set amount to the
recomputed checkout total, update savingsFraction, then refresh costMessage and
call checkAmountForErrors().
- Around line 446-451: After updating denominations in the MainActor.run block,
clear any stale selection by resetting selectedDenomination (and any related
selection state used by showCost) so a previously cached choice from local
merchant data cannot persist when the CTX response provides a new fixed list;
mirror the PiggyCards reset logic by setting selectedDenomination = nil (and if
present, selectedDenominationIndex or similar selection properties) immediately
after assigning self.denominations so UI logic (showCost) treats no selection as
current until the user picks a valid new denomination.
---
Outside diff comments:
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/DashSpendPayViewModel.swift:
- Around line 309-316: The conversion multiplying GiftCardInfo.amount (a Double)
by 100_000_000 and casting directly to UInt64 truncates fractional satoshis and
can underpay (e.g., 0.29 → 28_999_999); update the calculation that sets
dashAmountInSatoshis so it rounds the Double result before casting (e.g., use
round(...) or construct via Decimal and NSDecimalNumber/Decimal rounding) to
ensure exact satoshi amounts, then pass that rounded UInt64 into
sendCoinsService.sendCoins(address: giftCardInfo.paymentAddress, amount:
dashAmountInSatoshis) so PIN authorization flow remains unchanged.
---
Nitpick comments:
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/Components/DashSpendSinglePanel.swift:
- Line 64: SwiftLint wants the `@ViewBuilder` attributes on their own lines for
the view properties; locate the property declarations for costMessageView and
the other similar property (the one starting around line 76) and move the
`@ViewBuilder` attribute so it sits on its own line immediately above the property
declaration (e.g., place "@ViewBuilder" on a separate line before "private var
costMessageView: some View" and do the same for the other annotated view
property) to satisfy the style rule.
- Line 136: The NSLocalizedString call used for actionButtonText in
DashSpendSinglePanel.swift has an empty comment; update the comment argument to
a descriptive context (e.g., "DashSpend") so translators understand the usage.
Locate the actionButtonText: NSLocalizedString("Continue", comment: "") and
replace the empty comment with a meaningful string matching other entries in
this PR (for example "DashSpend") to keep comments consistent.
- Line 25: In DashSpendSinglePanel.swift remove the leftover commented-out
layout line "// .padding(.bottom, 30)" from the view body; if the
padding change was intentional and needs tracking, replace the commented line
with a concise TODO comment referencing the decision or link to an issue ID
instead of leaving commented code (locate the commented line near the
DashSpendSinglePanel view declaration and update accordingly).
- Line 214: The `@State` variable selected is explicitly initialized to nil which
is redundant; update the declaration of selected (the `@State` private var
selected: Int? property in DashSpendSinglePanel) to remove the "= nil" so it
becomes simply `@State` private var selected: Int? to satisfy SwiftLint and rely
on Swift's implicit nil initialization for optionals.
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/DashSpendPayViewModel.swift:
- Around line 414-613: Refactor updateMerchantInfo() by extracting
provider-specific normalization into two helper methods (e.g.,
normalizeCTXMerchant(merchantInfo:)->DashSpendNormalizedState and
normalizePiggyCardsState(giftCards:sourceId:)->DashSpendNormalizedState) so the
method only orchestrates fetches and applies a single normalized state on the
MainActor; move all DTO parsing and reconciliation logic currently using
ctxSpendRepository.getMerchant, PiggyCardsRepository.getGiftCards,
parseWholeDollarDenomination, providerConfiguredIsFixed, denomDiscounts,
denomInventory, rangeMin/rangeMax, etc. into those helpers (they should compute
isFixedDenomination, minimumAmount, maximumAmount, denominations,
savingsFraction, denominationDiscounts and denominationInventory), then call
await MainActor.run once to set self.* from the returned
DashSpendNormalizedState and finish with the existing
isLoading/objectWillChange/checkAmountForErrors flow.
🪄 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: 472b8ba9-3673-488b-b70e-6b60e366e57c
📒 Files selected for processing (4)
DashWallet.xcodeproj/project.pbxprojDashWallet/Sources/Models/Transactions/SendCoinsService.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendSinglePanel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift (1)
421-628:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRefactor
updateMerchantInfo()to pass configured SwiftLint complexity limits.This method is currently too complex (reported as 26), which is likely to fail linting and make future logic changes riskier. Split provider-specific branches into dedicated helpers and keep this method as orchestration only.
As per coding guidelines, "
**/*.swift: Use SwiftFormat and SwiftLint for Swift code formatting and linting as configured in .swiftformat and .swiftlint.yml files."🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/DashSpendPayViewModel.swift around lines 421 - 628, The updateMerchantInfo() function is too complex; refactor it into a thin orchestrator that delegates provider-specific work to new helper methods. Extract the CTX branch into a helper like loadCTXMerchantInfo(merchantId:) that calls ctxSpendRepository.getMerchant(...), computes newSavings/newMin/newMax/newDenominations and updates MainActor state, and extract the PiggyCards branch into loadPiggyCardsMerchantInfo(sourceId:merchantId:) that performs giftCards aggregation (allFixedDenominations, denomDiscounts, denomInventory, rangeMin/max/discount), computes finalFixed/finalMin/finalMax/finalDenominations/finalSavings and updates MainActor state (denominationDiscounts, denominationInventory, isFixedDenomination, minimumAmount, maximumAmount, denominations, savingsFraction). Keep updateMerchantInfo() responsible only for validation (merchantId, requiresAuth), setting isLoading, invoking the appropriate helper with await, and final common UI updates/error handling.
🧹 Nitpick comments (1)
DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift (1)
584-587: ⚡ Quick winAdd emoji prefixes to DSLogger entries for log filtering consistency.
These new logs should use the repository’s emoji markers (for example,
🔍/🎯) to stay searchable and consistent with existing debugging conventions.As per coding guidelines, "Use emoji debug markers for easy log filtering: 🎯 🌐 💾 🎨 🔍 📍 🗺️ (e.g., DSLogger.log("🔍 Message:"))."
Also applies to: 615-615, 664-664
🤖 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 `@DashWallet/Sources/UI/Explore` Dash/Views/DashSpend/DashSpendPayViewModel.swift around lines 584 - 587, The DSLogger.log call that prints piggy card mode and range is missing the repository's emoji debug marker convention; update the DSLogger.log invocation(s) (see DSLogger.log in this function and the other similar calls around the piggy card logic) to prefix the message with the appropriate emoji marker (e.g., "🔍" or "🎯") while keeping the existing payload that references finalFixed, finalMin, finalMax and finalDenominations.count so logs remain searchable and unchanged in content.
🤖 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 `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/Components/DashSpendFlexibleContent.swift:
- Around line 58-64: multipleActionEnabled currently omits the
payment-in-progress flag, and the button’s inProgress is hardcoded to false;
update the predicate in the multipleActionEnabled computed property to also
require !viewModel.isProcessingPayment, and change any places where the button’s
inProgress is set to false (e.g., the multiple-mode purchase button usage around
the inProgress parameter) to use viewModel.isProcessingPayment so the UI
disables and shows progress while a payment is processing; apply the same change
to the similar logic referenced at the other occurrence (lines 223–225).
---
Outside diff comments:
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/DashSpendPayViewModel.swift:
- Around line 421-628: The updateMerchantInfo() function is too complex;
refactor it into a thin orchestrator that delegates provider-specific work to
new helper methods. Extract the CTX branch into a helper like
loadCTXMerchantInfo(merchantId:) that calls ctxSpendRepository.getMerchant(...),
computes newSavings/newMin/newMax/newDenominations and updates MainActor state,
and extract the PiggyCards branch into
loadPiggyCardsMerchantInfo(sourceId:merchantId:) that performs giftCards
aggregation (allFixedDenominations, denomDiscounts, denomInventory,
rangeMin/max/discount), computes
finalFixed/finalMin/finalMax/finalDenominations/finalSavings and updates
MainActor state (denominationDiscounts, denominationInventory,
isFixedDenomination, minimumAmount, maximumAmount, denominations,
savingsFraction). Keep updateMerchantInfo() responsible only for validation
(merchantId, requiresAuth), setting isLoading, invoking the appropriate helper
with await, and final common UI updates/error handling.
---
Nitpick comments:
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/DashSpendPayViewModel.swift:
- Around line 584-587: The DSLogger.log call that prints piggy card mode and
range is missing the repository's emoji debug marker convention; update the
DSLogger.log invocation(s) (see DSLogger.log in this function and the other
similar calls around the piggy card logic) to prefix the message with the
appropriate emoji marker (e.g., "🔍" or "🎯") while keeping the existing payload
that references finalFixed, finalMin, finalMax and finalDenominations.count so
logs remain searchable and unchanged in content.
🪄 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: 07f05e82-4804-4066-8ed0-99c115311bdf
📒 Files selected for processing (6)
DashWallet/Sources/Models/Transactions/SendCoinsService.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendFixedContent.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendFlexibleContent.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendSinglePanel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayScreen.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Issue being fixed or feature implemented
MO-896— refactor DashSpend confirmation/card-details flow and improve sheet behavior for gift card details presentation.What was done?
GiftCardDetails/folder with:GiftCardDetailsViewGiftCardDetailsViewModelInfoCard,MerchantHeader,HowToUseSection,PoweredBySection,PurchaseSelectionSheet)GiftCardDetailsView(.swift/.ViewModel.swift)files in old locationHomeView/HomeViewControllerrouting and sheet wiringBlack1000Alpha5Black1000Alpha8How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Improvements
Bug Fixes