fix(tokens): show freeze confirmation dialog

[thepastaclaw]

Summary

• fix the Freeze token action button so it opens the confirmation dialog instead of clearing it
• align the Freeze flow with the existing Unfreeze/Mint confirmation-dialog pattern

Fixes #843.

Verification

• cargo check -q
• cargo clippy --all-features --all-targets -- -D warnings
• cargo fmt --check
• git diff --check
• pre-PR code review gate: ship

Manual acceptance test

1. Open a token where the Freeze action is available.
2. Enter/select an identity to freeze.
3. Click Freeze.
4. Expected: a Confirm Freeze dialog appears; confirming dispatches the freeze backend task and shows the normal progress banner.

[coderabbitai]

Warning

Rate limit exceeded

@lklimek has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 56 minutes and 52 seconds before requesting another review.

To continue reviewing without waiting, purchase usage credits in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 59afded2-e0db-4c4e-938e-23fec3da7ce9

📥 Commits

Reviewing files that changed from the base of the PR and between 971129c and 8b36373.

📒 Files selected for processing (4)

• src/ui/tokens/destroy_frozen_funds_screen.rs
• src/ui/tokens/freeze_tokens_screen.rs
• tests/kittest/main.rs
• tests/kittest/token_action_confirmation_dialogs.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[thepastaclaw]

✅ Review complete (commit 8b36373)

[thepastaclaw]

Code Review

PR is a correct, minimal fix that restores the Freeze confirmation dialog by mirroring the Unfreeze/Mint pattern. Three non-blocking observations: a real piece of dead code inside show_confirmation_popup (verified — the helper is gated by is_some() so its get_or_insert_with branch is unreachable), the absence of a kittest for this regression-prone interaction, and a minor i18n placeholder style nit. None block merge.

Reviewed commit: dd00249

🟡 1 suggestion(s) | 💬 2 nitpick(s)

2 additional findings

🟡 suggestion: Add a kittest for the Freeze confirmation flow to prevent regression recurrence

src/ui/tokens/freeze_tokens_screen.rs (lines 600-618)

This PR fixes a UI state regression where the Freeze button cleared confirmation_dialog instead of populating it — the bug compiled and shipped because no automated test exercises this path. tests/kittest/ has generic dialog/component coverage (confirmation_dialog.rs, message_banner.rs) but no FreezeTokensScreen/UnfreezeTokensScreen/MintTokensScreen test that clicks the action button and asserts the corresponding Confirm Freeze dialog appears. A small kittest that constructs the screen, simulates the button click, and asserts the dialog is visible would catch this exact class of regression in CI rather than relying on manual verification. Worth applying to the sibling unfreeze/mint screens too, since they share the duplicated pattern flagged in the other finding.

💬 nitpick: Dead code: dialog construction inside show_confirmation_popup is unreachable

src/ui/tokens/freeze_tokens_screen.rs (lines 225-249)

The call site at line 617 gates show_confirmation_popup behind if self.confirmation_dialog.is_some(), and the click handler at lines 603-613 always populates self.confirmation_dialog = Some(...) before that gate is reached. As a result, the get_or_insert_with(|| ConfirmationDialog::new("Confirm Freeze", msg)...) branch at lines 231-235 and the msg format at lines 226-229 are unreachable — the closure can never run because the option is always already Some. Not a behavioral bug, but it duplicates wording ("Confirm Freeze", confirm/cancel labels, the Are you sure... message) between the helper and the button handler, which makes future copy changes error-prone. Same redundancy exists in unfreeze_tokens_screen.rs. Cleanup option: simplify the helper to let Some(dialog) = self.confirmation_dialog.as_mut() else { return AppAction::None; }; and drop the msg/get_or_insert_with block, keeping construction solely in the click handler. Out of scope for this fix; track as a follow-up.

🤖 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/tokens/freeze_tokens_screen.rs`:
- [SUGGESTION] lines 600-618: Add a kittest for the Freeze confirmation flow to prevent regression recurrence
  This PR fixes a UI state regression where the Freeze button cleared `confirmation_dialog` instead of populating it — the bug compiled and shipped because no automated test exercises this path. `tests/kittest/` has generic dialog/component coverage (`confirmation_dialog.rs`, `message_banner.rs`) but no `FreezeTokensScreen`/`UnfreezeTokensScreen`/`MintTokensScreen` test that clicks the action button and asserts the corresponding `Confirm Freeze` dialog appears. A small kittest that constructs the screen, simulates the button click, and asserts the dialog is visible would catch this exact class of regression in CI rather than relying on manual verification. Worth applying to the sibling unfreeze/mint screens too, since they share the duplicated pattern flagged in the other finding.

[thepastaclaw]

Code Review

Focused, correct bugfix that restores the click → confirmation-dialog wiring on the Freeze screen, matching the existing Unfreeze/Mint pattern. Kittest coverage drives the real ScreenLike::ui() paint path so the regression cannot recur silently. One legitimate cleanup nit: the post-fix code constructs the dialog in two places, leaving show_confirmation_popup's get_or_insert_with branch dead and duplicating the prompt string.

Reviewed commit: ee1a08d

💬 2 nitpick(s)

1 additional finding

💬 nitpick: `show_confirmation_popup`'s `get_or_insert_with` branch is now unreachable

src/ui/tokens/freeze_tokens_screen.rs (lines 224-249)

After this fix, the popup is gated on if self.confirmation_dialog.is_some() at line 617, and the click handler at line 608 always assigns Some(ConfirmationDialog::new(...)) before that gate runs. The only other mutations inside show_confirmation_popup are take() on Confirmed/Canceled. That makes the get_or_insert_with block at lines 231–235 unreachable — but the format!("Are you sure you want to freeze identity {}?", self.freeze_identity_id) at line 226 still allocates a fresh String every frame the dialog is open. It also duplicates the exact prompt wording and builder calls already present at lines 604–612, so a future copy-edit to one site silently drifts from the other. Cleanest fix: drop the get_or_insert_with, replace with let Some(dialog) = self.confirmation_dialog.as_mut() else { return AppAction::None; };, and let the click handler remain the single source of construction. The same shape exists in unfreeze_tokens_screen.rs and mint_tokens_screen.rs — apply consistently across all three.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

Code Review

Focused fix verified at SHA 8b36373: the click→dialog wiring is correctly restored on the Freeze screen and the kittest harness covers Freeze/Unfreeze/Mint click→dialog paint paths, addressing the prior regression-test gap. One convergent quality issue remains: show_confirmation_popup on both Freeze and Destroy-Frozen-Funds screens contains an unreachable get_or_insert_with branch and reallocates a format!-built String every frame while the dialog is visible. The PR-scope-creep nitpick is meta and not actionable in the diff itself; surfaced as a low-priority note.

Reviewed commit: 8b36373

🟡 2 suggestion(s) | 💬 1 nitpick(s)

3 additional findings

🟡 suggestion: show_confirmation_popup carries unreachable dialog construction and re-allocates the prompt every frame

src/ui/tokens/freeze_tokens_screen.rs (lines 225-235)

After this fix, the click handler at lines 603-613 unconditionally assigns self.confirmation_dialog = Some(ConfirmationDialog::new("Confirm Freeze", msg)…), and ui() only calls show_confirmation_popup under if self.confirmation_dialog.is_some() (line 617). That means the closure passed to get_or_insert_with at lines 231-235 cannot run in normal flow — its constructor is dead code. Two consequences: (1) the format!("Are you sure you want to freeze identity {identity_id}?", …) at lines 226-229 still allocates a fresh String on every paint while the dialog is open, even though the stored dialog already owns a copy of that message; (2) the title "Confirm Freeze" and the confirm/cancel labels are duplicated across the click handler and the helper, so a future copy edit at one site will silently drift from the other. Replace the prologue with an early-return so the click handler is the single source of construction.

💡 Suggested change

    /// Confirmation popup
    fn show_confirmation_popup(&mut self, ui: &mut Ui) -> AppAction {
        let Some(confirmation_dialog) = self.confirmation_dialog.as_mut() else {
            return AppAction::None;
        };

        let response = confirmation_dialog.show(ui);

🟡 suggestion: show_confirmation_popup carries unreachable dialog construction and re-allocates the prompt every frame

src/ui/tokens/destroy_frozen_funds_screen.rs (lines 235-246)

Same shape as the Freeze screen. The click handler at lines 596-608 unconditionally assigns self.confirmation_dialog = Some(ConfirmationDialog::new("Confirm Destroy Frozen Funds", msg)…danger_mode(true)) and the helper call at line 612 is gated on is_some(). The get_or_insert_with closure at lines 241-246 is therefore unreachable, while format!("Are you sure you want to destroy frozen funds for identity {identity_id}? This action cannot be undone.", …) at lines 236-239 still allocates a String on every paint and duplicates the click-handler wording (including danger_mode(true)). Drop the dead constructor and let the click handler remain the single source of construction.

💡 Suggested change

    /// Confirmation popup
    fn show_confirmation_popup(&mut self, ui: &mut Ui) -> AppAction {
        let Some(confirmation_dialog) = self.confirmation_dialog.as_mut() else {
            return AppAction::None;
        };

        let response = confirmation_dialog.show(ui);

💬 nitpick: PR scope is broader than the title suggests

src/ui/tokens/freeze_tokens_screen.rs (line 1)

The minimal #843 fix is the click-handler change at freeze_tokens_screen.rs:603-613. The diff additionally rewrites DestroyFrozenFundsScreen (status enum reshape, error_message removal, MessageBanner-based progress with with_elapsed), introduces shared helpers (set_error_banner, load_identities_with_banner, validate_signing_key), replaces expect(...) panics with banner-driven flows, and adds a wallet_open_attempted: bool to fix a separate retry-loop issue. Each individual change is an improvement, but bundling them under a fix-title PR makes any single regression harder to revert in isolation and the PR description's verification plan only mentions the freeze flow. Consider splitting future work like this into a minimal fix(tokens): show freeze confirmation dialog PR followed by a refactor(tokens): centralize error banners PR — or at minimum, expand the PR description to enumerate every screen and helper changed.

🤖 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/tokens/freeze_tokens_screen.rs`:
- [SUGGESTION] lines 225-235: show_confirmation_popup carries unreachable dialog construction and re-allocates the prompt every frame
  After this fix, the click handler at lines 603-613 unconditionally assigns `self.confirmation_dialog = Some(ConfirmationDialog::new("Confirm Freeze", msg)…)`, and `ui()` only calls `show_confirmation_popup` under `if self.confirmation_dialog.is_some()` (line 617). That means the closure passed to `get_or_insert_with` at lines 231-235 cannot run in normal flow — its constructor is dead code. Two consequences: (1) the `format!("Are you sure you want to freeze identity {identity_id}?", …)` at lines 226-229 still allocates a fresh `String` on every paint while the dialog is open, even though the stored dialog already owns a copy of that message; (2) the title `"Confirm Freeze"` and the confirm/cancel labels are duplicated across the click handler and the helper, so a future copy edit at one site will silently drift from the other. Replace the prologue with an early-return so the click handler is the single source of construction.

In `src/ui/tokens/destroy_frozen_funds_screen.rs`:
- [SUGGESTION] lines 235-246: show_confirmation_popup carries unreachable dialog construction and re-allocates the prompt every frame
  Same shape as the Freeze screen. The click handler at lines 596-608 unconditionally assigns `self.confirmation_dialog = Some(ConfirmationDialog::new("Confirm Destroy Frozen Funds", msg)…danger_mode(true))` and the helper call at line 612 is gated on `is_some()`. The `get_or_insert_with` closure at lines 241-246 is therefore unreachable, while `format!("Are you sure you want to destroy frozen funds for identity {identity_id}? This action cannot be undone.", …)` at lines 236-239 still allocates a `String` on every paint and duplicates the click-handler wording (including `danger_mode(true)`). Drop the dead constructor and let the click handler remain the single source of construction.

Fresh dispatcher-requested review for this queue item. Same-SHA placeholder/fix-reply reviews already existed, so this review is posted explicitly rather than skipped by same-SHA dedup.

[lklimek]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

Code Review

The bugfix is correct and minimal: the Freeze button now populates confirmation_dialog instead of clearing it, and the new kittest in tests/kittest/token_action_confirmation_dialogs.rs exercises the real ScreenLike::ui() paint path for Freeze/Unfreeze/Mint, closing the regression-coverage gap. Two valid quality observations remain: (1) the click-handler now duplicates dialog construction with show_confirmation_popup, leaving the helper's get_or_insert_with branch unreachable and the prompt format! allocating every frame; (2) this PR moved Freeze and Destroy prompts to named {identity_id} placeholders but left the parallel Unfreeze/Mint prompts on positional {}, introducing an in-repo inconsistency.

Reviewed commit: 8b36373

💬 3 nitpick(s)

2 additional findings

💬 nitpick: `show_confirmation_popup`'s `get_or_insert_with` branch is unreachable; prompt allocates every frame

src/ui/tokens/freeze_tokens_screen.rs (lines 225-249)

Verified at 8b36373: show_confirmation_popup is only called from line 618 inside if self.confirmation_dialog.is_some() (line 617), and the click handler at lines 603-613 unconditionally assigns self.confirmation_dialog = Some(ConfirmationDialog::new("Confirm Freeze", msg)…) before that gate runs. The remaining mutations of confirmation_dialog inside the helper are take()/= None on Confirmed/Canceled, both of which exit before the next call. The get_or_insert_with closure at lines 231-235 therefore can never run. Two real consequences: (1) the "Confirm Freeze" title, confirm/cancel button labels, and prompt wording are now duplicated between the helper and the click handler — a future copy-edit at one site silently drifts from the other; (2) the format!("Are you sure you want to freeze identity {identity_id}?", …) at lines 226-229 still allocates a fresh String on every frame the dialog is open, even though the resulting string is then discarded because the closure never fires. Cleanest fix: drop the local msg/get_or_insert_with block and replace with let Some(confirmation_dialog) = self.confirmation_dialog.as_mut() else { return AppAction::None; };, leaving the click handler as the sole construction site. The same shape exists in unfreeze_tokens_screen.rs (lines 228-238 / 584-595), mint_tokens_screen.rs (lines 252-262), and destroy_frozen_funds_screen.rs — apply uniformly. Out of scope for this focused fix; track as a follow-up.

💡 Suggested change

        let Some(confirmation_dialog) = self.confirmation_dialog.as_mut() else {
            return AppAction::None;
        };

💬 nitpick: i18n placeholder style is inconsistent across token confirmation dialogs after this PR

src/ui/tokens/unfreeze_tokens_screen.rs (lines 229-232)

CLAUDE.md asks user-facing strings to use named placeholders so each string extracts as a single i18n translation unit. This PR converted freeze_tokens_screen.rs (lines 226-229, 604-607) and destroy_frozen_funds_screen.rs to named {identity_id}, but the parallel prompts in unfreeze_tokens_screen.rs (line 230 "Are you sure you want to unfreeze identity {}?" and line 587 in the click handler) and mint_tokens_screen.rs (line 254 "Are you sure you want to mint {} tokens to {}?" and the matching click handler) are unchanged. The result is a same-family family of confirmation dialogs that now uses two different placeholder styles. Either keep this PR scoped to the Freeze regression and revert the drive-by on destroy_frozen_funds_screen.rs, or apply the named-placeholder change consistently to Unfreeze and Mint as well. Not a defect — flagging because the inconsistency is what this PR introduced.

[thepastaclaw]

💬 Nitpick: mem::forget(temp) permanently leaks one tempdir per test invocation

std::mem::forget(temp) at line 60 is intentional so the data dir outlives the test (AppContext stores the path and SPV may lazily write into it). That is a reasonable trade-off for a short-lived cargo test process, but each of the three tests leaks its own tempdir and these accumulate on disk per test run. Two cleaner alternatives that preserve the lifetime guarantee without leaking: (1) hold the TempDir inside the harness's ScreenApp so its Drop runs after the harness is dropped; (2) use tempfile::TempDir::into_path(), which keeps the directory alive and returns the PathBuf without going through forget. Functionally equivalent, but avoids permanently leaking N tempdirs per CI shard run.

_{source: ['claude']}