fix: handle ContractNotFound and TokenNotFound in add token screen#718
fix: handle ContractNotFound and TokenNotFound in add token screen#718thepastaclaw wants to merge 1 commit into
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:
📝 WalkthroughWalkthroughAdds a one-time fallback from contract-ID to token-ID lookup when a contract lookup returns not-found and the input is valid Base58; introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 review |
✅ Actions performedReview triggered.
|
![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
🤖 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/tokens/add_token_by_id_screen.rs`:
- Around line 302-317: The ContractNotFound branch currently always sets
try_token_id_next and causes a loop; add a boolean field tried_token_id_lookup
to the AddTokenByIdScreen state, initialize it false in the constructor and
reset it to false in show_success_screen (and whenever starting a new lookup),
then update display_task_result's BackendTaskSuccessResult::ContractNotFound
handling to only set try_token_id_next = true and set tried_token_id_lookup =
true when tried_token_id_lookup is false (i.e., the initial contract-ID
fallback), and if tried_token_id_lookup is already true treat it as an error
(call MessageBanner::set_global and set self.status = AddTokenStatus::Error) so
a ContractNotFound from the FetchTokenByTokenId path does not loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7328d80d-4ac4-4db1-a36d-9046705536c2
📒 Files selected for processing (1)
src/ui/tokens/add_token_by_id_screen.rs
…otFound Add tried_token_id_lookup field to track whether the token ID fallback has already been attempted. Without this, when FetchTokenByTokenId finds a token but its subsequent contract fetch fails (returning ContractNotFound), the handler would set try_token_id_next = true again, creating an infinite loop. The guard ensures ContractNotFound only triggers the token ID fallback once. On subsequent ContractNotFound results, it shows an error banner and transitions to Error state. Addresses CodeRabbit review feedback on dashpay#718.
thepastaclaw
force-pushed
the
fix/token-search-not-found
branch
from
9032752 to
baff28e
Compare
|
Addressed the infinite loop concern — added a @coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/tokens/add_token_by_id_screen.rs (1)
272-281: Consider adding the same guard todisplay_messagefor consistency.The
display_messagemethod also setstry_token_id_next = truewhen handling "Contract not found" string messages (line 278), but doesn't checktried_token_id_lookup. While the typedBackendTaskSuccessResult::ContractNotFoundnow goes throughdisplay_task_resultcorrectly, this string-based path could theoretically trigger the same infinite loop if messages arrive via this route.♻️ Suggested fix for consistency
} else if msg.contains("Contract not found") { // Contract not found, try as token ID - if let Ok(_identifier) = + if !self.tried_token_id_lookup && Identifier::from_string(&self.contract_or_token_id_input, Encoding::Base58) - Identifier::from_string(&self.contract_or_token_id_input, Encoding::Base58) + .is_ok() { - { // We'll initiate a token ID search self.try_token_id_next = true; + self.tried_token_id_lookup = true; } else { self.status = AddTokenStatus::Error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/add_token_by_id_screen.rs` around lines 272 - 281, The display_message handler sets try_token_id_next = true when msg.contains("Contract not found") but lacks the same guard used elsewhere; update display_message (the method handling string messages) to check the tried_token_id_lookup flag before setting try_token_id_next (the same guard used when processing BackendTaskSuccessResult::ContractNotFound) and only set try_token_id_next = true if tried_token_id_lookup is false, otherwise set status to AddTokenStatus::Error to avoid re-triggering the lookup loop; locate display_message and modify the "Contract not found" branch accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/tokens/add_token_by_id_screen.rs`:
- Around line 272-281: The display_message handler sets try_token_id_next = true
when msg.contains("Contract not found") but lacks the same guard used elsewhere;
update display_message (the method handling string messages) to check the
tried_token_id_lookup flag before setting try_token_id_next (the same guard used
when processing BackendTaskSuccessResult::ContractNotFound) and only set
try_token_id_next = true if tried_token_id_lookup is false, otherwise set status
to AddTokenStatus::Error to avoid re-triggering the lookup loop; locate
display_message and modify the "Contract not found" branch accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82c1ccc3-68af-49a1-b587-1e175bb8c5e1
📒 Files selected for processing (1)
src/ui/tokens/add_token_by_id_screen.rs
thepastaclaw
force-pushed
the
fix/token-search-not-found
branch
from
d8c603d to
3717f62
Compare
|
Also added the @coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/tokens/add_token_by_id_screen.rs (1)
272-290: Consider extracting the shared fallback logic to reduce duplication.The ContractNotFound handling logic in
display_message(lines 272-290) anddisplay_task_result(lines 315-335) is nearly identical. Extracting this into a private helper method would improve maintainability and ensure both paths stay consistent.♻️ Optional: Extract helper method
+ /// Handles contract-not-found by attempting a one-time fallback to token ID lookup. + fn handle_contract_not_found(&mut self) { + if !self.tried_token_id_lookup + && Identifier::from_string(&self.contract_or_token_id_input, Encoding::Base58).is_ok() + { + self.try_token_id_next = true; + self.tried_token_id_lookup = true; + } else { + MessageBanner::set_global( + self.app_context.egui_ctx(), + "No contract or token found for the given identifier", + MessageType::Error, + ); + self.status = AddTokenStatus::Error; + } + }Then simplify both call sites:
// In display_message(): } else if msg.contains("Contract not found") { self.handle_contract_not_found(); } // In display_task_result(): BackendTaskSuccessResult::ContractNotFound => { self.handle_contract_not_found(); }Also applies to: 315-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/add_token_by_id_screen.rs` around lines 272 - 290, Extract the duplicated "Contract not found" fallback into a private helper on the same impl (e.g., fn handle_contract_not_found(&mut self)) that performs the Identifier::from_string(&self.contract_or_token_id_input, Encoding::Base58).is_ok() check, sets self.try_token_id_next = true and self.tried_token_id_lookup = true when valid, otherwise calls MessageBanner::set_global(self.app_context.egui_ctx(), "No contract or token found for the given identifier", MessageType::Error) and sets self.status = AddTokenStatus::Error; then replace the inline blocks in display_message and the BackendTaskSuccessResult::ContractNotFound arm inside display_task_result with calls to this new handle_contract_not_found method so both code paths share the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/tokens/add_token_by_id_screen.rs`:
- Around line 272-290: Extract the duplicated "Contract not found" fallback into
a private helper on the same impl (e.g., fn handle_contract_not_found(&mut
self)) that performs the
Identifier::from_string(&self.contract_or_token_id_input,
Encoding::Base58).is_ok() check, sets self.try_token_id_next = true and
self.tried_token_id_lookup = true when valid, otherwise calls
MessageBanner::set_global(self.app_context.egui_ctx(), "No contract or token
found for the given identifier", MessageType::Error) and sets self.status =
AddTokenStatus::Error; then replace the inline blocks in display_message and the
BackendTaskSuccessResult::ContractNotFound arm inside display_task_result with
calls to this new handle_contract_not_found method so both code paths share the
same logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aeba571b-22c6-4920-9ae4-ca45bd9b2aa5
📒 Files selected for processing (1)
src/ui/tokens/add_token_by_id_screen.rs
thepastaclaw
force-pushed
the
fix/token-search-not-found
branch
from
3717f62 to
326e1da
Compare
|
Extracted the duplicated |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-targeted fix for issue #691. The display_task_result wildcard was silently swallowing ContractNotFound and TokenNotFound, leaving the UI stuck in Searching state. The fix correctly handles both variants with a one-time fallback to token-ID lookup and proper Error state transitions. Both agents confirm no issues found.
Reviewed commit: 326e1da
|
✅ Review complete (commit 866fefa) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The fix correctly handles the previously-swallowed typed BackendTaskSuccessResult::ContractNotFound and TokenNotFound variants, ending the indefinite Searching hang. Verification confirmed several real follow-up concerns: incomplete state reset on a new search (stale fetched_contract/selected_token and a global tried_token_id_lookup flag that is not request-scoped), now-unreachable string-match branches in display_message, and missing tests/manual scenarios for what is fundamentally a state-machine fix. None rise to blocking after verification — the race produces a brief flicker rather than a hang.
Reviewed commit: 326e1da
🟡 5 suggestion(s) | 💬 2 nitpick(s)
2 additional findings
🟡 suggestion: `tried_token_id_lookup` is screen-global, not per-request — a stale result can mis-target the fallback
src/ui/tokens/add_token_by_id_screen.rs (lines 76-105)
The Search button (lines 76-80) is only disabled when the input is empty — it is enabled while status == Searching, so the user can dispatch lookup B while lookup A is still in flight. The new Search handler resets tried_token_id_lookup = false, but handle_contract_not_found() keys the fallback on the current contract_or_token_id_input, not on the request that produced the result. If A returns ContractNotFound after B was issued, handle_contract_not_found() sets try_token_id_next = true and tried_token_id_lookup = true against B's input, fires FetchTokenByTokenId(B), and then when B's own contract task returns ContractNotFound, the fallback is skipped because the flag is already set — the screen flickers to Error. The in-flight token-id request will eventually overwrite the state if it succeeds, but until then the user sees an incorrect error. Either disable Search while in Searching, or carry a request-scoped token (e.g. an incrementing search id) and ignore results that don't match.
🟡 suggestion: String-match fallbacks for 'Contract not found' / 'Token not found' in `display_message` are now dead code
src/ui/tokens/add_token_by_id_screen.rs (lines 290-296)
Now that the backend emits typed BackendTaskSuccessResult::ContractNotFound and BackendTaskSuccessResult::TokenNotFound (handled in the new arms of display_task_result), no caller produces a Message(msg) with MessageType::Success whose text contains these substrings. Verified via grep: the only producers of those literal strings (grovestark_prover.rs:158-159, tokens_screen/structs.rs:165/198/229, add_contracts_screen.rs:225, document_action_screen.rs:841) are unrelated paths and never reach this screen's display_message with MessageType::Success. Encoding the same intent twice (typed variant + string heuristic) is a maintenance hazard: future wording changes in the backend will silently desync. Remove lines 290-291 and the Token not found branch on line 292; keep only the Error fetching contracts branch, which is still live (backend format! Err strings at backend_task/tokens/mod.rs:524/550/554).
🤖 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/add_token_by_id_screen.rs`:
- [SUGGESTION] lines 83-85: `Search` does not clear `fetched_contract` and `selected_token`, allowing stale Add-Token state
On `Search` (lines 83-85) only `status` and `tried_token_id_lookup` are reset. `fetched_contract` and `selected_token` are not. `render_add_button()` (line 138) is rendered unconditionally and only checks that those two `Option`s are populated — not the current `status`. So if a successful first lookup is followed by a second lookup that ends in `ContractNotFound`/`TokenNotFound`, the screen sits in `AddTokenStatus::Error` while still showing a working `Add Token` button bound to the previous selection. Clicking it will save the stale token. Also clear `try_token_id_next` here for symmetry with the `add_another` reset path (lines 184-189). The enum `AddTokenStatus` no longer fully owns the screen-state invariant; each new lookup must explicitly clear the side-channel fields before dispatching the task.
- [SUGGESTION] lines 76-105: `tried_token_id_lookup` is screen-global, not per-request — a stale result can mis-target the fallback
The `Search` button (lines 76-80) is only disabled when the input is empty — it is enabled while `status == Searching`, so the user can dispatch lookup B while lookup A is still in flight. The new `Search` handler resets `tried_token_id_lookup = false`, but `handle_contract_not_found()` keys the fallback on the *current* `contract_or_token_id_input`, not on the request that produced the result. If A returns `ContractNotFound` after B was issued, `handle_contract_not_found()` sets `try_token_id_next = true` and `tried_token_id_lookup = true` against B's input, fires `FetchTokenByTokenId(B)`, and then when B's own contract task returns `ContractNotFound`, the fallback is skipped because the flag is already set — the screen flickers to `Error`. The in-flight token-id request will eventually overwrite the state if it succeeds, but until then the user sees an incorrect error. Either disable `Search` while in `Searching`, or carry a request-scoped token (e.g. an incrementing search id) and ignore results that don't match.
- [SUGGESTION] lines 196-329: No unit/UI tests cover the new `ContractNotFound`/`TokenNotFound` state machine
This PR is a state-machine fix — the bug was a wildcard arm silently swallowing a typed variant. The new logic introduces persistent flags (`try_token_id_next`, `tried_token_id_lookup`) plus a shared `handle_contract_not_found()` helper, none of which are exercised by tests. A small unit test on `AddTokenByIdScreen` could cover: (1) `display_task_result(ContractNotFound)` with a valid base58 input transitions out of `Searching` via `try_token_id_next=true` and sets `tried_token_id_lookup=true`; (2) a second `ContractNotFound` after the flag is set transitions to `Error` instead of looping; (3) `display_task_result(TokenNotFound)` transitions to `Error`; (4) clicking `Search` again resets the flag. CLAUDE.md also requires a manual test scenario file under `docs/ai-design/<date>-…/` referenced from the PR description; the PR description currently lists only `cargo check`/`cargo clippy`. Without focused coverage, the same class of regression (a new `_ => {}` arm swallowing a future variant) can re-emerge silently.
- [SUGGESTION] lines 290-296: String-match fallbacks for 'Contract not found' / 'Token not found' in `display_message` are now dead code
Now that the backend emits typed `BackendTaskSuccessResult::ContractNotFound` and `BackendTaskSuccessResult::TokenNotFound` (handled in the new arms of `display_task_result`), no caller produces a `Message(msg)` with `MessageType::Success` whose text contains these substrings. Verified via grep: the only producers of those literal strings (`grovestark_prover.rs:158-159`, `tokens_screen/structs.rs:165/198/229`, `add_contracts_screen.rs:225`, `document_action_screen.rs:841`) are unrelated paths and never reach this screen's `display_message` with `MessageType::Success`. Encoding the same intent twice (typed variant + string heuristic) is a maintenance hazard: future wording changes in the backend will silently desync. Remove lines 290-291 and the `Token not found` branch on line 292; keep only the `Error fetching contracts` branch, which is still live (backend `format!` Err strings at `backend_task/tokens/mod.rs:524/550/554`).
- [SUGGESTION] lines 316-326: `ContractNotFound` from the token-id path produces a misleading user message
`BackendTaskSuccessResult::ContractNotFound` is produced from two distinct paths in `src/backend_task/tokens/mod.rs`: (1) line 523 — initial `FetchTokenByContractId` returned `Ok(None)` (legitimate 'no contract'); and (2) line 549 — inside `FetchTokenByTokenId`, the `TokenContractInfo` lookup *succeeded* but the subsequent `DataContract::fetch_by_identifier` returned `Ok(None)` (a dangling token→contract pointer). When path (2) fires, `tried_token_id_lookup` is already `true`, so `handle_contract_not_found()` falls through to the generic 'No contract or token found for the given identifier' banner — but the token *was* resolvable, only its contract is missing. Either emit a distinct backend variant (e.g. `ContractNotFoundForToken`) or branch on `tried_token_id_lookup` here to surface a different, more accurate message for path (2).
| let now = Utc::now().timestamp() as u32; | ||
| self.status = AddTokenStatus::Searching(now); | ||
| self.tried_token_id_lookup = false; |
There was a problem hiding this comment.
🟡 Suggestion: Search does not clear fetched_contract and selected_token, allowing stale Add-Token state
On Search (lines 83-85) only status and tried_token_id_lookup are reset. fetched_contract and selected_token are not. render_add_button() (line 138) is rendered unconditionally and only checks that those two Options are populated — not the current status. So if a successful first lookup is followed by a second lookup that ends in ContractNotFound/TokenNotFound, the screen sits in AddTokenStatus::Error while still showing a working Add Token button bound to the previous selection. Clicking it will save the stale token. Also clear try_token_id_next here for symmetry with the add_another reset path (lines 184-189). The enum AddTokenStatus no longer fully owns the screen-state invariant; each new lookup must explicitly clear the side-channel fields before dispatching the task.
💡 Suggested change
| let now = Utc::now().timestamp() as u32; | |
| self.status = AddTokenStatus::Searching(now); | |
| self.tried_token_id_lookup = false; | |
| let now = Utc::now().timestamp() as u32; | |
| self.status = AddTokenStatus::Searching(now); | |
| self.fetched_contract = None; | |
| self.selected_token = None; | |
| self.try_token_id_next = false; | |
| self.tried_token_id_lookup = false; |
source: ['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/tokens/add_token_by_id_screen.rs`:
- [SUGGESTION] lines 83-85: `Search` does not clear `fetched_contract` and `selected_token`, allowing stale Add-Token state
On `Search` (lines 83-85) only `status` and `tried_token_id_lookup` are reset. `fetched_contract` and `selected_token` are not. `render_add_button()` (line 138) is rendered unconditionally and only checks that those two `Option`s are populated — not the current `status`. So if a successful first lookup is followed by a second lookup that ends in `ContractNotFound`/`TokenNotFound`, the screen sits in `AddTokenStatus::Error` while still showing a working `Add Token` button bound to the previous selection. Clicking it will save the stale token. Also clear `try_token_id_next` here for symmetry with the `add_another` reset path (lines 184-189). The enum `AddTokenStatus` no longer fully owns the screen-state invariant; each new lookup must explicitly clear the side-channel fields before dispatching the task.
| @@ -266,15 +288,7 @@ impl ScreenLike for AddTokenByIdScreen { | |||
| if msg.contains("DataContract successfully saved") { | |||
| self.status = AddTokenStatus::Complete; | |||
| } else if msg.contains("Contract not found") { | |||
| // Contract not found, try as token ID | |||
| if let Ok(_identifier) = | |||
| Identifier::from_string(&self.contract_or_token_id_input, Encoding::Base58) | |||
| { | |||
| // We'll initiate a token ID search | |||
| self.try_token_id_next = true; | |||
| } else { | |||
| self.status = AddTokenStatus::Error; | |||
| } | |||
| self.handle_contract_not_found(); | |||
| } else if msg.contains("Token not found") | |||
| || msg.contains("Error fetching contracts") | |||
| { | |||
| @@ -299,6 +313,17 @@ impl ScreenLike for AddTokenByIdScreen { | |||
| ) => { | |||
| self.handle_fetched_contract(contract, Some(token_position)); | |||
| } | |||
| BackendTaskSuccessResult::ContractNotFound => { | |||
| self.handle_contract_not_found(); | |||
| } | |||
| BackendTaskSuccessResult::TokenNotFound => { | |||
| MessageBanner::set_global( | |||
| self.app_context.egui_ctx(), | |||
| "No contract or token found for the given identifier", | |||
| MessageType::Error, | |||
| ); | |||
| self.status = AddTokenStatus::Error; | |||
| } | |||
| _ => {} | |||
| } | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: No unit/UI tests cover the new ContractNotFound/TokenNotFound state machine
This PR is a state-machine fix — the bug was a wildcard arm silently swallowing a typed variant. The new logic introduces persistent flags (try_token_id_next, tried_token_id_lookup) plus a shared handle_contract_not_found() helper, none of which are exercised by tests. A small unit test on AddTokenByIdScreen could cover: (1) display_task_result(ContractNotFound) with a valid base58 input transitions out of Searching via try_token_id_next=true and sets tried_token_id_lookup=true; (2) a second ContractNotFound after the flag is set transitions to Error instead of looping; (3) display_task_result(TokenNotFound) transitions to Error; (4) clicking Search again resets the flag. CLAUDE.md also requires a manual test scenario file under docs/ai-design/<date>-…/ referenced from the PR description; the PR description currently lists only cargo check/cargo clippy. Without focused coverage, the same class of regression (a new _ => {} arm swallowing a future variant) can re-emerge silently.
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/tokens/add_token_by_id_screen.rs`:
- [SUGGESTION] lines 196-329: No unit/UI tests cover the new `ContractNotFound`/`TokenNotFound` state machine
This PR is a state-machine fix — the bug was a wildcard arm silently swallowing a typed variant. The new logic introduces persistent flags (`try_token_id_next`, `tried_token_id_lookup`) plus a shared `handle_contract_not_found()` helper, none of which are exercised by tests. A small unit test on `AddTokenByIdScreen` could cover: (1) `display_task_result(ContractNotFound)` with a valid base58 input transitions out of `Searching` via `try_token_id_next=true` and sets `tried_token_id_lookup=true`; (2) a second `ContractNotFound` after the flag is set transitions to `Error` instead of looping; (3) `display_task_result(TokenNotFound)` transitions to `Error`; (4) clicking `Search` again resets the flag. CLAUDE.md also requires a manual test scenario file under `docs/ai-design/<date>-…/` referenced from the PR description; the PR description currently lists only `cargo check`/`cargo clippy`. Without focused coverage, the same class of regression (a new `_ => {}` arm swallowing a future variant) can re-emerge silently.
| BackendTaskSuccessResult::ContractNotFound => { | ||
| self.handle_contract_not_found(); | ||
| } | ||
| BackendTaskSuccessResult::TokenNotFound => { | ||
| MessageBanner::set_global( | ||
| self.app_context.egui_ctx(), | ||
| "No contract or token found for the given identifier", | ||
| MessageType::Error, | ||
| ); | ||
| self.status = AddTokenStatus::Error; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: ContractNotFound from the token-id path produces a misleading user message
BackendTaskSuccessResult::ContractNotFound is produced from two distinct paths in src/backend_task/tokens/mod.rs: (1) line 523 — initial FetchTokenByContractId returned Ok(None) (legitimate 'no contract'); and (2) line 549 — inside FetchTokenByTokenId, the TokenContractInfo lookup succeeded but the subsequent DataContract::fetch_by_identifier returned Ok(None) (a dangling token→contract pointer). When path (2) fires, tried_token_id_lookup is already true, so handle_contract_not_found() falls through to the generic 'No contract or token found for the given identifier' banner — but the token was resolvable, only its contract is missing. Either emit a distinct backend variant (e.g. ContractNotFoundForToken) or branch on tried_token_id_lookup here to surface a different, more accurate message for path (2).
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/tokens/add_token_by_id_screen.rs`:
- [SUGGESTION] lines 316-326: `ContractNotFound` from the token-id path produces a misleading user message
`BackendTaskSuccessResult::ContractNotFound` is produced from two distinct paths in `src/backend_task/tokens/mod.rs`: (1) line 523 — initial `FetchTokenByContractId` returned `Ok(None)` (legitimate 'no contract'); and (2) line 549 — inside `FetchTokenByTokenId`, the `TokenContractInfo` lookup *succeeded* but the subsequent `DataContract::fetch_by_identifier` returned `Ok(None)` (a dangling token→contract pointer). When path (2) fires, `tried_token_id_lookup` is already `true`, so `handle_contract_not_found()` falls through to the generic 'No contract or token found for the given identifier' banner — but the token *was* resolvable, only its contract is missing. Either emit a distinct backend variant (e.g. `ContractNotFoundForToken`) or branch on `tried_token_id_lookup` here to surface a different, more accurate message for path (2).
| @@ -266,15 +288,7 @@ impl ScreenLike for AddTokenByIdScreen { | |||
| if msg.contains("DataContract successfully saved") { | |||
| self.status = AddTokenStatus::Complete; | |||
| } else if msg.contains("Contract not found") { | |||
| // Contract not found, try as token ID | |||
| if let Ok(_identifier) = | |||
| Identifier::from_string(&self.contract_or_token_id_input, Encoding::Base58) | |||
| { | |||
| // We'll initiate a token ID search | |||
| self.try_token_id_next = true; | |||
| } else { | |||
| self.status = AddTokenStatus::Error; | |||
| } | |||
| self.handle_contract_not_found(); | |||
| } else if msg.contains("Token not found") | |||
| || msg.contains("Error fetching contracts") | |||
| { | |||
| @@ -299,6 +313,17 @@ impl ScreenLike for AddTokenByIdScreen { | |||
| ) => { | |||
| self.handle_fetched_contract(contract, Some(token_position)); | |||
| } | |||
| BackendTaskSuccessResult::ContractNotFound => { | |||
| self.handle_contract_not_found(); | |||
| } | |||
| BackendTaskSuccessResult::TokenNotFound => { | |||
| MessageBanner::set_global( | |||
| self.app_context.egui_ctx(), | |||
| "No contract or token found for the given identifier", | |||
| MessageType::Error, | |||
| ); | |||
| self.status = AddTokenStatus::Error; | |||
There was a problem hiding this comment.
💬 Nitpick: User-visible 'No contract or token found' string is duplicated
The literal "No contract or token found for the given identifier" appears at line 207 (inside handle_contract_not_found) and again at line 322 (inside the TokenNotFound arm). Both call sites also share the same set_global + status = AddTokenStatus::Error pattern. With two of three call sites within ~120 lines of each other, drift is realistic. Lift to a const &str or extract a set_not_found_error(&mut self) helper so wording stays in lock-step.
source: ['claude']
| MessageBanner::set_global( | ||
| self.app_context.egui_ctx(), | ||
| "No contract or token found for the given identifier", | ||
| MessageType::Error, | ||
| ); |
There was a problem hiding this comment.
💬 Nitpick: User-facing not-found message lacks a concrete self-service action
Per the project review skill: 'Every message must include a concrete self-service action … users must be able to self-resolve.' The new banner "No contract or token found for the given identifier" tells the user what happened but offers no next step. The action is non-obvious (verify the ID, switch network, etc.). A small wording tweak aligns with project convention. Note: this same string is duplicated at line 322 — fix both sites if you accept this.
💡 Suggested change
| MessageBanner::set_global( | |
| self.app_context.egui_ctx(), | |
| "No contract or token found for the given identifier", | |
| MessageType::Error, | |
| ); | |
| MessageBanner::set_global( | |
| self.app_context.egui_ctx(), | |
| "No contract or token found for that ID. Double-check the ID and the selected network.", | |
| MessageType::Error, | |
| ); | |
| self.status = AddTokenStatus::Error; |
source: ['claude']
thepastaclaw
force-pushed
the
fix/token-search-not-found
branch
from
326e1da to
aa81a8c
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR fixes the original _ => {} bug for ContractNotFound/TokenNotFound, resets stale state on each new search, disables the Search button while a lookup is in flight, removes dead string-match branches, and consolidates duplicate strings into constants with unit tests. Remaining issues are non-blocking: a mid-flight edit of the text field can still bind the fallback to a different identifier than was submitted, the new tests only cover pure helpers (not display_task_result where the original regression lived), the helper layer (NewLookupState/build_new_lookup_state/token_not_found_error_message) is over-engineered, user-facing messages lack a self-service action, and a redundant is_empty() guard remains.
Reviewed commit: aa81a8c
🟡 3 suggestion(s) | 💬 2 nitpick(s)
2 additional findings
🟡 suggestion: Identifier input stays editable during lookup — late `ContractNotFound` can fall back against a different ID
src/ui/tokens/add_token_by_id_screen.rs (lines 130-132)
Search is disabled while status == Searching(_) (lines 137-141), but the text edit at line 132 is always enabled. The fallback path reads self.contract_or_token_id_input directly in handle_contract_not_found (via resolve_contract_not_found, line 259-260) and then re-parses that live field when dispatching FetchTokenByTokenId (lines 428-438). A user can start lookup A, edit the field to B while A is in flight, and when A returns ContractNotFound the screen will silently launch the token-ID retry against B. The displayed result is then no longer tied to the request the user submitted. Either snapshot the identifier per request and use that snapshot in the result handlers, or disable text editing while Searching(_).
💡 Suggested change
ui.horizontal(|ui| {
ui.label("Contract or Token ID:");
ui.add_enabled_ui(!matches!(self.status, AddTokenStatus::Searching(_)), |ui| {
ui.text_edit_singleline(&mut self.contract_or_token_id_input);
});
});
💬 nitpick: Inner `is_empty()` check is dead — button is already disabled when input is empty
src/ui/tokens/add_token_by_id_screen.rs (lines 147-164)
The Search button at lines 136-141 is gated on !self.contract_or_token_id_input.is_empty(). By the time the click handler runs, the input is guaranteed non-empty, so the if !self.contract_or_token_id_input.is_empty() at line 147 can never be false. Drop the inner check and de-indent the body.
🤖 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/add_token_by_id_screen.rs`:
- [SUGGESTION] lines 130-132: Identifier input stays editable during lookup — late `ContractNotFound` can fall back against a different ID
`Search` is disabled while `status == Searching(_)` (lines 137-141), but the text edit at line 132 is always enabled. The fallback path reads `self.contract_or_token_id_input` directly in `handle_contract_not_found` (via `resolve_contract_not_found`, line 259-260) and then re-parses that live field when dispatching `FetchTokenByTokenId` (lines 428-438). A user can start lookup A, edit the field to B while A is in flight, and when A returns `ContractNotFound` the screen will silently launch the token-ID retry against B. The displayed result is then no longer tied to the request the user submitted. Either snapshot the identifier per request and use that snapshot in the result handlers, or disable text editing while `Searching(_)`.
- [SUGGESTION] lines 473-521: New tests don't exercise `display_task_result` — the actual site of the original regression
The original bug was that `display_task_result` had `_ => {}` that swallowed typed `ContractNotFound`/`TokenNotFound` variants, leaving the UI stuck in `Searching`. The four new tests only call pure helpers (`resolve_contract_not_found`, `build_new_lookup_state`, `token_not_found_error_message`). They would still pass if a future refactor reintroduced `_ => {}` in `display_task_result` (line 384), forgot to set `self.status = AddTokenStatus::Error` for `TokenNotFound`, or stopped toggling `try_token_id_next` in `handle_contract_not_found`. Add a focused test that drives `AddTokenByIdScreen::display_task_result()` with `ContractNotFound` and `TokenNotFound` and asserts the resulting `self.status` and the `try_token_id_next`/`tried_token_id_lookup` flags. This also covers the currently-untested invalid-base58 branch of `resolve_contract_not_found`.
- [SUGGESTION] lines 41-125: `NewLookupState` / `build_new_lookup_state` is over-engineered for a constant reset
`build_new_lookup_state` is the only constructor of `NewLookupState` and unconditionally sets `clear_fetched_contract = true`, `clear_selected_token = true`, `try_token_id_next = false`, `tried_token_id_lookup = false`. `begin_lookup` is the only caller and immediately destructures the struct back into individual assignments, conditionally clearing on flags that are always `true`. The struct, its `Debug`/`PartialEq` derives, and the standalone builder buy nothing — the unit test `new_search_resets_lookup_flags_and_stale_state` only asserts hard-coded literals, so it cannot catch any real regression. Per CLAUDE.md ("Don't create helpers, utilities, or abstractions for one-time operations"), collapse this into a direct field-clearing block inside `begin_lookup`.
| fn token_not_found_error_message() -> &'static str { | ||
| NO_CONTRACT_OR_TOKEN_FOUND_MESSAGE | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: token_not_found_error_message() is a dead wrapper around a constant
token_not_found_error_message() returns NO_CONTRACT_OR_TOKEN_FOUND_MESSAGE verbatim with no logic, and its only test (token_not_found_maps_to_error_message) asserts X == X. The single call site at line 379 can use the constant directly. Inline the constant and drop both the function and its test, or — if the intent is to allow the message to diverge from the contract-not-found wording later — introduce a second constant at that point.
source: ['claude']
thepastaclaw
force-pushed
the
fix/token-search-not-found
branch
from
aa81a8c to
a7cb9dd
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Most prior follow-ups are resolved on this head: text input is now disabled during lookup, focused display_task_result tests cover both ContractNotFound and TokenNotFound paths, the over-engineered NewLookupState builder was collapsed into begin_lookup, the redundant is_empty() guard was removed, and not-found strings were consolidated into constants. Two minor items remain: token_not_found_error_message() is a dead wrapper around a constant with a tautological test, and both not-found user messages still lack a concrete self-service action as required by the project review skill.
Reviewed commit: a7cb9dd
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/tokens/add_token_by_id_screen.rs`:
- [SUGGESTION] lines 47-50: Not-found banner strings omit a concrete self-service action
The project review skill requires every user-facing message to include a concrete self-service action so users can self-resolve. Both new constants describe what happened but offer no next step: "No contract or token found for the given identifier" and "Token was found, but its data contract could not be fetched". A new user has no hint to verify the ID, check the selected network, or retry — the common root causes for both errors. Because both strings are now centralized as consts, the change is mechanical and propagates to both call sites (line 246 via `handle_contract_not_found`, and line 356 for TokenNotFound).
| const NO_CONTRACT_OR_TOKEN_FOUND_MESSAGE: &str = | ||
| "No contract or token found for the given identifier"; | ||
| const TOKEN_CONTRACT_MISSING_MESSAGE: &str = | ||
| "Token was found, but its data contract could not be fetched"; |
There was a problem hiding this comment.
🟡 Suggestion: Not-found banner strings omit a concrete self-service action
The project review skill requires every user-facing message to include a concrete self-service action so users can self-resolve. Both new constants describe what happened but offer no next step: "No contract or token found for the given identifier" and "Token was found, but its data contract could not be fetched". A new user has no hint to verify the ID, check the selected network, or retry — the common root causes for both errors. Because both strings are now centralized as consts, the change is mechanical and propagates to both call sites (line 246 via handle_contract_not_found, and line 356 for TokenNotFound).
💡 Suggested change
| const NO_CONTRACT_OR_TOKEN_FOUND_MESSAGE: &str = | |
| "No contract or token found for the given identifier"; | |
| const TOKEN_CONTRACT_MISSING_MESSAGE: &str = | |
| "Token was found, but its data contract could not be fetched"; | |
| const NO_CONTRACT_OR_TOKEN_FOUND_MESSAGE: &str = | |
| "No contract or token found for that ID. Double-check the ID and the selected network, then try again."; | |
| const TOKEN_CONTRACT_MISSING_MESSAGE: &str = | |
| "Token was found, but its data contract could not be fetched. Try again, or verify you are on the correct network."; |
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/tokens/add_token_by_id_screen.rs`:
- [SUGGESTION] lines 47-50: Not-found banner strings omit a concrete self-service action
The project review skill requires every user-facing message to include a concrete self-service action so users can self-resolve. Both new constants describe what happened but offer no next step: "No contract or token found for the given identifier" and "Token was found, but its data contract could not be fetched". A new user has no hint to verify the ID, check the selected network, or retry — the common root causes for both errors. Because both strings are now centralized as consts, the change is mechanical and propagates to both call sites (line 246 via `handle_contract_not_found`, and line 356 for TokenNotFound).
The display_task_result method was missing handlers for
BackendTaskSuccessResult::ContractNotFound and
BackendTaskSuccessResult::TokenNotFound variants. These fell through
to the wildcard _ => {} match, causing the screen to stay stuck in
the Searching state indefinitely.
Now:
- ContractNotFound triggers fallback to token ID lookup (existing
try_token_id_next logic), or shows an error banner if the input
is not a valid identifier
- TokenNotFound shows an error banner and transitions to Error state
Fixes dashpay#691
thepastaclaw
force-pushed
the
fix/token-search-not-found
branch
from
a7cb9dd to
866fefa
Compare
1 participant
Issue
Fixes #691
Description
The Add/Search Token screen gets stuck searching indefinitely when a contract or token is not found on the network.
Root cause:
AddTokenByIdScreen::display_task_resulthas a_ => {}wildcard that silently swallowsBackendTaskSuccessResult::ContractNotFoundandBackendTaskSuccessResult::TokenNotFound. When either variant is returned, the UI stays inSearching(timestamp)state forever with no user feedback.Fix:
ContractNotFound— falls back to interpreting the input as a Token ID (via the existingtry_token_id_nextmechanism), matching the existing fallback pattern elsewhere in the screenTokenNotFound— shows an error viaMessageBannerand transitions toErrorstateMessageBannersystem consistent with other error handling in the same fileValidation
cargo check✅cargo clippy -- -D warnings✅ContractNotFoundandTokenNotFoundare returned by the backend task handlers insrc/backend_task/tokens/mod.rs(lines 523, 549, 553) and were previously swallowed by the wildcardSummary by CodeRabbit