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 complete (commit 4455b3a) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small UI fix adding wallet-alias trimming and a 64-char cap across three screens. The changes are correct, but the three screens are not consistent with each other: add-new-wallet errors on whitespace-only input, import-mnemonic silently normalizes to the default, and there are some duplicated checks / counter bookkeeping quirks worth tightening.
Reviewed commit: 2c71fd6
🟡 2 suggestion(s) | 💬 2 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/ui/wallets/add_new_wallet_screen.rs`:
- [SUGGESTION] lines 31-52: Whitespace-only alias behavior is inconsistent across wallet screens
`add_new_wallet_screen::wallet_alias_validation_error` rejects a whitespace-only `alias_input` with an error and `save_wallet` bails at lines 176-178, so `" "` never reaches the default-name branch at lines 180-188. The sibling screens take the opposite path: `import_mnemonic_screen.rs:146` and `:214` treat a trimmed-empty alias as a blank input and fall back to the autogenerated `"Wallet N" / "Key N"` default silently, and they also lack the helper copy, counter, and validation error label that add-new grew. The result is two different UX outcomes for the same input, and no shared validator.
Pick one semantic and apply it everywhere: either promote `wallet_alias_validation_error` (plus the helper text and counter) into a shared module used by both `add_new_wallet_screen` and `import_mnemonic_screen`, or drop the whitespace-only error in add-new so trimmed-empty input falls back to the default name like the other screens do. Given the PR title ("add wallet alias trimming"), treating trimmed-empty as blank is the more natural reading; a clear helper line ("Leave blank to use a default wallet name.") plus silent fallback avoids blocking submission on whitespace.
- [SUGGESTION] lines 715-727: Character counter mixes raw and trimmed counts
The visibility gate uses `raw_char_count = self.alias_input.chars().count()` but the number rendered is `displayed_char_count = self.alias_input.trim().chars().count()`. `TextEdit::char_limit` at line 705 is enforced on the raw string, so a user typing 60 printable characters plus 4 trailing spaces sees the counter appear (raw=64 > 50) reading `60/64` even though the widget will refuse further input. Conversely, 55 characters of pure whitespace would render `0/64` while the widget is about to block the next keystroke.
Use the same value for both the threshold and the display. `raw_char_count` matches what `char_limit` actually caps and is the most accurate signal for the user.
| fn wallet_alias_validation_error(alias_input: &str) -> Option<String> { | ||
| if alias_input.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| if alias_input.chars().count() > WALLET_ALIAS_MAX_CHARS { | ||
| return Some(format!( | ||
| "Wallet name must be {} characters or fewer.", | ||
| WALLET_ALIAS_MAX_CHARS | ||
| )); | ||
| } | ||
|
|
||
| let trimmed = alias_input.trim(); | ||
| if trimmed.is_empty() { | ||
| return Some( | ||
| "Wallet name cannot be only whitespace. Clear the field to use the default name." | ||
| .to_string(), | ||
| ); | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Whitespace-only alias behavior is inconsistent across wallet screens
add_new_wallet_screen::wallet_alias_validation_error rejects a whitespace-only alias_input with an error and save_wallet bails at lines 176-178, so " " never reaches the default-name branch at lines 180-188. The sibling screens take the opposite path: import_mnemonic_screen.rs:146 and :214 treat a trimmed-empty alias as a blank input and fall back to the autogenerated "Wallet N" / "Key N" default silently, and they also lack the helper copy, counter, and validation error label that add-new grew. The result is two different UX outcomes for the same input, and no shared validator.
Pick one semantic and apply it everywhere: either promote wallet_alias_validation_error (plus the helper text and counter) into a shared module used by both add_new_wallet_screen and import_mnemonic_screen, or drop the whitespace-only error in add-new so trimmed-empty input falls back to the default name like the other screens do. Given the PR title ("add wallet alias trimming"), treating trimmed-empty as blank is the more natural reading; a clear helper line ("Leave blank to use a default wallet name.") plus silent fallback avoids blocking submission on whitespace.
source: ['claude', 'codex']
🤖 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/ui/wallets/add_new_wallet_screen.rs`:
- [SUGGESTION] lines 31-52: Whitespace-only alias behavior is inconsistent across wallet screens
`add_new_wallet_screen::wallet_alias_validation_error` rejects a whitespace-only `alias_input` with an error and `save_wallet` bails at lines 176-178, so `" "` never reaches the default-name branch at lines 180-188. The sibling screens take the opposite path: `import_mnemonic_screen.rs:146` and `:214` treat a trimmed-empty alias as a blank input and fall back to the autogenerated `"Wallet N" / "Key N"` default silently, and they also lack the helper copy, counter, and validation error label that add-new grew. The result is two different UX outcomes for the same input, and no shared validator.
Pick one semantic and apply it everywhere: either promote `wallet_alias_validation_error` (plus the helper text and counter) into a shared module used by both `add_new_wallet_screen` and `import_mnemonic_screen`, or drop the whitespace-only error in add-new so trimmed-empty input falls back to the default name like the other screens do. Given the PR title ("add wallet alias trimming"), treating trimmed-empty as blank is the more natural reading; a clear helper line ("Leave blank to use a default wallet name.") plus silent fallback avoids blocking submission on whitespace.
| let raw_char_count = self.alias_input.chars().count(); | ||
| let displayed_char_count = self.alias_input.trim().chars().count(); | ||
| if raw_char_count > WALLET_ALIAS_COUNTER_SHOW_THRESHOLD { | ||
| ui.label( | ||
| RichText::new(format!( | ||
| "{}/{}", | ||
| displayed_char_count, WALLET_ALIAS_MAX_CHARS | ||
| )) | ||
| .weak() | ||
| .size(12.0), | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🟡 Suggestion: Character counter mixes raw and trimmed counts
The visibility gate uses raw_char_count = self.alias_input.chars().count() but the number rendered is displayed_char_count = self.alias_input.trim().chars().count(). TextEdit::char_limit at line 705 is enforced on the raw string, so a user typing 60 printable characters plus 4 trailing spaces sees the counter appear (raw=64 > 50) reading 60/64 even though the widget will refuse further input. Conversely, 55 characters of pure whitespace would render 0/64 while the widget is about to block the next keystroke.
Use the same value for both the threshold and the display. raw_char_count matches what char_limit actually caps and is the most accurate signal for the user.
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/ui/wallets/add_new_wallet_screen.rs`:
- [SUGGESTION] lines 715-727: Character counter mixes raw and trimmed counts
The visibility gate uses `raw_char_count = self.alias_input.chars().count()` but the number rendered is `displayed_char_count = self.alias_input.trim().chars().count()`. `TextEdit::char_limit` at line 705 is enforced on the raw string, so a user typing 60 printable characters plus 4 trailing spaces sees the counter appear (raw=64 > 50) reading `60/64` even though the widget will refuse further input. Conversely, 55 characters of pure whitespace would render `0/64` while the widget is about to block the next keystroke.
Use the same value for both the threshold and the display. `raw_char_count` matches what `char_limit` actually caps and is the most accurate signal for the user.
| let trimmed_alias = self.rename_input.trim(); | ||
| if trimmed_alias.is_empty() { | ||
| self.display_message( | ||
| "Wallet name cannot be empty or whitespace.", | ||
| MessageType::Error, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if trimmed_alias.chars().count() > WALLET_ALIAS_MAX_CHARS { | ||
| self.display_message( | ||
| "Wallet name must be 64 characters or fewer.", | ||
| MessageType::Error, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Rename dialog duplicates a length check the TextEdit already enforces
The rename TextEdit at line 2352 is configured with .char_limit(WALLET_ALIAS_MAX_CHARS), so the guard at lines 2378-2384 (if trimmed_alias.chars().count() > WALLET_ALIAS_MAX_CHARS) is unreachable via the UI and the error string duplicates the literal in add_new_wallet_screen.rs. Either drop the branch (leaving only the whitespace-only check, which the widget can still produce) or route both screens through the shared validator suggested above so the wording and rule live in one place.
source: ['claude']
| if alias_input.chars().count() > WALLET_ALIAS_MAX_CHARS { | ||
| return Some(format!( | ||
| "Wallet name must be {} characters or fewer.", | ||
| WALLET_ALIAS_MAX_CHARS | ||
| )); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Length branch is unreachable from the UI and uses positional format specifiers
wallet_alias_validation_error checks > WALLET_ALIAS_MAX_CHARS, but its only caller is save_wallet, reading from an alias_input whose TextEdit sets .char_limit(WALLET_ALIAS_MAX_CHARS) (line 705). The widget enforces the cap by characters, so this branch can't fire through normal UI flow. The message itself also uses positional {} ("Wallet name must be {} characters or fewer.") while CLAUDE.md asks for named placeholders (e.g. {max}) so strings are i18n-extractable as single units. Cheapest fix: drop the branch and keep only the whitespace-only check. Alternative: switch to "Wallet name must be {max} characters or fewer.".
source: ['claude']
thepastaclaw
force-pushed
the
fix/wallet-alias-trim
branch
from
2c71fd6 to
0e46d7d
Compare
|
Addressed the remaining alias follow-ups on the latest push ( |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR scope is correct: only 3 files (~77 lines) changed against v1.0-dev — Codex's 339-file blocking concern is a false positive caused by comparing against a stale local v1.0-dev. The change itself is functionally correct, but the validation logic is implemented inline across three screens with inconsistent UX (counter+inline error vs char_limit-only vs error-on-Save), the 64-char threshold is duplicated in three places, and the validator violates the project rule that pure input validation belongs in model/. No blockers.
Reviewed commit: 0e46d7d
🟡 3 suggestion(s) | 💬 2 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/ui/wallets/add_new_wallet_screen.rs`:
- [SUGGESTION] lines 28-44: Wallet alias validation belongs in `model/`, not duplicated across UI screens
CLAUDE.md is explicit: "Pure input validation (format, length, character sets) lives in `model/` as stateless functions — single source of truth, unit-testable, no dependencies on `AppContext` or `Sdk`. … UI screens may call model validators for instant user feedback, but must never implement their own validation logic — always delegate to the model function." This PR introduces three independent enforcement sites for the same rule:
- `add_new_wallet_screen.rs:28` — `const WALLET_ALIAS_MAX_CHARS: usize = 64` plus `wallet_alias_validation_error()` returning `Option<String>` (a user-facing message rather than a typed error)
- `wallets_screen/mod.rs:84` — a duplicate `const WALLET_ALIAS_MAX_CHARS: usize = 64` plus inline length+empty checks at 2369–2384 with the limit hard-coded as the literal `"64"` in the error message
- `import_mnemonic_screen.rs:630` — a raw `.char_limit(64)` literal with no shared constant
The user-facing strings have already drifted ("Wallet name must be {} characters or fewer." vs "Wallet name must be 64 characters or fewer.") and will continue to drift on every future tweak. A single `model::wallet::alias` module exporting `pub const MAX_CHARS: usize = 64;` and `pub fn validate(&str) -> Result<String, AliasError>` (returning the trimmed value or a typed `AliasError { TooLong { max, actual }, Empty }`) would give one source of truth for the threshold, make the trim/empty/length behavior consistent, separate the rule from its `Display` rendering (matchable, testable, translatable), and let all three screens import the same symbols.
- [SUGGESTION] lines 31-44: Stateless alias validator is introduced with no unit tests
`wallet_alias_validation_error` is a pure function with no dependencies — exactly the kind of code the project's testing conventions call out as belonging in inline `#[test]` modules. Add cases for empty (None), single ASCII char (None), exactly 64 chars (None), 65 chars (Some), 64 multi-byte unicode codepoints (None — verifying we count chars, not bytes), and 65 multi-byte codepoints (Some). This locks in the `chars().count()` semantics so a future refactor to `len()` doesn't regress to byte-counting. The whitespace-only case (currently allowed and silently substituted with the default name in `save_wallet`) is also worth pinning so the asymmetry with the rename screen is intentional. Becomes more valuable once the helper moves to `model/`.
In `src/ui/wallets/import_mnemonic_screen.rs`:
- [SUGGESTION] lines 627-631: Import-mnemonic alias input has no visible feedback when the limit is hit
In `add_new_wallet_screen.rs` the user sees a live counter (`N/64` once they pass 50 chars), a helper line ("Leave blank to use a default wallet name."), and an inline error from `wallet_alias_validation_error`. The rename flow surfaces an error banner on Save. But the import-mnemonic flow only adds `egui::TextEdit::singleline(&mut self.alias_input).char_limit(64)` — no counter, no helper text, and `char_limit` silently truncates pasted overflow with no message. This is a confusing UX divergence in the same PR. Reuse the same helper/counter UI here, or — better — drive both screens from a shared component once the validator is moved into `model/` (see the architecture finding).
| const WALLET_ALIAS_MAX_CHARS: usize = 64; | ||
| const WALLET_ALIAS_COUNTER_SHOW_THRESHOLD: usize = 50; | ||
|
|
||
| fn wallet_alias_validation_error(alias_input: &str) -> Option<String> { | ||
| if alias_input.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| if alias_input.chars().count() > WALLET_ALIAS_MAX_CHARS { | ||
| return Some(format!( | ||
| "Wallet name must be {} characters or fewer.", | ||
| WALLET_ALIAS_MAX_CHARS | ||
| )); | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Wallet alias validation belongs in model/, not duplicated across UI screens
CLAUDE.md is explicit: "Pure input validation (format, length, character sets) lives in model/ as stateless functions — single source of truth, unit-testable, no dependencies on AppContext or Sdk. … UI screens may call model validators for instant user feedback, but must never implement their own validation logic — always delegate to the model function." This PR introduces three independent enforcement sites for the same rule:
add_new_wallet_screen.rs:28—const WALLET_ALIAS_MAX_CHARS: usize = 64pluswallet_alias_validation_error()returningOption<String>(a user-facing message rather than a typed error)wallets_screen/mod.rs:84— a duplicateconst WALLET_ALIAS_MAX_CHARS: usize = 64plus inline length+empty checks at 2369–2384 with the limit hard-coded as the literal"64"in the error messageimport_mnemonic_screen.rs:630— a raw.char_limit(64)literal with no shared constant
The user-facing strings have already drifted ("Wallet name must be {} characters or fewer." vs "Wallet name must be 64 characters or fewer.") and will continue to drift on every future tweak. A single model::wallet::alias module exporting pub const MAX_CHARS: usize = 64; and pub fn validate(&str) -> Result<String, AliasError> (returning the trimmed value or a typed AliasError { TooLong { max, actual }, Empty }) would give one source of truth for the threshold, make the trim/empty/length behavior consistent, separate the rule from its Display rendering (matchable, testable, translatable), and let all three screens import the same symbols.
source: ['claude', 'codex']
🤖 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/ui/wallets/add_new_wallet_screen.rs`:
- [SUGGESTION] lines 28-44: Wallet alias validation belongs in `model/`, not duplicated across UI screens
CLAUDE.md is explicit: "Pure input validation (format, length, character sets) lives in `model/` as stateless functions — single source of truth, unit-testable, no dependencies on `AppContext` or `Sdk`. … UI screens may call model validators for instant user feedback, but must never implement their own validation logic — always delegate to the model function." This PR introduces three independent enforcement sites for the same rule:
- `add_new_wallet_screen.rs:28` — `const WALLET_ALIAS_MAX_CHARS: usize = 64` plus `wallet_alias_validation_error()` returning `Option<String>` (a user-facing message rather than a typed error)
- `wallets_screen/mod.rs:84` — a duplicate `const WALLET_ALIAS_MAX_CHARS: usize = 64` plus inline length+empty checks at 2369–2384 with the limit hard-coded as the literal `"64"` in the error message
- `import_mnemonic_screen.rs:630` — a raw `.char_limit(64)` literal with no shared constant
The user-facing strings have already drifted ("Wallet name must be {} characters or fewer." vs "Wallet name must be 64 characters or fewer.") and will continue to drift on every future tweak. A single `model::wallet::alias` module exporting `pub const MAX_CHARS: usize = 64;` and `pub fn validate(&str) -> Result<String, AliasError>` (returning the trimmed value or a typed `AliasError { TooLong { max, actual }, Empty }`) would give one source of truth for the threshold, make the trim/empty/length behavior consistent, separate the rule from its `Display` rendering (matchable, testable, translatable), and let all three screens import the same symbols.
|
|
||
| ui.horizontal(|ui| { | ||
| ui.label("Name:"); | ||
| ui.text_edit_singleline(&mut self.alias_input); | ||
| ui.add(egui::TextEdit::singleline(&mut self.alias_input).char_limit(64)); | ||
| }); |
There was a problem hiding this comment.
🟡 Suggestion: Import-mnemonic alias input has no visible feedback when the limit is hit
In add_new_wallet_screen.rs the user sees a live counter (N/64 once they pass 50 chars), a helper line ("Leave blank to use a default wallet name."), and an inline error from wallet_alias_validation_error. The rename flow surfaces an error banner on Save. But the import-mnemonic flow only adds egui::TextEdit::singleline(&mut self.alias_input).char_limit(64) — no counter, no helper text, and char_limit silently truncates pasted overflow with no message. This is a confusing UX divergence in the same PR. Reuse the same helper/counter UI here, or — better — drive both screens from a shared component once the validator is moved into model/ (see the architecture finding).
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/ui/wallets/import_mnemonic_screen.rs`:
- [SUGGESTION] lines 627-631: Import-mnemonic alias input has no visible feedback when the limit is hit
In `add_new_wallet_screen.rs` the user sees a live counter (`N/64` once they pass 50 chars), a helper line ("Leave blank to use a default wallet name."), and an inline error from `wallet_alias_validation_error`. The rename flow surfaces an error banner on Save. But the import-mnemonic flow only adds `egui::TextEdit::singleline(&mut self.alias_input).char_limit(64)` — no counter, no helper text, and `char_limit` silently truncates pasted overflow with no message. This is a confusing UX divergence in the same PR. Reuse the same helper/counter UI here, or — better — drive both screens from a shared component once the validator is moved into `model/` (see the architecture finding).
| fn wallet_alias_validation_error(alias_input: &str) -> Option<String> { | ||
| if alias_input.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| if alias_input.chars().count() > WALLET_ALIAS_MAX_CHARS { | ||
| return Some(format!( | ||
| "Wallet name must be {} characters or fewer.", | ||
| WALLET_ALIAS_MAX_CHARS | ||
| )); | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Stateless alias validator is introduced with no unit tests
wallet_alias_validation_error is a pure function with no dependencies — exactly the kind of code the project's testing conventions call out as belonging in inline #[test] modules. Add cases for empty (None), single ASCII char (None), exactly 64 chars (None), 65 chars (Some), 64 multi-byte unicode codepoints (None — verifying we count chars, not bytes), and 65 multi-byte codepoints (Some). This locks in the chars().count() semantics so a future refactor to len() doesn't regress to byte-counting. The whitespace-only case (currently allowed and silently substituted with the default name in save_wallet) is also worth pinning so the asymmetry with the rename screen is intentional. Becomes more valuable once the helper moves to model/.
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/ui/wallets/add_new_wallet_screen.rs`:
- [SUGGESTION] lines 31-44: Stateless alias validator is introduced with no unit tests
`wallet_alias_validation_error` is a pure function with no dependencies — exactly the kind of code the project's testing conventions call out as belonging in inline `#[test]` modules. Add cases for empty (None), single ASCII char (None), exactly 64 chars (None), 65 chars (Some), 64 multi-byte unicode codepoints (None — verifying we count chars, not bytes), and 65 multi-byte codepoints (Some). This locks in the `chars().count()` semantics so a future refactor to `len()` doesn't regress to byte-counting. The whitespace-only case (currently allowed and silently substituted with the default name in `save_wallet`) is also worth pinning so the asymmetry with the rename screen is intentional. Becomes more valuable once the helper moves to `model/`.
| if alias_input.chars().count() > WALLET_ALIAS_MAX_CHARS { | ||
| return Some(format!( | ||
| "Wallet name must be {} characters or fewer.", | ||
| WALLET_ALIAS_MAX_CHARS | ||
| )); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Use a named placeholder for the max-length value in the error string
CLAUDE.md asks for i18n-ready strings with named placeholders ({max}) so each user-facing string is a single extractable translation unit. format!("Wallet name must be {} characters or fewer.", WALLET_ALIAS_MAX_CHARS) uses an anonymous positional {} — translators won't see what {} refers to. The same hardcoded literal at wallets_screen/mod.rs:2380 has the inverse problem (the number is baked into the string and will silently disagree with the constant if it ever moves). Use format!("Wallet name must be {max} characters or fewer.", max = WALLET_ALIAS_MAX_CHARS) for both call sites.
💡 Suggested change
| if alias_input.chars().count() > WALLET_ALIAS_MAX_CHARS { | |
| return Some(format!( | |
| "Wallet name must be {} characters or fewer.", | |
| WALLET_ALIAS_MAX_CHARS | |
| )); | |
| } | |
| if alias_input.chars().count() > WALLET_ALIAS_MAX_CHARS { | |
| return Some(format!( | |
| "Wallet name must be {max} characters or fewer.", | |
| max = WALLET_ALIAS_MAX_CHARS | |
| )); | |
| } |
source: ['claude']
| if alias_input.chars().count() > WALLET_ALIAS_MAX_CHARS { | ||
| return Some(format!( | ||
| "Wallet name must be {} characters or fewer.", | ||
| WALLET_ALIAS_MAX_CHARS | ||
| )); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: The > WALLET_ALIAS_MAX_CHARS branch is unreachable through the UI
TextEdit::singleline(...).char_limit(WALLET_ALIAS_MAX_CHARS) at line 697 prevents self.alias_input from ever exceeding 64 Unicode chars (egui's char_limit truncates pasted overflow as well), so the > WALLET_ALIAS_MAX_CHARS branch can't fire through normal interaction and the early-return at line 168 is dead code on this path. This is fine as defense-in-depth if the validator is ever called from a context without the input cap (e.g. once it moves to model/), but worth noting that the user will never see this error string today. If the validator is moved to model/, also cover the empty-after-trim case so the asymmetry between create (auto-generate name) and rename (reject empty) is explicit rather than hidden across two helpers.
source: ['claude']
thepastaclaw
force-pushed
the
fix/wallet-alias-trim
branch
from
0e46d7d to
4455b3a
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The earlier broad-scope blocker from the old SHA no longer applies on 4455b3a: against upstream/v1.0-dev, this PR is now a narrow 7-file wallet-alias change that adds shared alias validation, trims leading/trailing whitespace before save, and enforces the 64-character UI/input limit on the relevant wallet-name flows. I re-checked the actual upstream merge diff and did not find a current-SHA defect to carry forward.
Reviewed commit: 4455b3a
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