fix: surface specific autobackup-failure tooltips in CoinJoin status - #7515
fix: surface specific autobackup-failure tooltips in CoinJoin status#7515PastaPastaPasta wants to merge 1 commit into
Conversation
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
WalkthroughThe overview page assigns separate tooltips for disabled backups ( Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
|
✅ Final review complete — no blockers (commit f16aea4) |
de22703 to
113dcae
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 113dcaed6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (nWalletBackups == -1) { | ||
| ui->labelCoinJoinEnabled->setToolTip(tr("ERROR! Failed to create automatic backup") + ", " + | ||
| tr("see debug.log for details.") + "<br><br>" + | ||
| tr("Mixing is disabled, please close your wallet and fix the issue!")); |
There was a problem hiding this comment.
When AutoBackupWallet() sets nWalletBackups to -1 while mixing is active, this relocated guard has already been passed. Removing the later -1 block therefore lets the current invocation continue, display the status as enabled, and leave the client mixing until a subsequent GUI timer tick reaches this guard; CheckAutomaticBackup() merely returns false for -1 and does not call stopMixing(). Preserve the immediate DisableCoinJoinCompletely() path so the modal's claim that mixing is disabled is true as soon as the backup fails.
Useful? React with 👍 / 👎.
![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
🤖 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 `@src/qt/overviewpage.cpp`:
- Around line 529-536: Restore a post-autoBackupWallet() status check in the
CoinJoin update flow so failures changing nWalletBackups to -1 or -2 are handled
during the current invocation. Reuse the existing per-status behavior, including
DisableCoinJoinCompletely() and the persistent error tooltip for -1 and the
warning tooltip for -2, while retaining the early nWalletBackups guard for
subsequent invocations.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f4fa8d75-7476-493e-9964-0d8fbbb57df7
📒 Files selected for processing (1)
src/qt/overviewpage.cpp
daef89e to
60ba279
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60ba2792ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (nWalletBackups == -2) { | ||
| ui->labelCoinJoinEnabled->setToolTip(tr("WARNING! Failed to replenish keypool, please unlock your wallet to do so.")); |
There was a problem hiding this comment.
In a multiwallet GUI, this branch also runs on OverviewPages belonging to wallets that did not encounter the failure: nWalletBackups is process-global, while each WalletView owns a separate OverviewPage. If locked legacy wallet A sets the global to -2, switching to an already-unlocked or descriptor wallet B now shows “please unlock your wallet”; acting on B cannot replenish A's keypool, so the warning gives no usable recovery path. Track the failure per wallet or make the tooltip identify that another loaded wallet may need unlocking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/qt/overviewpage.cpp`:
- Around line 667-674: Update the post-autoBackupWallet() backup-state check
around nWalletBackups to handle every non-positive value, not only -1. Reuse the
existing 0/-1/-2 tooltip mapping, disable CoinJoin immediately, and return
before the enabled-rendering path; preserve the existing Dash-specific
backup-state 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78d12259-59ef-4b1d-bf5c-986baa535510
📒 Files selected for processing (1)
src/qt/overviewpage.cpp
…guard The early guard in OverviewPage::coinJoinStatus() treated every non-positive nWalletBackups value the same, always showing the generic 'Automatic backups are disabled' tooltip. On every timer tick this overwrote the more specific messages for -1 (backup failed) and -2 (keypool not replenished, wallet locked), so the later per-status block only ever showed its text for the single tick in which autoBackupWallet() freshly failed. Move the status-specific tooltips into the guard, which now handles all three states persistently, and reduce the later block to what still matters in the same invocation: stopping mixing immediately on -1 (CCoinJoinClientManager::CheckAutomaticBackup() does not call stopMixing() for -1, and the modal at the failure site already tells the user mixing is disabled). Its tooltip assignments are dropped since they were overwritten by the guard within a second anyway.
60ba279 to
f16aea4
Compare
|
Ok; I think I like how this is now; Ready for review :) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR fixes a real bug where the top-level guard in OverviewPage::coinJoinStatus() overwrote specific -1/-2 autobackup-failure tooltips with a generic message on every subsequent timer tick. Verified against the actual diff: the fix correctly moves the three status-specific tooltips (0, -1, -2) into the persistent guard and trims the later per-invocation block to only the immediate stopMixing() call needed for -1 (since CCoinJoinClientManager::CheckAutomaticBackup() doesn't call it). No correctness issues found. The one suggestion — adding regression coverage for the tooltip selection — is well-grounded: src/qt/test/wallettests.cpp already constructs an OverviewPage with ClientModel/WalletModel and could be extended to assert on labelCoinJoinEnabled->toolTip() for each nWalletBackups value.
Source: reviewers codex/gpt-5.6-sol (general, dash-core-commit-history) and claude/claude-sonnet-5 (general, dash-core-commit-history retry); verifier claude/claude-sonnet-5 (final-verifier). The openclaw-agent coordinator is orchestration-only.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s)
🤖 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/qt/overviewpage.cpp`:
- [SUGGESTION] src/qt/overviewpage.cpp:526-536: Add regression coverage for backup-status tooltip selection
This three-way branch (nWalletBackups == 0 / -1 / -2) is the core of the bug fix, but nothing verifies it stays correct going forward — a future refactor could silently reintroduce the original bug where the generic tooltip overwrites the specific ones. src/qt/test/wallettests.cpp already builds an OverviewPage wired to ClientModel and WalletModel in TestGUI, so this is directly testable: set OverviewPage's nWalletBackups to each of 0, -1, -2, call coinJoinStatus(true), and assert ui->labelCoinJoinEnabled->toolTip() matches the expected string for each. Restore the prior nWalletBackups value afterward so it doesn't leak into other assertions in the same test.
| // Disable any PS UI for masternode or when autobackup is disabled or failed for whatever reason | ||
| if (clientModel->node().isMasternode() || nWalletBackups <= 0) { | ||
| DisableCoinJoinCompletely(); | ||
| if (nWalletBackups <= 0) { | ||
| if (nWalletBackups == 0) { | ||
| ui->labelCoinJoinEnabled->setToolTip(tr("Automatic backups are disabled, no mixing available!")); | ||
| } else if (nWalletBackups == -1) { | ||
| ui->labelCoinJoinEnabled->setToolTip(tr("ERROR! Failed to create automatic backup") + ", " + | ||
| tr("see debug.log for details.") + "<br><br>" + | ||
| tr("Mixing is disabled, please close your wallet and fix the issue!")); | ||
| } else if (nWalletBackups == -2) { | ||
| ui->labelCoinJoinEnabled->setToolTip(tr("WARNING! Failed to replenish keypool, please unlock your wallet to do so.")); |
There was a problem hiding this comment.
🟡 Suggestion: Add regression coverage for backup-status tooltip selection
This three-way branch (nWalletBackups == 0 / -1 / -2) is the core of the bug fix, but nothing verifies it stays correct going forward — a future refactor could silently reintroduce the original bug where the generic tooltip overwrites the specific ones. src/qt/test/wallettests.cpp already builds an OverviewPage wired to ClientModel and WalletModel in TestGUI, so this is directly testable: set OverviewPage's nWalletBackups to each of 0, -1, -2, call coinJoinStatus(true), and assert ui->labelCoinJoinEnabled->toolTip() matches the expected string for each. Restore the prior nWalletBackups value afterward so it doesn't leak into other assertions in the same test.
source: ['codex']
2 participants
Issue being fixed or feature implemented
OverviewPage::coinJoinStatus()has an early guard that treats every non-positivenWalletBackupsvalue the same: it disables CoinJoin and sets the generic "Automatic backups are disabled, no mixing available!" tooltip, then returns. ButnWalletBackupsdistinguishes three states:0(backups disabled),-1(automatic backup failed) and-2(backup succeeded but keypool was not replenished because the wallet is locked).The function has more specific handling for
-1("ERROR! Failed to create automatic backup") and-2("WARNING! Failed to replenish keypool, please unlock your wallet") further down, but that code only takes effect within the single invocation whereautoBackupWallet()freshly fails. On every subsequent timer tick the early<= 0guard runs first and overwrites the specific tooltip with the generic one, so users can't tell "I turned backups off" apart from "backups are failing" or "unlock your wallet to replenish the keypool".This was noticed via a CodeRabbit review comment on #7005, but the bug predates that PR, so it is fixed here separately against
develop.What was done?
The early guard now sets the status-specific tooltip directly: the generic "backups disabled" message only for
nWalletBackups == 0, the backup-failure error for-1, and the unlock-your-wallet warning for-2. CoinJoin remains disabled in all three cases, preserving existing behavior.The later per-status block is reduced to what still matters within the same invocation: stopping mixing immediately on
-1(CCoinJoinClientManager::CheckAutomaticBackup()does not callstopMixing()for-1, and the modal at the failure site already tells the user mixing is disabled). Its tooltip assignments — including the whole-2branch, which only set a tooltip — are dropped, since the guard overwrote them within one timer tick anyway; the guard now shows the matching message persistently instead.How Has This Been Tested?
The change is a tooltip-selection change inside an existing GUI code path with no logic change to when CoinJoin is disabled; relying on CI for build verification. There is no existing GUI test coverage for this path.
Breaking Changes
None.
Checklist: