fix(qt): preserve visible transaction type filter - #7611
Conversation
Use -1 as the default transactionType setting so first-run defaults can differ from an explicit "All" selection, and only reset the combo when the saved index is invalid or hidden by CoinJoin visibility.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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:
If these PRs merge firstThis PR will likely need a rebase:
|
|
⛔ Blockers found — Opus deferred (commit e05231a) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThe transaction view now defaults missing transaction-type settings to Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change preserves the selected transaction type filter and adds coverage for saved, hidden, and unset filter states; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The production change correctly preserves valid visible transaction filters and applies the appropriate fallback for invalid or hidden selections. The new Qt test must acquire the wallet lock before descriptor setup to compile under CI's thread-safety warnings-as-errors configuration; the new Dash-specific test files should also be added to the non-backported registry.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 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/test/transactionviewtests.cpp`:
- [BLOCKING] src/qt/test/transactionviewtests.cpp:115-116: Hold cs_wallet while setting up descriptor managers
`CWallet::SetupDescriptorScriptPubKeyMans()` is declared `EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)`, and both overloads begin with `AssertLockHeld(cs_wallet)`. This call is made without holding the lock, so Clang emits a `-Wthread-safety` diagnostic; Dash CI enables both that warning and `--enable-werror`, making the Qt test fail to compile. A debug lock-order build would also abort when the test reaches this call. Existing Qt wallet tests acquire `wallet->cs_wallet` around the same setup operation.
- [SUGGESTION] src/qt/test/transactionviewtests.cpp:1: Register the new Dash-specific Qt test files
`transactionviewtests.cpp` and `transactionviewtests.h` are newly authored Dash-specific files, but no matching entry was added to `test/util/data/non-backported.txt`. That registry supplies the file list to `test/lint/lint-cppcheck-dash.py`, so these files currently bypass the additional Dash-specific cppcheck coverage. Add `src/qt/test/transactionviewtests.*` to the registry.
| wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); | ||
| wallet->SetupDescriptorScriptPubKeyMans("", ""); |
There was a problem hiding this comment.
🔴 Blocking: Hold cs_wallet while setting up descriptor managers
CWallet::SetupDescriptorScriptPubKeyMans() is declared EXCLUSIVE_LOCKS_REQUIRED(cs_wallet), and both overloads begin with AssertLockHeld(cs_wallet). This call is made without holding the lock, so Clang emits a -Wthread-safety diagnostic; Dash CI enables both that warning and --enable-werror, making the Qt test fail to compile. A debug lock-order build would also abort when the test reaches this call. Existing Qt wallet tests acquire wallet->cs_wallet around the same setup operation.
| wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); | |
| wallet->SetupDescriptorScriptPubKeyMans("", ""); | |
| wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); | |
| { | |
| LOCK(wallet->cs_wallet); | |
| wallet->SetupDescriptorScriptPubKeyMans("", ""); | |
| } |
source: ['codex']
There was a problem hiding this comment.
Addressed in 5c14ee0a3f7.
Wrapped SetupDescriptorScriptPubKeyMans("", "") in a narrow LOCK(wallet->cs_wallet) scope (same pattern as addressbooktests.cpp / wallettests.cpp). Verified with a one-shot clang++ compile using -Wthread-safety + -Werror=thread-safety-analysis (object built cleanly); negative control without the lock fails with the expected requires holding mutex 'wallet->cs_wallet' diagnostic.
| @@ -0,0 +1,187 @@ | |||
| // Copyright (c) 2026 The Dash Core developers | |||
There was a problem hiding this comment.
🟡 Suggestion: Register the new Dash-specific Qt test files
transactionviewtests.cpp and transactionviewtests.h are newly authored Dash-specific files, but no matching entry was added to test/util/data/non-backported.txt. That registry supplies the file list to test/lint/lint-cppcheck-dash.py, so these files currently bypass the additional Dash-specific cppcheck coverage. Add src/qt/test/transactionviewtests.* to the registry.
source: ['codex']
There was a problem hiding this comment.
Addressed in 5c14ee0a3f7.
Added src/qt/test/transactionviewtests.* to test/util/data/non-backported.txt after the existing src/qt/* entries (before src/rpc/), matching the registry's established ordering/style.
2 participants
Issue being fixed or feature implemented
The Qt transaction history type filter always reset to "All" or "Most Common" when the wallet model attached, even when the user had previously selected another filter such as "Data Transaction". A missing saved setting was also indistinguishable from an explicit "All" selection because
QSettings::value("transactionType").toInt()defaults to0.What was done?
-1as the default when notransactionTypesetting exists, so first-run defaults can follow CoinJoin visibility rules.updateCoinJoinVisibility(); only fall back when the current index is invalid or hidden.How Has This Been Tested?
TransactionViewTests::transactionTypeSettingPersistenceinsrc/qt/test/transactionviewtests.cpp.Breaking Changes
None.
Checklist:
This pull request was created by Codex.
Made with Cursor