fix: show parsed platform-address transitions#853
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded serialization fallback handling to the state transition visualizer. When JSON serialization fails (e.g., for non-string map keys), a helper function returns a JSON string containing the error and debug representation. The screen now stores parsed transitions directly for broadcast instead of re-parsing from JSON. ChangesState Transition Serialization Robustness
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 |
|
✅ Review complete (commit 9f9952b) |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/tools/transition_visualizer_screen.rs`:
- Around line 31-35: The tracing::warn! call currently includes the full
transition payload via state_transition = %debug_output which can leak sensitive
or very large data; update the warn to only log the error/context (e.g., keep
error = %error and a short message) and move the full payload logging to a
separate tracing::debug! or tracing::trace! line that logs state_transition =
%debug_output so detailed payloads are only emitted at lower log levels; locate
the tracing::warn! usage in transition_visualizer_screen.rs and adjust
accordingly (remove state_transition from the warn and add a debug/trace log
using debug_output).
- Around line 280-283: Replace the inline parse_error usage for the broadcast
case with the app-wide MessageBanner so the message follows the standard UI
pattern: instead of setting self.parse_error = Some(("Parsed transition is
unavailable for broadcast".to_string(), Instant::now())), create/assign a
MessageBanner (using the existing MessageBanner type/constructor and the
screen's banner/state field) with the same text and appropriate severity, and
remove or keep parse_error only for internal/parse-only errors; update the
branch in the method that sets this message (the code that currently assigns
self.parse_error) to invoke the MessageBanner API (e.g., set the screen's
message banner field or call its display function) so the message is rendered
via MessageBanner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c3235b6-81b8-4c32-8d02-f665b0024f29
📒 Files selected for processing (1)
src/ui/tools/transition_visualizer_screen.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The core fix — retaining the typed StateTransition so AddressFundsTransfer payloads can still be broadcast — is sound. Validated findings cluster around three areas: warn-level log noise on every keystroke, an unreachable else-branch from coupled Option<...> state, and a fragile test that asserts on serde_json's internal error wording. The debug fallback is embedded as a JSON string, which limits the readability gain advertised in the PR title.
Reviewed commit: 9f9952b
Fresh dispatcher rerun for this queue item. Same-SHA automated reviews already existed, so I am posting this as a top-level review to avoid duplicating inline threads while still recording the fresh verification.
🟡 4 suggestion(s) | 💬 2 nitpick(s)
🟡 suggestion: warn-level log fires on every keystroke during typing
src/ui/tools/transition_visualizer_screen.rs (lines 173-177)
parse_input() runs whenever the input field changes (line 204–207), so this tracing::warn! will fire for every intermediate state where the user has typed bytes that decode but do not yet form a complete StateTransition. The result is warning-level log spam during normal interactive use, which makes real warnings harder to spot and adds avoidable work on the UI thread. Either downgrade to debug/trace, or only log when the user explicitly triggers parsing (e.g. on blur or via a button). The deserialize-failure case is the expected state during typing, not an exceptional condition.
💡 Suggested change
Err(e) => {
tracing::debug!(
error = %e,
"failed to deserialize state transition input"
);
self.parse_error =
Some((format!("Failed to parse: {}", e), Instant::now()));
}
🟡 suggestion: Coupled Option fields create an unreachable error branch
src/ui/tools/transition_visualizer_screen.rs (lines 282-288)
parsed_json and parsed_state_transition are always set together (lines 157–158) and cleared together (lines 121–122), and the broadcast button only renders when parsed_json.is_some() (line 244). The else arm here can therefore never execute in practice, and its banner text ("Parsed transition is unavailable for broadcast") is jargon with no self-service action — it would not pass the project's user-facing message rules if it ever did fire. Collapse the two correlated fields into a single Option<(StateTransition, String)> (or a small ParsedTransition { st, json } struct). That makes the invariant type-enforced, removes this dead branch, and lets the broadcast path drop the is_some() check entirely.
🟡 suggestion: Debug fallback is embedded inside a JSON string and renders escaped
src/ui/tools/transition_visualizer_screen.rs (lines 40-44)
serde_json::to_string_pretty escapes newlines in string values, so the multi-line {:#?} output ends up as a single line containing literal \n sequences inside the "debug" field. For exactly the transitions that hit this fallback (the case the PR is meant to fix), the user sees an unreadable escaped blob instead of structured debug output — only the warning and serialization_error keys are useful. Consider either returning a plain-text rendering for the fallback (header + raw {:#?} body) so newlines are preserved in the TextEdit::multiline, or rendering the debug section in a separate non-JSON pane.
🟡 suggestion: Test does not cover the screen-level broadcast invariant introduced by this PR
src/ui/tools/transition_visualizer_screen.rs (lines 566-581)
The production change isn't just the JSON fallback helper — it's the combination of storing parsed_state_transition during parsing (line 158) and using that retained value in the broadcast button (line 267). The new test calls state_transition_to_pretty_json() directly, so it would still pass if a future refactor removed self.parsed_state_transition = Some(state_transition) or reverted the broadcast path to re-parsing self.parsed_json. A focused screen-level test (or kittest) should assert that feeding the issue #573 payload through the screen produces an AppAction::BackendTask(BackendTask::BroadcastStateTransition(_)) even when the rendered pane is the debug fallback.
💬 nitpick: Assertion couples to serde_json's internal error wording
src/ui/tools/transition_visualizer_screen.rs (lines 578-579)
assert!(json.contains("key must be a string")) depends on the exact text of serde_json::Error::Display for non-string map keys, which is upstream's free-form message and can change across patch releases. The project guidance against depending on free-form error strings applies here in spirit. Assert on what we control instead: parse the JSON structurally and check that the serialization_error and debug keys are present and non-empty, and that debug contains a recognizable variant string from StateTransition's Debug output. The other two asserts already demonstrate the fallback path was taken with the expected variant.
💬 nitpick: Helper returns Result and the Err arm is effectively unreachable
src/ui/tools/transition_visualizer_screen.rs (lines 25-53)
The fallback object built at line 40–44 contains only String values, so serde_json::to_string_pretty will not fail in practice. The outer Err arm at the call site (line 168–170) is therefore dead, and the message it would surface ("Failed to serialize state transition to JSON (...) and failed to build debug fallback (...)") leaks raw serde error text into the UI in violation of the project's user-facing message rules. Either make the helper infallible (return String and only log internally on the unlikely fallback failure), or — if a fallible signature is preferred — introduce a small thiserror-derived enum and surface a user-appropriate message at the call site with technical details attached via BannerHandle::with_details(e).
🤖 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/tools/transition_visualizer_screen.rs`:
- [SUGGESTION] lines 173-177: warn-level log fires on every keystroke during typing
`parse_input()` runs whenever the input field changes (line 204–207), so this `tracing::warn!` will fire for every intermediate state where the user has typed bytes that decode but do not yet form a complete StateTransition. The result is warning-level log spam during normal interactive use, which makes real warnings harder to spot and adds avoidable work on the UI thread. Either downgrade to `debug`/`trace`, or only log when the user explicitly triggers parsing (e.g. on blur or via a button). The deserialize-failure case is the expected state during typing, not an exceptional condition.
- [SUGGESTION] lines 282-288: Coupled Option fields create an unreachable error branch
`parsed_json` and `parsed_state_transition` are always set together (lines 157–158) and cleared together (lines 121–122), and the broadcast button only renders when `parsed_json.is_some()` (line 244). The `else` arm here can therefore never execute in practice, and its banner text (`"Parsed transition is unavailable for broadcast"`) is jargon with no self-service action — it would not pass the project's user-facing message rules if it ever did fire. Collapse the two correlated fields into a single `Option<(StateTransition, String)>` (or a small `ParsedTransition { st, json }` struct). That makes the invariant type-enforced, removes this dead branch, and lets the broadcast path drop the `is_some()` check entirely.
- [SUGGESTION] lines 40-44: Debug fallback is embedded inside a JSON string and renders escaped
`serde_json::to_string_pretty` escapes newlines in string values, so the multi-line `{:#?}` output ends up as a single line containing literal `\n` sequences inside the `"debug"` field. For exactly the transitions that hit this fallback (the case the PR is meant to fix), the user sees an unreadable escaped blob instead of structured debug output — only the `warning` and `serialization_error` keys are useful. Consider either returning a plain-text rendering for the fallback (header + raw `{:#?}` body) so newlines are preserved in the `TextEdit::multiline`, or rendering the debug section in a separate non-JSON pane.
- [SUGGESTION] lines 566-581: Test does not cover the screen-level broadcast invariant introduced by this PR
The production change isn't just the JSON fallback helper — it's the combination of storing `parsed_state_transition` during parsing (line 158) and using that retained value in the broadcast button (line 267). The new test calls `state_transition_to_pretty_json()` directly, so it would still pass if a future refactor removed `self.parsed_state_transition = Some(state_transition)` or reverted the broadcast path to re-parsing `self.parsed_json`. A focused screen-level test (or kittest) should assert that feeding the issue #573 payload through the screen produces an `AppAction::BackendTask(BackendTask::BroadcastStateTransition(_))` even when the rendered pane is the debug fallback.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Narrowly scoped fix that correctly decouples broadcast from JSON serialization for transitions whose internal map keys aren't string-renderable. No correctness bugs found, but the change introduces a coupled-Option invariant with a dead error branch (and a non-compliant user-facing message), leaves contract-ID discovery dependent on the lossy rendered JSON, and adds a regression test that exercises the formatter rather than the retained-StateTransition invariant the fix actually relies on.
Reviewed commit: 9f9952b
Fresh dispatcher gate run for the claimed queue item. review_poster.py detected an existing same-SHA review, so this records the fresh verification as a top-level review without duplicating inline threads.
🟡 3 suggestion(s) | 💬 2 nitpick(s)
🟡 suggestion: Combine `parsed_json` and `parsed_state_transition`; the `else` branch is unreachable and its message violates the user-message rules
src/ui/tools/transition_visualizer_screen.rs (lines 264-289)
parsed_state_transition is only ever assigned alongside parsed_json (lines 157-158) and cleared with it (121-122). The Broadcast button renders strictly inside if let Some(ref json) = self.parsed_json (244), so at click time self.parsed_state_transition.clone() is always Some(_) and the else arm at 282-288 is dead code. The two fields encode a single invariant the type system isn't enforcing — model them as one Option<(String, StateTransition)> (or a small named struct) so the invariant is structural and parse_input can't ever desync them. As a side effect, the unreachable error banner goes away — its wording ("Parsed transition is unavailable for broadcast") also fails the CLAUDE.md user-message rule of "what happened + what to do" (no concrete self-service action), so removing it is a net win.
🟡 suggestion: Contract-ID detection still keys off the rendered JSON, silently losing the "Fetch Contract" affordance on the fallback path
src/ui/tools/transition_visualizer_screen.rs (lines 153-166)
state_transition_to_pretty_json returns Ok(_) for both the real serialization and the debug-fallback wrapper, so parsed_json is always set when deserialization succeeds. But extract_contract_ids then walks that string. On the fallback path the string is {warning, serialization_error, debug: "<format!('{:#?}', ...)>"} — a single opaque blob — so any transition variant that both hits the fallback and carries singleContract bounds will silently lose its contract reference, and the user will never see the "Fetch contract" link even though deserialization fully succeeded. There is no real harm today (the platform-address transition this PR targets has no contract bound), but the data is now in two representations of different fidelity and behavior is keyed off the lossy one. Drive contract-ID discovery from the typed StateTransition (which is now retained on self) before rendering, instead of round-tripping through the JSON string.
🟡 suggestion: Test does not exercise the broadcast path or the retained-`StateTransition` invariant
src/ui/tools/transition_visualizer_screen.rs (lines 566-581)
The actual #573 fix is split across two behaviors: parse_input() now retains the typed transition in parsed_state_transition (157-158), and Broadcast sends that stored value rather than reparsing parsed_json (267-281). The new test only calls state_transition_to_pretty_json(), so a future refactor that drops self.parsed_state_transition = Some(state_transition) — or reverts Broadcast to serde_json::from_str(json) — would still pass. Add a screen-level test that drives parse_input() with the issue #573 payload and asserts both parsed_json.is_some() and parsed_state_transition.is_some() (the test module already has access to private fields).
💬 nitpick: Test asserts on serde/dpp-internal wording
src/ui/tools/transition_visualizer_screen.rs (lines 578-580)
json.contains("key must be a string") couples to serde_json's internal error string for non-string map keys, and json.contains("AddressFundsTransfer") couples to a dpp variant name. Either can shift without changing behavior this PR cares about. Prefer structural assertions: parse json back as Value and check that warning, serialization_error, and debug keys are present and non-empty (the warning literal is the only string this PR controls).
💬 nitpick: `Result` is stringly-typed and the `Err` arm is effectively unreachable
src/ui/tools/transition_visualizer_screen.rs (lines 25-53)
CLAUDE.md error-handling guidance discourages stringified errors. In practice the outer serde_json::to_string_pretty over the json!{...} literal here has no failure mode (no NaN/Infinity floats, no non-string map keys), so the map_err arm is unreachable. Either drop the Result and return String infallibly (with .expect() on the fallback to assert the invariant), or surface a typed error (Result<String, serde_json::Error> or a small enum) so callers don't have to format and re-parse. Low impact since the only callsite consumes the message as a parse_error display, but this is a fresh helper and worth getting right.
🤖 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/tools/transition_visualizer_screen.rs`:
- [SUGGESTION] lines 264-289: Combine `parsed_json` and `parsed_state_transition`; the `else` branch is unreachable and its message violates the user-message rules
`parsed_state_transition` is only ever assigned alongside `parsed_json` (lines 157-158) and cleared with it (121-122). The Broadcast button renders strictly inside `if let Some(ref json) = self.parsed_json` (244), so at click time `self.parsed_state_transition.clone()` is always `Some(_)` and the `else` arm at 282-288 is dead code. The two fields encode a single invariant the type system isn't enforcing — model them as one `Option<(String, StateTransition)>` (or a small named struct) so the invariant is structural and `parse_input` can't ever desync them. As a side effect, the unreachable error banner goes away — its wording ("Parsed transition is unavailable for broadcast") also fails the CLAUDE.md user-message rule of "what happened + what to do" (no concrete self-service action), so removing it is a net win.
- [SUGGESTION] lines 153-166: Contract-ID detection still keys off the rendered JSON, silently losing the "Fetch Contract" affordance on the fallback path
`state_transition_to_pretty_json` returns `Ok(_)` for both the real serialization and the debug-fallback wrapper, so `parsed_json` is always set when deserialization succeeds. But `extract_contract_ids` then walks that string. On the fallback path the string is `{warning, serialization_error, debug: "<format!('{:#?}', ...)>"}` — a single opaque blob — so any transition variant that both hits the fallback and carries `singleContract` bounds will silently lose its contract reference, and the user will never see the "Fetch contract" link even though deserialization fully succeeded. There is no real harm today (the platform-address transition this PR targets has no contract bound), but the data is now in two representations of different fidelity and behavior is keyed off the lossy one. Drive contract-ID discovery from the typed `StateTransition` (which is now retained on `self`) before rendering, instead of round-tripping through the JSON string.
- [SUGGESTION] lines 566-581: Test does not exercise the broadcast path or the retained-`StateTransition` invariant
The actual #573 fix is split across two behaviors: `parse_input()` now retains the typed transition in `parsed_state_transition` (157-158), and Broadcast sends that stored value rather than reparsing `parsed_json` (267-281). The new test only calls `state_transition_to_pretty_json()`, so a future refactor that drops `self.parsed_state_transition = Some(state_transition)` — or reverts Broadcast to `serde_json::from_str(json)` — would still pass. Add a screen-level test that drives `parse_input()` with the issue #573 payload and asserts both `parsed_json.is_some()` and `parsed_state_transition.is_some()` (the test module already has access to private fields).
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
PR Description
Summary
serialized as JSON
StateTransitionseparately so fallback-renderedtransitions can still be broadcast
Fixes #573.
Validation
cargo fmt --allplatform_address_transition_with_non_string_map_keys_uses_debug_fallbackcargo clippy --lib -- -D warningsSummary by CodeRabbit
Release Notes