fix(wallet): correct network-switched explore.db, merchant logo placeholder, and test-merchant re-seed#856
Open
romchornyi wants to merge 7 commits into
Open
fix(wallet): correct network-switched explore.db, merchant logo placeholder, and test-merchant re-seed#856romchornyi wants to merge 7 commits into
romchornyi wants to merge 7 commits into
Conversation
Sync bookkeeping is per-network (kExploreDatabaseLastVersion_Mainnet/_Testnet) but both networks share a single explore.db on disk. After switching chains the saved version for the new network already read as up to date and the file check passed, so the app kept serving the other network's merchants — testnet users saw the mainnet database (no Brinker, fewer merchants than Android). Record which network the installed database belongs to and force a download when it differs. Existing installs have no marker, so they refresh once on next launch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…new screen
The cherry-picked commit did not build against this tree, so two call sites
needed repair:
- DashSpendConfirmationDialog used `.placeholder { }`, a modifier
SDWebImageSwiftUI 3.1.3 does not have — the pod was already on this version
when the commit was written, so it can never have compiled. Rewritten with the
content/placeholder initializer, the form the other ported call sites use. The
`.fade` error beside it was only a knock-on from the broken chain.
Also route AllMerchantLocationsView's header through MerchantLogoPlaceholder.
That screen landed after the original commit, so it was still drawing the flat
empty-logo asset while every other merchant surface had moved on.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The PiggyCards test merchants are seeded once per session, guarded by `didInsertTestMerchants`. That was fine while the database file never changed mid-session, but the network-switch re-download in this branch replaces explore.db in place: after switching to testnet the fresh archive has no test merchants, yet the guard was already set from the mainnet database seeded at launch, so `addPiggyCardsTestMerchants` returned early and the testnet list was missing Piggy Cards Test Merchant and the four flexible test brands. Clear the guard when `databaseHasBeenUpdatedNotification` fires, so seeding re-evaluates against the new file. The existing count check still short-circuits before touching the FTS triggers when the merchants are already present, so a normal sync that changes nothing does no extra work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesMerchant logo placeholders
Explore Dash database synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
romchornyi
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: 3
🧹 Nitpick comments (3)
DashWallet/Sources/Models/Explore Dash/Services/ExploreDatabaseSyncManager.swift (1)
128-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the network log marker.
Prefix this
DSLoggermessage with🌐for consistent network-log filtering.As per coding guidelines, “Use emoji logging prefixes … with DSLogger for easier debugging and log filtering.”
🤖 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 128, Update the DSLogger message in the ExploreDatabaseSyncManager network-mismatch path to begin with the 🌐 network log marker, preserving the existing message content and interpolation.Source: Coding guidelines
DashWallet/Sources/UI/SwiftUI Components/MerchantLogoPlaceholder.swift (1)
82-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated hue-derivation logic between
color(for:)anduiColor(for:).Both methods repeat the identical trim/lowercase/hash/modulo computation to get
hue. Consider extracting a sharedprivate static func hue(for name: String) -> Intused by both.♻️ Proposed refactor
+ private static func hue(for name: String) -> Int { + let key = name.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() + return key.isEmpty ? 0 : ((javaHashCode(key) % 360) + 360) % 360 + } + static func color(for name: String) -> Color { - let key = name.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() - let hue = key.isEmpty ? 0 : ((javaHashCode(key) % 360) + 360) % 360 - return Color(hue: Double(hue) / 360.0, saturation: 0.3, brightness: 0.6) + return Color(hue: Double(hue(for: name)) / 360.0, saturation: 0.3, brightness: 0.6) }static func uiColor(for name: String) -> UIColor { - let key = name.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() - let hue = key.isEmpty ? 0 : ((javaHashCode(key) % 360) + 360) % 360 - return UIColor(hue: CGFloat(hue) / 360.0, saturation: 0.3, brightness: 0.6, alpha: 1) + return UIColor(hue: CGFloat(hue(for: name)) / 360.0, saturation: 0.3, brightness: 0.6, alpha: 1) }Also applies to: 113-117
🤖 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/MerchantLogoPlaceholder.swift around lines 82 - 86, Extract the duplicated name normalization and hash-based hue calculation from color(for:) and uiColor(for:) into a shared private static hue(for name: String) -> Int helper. Update both methods to use this helper while preserving the existing empty-name and modulo behavior.DashWallet/Sources/UI/Explore Dash/Merchants & ATMs/List/Cells/PointOfUseItemCell.swift (1)
49-56: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache generated placeholder images instead of re-rendering on every reuse. Both call sites regenerate a
MerchantLogoPlaceholder.image(...)bitmap (font-fit math, path fill, text draw) unconditionally on every cell/annotation reuse cycle, even when the result will be immediately replaced by a cached SDWebImage load. Adding a memoization cache (e.g.,NSCache<NSString, UIImage>keyed byname|size|cornerRadius) insideMerchantLogoPlaceholder.image(...)would fix both sites at once.
DashWallet/Sources/UI/Explore Dash/Merchants & ATMs/List/Cells/PointOfUseItemCell.swift#L49-L56: memoize/reuse the placeholder image generated inupdate(with:)instead of rendering it on every cell configuration.DashWallet/Sources/UI/Explore Dash/Merchants & ATMs/List/Views/ExploreMapAnnotationView.swift#L67-L74: memoize/reuse the placeholder image generated inupdate(with:)instead of rendering it on every annotation reuse.🤖 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/Merchants & ATMs/List/Cells/PointOfUseItemCell.swift around lines 49 - 56, Cache generated placeholder images inside MerchantLogoPlaceholder.image(...) using a key composed of merchant name, size, and corner radius. Apply this shared memoization to both update(with:) call sites: DashWallet/Sources/UI/Explore Dash/Merchants & ATMs/List/Cells/PointOfUseItemCell.swift#L49-L56 and DashWallet/Sources/UI/Explore Dash/Merchants & ATMs/List/Views/ExploreMapAnnotationView.swift#L67-L74, preserving their existing image-loading behavior.
🤖 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/Explore`
Dash/Services/ExploreDatabaseSyncManager.swift:
- Around line 49-53: Update syncIfNeeded and downloadDatabase to snapshot the
current network name and StorageReference once at the start of the full sync,
then reuse those values for metadata and data requests. Before extraction and
marker persistence, verify the snapshot still matches the active network; if it
changed, discard or restart the operation so the extracted archive cannot update
the wrong network’s marker.
In `@DashWallet/Sources/UI/Explore`
Dash/Views/DashSpend/DashSpendConfirmationDialog.swift:
- Around line 89-100: Update the MerchantLogoPlaceholder invocation in the
WebImage placeholder of DashSpendConfirmationDialog to pass usableFraction:
0.84, matching the surrounding RoundedRectangle clipShape and the established
POIDetailsView and AllMerchantLocationsView usage.
In `@DashWallet/Sources/UI/SwiftUI` Components/MerchantLogoPlaceholder.swift:
- Line 132: Update the renderer.image closure in MerchantLogoPlaceholder to omit
the unused ctx parameter, using the appropriate closure syntax while preserving
the existing image-rendering behavior.
---
Nitpick comments:
In `@DashWallet/Sources/Models/Explore`
Dash/Services/ExploreDatabaseSyncManager.swift:
- Line 128: Update the DSLogger message in the ExploreDatabaseSyncManager
network-mismatch path to begin with the 🌐 network log marker, preserving the
existing message content and interpolation.
In `@DashWallet/Sources/UI/Explore` Dash/Merchants &
ATMs/List/Cells/PointOfUseItemCell.swift:
- Around line 49-56: Cache generated placeholder images inside
MerchantLogoPlaceholder.image(...) using a key composed of merchant name, size,
and corner radius. Apply this shared memoization to both update(with:) call
sites: DashWallet/Sources/UI/Explore Dash/Merchants &
ATMs/List/Cells/PointOfUseItemCell.swift#L49-L56 and
DashWallet/Sources/UI/Explore Dash/Merchants &
ATMs/List/Views/ExploreMapAnnotationView.swift#L67-L74, preserving their
existing image-loading behavior.
In `@DashWallet/Sources/UI/SwiftUI` Components/MerchantLogoPlaceholder.swift:
- Around line 82-86: Extract the duplicated name normalization and hash-based
hue calculation from color(for:) and uiColor(for:) into a shared private static
hue(for name: String) -> Int helper. Update both methods to use this helper
while preserving the existing empty-name and modulo behavior.
🪄 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: 07186338-afcc-4524-aaa3-8ffe0145f304
📒 Files selected for processing (11)
DashWallet.xcodeproj/project.pbxprojDashWallet/Sources/Models/Explore Dash/Infrastructure/Database Connection/ExploreDatabaseConnection.swiftDashWallet/Sources/Models/Explore Dash/Services/ExploreDatabaseSyncManager.swiftDashWallet/Sources/UI/Explore Dash/Merchants & ATMs/Details/Views/POIDetailsView.swiftDashWallet/Sources/UI/Explore Dash/Merchants & ATMs/List/AllMerchantLocations/AllMerchantLocationsView.swiftDashWallet/Sources/UI/Explore Dash/Merchants & ATMs/List/Cells/PointOfUseItemCell.swiftDashWallet/Sources/UI/Explore Dash/Merchants & ATMs/List/Views/ExploreMapAnnotationView.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/Components/DashSpendPayIntro.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendConfirmationDialog.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetails/Components/GiftCardDetailsMerchantHeader.swiftDashWallet/Sources/UI/SwiftUI Components/MerchantLogoPlaceholder.swift
The network mismatch check only runs from `start()` (once, at first configure) and the 24h timer, so switching mainnet⇄testnet while the app is running did not trigger it: the wrong network's merchants stayed until the next cold launch. That matched the report — switch to testnet, and Brinker only appeared after quitting and reopening the app. Observe DWCurrentNetworkDidChange and re-run syncIfNeeded on a switch. The chain is already reset before the notification is posted, so the installedDatabaseNetwork mismatch is seen immediately and forces a same-session re-download; the download then re-seeds the test merchants via the existing notification path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Snapshot the network name and its StorageReference once in syncIfNeeded and thread both through downloadDatabase. storageRef and currentNetworkName are computed from the live chain, so a switch between the metadata fetch, the data fetch and the marker save could pair one archive's timestamp/checksum with another's bytes, or record installedDatabaseNetwork for a network whose archive was never extracted. Each async hop now bails when the chain has moved on, and the download is discarded before extraction rather than overwriting explore.db with the wrong network's data — the switch already scheduled its own sync. The now-unused networkSpecificFileName is folded into downloadDatabase. - Pass usableFraction: 0.84 to the DashSpendConfirmationDialog placeholder. It is clipped to a rounded rect, not a circle, so it should use the same fraction as the other rounded-rect sites instead of the circle-tuned 0.66 default. - Replace the unused ctx closure parameter in MerchantLogoPlaceholder with _. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Updates MARKETING_VERSION across all 20 build configurations to 8.7.0; CURRENT_PROJECT_VERSION was already 6 across all 28. Every target — dashwallet, dashpay, TodayExtension, WatchApp and its extension — reports the same version. The Info.plist files resolve both through build settings and needed no edit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Issue being fixed or feature implemented
Three related Explore Dash problems, all stemming from how
explore.dbis downloaded and replaced. On testnet the merchant list was showing the mainnet database (no Brinker, fewer merchants than Android), merchants without a logo drew a flat empty asset, and the PiggyCards test merchants disappeared after a network switch.Wrong database served after a network switch. Sync bookkeeping is per-network (
kExploreDatabaseLastVersion_Mainnet/_Testnet) but both networks share oneexplore.dbon disk. After switching chains the saved version for the new network already read as up to date and the file check passed, so the app kept serving the other network's merchants. The record of which network the file belongs to is now stored, and a download is forced when it differs.storageRefis also computed per access rather than cached at init, so the forced download targets the current network's archive instead of the launch network's.No fallback when a merchant has no logo. Merchant surfaces drew the flat
emptyLogoImageNameasset. They now use a generatedMerchantLogoPlaceholder— the merchant's name or initials on a colour derived deterministically from the name, matching the AndroidMerchantInitialIcon(HSV, JavahashCodeover the name so colours line up across platforms).Test merchants vanished after the database was replaced. The PiggyCards test merchants are seeded once per session, guarded by
didInsertTestMerchants. That was safe while the file never changed mid-session, but the network-switch re-download replacesexplore.dbin place: after switching to testnet the fresh archive has no test merchants, yet the guard was already set from the mainnet database seeded at launch, so seeding returned early. The guard is now cleared when the database-updated notification fires.What was done?
storageRefper access.MerchantLogoPlaceholderand route the merchant logo slots through it (POI details, list cell, map annotation, DashSpend pay intro / confirmation dialog, gift-card header, and the redesigned all-locations header).didInsertTestMerchantsondatabaseHasBeenUpdatedNotificationso the test merchants re-seed against the new file. The existing count check still short-circuits before touching the FTS triggers when they are already present.How Has This Been Tested?
xcodebuild -scheme dashwalletsucceeds. Verified against the two booted simulators:Piggy Cards Test Merchantentries present, where before it held the stale mainnet file.2e393…merchants were absent (count 0) while the FTS triggers were intact, so clearing the session guard lets the count check fall through to insertion. The same five IDs are already present on the other simulator, exercising the insert path.Two cherry-picked commits did not build against this tree and were repaired:
DashSpendConfirmationDialogused a.placeholder { }modifier absent from SDWebImageSwiftUI 3.1.3, andExploreDatabaseSyncManagerreferenced acurrentNetworkNamethat existed nowhere — the originalDWEnvironment…isMainnet()call was restored.Breaking Changes
None. Existing installs have no network marker, so the database refreshes once on the next launch.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes