fix: auto-fetch profile from network when no cached data exists#763
fix: auto-fetch profile from network when no cached data exists#763thepastaclaw wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds automatic profile loading when no cached profile exists. Previously, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes the “No Profile Loaded” state after importing a wallet by automatically triggering a network profile fetch when no cached profile exists in the local database.
Changes:
- Enhance the
Ok(None)database-load path to queue an automatic network fetch viapending_action. - Improve debug logging to indicate the auto-fetch behavior when cache is missing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The auto-fetch fix correctly queues a network load when no cached profile exists, and the loop-safety guard via profile_load_attempted + pending_action.is_none() is sound. The identity is properly captured in the backend task at queue time. However, the pre-existing lack of identity-matching in display_task_result() means the auto-fetch widens the window for a stale result to be applied to a different identity, and transient network failures are indistinguishable from a genuinely absent profile.
Reviewed commit: 4181813
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🟡 suggestion: Pre-existing: Err branch does not set profile_load_attempted, causing repeated error logs
src/ui/dashpay/profile_screen.rs (lines 336-338)
The Err(e) branch only logs the error without setting self.profile_load_attempted = true. Since refresh() (line 370–375) re-calls load_profile_from_database() every frame when profile.is_none() && !profile_load_attempted, a persistent DB error (e.g., schema mismatch after upgrade) will log on every frame indefinitely. This is pre-existing code not changed by the PR, but directly adjacent to the new Ok(None) handling and analogous in nature — the PR fixed the Ok(None) path for this exact symptom.
💡 Suggested change
Err(e) => {
tracing::error!("Error loading profile from database: {}", e);
self.profile_load_attempted = true;
}
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/dashpay/profile_screen.rs`:
- [SUGGESTION] lines 321-334: Auto-fetch widens the window for stale profile results to land on the wrong identity
The identity is correctly captured in `DashPayTask::LoadProfile { identity }` at line 333 (via `trigger_load_profile()` at line 344). However, `BackendTaskSuccessResult::DashPayProfile` carries no identity ID, so `display_task_result()` (line 1424) unconditionally applies the result to `self.selected_identity` — including DB cache writes at lines 1445–1493. This race is **pre-existing** (the manual Refresh button has the same vulnerability), but the auto-fetch meaningfully widens the window: the network round-trip can take seconds, during which the user may switch the identity selector (line 635), causing the old identity's profile to be displayed and cached under the new identity. The fix would be to include the source identity ID in `DashPayProfile` and skip application when it doesn't match `self.selected_identity`.
- [SUGGESTION] lines 328-333: Transient network failure renders as 'no profile exists'
Line 332 sets `profile_load_attempted = true` before the network result arrives. If the fetch fails, `display_message()` (line 1416) clears `loading`/`saving` but does not reset `profile_load_attempted`. The render path at line 1250 then shows "This identity doesn't have a DashPay profile yet" — indistinguishable from a genuinely absent profile. This is the same behavior as the manual Refresh flow, but with auto-fetch the user never explicitly requested the load, so seeing a definitive "no profile" message after a silent network failure is more confusing. Consider resetting `profile_load_attempted` in `display_message()` on error so `refresh()` can re-attempt, or distinguishing "confirmed absent" from "fetch failed" in the UI.
- [SUGGESTION] lines 336-338: Pre-existing: Err branch does not set profile_load_attempted, causing repeated error logs
The `Err(e)` branch only logs the error without setting `self.profile_load_attempted = true`. Since `refresh()` (line 370–375) re-calls `load_profile_from_database()` every frame when `profile.is_none() && !profile_load_attempted`, a persistent DB error (e.g., schema mismatch after upgrade) will log on every frame indefinitely. This is pre-existing code not changed by the PR, but directly adjacent to the new `Ok(None)` handling and analogous in nature — the PR fixed the `Ok(None)` path for this exact symptom.
| Ok(None) => { | ||
| tracing::debug!("No profile found in database for identity {}", identity_id); | ||
| tracing::debug!( | ||
| "No profile found in database for identity {}, will auto-fetch from network", | ||
| identity_id | ||
| ); | ||
| // No cached profile exists — queue an automatic network fetch | ||
| // so the user doesn't have to manually click "Refresh". | ||
| if self.pending_action.is_none() { | ||
| // Mark that we've already attempted to load a profile to | ||
| // avoid repeatedly queuing the same work if this method | ||
| // is called again before the pending action executes. | ||
| self.profile_load_attempted = true; | ||
| self.pending_action = Some(Box::new(self.trigger_load_profile())); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Auto-fetch widens the window for stale profile results to land on the wrong identity
The identity is correctly captured in DashPayTask::LoadProfile { identity } at line 333 (via trigger_load_profile() at line 344). However, BackendTaskSuccessResult::DashPayProfile carries no identity ID, so display_task_result() (line 1424) unconditionally applies the result to self.selected_identity — including DB cache writes at lines 1445–1493. This race is pre-existing (the manual Refresh button has the same vulnerability), but the auto-fetch meaningfully widens the window: the network round-trip can take seconds, during which the user may switch the identity selector (line 635), causing the old identity's profile to be displayed and cached under the new identity. The fix would be to include the source identity ID in DashPayProfile and skip application when it doesn't match self.selected_identity.
source: ['codex-general', 'codex-rust', 'claude-rust']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/dashpay/profile_screen.rs`:
- [SUGGESTION] lines 321-334: Auto-fetch widens the window for stale profile results to land on the wrong identity
The identity is correctly captured in `DashPayTask::LoadProfile { identity }` at line 333 (via `trigger_load_profile()` at line 344). However, `BackendTaskSuccessResult::DashPayProfile` carries no identity ID, so `display_task_result()` (line 1424) unconditionally applies the result to `self.selected_identity` — including DB cache writes at lines 1445–1493. This race is **pre-existing** (the manual Refresh button has the same vulnerability), but the auto-fetch meaningfully widens the window: the network round-trip can take seconds, during which the user may switch the identity selector (line 635), causing the old identity's profile to be displayed and cached under the new identity. The fix would be to include the source identity ID in `DashPayProfile` and skip application when it doesn't match `self.selected_identity`.
| if self.pending_action.is_none() { | ||
| // Mark that we've already attempted to load a profile to | ||
| // avoid repeatedly queuing the same work if this method | ||
| // is called again before the pending action executes. | ||
| self.profile_load_attempted = true; | ||
| self.pending_action = Some(Box::new(self.trigger_load_profile())); |
There was a problem hiding this comment.
🟡 Suggestion: Transient network failure renders as 'no profile exists'
Line 332 sets profile_load_attempted = true before the network result arrives. If the fetch fails, display_message() (line 1416) clears loading/saving but does not reset profile_load_attempted. The render path at line 1250 then shows "This identity doesn't have a DashPay profile yet" — indistinguishable from a genuinely absent profile. This is the same behavior as the manual Refresh flow, but with auto-fetch the user never explicitly requested the load, so seeing a definitive "no profile" message after a silent network failure is more confusing. Consider resetting profile_load_attempted in display_message() on error so refresh() can re-attempt, or distinguishing "confirmed absent" from "fetch failed" in the UI.
source: ['codex-general', 'codex-rust']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/dashpay/profile_screen.rs`:
- [SUGGESTION] lines 328-333: Transient network failure renders as 'no profile exists'
Line 332 sets `profile_load_attempted = true` before the network result arrives. If the fetch fails, `display_message()` (line 1416) clears `loading`/`saving` but does not reset `profile_load_attempted`. The render path at line 1250 then shows "This identity doesn't have a DashPay profile yet" — indistinguishable from a genuinely absent profile. This is the same behavior as the manual Refresh flow, but with auto-fetch the user never explicitly requested the load, so seeing a definitive "no profile" message after a silent network failure is more confusing. Consider resetting `profile_load_attempted` in `display_message()` on error so `refresh()` can re-attempt, or distinguishing "confirmed absent" from "fetch failed" in the UI.
| self.profile_load_attempted = true; | ||
| self.pending_action = Some(Box::new(self.trigger_load_profile())); |
There was a problem hiding this comment.
💬 Nitpick: Redundant profile_load_attempted assignment
Line 332 sets self.profile_load_attempted = true before calling self.trigger_load_profile() on line 333, which also sets self.profile_load_attempted = true unconditionally (line 346). The guard at line 328 (pending_action.is_none()) already prevents re-entry, and load_profile_from_database() is inside if let Some(identity) = &self.selected_identity so the else branch of trigger_load_profile() cannot execute. The duplicate assignment is harmless but technically redundant.
source: ['claude-general', 'claude-rust']
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
When a wallet with an existing identity is imported, identity discovery stores the identity in the local DB but the DashPay profile is not cached. Previously, the Profile Screen would show 'No Profile Loaded' and require the user to manually click 'Refresh' to fetch from the network. Now, when load_profile_from_database() finds no cached profile data, it automatically queues a network fetch via pending_action. The queueing is guarded with pending_action.is_none() and marks profile_load_attempted before the pending action executes so refresh() does not repeatedly enqueue the same work. Fixes dashpay#759
thepastaclaw
force-pushed
the
fix/auto-load-profile-on-wallet-import
branch
from
4181813 to
129c47d
Compare
|
✅ Review complete (commit 129c47d) |
Issue
Closes #759 — After importing a wallet with an identity, the My DashPay Profile screen shows "No Profile Loaded" and requires a manual Refresh click.
Root Cause
ProfileScreen::load_profile_from_database()does nothing in theOk(None)case (no cached profile in local SQLite). It leavesprofile_load_attempted = falsebut doesn't trigger a network fetch, so the user sees a blank screen until they manually click Refresh.Fix
When no cached profile data exists (
Ok(None)branch), queue an automatic network fetch viatrigger_load_profile()using the existingpending_actionmechanism. This is safe from infinite loops becausetrigger_load_profile()setsprofile_load_attempted = true, andload_profile_from_database()is only called when!profile_load_attempted.Platform Dependency Update
Updated
dashpay/platformandrust-dashcoreto latest revisions and fixed three breaking API changes:FeeLevelenum removed → useFeeRate::normal()static constructorset_fee_level()removed → useset_fee_rate()NetworkExttrait removed → methods now inherent onNetworkTesting
cargo build— cleancargo clippy --all-features --all-targets -- -D warnings— clean🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit