fix(ui): add wallet alias trimming and 64-char length limit#624
fix(ui): add wallet alias trimming and 64-char length limit#624thepastaclaw wants to merge 1 commit intodashpay:v1.0-devfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change enhances wallet name input validation and user experience by enforcing a 64-character limit, implementing consistent trimming logic, displaying a dynamic character counter, and adding a helper message about default wallet names. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/ui/wallets/add_new_wallet_screen.rs (1)
788-794: PreferTextEdit::char_limit(64)over post-render truncation.The current approach truncates
alias_inputafter the widget has already rendered the updated text (line 790 runs first), so the 65th character flashes on screen for one frame before disappearing on the next repaint.
egui::TextEdithas achar_limit: usizefield, andchar_limit()"Restricts input to maximum number of characters", preventing over-limit characters from being accepted in the first place. This makes the manual check redundant.♻️ Suggested refactor
- ui.horizontal(|ui| { - ui.label("Wallet Name:"); - ui.text_edit_singleline(&mut self.alias_input); - }); - if self.alias_input.chars().count() > 64 { - self.alias_input = self.alias_input.chars().take(64).collect(); - } + ui.horizontal(|ui| { + ui.label("Wallet Name:"); + ui.add(egui::TextEdit::singleline(&mut self.alias_input).char_limit(64)); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/add_new_wallet_screen.rs` around lines 788 - 794, Replace the post-render truncation of self.alias_input with egui's built-in char limit: use ui.add(egui::TextEdit::singleline(&mut self.alias_input).char_limit(64)) (or the equivalent TextEdit::char_limit(64) builder) in place of ui.text_edit_singleline(&mut self.alias_input), and remove the manual check/assignment that trims self.alias_input.chars().take(64).collect(); this prevents the 65th character flash and centralizes the limit in the TextEdit widget.
🤖 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/ui/wallets/add_new_wallet_screen.rs`:
- Around line 800-808: The counter visibility currently uses trimmed length
(computed via self.alias_input.trim().chars().count()) while the input is capped
at 64 characters untrimmed, allowing whitespace to fill the field without
triggering the counter; change the threshold check to use the untrimmed
character count (self.alias_input.chars().count()) for deciding when to show the
counter, but continue to display the trimmed count in the UI (so the label still
shows the effective saved length), keeping the 64-character enforcement as-is.
- Line 243: The rename dialog currently truncates the rename_input by bytes
which can panic on UTF-8 boundaries; replace the byte-truncation in the rename
dialog handling (field self.rename_input inside the rename dialog code in
wallets_screen mod) with a char-boundary-safe truncation that builds a new
String from the first 64 chars (use chars().take(64).collect into a new String)
and then assign that new String back to self.rename_input so no truncate() on a
possibly-mid-character byte index is used.
---
Nitpick comments:
In `@src/ui/wallets/add_new_wallet_screen.rs`:
- Around line 788-794: Replace the post-render truncation of self.alias_input
with egui's built-in char limit: use ui.add(egui::TextEdit::singleline(&mut
self.alias_input).char_limit(64)) (or the equivalent TextEdit::char_limit(64)
builder) in place of ui.text_edit_singleline(&mut self.alias_input), and remove
the manual check/assignment that trims
self.alias_input.chars().take(64).collect(); this prevents the 65th character
flash and centralizes the limit in the TextEdit widget.
|
Coderabbit has feedback + I don't think we should trim to 64, we should reject above 64 |
|
Addressed in 7cd3644:
Validation run:
|
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Seems to work fine, limits to 64, trims whitespace
a strange behavior; a user can have two wallets with same name; but that's not related to this I don't think
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean alias validation for add-wallet and rename screens with egui char_limit() enforcement. However, the import wallet flow still bypasses the new validation — imported wallets can still have overlong or untrimmed aliases.
Reviewed commit: 7cd3644
🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: Import wallet flow bypasses alias trimming and 64-char limit
src/ui/wallets/import_mnemonic_screen.rs (line 127)
The ImportMnemonicScreen still writes self.alias_input.clone() directly for both HD-wallet import and single-key import with no trimming or length validation. Users can create wallets with leading/trailing whitespace, whitespace-only aliases, or names longer than 64 characters through the import flows, despite this PR introducing that invariant elsewhere.
Consider adding the same TextEdit::char_limit(WALLET_ALIAS_MAX_CHARS) to the import screen's text field and applying .trim() before persisting.
🤖 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/wallets/import_mnemonic_screen.rs`:
- [SUGGESTION] line 127: Import wallet flow bypasses alias trimming and 64-char limit
The `ImportMnemonicScreen` still writes `self.alias_input.clone()` directly for both HD-wallet import and single-key import with no trimming or length validation. Users can create wallets with leading/trailing whitespace, whitespace-only aliases, or names longer than 64 characters through the import flows, despite this PR introducing that invariant elsewhere.
Consider adding the same `TextEdit::char_limit(WALLET_ALIAS_MAX_CHARS)` to the import screen's text field and applying `.trim()` before persisting.
rust-dashcore #624 changed TransactionRecord::label from Option<String> to String (empty string = no label). Convert at the evo-tool boundary by mapping empty to None so WalletTransaction.label stays Option<String>.
…tionRecord::label Brings in two evo-tool fixes from feat/platform-wallet-run: 1. fix(wallet): use try_state for UI to avoid blocking_read panic PlatformWallet::state_blocking / state_mut_blocking panic when called from a tokio runtime context, and the egui main thread runs inside runtime.block_on(start) at main.rs:29. Every UI render callback and ZMQ tx-finality handler was hitting 'Cannot block the current thread from within a runtime' on first interaction. UI path: migrate all state_blocking() call sites to try_state() with None-guard fallthroughs. ZMQ path: dispatch the wallet mutation via tokio::spawn so the state_mut().await runs on a worker thread, not the main thread. Backend tasks under spawn_blocking: brief try_state retry loop with a new TaskError::WalletBusy variant. 2. fix(wallet): adapt to TransactionRecord::label -> String Upstream PR #624 switched dashcore's TransactionRecord.label from Option<String> to String. Map empty to None at the evo-tool boundary so WalletTransaction.label stays Option<String>. # Conflicts: # src/backend_task/dashpay/platform_wallet_cache.rs # src/model/wallet/mod.rs
thepastaclaw
force-pushed
the
fix/wallet-alias-trim
branch
from
7cd3644 to
2c71fd6
Compare
|
⏳ Review in progress (commit 2c71fd6) |
3 participants
Issue
Wallet alias input has no length limit and doesn't trim whitespace, which can lead to:
Changes
src/ui/wallets/add_new_wallet_screen.rs:N/64shown when >50 chars)Validation
cargo check✅cargo clippy --all-features --all-targets -- -D warnings✅cargo fmt✅Summary by CodeRabbit
Release Notes