feat: modernize launch splash#7210
Conversation
![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: fcf97a2ad4
ℹ️ 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 (phase->resetsBar) { | ||
| // Long independent phases get their own full bar | ||
| displayProgress = 0.0; | ||
| phaseStart = phase->start; |
There was a problem hiding this comment.
In showMessage, every message that matches a resetsBar phase unconditionally sets displayProgress = 0.0. During wallet rescans, ShowProgress emits repeated "Rescanning…" updates with increasing percentages (src/wallet/wallet.cpp emits this at startup and then periodically), so each update re-enters this block and forces the bar back to zero before it can advance, producing visible backward jumps and inaccurate progress reporting for long rescans. The reset should happen only when entering the phase, not on every progress callback.
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a phase-driven, animated SplashScreen with PhaseInfo and phase lookup mapping init messages to progress ranges and flags (resetsBar, snapsToEnd, long phases). Replaces direct painting with a layered canvas that renders a rounded card, logo, title/version, optional network badge, and an integrated bottom progress bar. Introduces thread-safe PostMessageAndProgress to marshal messages/progress to the GUI thread, public setProgress(int) and calcOverallProgress(), an animation timer that interpolates displayProgress toward phase targets, and message routing that captures color safely. Separately, introduces a distinct wallet "loading" notification path (HandleLoadWalletLoading / NotifyWalletLoading) and registers/loading callback storage in WalletContext. Sequence Diagram(s)sequenceDiagram
participant Core
participant GUI as "GUI Thread"
participant Splash as "SplashScreen"
participant Anim as "Anim Timer"
participant Paint as "Paint / QWidget"
Core->>GUI: PostMessageAndProgress(msg, progress, color)
GUI->>Splash: showMessage(msg, color)
GUI->>Splash: setProgress(subProgress)
Splash->>Splash: LookupPhase(msg) -> set phaseStart/phaseEnd/flags
Splash->>Anim: ensure animTimer running
Anim->>Splash: timeout tick
Splash->>Splash: calcOverallProgress() -> update displayProgress
Splash->>Paint: schedule repaint()
Paint->>Splash: paintEvent -> draw card, logo, texts, progress bar
sequenceDiagram
participant WalletFactory as "CWallet::Create"
participant WalletContext
participant LoaderHandlers as "wallet_loading_fns"
participant ExternalHandlers as "WalletLoader callbacks"
WalletFactory->>WalletContext: NotifyWalletLoading(wallet)
WalletContext->>LoaderHandlers: iterate handlers
LoaderHandlers->>ExternalHandlers: invoke LoadWalletFn(std::shared_ptr<CWallet>)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![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 (1)
src/qt/splashscreen.cpp (1)
386-389: Assert the documented-1or0..100contract insetProgress().
calcOverallProgress()assumes that range when it scales the current phase. Guarding it here makes bad signal values fail fast instead of pushing the bar outside its phase window.Based on learnings, use fail-fast asserts in setters to enforce invariants and catch programmer errors early during development.Suggested fix
void SplashScreen::setProgress(int value) { + assert(value == -1 || (value >= 0 && value <= 100)); curProgress = value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/splashscreen.cpp` around lines 386 - 389, Add a fail-fast assertion in SplashScreen::setProgress to enforce the documented contract (value == -1 or 0..100) before assigning curProgress; specifically, in the SplashScreen::setProgress(int value) method assert that value == -1 || (value >= 0 && value <= 100) so invalid signal values are caught early (calcOverallProgress relies on this invariant).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/splashscreen.cpp`:
- Around line 357-381: The code currently reinitializes phase state every time
showMessage() is called (via PostMessageAndProgress()), causing timers and
progress bases to restart for repeated identical messages; modify the logic to
only reinitialize when the phase actually changes by comparing the
LookupPhase(message) result to a stored previous-phase pointer (e.g. add a
member like lastPhase or lastPhaseMessage and update it after changes). Only run
the block that sets displayProgress, phaseStart, phaseEnd, phaseIsLong,
animTimer.start/stop and phaseTimer.restart when the new phase pointer differs
from lastPhase; when LookupPhase(message) returns nullptr (message-only phase)
explicitly reset curProgress to 0 so exponential-progress handling isn’t
skipped, and update lastPhase appropriately. Ensure lastPhase is updated
whenever you perform a real transition.
---
Nitpick comments:
In `@src/qt/splashscreen.cpp`:
- Around line 386-389: Add a fail-fast assertion in SplashScreen::setProgress to
enforce the documented contract (value == -1 or 0..100) before assigning
curProgress; specifically, in the SplashScreen::setProgress(int value) method
assert that value == -1 || (value >= 0 && value <= 100) so invalid signal values
are caught early (calcOverallProgress relies on this invariant).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43db32d3-f40c-47a0-b25d-c242a5afbff3
📒 Files selected for processing (2)
src/qt/splashscreen.cppsrc/qt/splashscreen.h
…yout Redesign the splash screen with a modern card-style appearance featuring rounded corners, a translucent background with OS compositor support, and a refined visual layout with properly scoped QPainter lifetime.
Add a progress bar that tracks initialization phases with weighted time estimates. Includes smooth easing transitions, phase-aware progress for long operations like wallet loading and rescan, and snap-to-100% on completion. - Phase lookup uses _() to translate keys at runtime for locale-safe matching - messageColor is const, captured by value in lambdas for thread safety - Animation timer deferred until first message to avoid idle repaints - Time-based exponential fill capped at 95% (EXPONENTIAL_FILL_CAP)
PastaPastaPasta
force-pushed
the
feat/improve-ui
branch
from
fcf97a2 to
09fc59c
Compare
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98333549c2
ℹ️ 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".
src/qt/splashscreen.cpp
Outdated
| if (phase != nullptr && phase != m_current_phase) { | ||
| m_current_phase = phase; |
There was a problem hiding this comment.
showMessage only reinitializes a phase when phase != m_current_phase, so resetsBar phases are not reset when the same phase text reappears later. Wallet rescans emit titles prefixed with each wallet name (src/wallet/wallet.cpp uses "<wallet> Rescanning…"), so rescanning wallet #2 still maps to the same PhaseInfo and skips this block; because the animation never moves backward unless displayProgress is explicitly zeroed, the bar remains near completion and reports incorrect progress for subsequent wallets during multi-wallet startup.
Useful? React with 👍 / 👎.
During wallet rescans, repeated ShowProgress callbacks with the same message re-entered the resetsBar block, slamming displayProgress back to 0 and restarting phaseTimer on every update. Track the current phase pointer and skip reinitialization when the phase hasn't changed.
When multiple wallets rescan at startup, each emits ShowProgress with a wallet-name-prefixed message (e.g. '[wallet2] Rescanning…'). Since these map to the same PhaseInfo entry, the phase \!= m_current_phase guard skipped reinitialization for subsequent wallets. Track the triggering message text and also reinitialize resetsBar phases when the message changes.
PastaPastaPasta
force-pushed
the
feat/improve-ui
branch
from
9833354 to
4c97eb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/qt/splashscreen.cpp (1)
394-397: Assert thesetProgress()invariant at the boundary.This slot is the single ingress for the documented
-1/0..100contract. Failing fast here is better than lettingcalcOverallProgress()accept impossible values and render misleading widths.Based on learnings, use fail-fast asserts in setters to enforce invariants.🛡️ Suggested change
void SplashScreen::setProgress(int value) { + assert(value == -1 || (value >= 0 && value <= 100)); curProgress = value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/splashscreen.cpp` around lines 394 - 397, Add a fail-fast assertion to SplashScreen::setProgress to enforce the documented contract that the incoming value must be either -1 or within 0..100; validate the parameter at the boundary (in SplashScreen::setProgress) and assert (or otherwise abort in debug builds) when value is outside these allowed ranges before assigning curProgress so calcOverallProgress() never receives impossible values.src/qt/splashscreen.h (1)
71-80: Prefer the usual=initializer style for the new primitive members.These POD defaults stand out from the surrounding Dash/Bitcoin Core header style, which generally uses simple
=initializers for primitive members.Based on learnings, in header files prefer in-class initialization for POD types and use simple defaults for primitive members.♻️ Suggested change
- int curProgress{-1}; + int curProgress = -1; // Phase-based progress tracking - qreal phaseStart{0.0}; // Overall progress at start of current phase - qreal phaseEnd{0.0}; // Overall progress at end of current phase - bool phaseIsLong{false}; // True for long independent phases (rescan, wallet load) + qreal phaseStart = 0.0; // Overall progress at start of current phase + qreal phaseEnd = 0.0; // Overall progress at end of current phase + bool phaseIsLong = false; // True for long independent phases (rescan, wallet load) QElapsedTimer phaseTimer; // Time since current phase started - const struct PhaseInfo* m_current_phase{nullptr}; // Current phase (defined in splashscreen.cpp) + const struct PhaseInfo* m_current_phase = nullptr; // Current phase (defined in splashscreen.cpp) QString m_current_phase_message; // Message that triggered current phase - qreal displayProgress{0.0}; // Smoothly animated display value (0.0-1.0) + qreal displayProgress = 0.0; // Smoothly animated display value (0.0-1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/splashscreen.h` around lines 71 - 80, The header uses brace initializers for primitive/POD members; change them to the usual equals style: replace the brace initializers for curProgress, phaseStart, phaseEnd, phaseIsLong, displayProgress and m_current_phase (and any other primitive/POD members in the same block) to use = -1, = 0.0, = 0.0, = false, = 0.0 and = nullptr respectively; leave complex types like QElapsedTimer and QString as-is if desired. This keeps the class consistent with surrounding Dash/Bitcoin Core header style and makes the defaults easier to read.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/splashscreen.cpp`:
- Around line 217-230: The timeout lambda attached to animTimer never stops
until 100%, causing needless repaints when the target stalls; modify the lambda
(the QTimer::timeout connection using calcOverallProgress(), displayProgress,
and target) to stop animTimer once displayProgress reaches the current target
(use the same snap epsilon, e.g. when target - displayProgress < 0.002), i.e.
set displayProgress = target and call animTimer.stop() when close to target so
the timer halts even if overall progress < 0.999; keep update() behavior so the
final frame is painted before stopping.
---
Nitpick comments:
In `@src/qt/splashscreen.cpp`:
- Around line 394-397: Add a fail-fast assertion to SplashScreen::setProgress to
enforce the documented contract that the incoming value must be either -1 or
within 0..100; validate the parameter at the boundary (in
SplashScreen::setProgress) and assert (or otherwise abort in debug builds) when
value is outside these allowed ranges before assigning curProgress so
calcOverallProgress() never receives impossible values.
In `@src/qt/splashscreen.h`:
- Around line 71-80: The header uses brace initializers for primitive/POD
members; change them to the usual equals style: replace the brace initializers
for curProgress, phaseStart, phaseEnd, phaseIsLong, displayProgress and
m_current_phase (and any other primitive/POD members in the same block) to use =
-1, = 0.0, = 0.0, = false, = 0.0 and = nullptr respectively; leave complex types
like QElapsedTimer and QString as-is if desired. This keeps the class consistent
with surrounding Dash/Bitcoin Core header style and makes the defaults easier to
read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 37802fed-e0b6-4484-accc-6eace07d8130
📒 Files selected for processing (2)
src/qt/splashscreen.cppsrc/qt/splashscreen.h
| connect(&animTimer, &QTimer::timeout, this, [this]() { | ||
| qreal target = calcOverallProgress(); | ||
| // Smoothly animate toward target (never go backwards) | ||
| if (target > displayProgress) { | ||
| qreal gap = target - displayProgress; | ||
| // Faster easing for larger gaps so fast phases don't lag behind | ||
| qreal ease = gap > 0.1 ? 0.3 : 0.15; | ||
| displayProgress += gap * ease; | ||
| // Snap if very close | ||
| if (target - displayProgress < 0.002) displayProgress = target; | ||
| } | ||
| update(); | ||
| // Stop timer once progress is complete | ||
| if (displayProgress >= 0.999) animTimer.stop(); |
There was a problem hiding this comment.
Stop the animation timer once the current target is reached.
This lambda keeps calling update() every 30 ms and only stops at 100%. In a progress-backed phase—or if the first message does not resolve to a PHASE_TABLE entry so the target stays flat—the splash will repaint indefinitely with no visual change.
💡 Suggested change
connect(&animTimer, &QTimer::timeout, this, [this]() {
- qreal target = calcOverallProgress();
+ const qreal target = calcOverallProgress();
+ const bool time_driven = curProgress < 0 && phaseEnd > phaseStart;
// Smoothly animate toward target (never go backwards)
if (target > displayProgress) {
qreal gap = target - displayProgress;
// Faster easing for larger gaps so fast phases don't lag behind
qreal ease = gap > 0.1 ? 0.3 : 0.15;
displayProgress += gap * ease;
// Snap if very close
if (target - displayProgress < 0.002) displayProgress = target;
}
- update();
- // Stop timer once progress is complete
- if (displayProgress >= 0.999) animTimer.stop();
+ if (target > displayProgress || time_driven) {
+ update();
+ } else {
+ animTimer.stop();
+ }
+ if (displayProgress >= 0.999) animTimer.stop();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/splashscreen.cpp` around lines 217 - 230, The timeout lambda attached
to animTimer never stops until 100%, causing needless repaints when the target
stalls; modify the lambda (the QTimer::timeout connection using
calcOverallProgress(), displayProgress, and target) to stop animTimer once
displayProgress reaches the current target (use the same snap epsilon, e.g. when
target - displayProgress < 0.002), i.e. set displayProgress = target and call
animTimer.stop() when close to target so the timer halts even if overall
progress < 0.999; keep update() behavior so the final frame is painted before
stopping.
During startup, the splash only subscribed to wallet ShowProgress after a wallet finished loading. That meant startup rescans were seen as a generic init message and the new PHASE_TABLE logic fell back to the synthetic time-based long-phase path, even while the wallet log reported real rescan percentages. Add a separate wallet-loading callback that fires once a wallet object exists and can emit progress, but before AttachChain begins startup rescan work. The splash now subscribes through that early hook, so startup wallet rescans use the real wallet ShowProgress values instead of the fake exponential fill.
Reset curProgress whenever the splash changes phases so stale numeric progress from the previous phase cannot leak into the next one before a fresh ShowProgress update arrives. Also drop the separate phaseIsLong state and use a single fallback curve for phases that only emit initMessage(). After wiring startup rescans to real wallet ShowProgress updates, the extra slow-fallback state no longer buys us anything.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da5e9c1212
ℹ️ 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".
| for (const auto& [phase, translated] : cache) { | ||
| if (message.contains(translated, Qt::CaseInsensitive)) { | ||
| return phase; |
There was a problem hiding this comment.
If a wallet name contains one of the earlier phase labels, rescan updates from CWallet::ScanForWalletTransactions() arrive as [%wallet%] Rescanning…, and this contains() scan will classify them as the earlier phase instead of Rescanning…. For example, a wallet named Verifying blocks… will make the splash treat rescan percentages as block-verification progress, so the bar won't reset and will use the wrong range. Because GetDisplayName() includes the wallet name verbatim, phase matching needs to ignore the wallet prefix or otherwise anchor the check to the actual phase text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/qt/splashscreen.h (1)
45-46: Assert thesetProgress()contract.The new API documents
-1or0..100, but insrc/qt/splashscreen.cpp, Lines 400-403 the setter currently just assigns the value. A bad caller will silently skew the overall-progress math instead of failing fast.Suggested change
void SplashScreen::setProgress(int value) { assert(value == -1 || (value >= 0 && value <= 100)); curProgress = value; }Based on learnings: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/splashscreen.h` around lines 45 - 46, Add a fail-fast assertion in the SplashScreen::setProgress implementation to enforce the API contract (value == -1 or 0..100) before assigning to curProgress; locate the setter in splashscreen.cpp (function SplashScreen::setProgress) and add an assert that value == -1 || (value >= 0 && value <= 100) then assign curProgress = value so invalid callers fail fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/wallet.cpp`:
- Around line 203-208: NotifyWalletLoading is publishing an owning wrapper (via
interfaces::MakeWallet(context, wallet)) to subscribers before wallet startup
fully succeeds, which can leave a half-initialized CWallet retained; change the
behavior so the callback receives a non-owning/progress-only handle or a weak
reference instead of an owning shared_ptr, or delay calling
context.wallet_loading_fns until after startup steps (e.g., AttachChain) succeed
and only then publish the owning wrapper. Specifically, modify
NotifyWalletLoading to either pass a non-owning observer/weak_ptr (so
subscribers like splashscreen's m_connected_wallets don't hold ownership) or add
an explicit failure cleanup path that releases any early subscribers if later
startup fails; update uses of interfaces::MakeWallet and wallet_loading_fns
accordingly to reflect the non-owning handle contract.
---
Nitpick comments:
In `@src/qt/splashscreen.h`:
- Around line 45-46: Add a fail-fast assertion in the SplashScreen::setProgress
implementation to enforce the API contract (value == -1 or 0..100) before
assigning to curProgress; locate the setter in splashscreen.cpp (function
SplashScreen::setProgress) and add an assert that value == -1 || (value >= 0 &&
value <= 100) then assign curProgress = value so invalid callers fail fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e2c2427d-e328-4f10-a5d5-b38b8f1b39e9
📒 Files selected for processing (7)
src/interfaces/wallet.hsrc/qt/splashscreen.cppsrc/qt/splashscreen.hsrc/wallet/context.hsrc/wallet/interfaces.cppsrc/wallet/wallet.cppsrc/wallet/wallet.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/qt/splashscreen.cpp
| void NotifyWalletLoading(WalletContext& context, const std::shared_ptr<CWallet>& wallet) | ||
| { | ||
| LOCK(context.wallets_mutex); | ||
| for (auto& load_wallet : context.wallet_loading_fns) { | ||
| load_wallet(interfaces::MakeWallet(context, wallet)); | ||
| } |
There was a problem hiding this comment.
Don’t publish an owning wallet wrapper before startup succeeds.
In src/qt/splashscreen.cpp, Lines 318-330 retain this wrapper in m_connected_wallets. Because Lines 3300-3304 fire the callback before the remaining startup path is known to succeed, any later failure here (for example AttachChain()) leaves that retained shared_ptr holding a half-initialized CWallet and its DB/resources until the splash is torn down. The loading path needs a non-owning/progress-only handle here, or an explicit failure cleanup path that releases early subscribers.
Also applies to: 3300-3304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.cpp` around lines 203 - 208, NotifyWalletLoading is
publishing an owning wrapper (via interfaces::MakeWallet(context, wallet)) to
subscribers before wallet startup fully succeeds, which can leave a
half-initialized CWallet retained; change the behavior so the callback receives
a non-owning/progress-only handle or a weak reference instead of an owning
shared_ptr, or delay calling context.wallet_loading_fns until after startup
steps (e.g., AttachChain) succeed and only then publish the owning wrapper.
Specifically, modify NotifyWalletLoading to either pass a non-owning
observer/weak_ptr (so subscribers like splashscreen's m_connected_wallets don't
hold ownership) or add an explicit failure cleanup path that releases any early
subscribers if later startup fails; update uses of interfaces::MakeWallet and
wallet_loading_fns accordingly to reflect the non-owning handle contract.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Well-crafted visual modernization with a thoughtful phase-based progress design. No blocking issues. The primary concern is the exponential time constant (tau=5s) causing the bar to appear stuck during long-running phases like block index loading, with a secondary concern about multi-wallet loading visibility.
Reviewed commit: da5e9c1
🟡 2 suggestion(s) | 💬 1 nitpick(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/splashscreen.cpp`:
- [SUGGESTION] lines 282-283: Exponential tau=5s fills the bar too quickly for long-running phases
The time-based fill `1 - exp(-elapsed/5)` reaches 95% of a phase's range in ~15 seconds. For 'Loading block index…' (range 0.03–0.75, the largest time-based phase), this means the bar hits ~0.68 at 15s and asymptotically caps at ~0.71, appearing stuck for the remainder of what can be a multi-minute operation on nodes with large blockchains (500k+ blocks). A tau of 30–60s would spread the visual fill more evenly across realistic load times, or the tau could be scaled per-phase based on the range width.
- [SUGGESTION] line 64: Multi-wallet 'Loading wallet…' progress is invisible for the second wallet onward
'Loading wallet…' has `resetsBar=false`, and `initMessage()` sends the identical string `_("Loading wallet…").translated` for every wallet (src/wallet/load.cpp:129). Since 'Verifying wallet(s)…' fires once (load.cpp:56) — not per-wallet — consecutive 'Loading wallet…' messages have the same phase pointer AND the same message text, so `phase_changed` is false. The `phaseTimer` is never restarted, the time-based fill is already near its cap from the first wallet, and `displayProgress` stays put. The second wallet's loading time is invisible to the user.
Consider either setting `resetsBar=true` for this phase (matching how 'Rescanning…' handles multi-wallet), or prefixing the wallet name to the initMessage so the `message != m_current_phase_message` check triggers a phase restart.
| qreal fraction = 1.0 - std::exp(-elapsed / 5.0); | ||
| return phaseStart + (phaseEnd - phaseStart) * fraction * EXPONENTIAL_FILL_CAP; |
There was a problem hiding this comment.
🟡 Suggestion: Exponential tau=5s fills the bar too quickly for long-running phases
The time-based fill 1 - exp(-elapsed/5) reaches 95% of a phase's range in ~15 seconds. For 'Loading block index…' (range 0.03–0.75, the largest time-based phase), this means the bar hits ~0.68 at 15s and asymptotically caps at ~0.71, appearing stuck for the remainder of what can be a multi-minute operation on nodes with large blockchains (500k+ blocks). A tau of 30–60s would spread the visual fill more evenly across realistic load times, or the tau could be scaled per-phase based on the range width.
source: ['claude']
🤖 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/qt/splashscreen.cpp`:
- [SUGGESTION] lines 282-283: Exponential tau=5s fills the bar too quickly for long-running phases
The time-based fill `1 - exp(-elapsed/5)` reaches 95% of a phase's range in ~15 seconds. For 'Loading block index…' (range 0.03–0.75, the largest time-based phase), this means the bar hits ~0.68 at 15s and asymptotically caps at ~0.71, appearing stuck for the remainder of what can be a multi-minute operation on nodes with large blockchains (500k+ blocks). A tau of 30–60s would spread the visual fill more evenly across realistic load times, or the tau could be scaled per-phase based on the range width.
| {"Starting network threads…", 0.90, 0.94, false, false}, | ||
| {"Rescanning…", 0.00, 1.00, true, false}, | ||
| {"Verifying wallet(s)…", 0.00, 0.20, true, false}, | ||
| {"Loading wallet…", 0.20, 1.00, false, false}, |
There was a problem hiding this comment.
🟡 Suggestion: Multi-wallet 'Loading wallet…' progress is invisible for the second wallet onward
'Loading wallet…' has resetsBar=false, and initMessage() sends the identical string _("Loading wallet…").translated for every wallet (src/wallet/load.cpp:129). Since 'Verifying wallet(s)…' fires once (load.cpp:56) — not per-wallet — consecutive 'Loading wallet…' messages have the same phase pointer AND the same message text, so phase_changed is false. The phaseTimer is never restarted, the time-based fill is already near its cap from the first wallet, and displayProgress stays put. The second wallet's loading time is invisible to the user.
Consider either setting resetsBar=true for this phase (matching how 'Rescanning…' handles multi-wallet), or prefixing the wallet name to the initMessage so the message != m_current_phase_message check triggers a phase restart.
source: ['claude']
🤖 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/qt/splashscreen.cpp`:
- [SUGGESTION] line 64: Multi-wallet 'Loading wallet…' progress is invisible for the second wallet onward
'Loading wallet…' has `resetsBar=false`, and `initMessage()` sends the identical string `_("Loading wallet…").translated` for every wallet (src/wallet/load.cpp:129). Since 'Verifying wallet(s)…' fires once (load.cpp:56) — not per-wallet — consecutive 'Loading wallet…' messages have the same phase pointer AND the same message text, so `phase_changed` is false. The `phaseTimer` is never restarted, the time-based fill is already near its cap from the first wallet, and `displayProgress` stays put. The second wallet's loading time is invisible to the user.
Consider either setting `resetsBar=true` for this phase (matching how 'Rescanning…' handles multi-wallet), or prefixing the wallet name to the initMessage so the `message != m_current_phase_message` check triggers a phase restart.
| static const PhaseInfo PHASE_TABLE[] = { | ||
| {"Loading P2P addresses…", 0.00, 0.02, false, false}, | ||
| {"Loading banlist…", 0.02, 0.03, false, false}, | ||
| {"Loading block index…", 0.03, 0.75, false, false}, | ||
| {"Verifying blocks…", 0.75, 0.88, false, false}, | ||
| {"Replaying blocks…", 0.75, 0.88, false, false}, | ||
| {"Pruning blockstore…", 0.88, 0.90, false, false}, | ||
| {"Starting network threads…", 0.90, 0.94, false, false}, | ||
| {"Rescanning…", 0.00, 1.00, true, false}, | ||
| {"Verifying wallet(s)…", 0.00, 0.20, true, false}, | ||
| {"Loading wallet…", 0.20, 1.00, false, false}, | ||
| {"Done loading", 0.94, 1.00, false, true}, | ||
| }; |
There was a problem hiding this comment.
💬 Nitpick: Phase-based progress state machine has no unit tests
The phase lookup, progress calculation, phase transition logic, and multi-wallet edge cases form a non-trivial state machine. Extracting the progress logic (phase table, calcOverallProgress, phase transition in showMessage) into a testable non-Qt class would allow testing the state transitions and multi-wallet scenarios without a full Qt test harness. Low priority given this is non-consensus GUI code, but would help catch regressions as the phase table evolves.
source: ['claude']
4 participants
Issue being fixed or feature implemented
Old
New
Screen.Recording.2026-03-11.at.23.21.01.mov
What was done?
Commit 1: Visual modernization
QPainterlifetime to release the pixmap before timer/signal setupCommit 2: Phase-based progress bar
ShowProgresscallbacks (block verification, replaying, rescanning) interpolate linearly within their range_()at runtime so matching works in all localesmessageColorisconstand captured by value in signal lambdas for thread-safe cross-thread callbacksHow Has This Been Tested?
--rescanflag (bar resets and uses slow curve)Breaking Changes
None. Visual-only changes to the splash screen with no impact on consensus, RPC, or wallet behavior.
Checklist: