Open
Conversation
- Export CSV - Zenledger Changed the navigation bar for sheet "Import private key"
📝 WalkthroughWalkthroughAdds a CSV export SwiftUI sheet and related assets, updates Tools menu to present the sheet with iOS-version-specific detents, refactors several sheets to use a shared NavBarClose, and bumps MARKETING_VERSION from 8.5.5 to 8.5.6 in the Xcode project. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Menu as ToolsMenuScreen
participant Sheet as CSVExportSheet
participant Exporter as ExportService
User->>Menu: Tap "Export CSV"
Menu->>Sheet: present sheet (showCSVExportSheet = true)
User->>Sheet: Tap "Export CSV" button
Sheet->>Sheet: dismiss sheet
Sheet->>Exporter: call onExport()
Exporter-->>User: export completes / shares CSV
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
ross-dash
changed the title
ross-dash
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
DashWallet/Sources/UI/Menu/Tools/CSVExportSheet.swift (2)
81-84: Consider deferringonExport()until after sheet dismissal completes.Calling
onExport()immediately afterdismiss()works in this case becausehandleCSVExportinToolsMenuScreenstarts an async Task. However, for more predictable sequencing, you could use the sheet'sonDismisscallback pattern (similar to howZenLedgerInfoSheethandlessafariLink).Current behavior is functional, so this is just a consistency consideration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DashWallet/Sources/UI/Menu/Tools/CSVExportSheet.swift` around lines 81 - 84, The CSVExportSheet currently calls presentationMode.wrappedValue.dismiss() then immediately invokes onExport(), which can race with dismissal; update CSVExportSheet to defer calling onExport() until the sheet's dismissal completes by moving the export trigger into an onDismiss handler (similar to ZenLedgerInfoSheet's safariLink pattern): store a local Bool (e.g., shouldTriggerExport) or set a closure to be called from onDismiss, set that flag/closure in the current action instead of calling onExport() directly, and then call onExport() from the sheet's onDismiss so ToolsMenuScreen.handleCSVExport runs after dismissal.
22-25: Place@Environmentattributes on separate lines per SwiftLint rule.SwiftLint flags these attributes for placement on the same line as their property declarations.
♻️ Proposed fix
struct CSVExportSheet: View { - `@Environment`(\.presentationMode) private var presentationMode - `@Environment`(\.colorScheme) private var colorScheme + `@Environment`(\.presentationMode) + private var presentationMode + `@Environment`(\.colorScheme) + private var colorScheme let onExport: () -> Void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DashWallet/Sources/UI/Menu/Tools/CSVExportSheet.swift` around lines 22 - 25, The `@Environment` attributes in the CSVExportSheet struct are placed on the same line as their property declarations which violates SwiftLint; move each attribute to its own line directly above the corresponding property (i.e., place `@Environment`(\.presentationMode) on its own line above private var presentationMode and `@Environment`(\.colorScheme) on its own line above private var colorScheme) while keeping the let onExport: () -> Void unchanged.DashWallet/Sources/UI/Menu/Tools/ZenLedger/ZenLedgerInfoSheet.swift (2)
25-27: Place@Environmentattributes on separate lines per SwiftLint rule.The
@Environment(\.colorScheme)attribute should be on its own line.♻️ Proposed fix
`@Environment`(\.presentationMode) private var presentationMode - `@Environment`(\.colorScheme) private var colorScheme + `@Environment`(\.colorScheme) + private var colorScheme🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DashWallet/Sources/UI/Menu/Tools/ZenLedger/ZenLedgerInfoSheet.swift` around lines 25 - 27, The two `@Environment` property wrappers are placed on the same line; split them so each attribute is on its own line to satisfy SwiftLint. Update the declarations in ZenLedgerInfoSheet (the `@Environment`(\.presentationMode) private var presentationMode and `@Environment`(\.colorScheme) private var colorScheme) so each `@Environment`(...) is written on a separate line above its corresponding property.
70-78: Avoid trailing closure syntax with multiple closure arguments.SwiftLint flags this Button because it uses trailing closure syntax when passing both
actionandlabelclosures.♻️ Proposed fix using explicit label parameter
- Button(action: { + Button( + action: { safariLink = "https://app.zenledger.io/new_sign_up/" presentationMode.wrappedValue.dismiss() - }) { - Text("zenledger.io") - .font(.system(size: 13, weight: .semibold)) - .foregroundColor(.blue) - } + }, + label: { + Text("zenledger.io") + .font(.system(size: 13, weight: .semibold)) + .foregroundColor(.blue) + } + ) .padding(.top, 8)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DashWallet/Sources/UI/Menu/Tools/ZenLedger/ZenLedgerInfoSheet.swift` around lines 70 - 78, SwiftLint complains because the Button in ZenLedgerInfoSheet uses trailing-closure syntax while supplying both an action and a label closure; change the Button to the explicit two-argument form so the label is passed via the label parameter (e.g. Button(action: { safariLink = "https://app.zenledger.io/new_sign_up/"; presentationMode.wrappedValue.dismiss() }, label: { Text("zenledger.io").font(.system(size: 13, weight: .semibold)).foregroundColor(.blue) })). Keep the existing .padding(.top, 8) modifier and references to safariLink and presentationMode as-is.DashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swift (2)
240-264: Wrap debug print statements in#if DEBUGblocks.Multiple
print()statements throughout this method and inQRScannerDelegateare not wrapped in conditional compilation, which can impact performance in release builds.As per coding guidelines: "Wrap debug print statements in
#ifDEBUG blocks or use conditional compilation to prevent performance impact from excessive logging."♻️ Proposed fix
private func presentScanner() { - print("🔍 DEBUG: presentScanner called in ToolsMenuScreen") + `#if` DEBUG + print("🔍 DEBUG: presentScanner called in ToolsMenuScreen") + `#endif` // Wait for sheet dismissal animation to complete before presenting scanner DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) { let scannerVC = DWQRScanViewController() scannerVC.modalPresentationStyle = .fullScreen // Create and set delegate let delegate = QRScannerDelegate(viewController: scannerVC) self.scannerDelegate = delegate scannerVC.model.delegate = delegate - print("🔍 DEBUG: About to present scanner from vc: \(self.vc)") + `#if` DEBUG + print("🔍 DEBUG: About to present scanner from vc: \(self.vc)") + `#endif` // Present from the topmost view controller in the navigation stack if let topVC = self.vc.topViewController { - print("🔍 DEBUG: Presenting from top VC: \(topVC)") + `#if` DEBUG + print("🔍 DEBUG: Presenting from top VC: \(topVC)") + `#endif` topVC.present(scannerVC, animated: true) { - print("🔍 DEBUG: Scanner presented successfully") + `#if` DEBUG + print("🔍 DEBUG: Scanner presented successfully") + `#endif` } } else { - print("🔍 DEBUG: No top VC, presenting from nav controller") + `#if` DEBUG + print("🔍 DEBUG: No top VC, presenting from nav controller") + `#endif` self.vc.present(scannerVC, animated: true, completion: nil) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swift` around lines 240 - 264, The debug print statements in ToolsMenuScreen (the presentScanner block) and in QRScannerDelegate should be conditionally compiled so they are omitted from release builds; wrap each print(...) call inside a `#if` DEBUG / `#endif` block (or replace with a debug-only logging helper) around the prints in the presentScanner logic where DWQRScanViewController is created and presented and also in QRScannerDelegate where similar prints exist, ensuring scannerDelegate, scannerVC presentation logs, and any delegate debug logs are only executed in DEBUG builds.
288-308: Wrap debug print statements in#if DEBUGblocks in QRScannerDelegate.Same issue as above - the delegate class contains multiple unguarded
print()statements.♻️ Proposed fix
init(viewController: UIViewController) { self.viewController = viewController - print("🔍 DEBUG: QRScannerDelegate initialized in ToolsMenuScreen") + `#if` DEBUG + print("🔍 DEBUG: QRScannerDelegate initialized in ToolsMenuScreen") + `#endif` } func qrScanModel(_ viewModel: DWQRScanModel, didScanPaymentInput paymentInput: DWPaymentInput) { - print("🔍 DEBUG: didScanPaymentInput called") + `#if` DEBUG + print("🔍 DEBUG: didScanPaymentInput called") + `#endif` viewController?.dismiss(animated: true) { - print("🔍 DEBUG: Scanner dismissed after scan") + `#if` DEBUG + print("🔍 DEBUG: Scanner dismissed after scan") + `#endif` } } func qrScanModelDidCancel(_ viewModel: DWQRScanModel) { - print("🔍 DEBUG: qrScanModelDidCancel called - X button pressed") + `#if` DEBUG + print("🔍 DEBUG: qrScanModelDidCancel called - X button pressed") + `#endif` viewController?.dismiss(animated: true) { - print("🔍 DEBUG: Scanner dismissed after cancel") + `#if` DEBUG + print("🔍 DEBUG: Scanner dismissed after cancel") + `#endif` } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swift` around lines 288 - 308, The QRScannerDelegate class currently contains unguarded print() debug statements; wrap each debug print in a conditional compile block so they only run in debug builds (use `#if` DEBUG / `#endif`). Specifically update the initializer in QRScannerDelegate and the two delegate methods qrScanModel(_:didScanPaymentInput:) and qrScanModelDidCancel(_:) to enclose their print(...) calls inside `#if` DEBUG blocks, preserving existing behavior otherwise and keeping the dismiss calls unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DashWallet/Sources/UI/SwiftUI` Components/NavigationBar.swift:
- Line 1: Remove the leading whitespace from the header comment in
NavigationBar.swift (line 1) so the file starts with the comment marker at
column 0; update the top-of-file comment to have no leading space to satisfy the
leading_whitespace SwiftLint rule and ensure SwiftFormat/SwiftLint pass for this
file.
---
Nitpick comments:
In `@DashWallet/Sources/UI/Menu/Tools/CSVExportSheet.swift`:
- Around line 81-84: The CSVExportSheet currently calls
presentationMode.wrappedValue.dismiss() then immediately invokes onExport(),
which can race with dismissal; update CSVExportSheet to defer calling onExport()
until the sheet's dismissal completes by moving the export trigger into an
onDismiss handler (similar to ZenLedgerInfoSheet's safariLink pattern): store a
local Bool (e.g., shouldTriggerExport) or set a closure to be called from
onDismiss, set that flag/closure in the current action instead of calling
onExport() directly, and then call onExport() from the sheet's onDismiss so
ToolsMenuScreen.handleCSVExport runs after dismissal.
- Around line 22-25: The `@Environment` attributes in the CSVExportSheet struct
are placed on the same line as their property declarations which violates
SwiftLint; move each attribute to its own line directly above the corresponding
property (i.e., place `@Environment`(\.presentationMode) on its own line above
private var presentationMode and `@Environment`(\.colorScheme) on its own line
above private var colorScheme) while keeping the let onExport: () -> Void
unchanged.
In `@DashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swift`:
- Around line 240-264: The debug print statements in ToolsMenuScreen (the
presentScanner block) and in QRScannerDelegate should be conditionally compiled
so they are omitted from release builds; wrap each print(...) call inside a `#if`
DEBUG / `#endif` block (or replace with a debug-only logging helper) around the
prints in the presentScanner logic where DWQRScanViewController is created and
presented and also in QRScannerDelegate where similar prints exist, ensuring
scannerDelegate, scannerVC presentation logs, and any delegate debug logs are
only executed in DEBUG builds.
- Around line 288-308: The QRScannerDelegate class currently contains unguarded
print() debug statements; wrap each debug print in a conditional compile block
so they only run in debug builds (use `#if` DEBUG / `#endif`). Specifically update
the initializer in QRScannerDelegate and the two delegate methods
qrScanModel(_:didScanPaymentInput:) and qrScanModelDidCancel(_:) to enclose
their print(...) calls inside `#if` DEBUG blocks, preserving existing behavior
otherwise and keeping the dismiss calls unchanged.
In `@DashWallet/Sources/UI/Menu/Tools/ZenLedger/ZenLedgerInfoSheet.swift`:
- Around line 25-27: The two `@Environment` property wrappers are placed on the
same line; split them so each attribute is on its own line to satisfy SwiftLint.
Update the declarations in ZenLedgerInfoSheet (the
`@Environment`(\.presentationMode) private var presentationMode and
`@Environment`(\.colorScheme) private var colorScheme) so each `@Environment`(...)
is written on a separate line above its corresponding property.
- Around line 70-78: SwiftLint complains because the Button in
ZenLedgerInfoSheet uses trailing-closure syntax while supplying both an action
and a label closure; change the Button to the explicit two-argument form so the
label is passed via the label parameter (e.g. Button(action: { safariLink =
"https://app.zenledger.io/new_sign_up/"; presentationMode.wrappedValue.dismiss()
}, label: { Text("zenledger.io").font(.system(size: 13, weight:
.semibold)).foregroundColor(.blue) })). Keep the existing .padding(.top, 8)
modifier and references to safariLink and presentationMode as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 929eea99-ec61-4505-bdd2-101ee05c50f2
⛔ Files ignored due to path filters (6)
DashWallet/Resources/AppAssets.xcassets/Illustration/csv-export-large.imageset/export.csv.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/Illustration/csv-export-large.imageset/export.csv@2x.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/Illustration/csv-export-large.imageset/export.csv@3x.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/Illustration/zenledger-large.imageset/zenledger-large.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/Illustration/zenledger-large.imageset/zenledger-large@2x.pngis excluded by!**/*.pngDashWallet/Resources/AppAssets.xcassets/Illustration/zenledger-large.imageset/zenledger-large@3x.pngis excluded by!**/*.png
📒 Files selected for processing (9)
DashWallet.xcodeproj/project.pbxprojDashWallet/Resources/AppAssets.xcassets/Illustration/Contents.jsonDashWallet/Resources/AppAssets.xcassets/Illustration/csv-export-large.imageset/Contents.jsonDashWallet/Resources/AppAssets.xcassets/Illustration/zenledger-large.imageset/Contents.jsonDashWallet/Sources/UI/Menu/Tools/CSVExportSheet.swiftDashWallet/Sources/UI/Menu/Tools/ImportWallet/ImportPrivateKeySheet.swiftDashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swiftDashWallet/Sources/UI/Menu/Tools/ZenLedger/ZenLedgerInfoSheet.swiftDashWallet/Sources/UI/SwiftUI Components/NavigationBar.swift
| @@ -1,4 +1,4 @@ | |||
| // | |||
| // | |||
There was a problem hiding this comment.
Remove leading whitespace in the header comment.
Line 1 starts with a leading space and triggers the leading_whitespace lint warning.
[sraise]
Proposed fix
- //
+//As per coding guidelines, "Use SwiftFormat and SwiftLint for Swift code formatting and linting as configured in .swiftformat and .swiftlint.yml files."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // | |
| // |
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 1-1: Files should not contain leading whitespace
(leading_whitespace)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DashWallet/Sources/UI/SwiftUI` Components/NavigationBar.swift at line 1,
Remove the leading whitespace from the header comment in NavigationBar.swift
(line 1) so the file starts with the comment marker at column 0; update the
top-of-file comment to have no leading space to satisfy the leading_whitespace
SwiftLint rule and ensure SwiftFormat/SwiftLint pass for this file.
This reverts commit 527ebef.
…to feat/update-csv-export-and-zenledger
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swift (1)
125-135: LGTM - Consider extracting repeated version-checking pattern.The iOS version-aware sheet presentation is correct. The same pattern is repeated for CSV export, ZenLedger, ImportPrivateKey, and ExtendedPublicKey sheets (lines 125-182). This could be extracted into a reusable
ViewModifierto reduce duplication.💡 Optional: Extract to a reusable modifier
struct AdaptiveSheetModifier<SheetContent: View>: ViewModifier { `@Binding` var isPresented: Bool let detent: PresentationDetent let cornerRadius: CGFloat let content: () -> SheetContent func body(content: Content) -> some View { content.sheet(isPresented: $isPresented) { if `#available`(iOS 16.4, *) { self.content() .presentationDetents([detent]) .presentationCornerRadius(cornerRadius) } else if `#available`(iOS 16.0, *) { self.content() .presentationDetents([detent]) } else { self.content() } } } }Usage:
.modifier(AdaptiveSheetModifier( isPresented: $showCSVExportSheet, detent: .large, cornerRadius: 32 ) { CSVExportSheet(onExport: handleCSVExport) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swift` around lines 125 - 135, Extract the repeated iOS-version sheet presentation logic in ToolsMenuScreen (used for showCSVExportSheet with CSVExportSheet and similarly for ZenLedger, ImportPrivateKey, ExtendedPublicKey) into a reusable ViewModifier (e.g., AdaptiveSheetModifier) that accepts a Binding<Bool> isPresented, detent and cornerRadius plus a content closure; replace each .sheet(...) block in ToolsMenuScreen with .modifier(AdaptiveSheetModifier(isPresented: $showCSVExportSheet, detent: .large, cornerRadius: 32) { CSVExportSheet(onExport: handleCSVExport) }) and similarly for the ZenLedger/ImportPrivateKey/ExtendedPublicKey handlers so the `#available` checks live only inside the modifier and duplication is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@DashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swift`:
- Around line 125-135: Extract the repeated iOS-version sheet presentation logic
in ToolsMenuScreen (used for showCSVExportSheet with CSVExportSheet and
similarly for ZenLedger, ImportPrivateKey, ExtendedPublicKey) into a reusable
ViewModifier (e.g., AdaptiveSheetModifier) that accepts a Binding<Bool>
isPresented, detent and cornerRadius plus a content closure; replace each
.sheet(...) block in ToolsMenuScreen with
.modifier(AdaptiveSheetModifier(isPresented: $showCSVExportSheet, detent:
.large, cornerRadius: 32) { CSVExportSheet(onExport: handleCSVExport) }) and
similarly for the ZenLedger/ImportPrivateKey/ExtendedPublicKey handlers so the
`#available` checks live only inside the modifier and duplication is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cbf1d4c-a07f-4db9-803e-4f3cfac81dbf
📒 Files selected for processing (4)
DashWallet.xcodeproj/project.pbxprojDashWallet/Sources/UI/Menu/Tools/ImportWallet/ImportPrivateKeySheet.swiftDashWallet/Sources/UI/Menu/Tools/ToolsMenuScreen.swiftDashWallet/en.lproj/Localizable.strings
✅ Files skipped from review due to trivial changes (1)
- DashWallet/en.lproj/Localizable.strings
🚧 Files skipped from review as they are similar to previous changes (1)
- DashWallet/Sources/UI/Menu/Tools/ImportWallet/ImportPrivateKeySheet.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changed the navigation bar for sheet "Import private key"
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Improvements
Version