Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThe PR adds a Ratatui-based Terminal UI as a new workspace crate Assessment against linked issues
Suggested reviewers
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-processor/src/executor/processing.rs (2)
141-144: Replace.expect()with proper error handling.Per coding guidelines,
.expect()in production code undermagicblock-*paths should be treated as a major issue. While the single transaction result may be guaranteed by the current implementation, this could mask issues if the upstream contract changes.Proposed fix
- let mut result = output - .processing_results - .pop() - .expect("single transaction result is guaranteed"); + let mut result = match output.processing_results.pop() { + Some(r) => r, + None => { + error!("Unexpected empty processing results for single transaction"); + return (Err(TransactionError::SanitizeFailure), output.balances); + } + };Alternatively, if this truly represents an invariant that should never be violated, consider using
debug_assert!to verify in tests while providing a safe fallback in release builds.
278-283: Replace.unwrap()with proper error handling.Per coding guidelines,
.unwrap()in production code undermagicblock-*paths should be treated as a major issue. A poisoned lock could cause a panic here.Proposed fix using poison recovery
if succeeded && !executed.programs_modified_by_tx.is_empty() { - self.processor + match self.processor .program_cache .write() - .unwrap() - .merge(&executed.programs_modified_by_tx); + { + Ok(mut cache) => cache.merge(&executed.programs_modified_by_tx), + Err(poisoned) => { + // Recover from poisoned lock - the data may still be valid + poisoned.into_inner().merge(&executed.programs_modified_by_tx); + } + } }magicblock-processor/tests/simulation.rs (1)
56-69: Only timeout should indicate "no notification"—broadcast errors mask regressions.
Ok(Err(_))includesRecvError::Closed(sender dropped) andRecvError::Lagged(buffer overflow), neither of which means no notification was sent. Accepting these conditions as success can hide channel or buffer management issues. Only a timeout (Err(_)) correctly verifies no message arrived within the time window.Proposed fix
- assert!( - matches!( - timeout(TIMEOUT, status_rx.recv()).await, - Err(_) | Ok(Err(_)) - ), - "Simulation triggered status update" - ); + assert!( + matches!(timeout(TIMEOUT, status_rx.recv()).await, Err(_)), + "Simulation triggered status update or channel error" + );
🤖 Fix all issues with AI agents
In `@magicblock-tui/src/app.rs`:
- Around line 254-259: The #[allow(dead_code)] on RpcResponse::error is
incorrect because the field is actually used; remove the attribute from the
struct declaration so the error field is treated as used and the compiler
warning suppression is eliminated. Locate the RpcResponse<T> struct (symbol:
RpcResponse) and delete the #[allow(dead_code)] annotation above the error:
Option<RpcError> field; ensure the code that checks rpc_response.error (e.g., if
let Some(err) = rpc_response.error) remains unchanged.
- Around line 363-379: Move the duplicated url_encode function into a single
shared location and update callers to use that symbol: create a new utils module
(e.g., utils.rs or a mod utils) and place the url_encode implementation there
with appropriate visibility (pub(crate) or pub) and a clear function name
url_encode; remove the duplicate definitions from magicblock-tui/src/app.rs and
magicblock-tui/src/state.rs and replace their local calls with the shared
utils::url_encode (or re-export it from the crate root if you prefer shorter
paths) so there is only one canonical implementation referenced by both modules.
- Around line 381-398: The function open_url_in_browser only implements macos,
linux, and windows and will silently succeed on unsupported targets; add a
fallback cfg block (#[cfg(not(any(target_os = "macos", target_os = "linux",
target_os = "windows")))]) inside the function that returns a descriptive
std::io::Error (e.g., via std::io::Error::new with ErrorKind::Other) indicating
that opening URLs is unsupported on this platform so callers get a clear failure
instead of a silent no-op.
In `@magicblock-tui/src/logger.rs`:
- Around line 59-75: In record_debug, replace the manual start/end-quote checks
and slicing used to strip surrounding quotes from self.message with a call to
trim_matches('"') so the quote-removal is more idiomatic and robust; update both
places in the function (the branch where field.name() == "message" and the
fallback branch where self.message is set from the first field) to assign
self.message = self.message.trim_matches('"').to_string() after formatting.
In `@magicblock-tui/src/ui.rs`:
- Around line 476-488: The truncation uses a byte-slice (&log[..popup_width as
usize - 7]) which can panic on multi-byte UTF-8; replace that unsafe byte-slice
with a safe character/grapheme-aware truncation: compute the max character count
(e.g., let n = popup_width as usize - 7) and build the truncated string via
log.chars().take(n).collect::<String>() or, for correct user-visible width, use
the unicode-segmentation or unicode-width crate to truncate by grapheme
clusters/column width, then append "..." as before; update the construction of
truncated in the loop that iterates detail.logs and uses
popup_width/popup_height, leaving lines.push and styling unchanged.
♻️ Duplicate comments (1)
magicblock-tui/src/state.rs (1)
199-215: Duplicate function - see comment inapp.rs.This
url_encodefunction is duplicated fromapp.rs. Consolidate into a shared location.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
magicblock-processor/src/executor/processing.rs (2)
278-281: Use proper error handling instead of.unwrap()on lock acquisition.The
.unwrap()onprogram_cache.write()will panic if theRwLockis poisoned (i.e., a thread panicked while holding the lock). This could crash the validator.Suggested fix
Consider handling the poisoned lock case, or at minimum document why panic is acceptable here:
if succeeded && !executed.programs_modified_by_tx.is_empty() { self.processor .program_cache .write() - .unwrap() + .unwrap_or_else(|poisoned| poisoned.into_inner()) .merge(&executed.programs_modified_by_tx); }Alternatively, if panicking is intentional (treating a poisoned lock as unrecoverable), add a comment explaining this invariant.
As per coding guidelines,
.unwrap()in production Rust code withinmagicblock-*paths should not be used without proper error handling or explicit justification.
139-142: Consider documenting the invariant for the.expect()call.While the invariant "single transaction result is guaranteed" is correct (the input is
&[SanitizedTransaction; 1]), per coding guidelines,.expect()in production code should have explicit justification or be converted to proper error handling.The current usage is acceptable given the strong invariant, but consider either:
- Adding a brief comment above explaining why this is truly infallible, or
- Using a match with an
unreachable!()arm to make the invariant even more explicitThis is a minor suggestion since the invariant is clearly documented in the expect message.
magicblock-api/src/magic_validator.rs (4)
297-308: Replace.expect()with proper error handling.Per coding guidelines,
.expect()in production code is a major issue. This runtime construction could fail and should propagate an error rather than panic.Proposed fix
Consider propagating the error through the thread spawn mechanism or handling it more gracefully:
let rpc_handle = thread::spawn(move || { let workers = (num_cpus::get() / 2 - 1).max(1); let runtime = Builder::new_multi_thread() .worker_threads(workers) .enable_all() .thread_name("rpc-worker") .build() - .expect("failed to bulid async runtime for rpc service"); + .map_err(|e| { + error!("Failed to build async runtime for RPC service: {e}"); + e + }) + .ok(); + + if let Some(runtime) = runtime { + runtime.block_on(rpc.run()); + drop(runtime); + } - runtime.block_on(rpc.run()); - - drop(runtime); info!("RPC runtime shutdown"); });Alternatively, if this is truly a fatal condition where the validator cannot function without the RPC runtime, document the invariant clearly.
310-313: Replace.expect()with proper error handling.Per coding guidelines,
.expect()in production code is a major issue.Proposed fix
let task_scheduler_db_path = - SchedulerDatabase::path(ledger.ledger_path().parent().expect( - "ledger_path didn't have a parent, should never happen", - )); + SchedulerDatabase::path( + ledger + .ledger_path() + .parent() + .ok_or_else(|| ApiError::InvalidLedgerPath("ledger path has no parent".into()))?, + );
319-323: Replace.expect()with proper error handling.Per coding guidelines,
.expect()in production code is a major issue.Proposed fix
dispatch .tasks_service .take() - .expect("tasks_service should be initialized"), + .ok_or(ApiError::MissingTasksService)?,You'll need to add a corresponding variant to
ApiError.
671-674: Replace.expect()with proper error handling.Per coding guidelines,
.expect()in production code is a major issue.Proposed fix
let task_scheduler = self .task_scheduler .take() - .expect("task_scheduler should be initialized"); + .ok_or(ApiError::TaskSchedulerNotInitialized)?;This would require changing the return type of
start()to accommodate the error, or logging and returning early if this is a non-recoverable state.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
magicblock-processor/src/executor/processing.rs (2)
139-142: Use of.expect()in production code.Per coding guidelines,
.expect()should be avoided in production Rust code withinmagicblock-*crates. While the invariant "single transaction result is guaranteed" may hold, consider using proper error handling or explicitly documenting why this panic is acceptable.Proposed fix
- let mut result = output - .processing_results - .pop() - .expect("single transaction result is guaranteed"); + let mut result = output + .processing_results + .pop() + .ok_or(TransactionError::SanitizeFailure)?;Alternatively, if the invariant is architecturally guaranteed, add a comment explaining why the panic is safe and acceptable.
286-291: Use of.unwrap()in production code.Per coding guidelines,
.unwrap()should be avoided inmagicblock-*production code. Theprogram_cache.write().unwrap()could panic if the RwLock is poisoned.Proposed fix options
Option 1: Add explicit handling or recovery:
- self.processor - .program_cache - .write() - .unwrap() - .merge(&executed.programs_modified_by_tx); + if let Ok(mut cache) = self.processor.program_cache.write() { + cache.merge(&executed.programs_modified_by_tx); + } else { + error!("Program cache lock poisoned, skipping merge"); + }Option 2: If panicking on poison is intentional, document the invariant:
+ // SAFETY: A poisoned program cache indicates a critical bug; + // panicking is the correct behavior to prevent inconsistent state. self.processor .program_cache .write() .unwrap()magicblock-processor/src/executor/mod.rs (2)
131-139: Use of.expect()in production code.Per coding guidelines,
.expect()should be avoided inmagicblock-*production code. While runtime creation failure is typically fatal, consider propagating the error or documenting why panicking is acceptable here.Proposed fix
If panicking is intentional for this unrecoverable error, add documentation:
std::thread::spawn(move || { + // INVARIANT: Runtime creation failure is unrecoverable; the executor + // cannot function without it. Panicking is the correct behavior. let runtime = Builder::new_current_thread() .thread_name(format!("txn-executor-{}", self.id)) .build() .expect("Failed to build executor runtime");Alternatively, return a
Resultfromspawnto let the caller handle the failure.
186-188: Use of.unwrap()in production code.Per coding guidelines,
.unwrap()should be avoided inmagicblock-*production code. Thewrite().unwrap()on the sysvar cache could panic if the lock is poisoned.Proposed fix
- let mut cache = self.processor.writable_sysvar_cache().write().unwrap(); + let Ok(mut cache) = self.processor.writable_sysvar_cache().write() else { + warn!("Sysvar cache lock poisoned, skipping sysvar update"); + return; + };Or document the invariant if panicking is intentional.
magicblock-api/src/magic_validator.rs (1)
244-259:tui_transaction_status_txis used before declaration whentuiis enabled.
The channel is created afterTransactionSchedulerState(line 297), but referenced within it (line 258), causing a compilation failure when the feature flag is enabled. Move TUI channel creation above the scheduler-state construction.Suggested fix
validator::init_validator_authority(identity_keypair); let base_fee = config.validator.basefee; + + #[cfg(feature = "tui")] + let (tui_block_tx, tui_block_rx) = tui_block_channel(); + #[cfg(feature = "tui")] + let (tui_transaction_status_tx, tui_transaction_status_rx) = + tui_transaction_status_channel(); + let txn_scheduler_state = TransactionSchedulerState { accountsdb: accountsdb.clone(), ledger: ledger.clone(), @@ shutdown: token.clone(), #[cfg(feature = "tui")] tui_transaction_status_tx: Some(tui_transaction_status_tx.clone()), }; @@ - #[cfg(feature = "tui")] - let (tui_block_tx, tui_block_rx) = tui_block_channel(); - #[cfg(feature = "tui")] - let (tui_transaction_status_tx, tui_transaction_status_rx) = - tui_transaction_status_channel();
🤖 Fix all issues with AI agents
In `@magicblock-api/src/slot.rs`:
- Around line 72-93: The advance_slot_and_update_ledger_with_tui function
duplicates slot-advance logic found in the non-TUI path; extract the shared code
that computes latest_block, current_slot, blockhash, next_slot and performs
accountsdb.set_slot(next_slot) into a single helper (e.g., advance_slot_helper
or compute_and_set_next_slot) that returns the values needed (next_slot and
blockhash or a small struct), then call that helper from
advance_slot_and_update_ledger_with_tui and the non-TUI function so both use the
same logic and avoid drift; locate the logic around latest_block, current_slot,
the Hasher usage, next_slot and accountsdb.set_slot to move into the helper.
- Around line 95-98: The code calls
SystemTime::now().duration_since(UNIX_EPOCH).unwrap() to produce a timestamp
which will panic on SystemTimeError; replace the unwrap with proper error
handling by matching the Result (or using map/map_err) and either
propagate/return an error from the surrounding function or log the failure and
use a safe fallback timestamp; update both occurrences (the timestamp binding at
the shown diff and the instance inside advance_slot_and_update_ledger()) and
reference the timestamp variable and the advance_slot_and_update_ledger()
function when making the change so you handle the error consistently across both
locations.
In `@magicblock-api/src/tickers.rs`:
- Around line 85-140: The two functions init_slot_ticker_with_tui and
init_slot_ticker share the same loop logic; extract that loop into a single
helper (e.g., run_slot_ticker_loop) which accepts the shared inputs (accountsdb:
Arc<AccountsDb>, ledger: Arc<Ledger>, transaction_scheduler:
TransactionSchedulerHandle, block_updates_tx: BlockUpdateTx, exit:
Arc<AtomicBool>, tick_duration: Duration, committor_processor: Option<Arc<impl
ScheduledCommitsProcessor>>) plus an optional tui sender
(Option<TuiBlockUpdateTx>) or a trait object so the helper can conditionally
call advance_slot_and_update_ledger_with_tui vs advance_slot_and_update_ledger;
update both init_slot_ticker_with_tui and init_slot_ticker to spawn the same
helper and remove duplicated code (reference symbols: init_slot_ticker_with_tui,
init_slot_ticker, advance_slot_and_update_ledger_with_tui,
advance_slot_and_update_ledger, handle_scheduled_commits, MagicContext).
- Around line 124-125: Replace the panic-causing expect on
accountsdb.get_account(&magic_program::MAGIC_CONTEXT_PUBKEY) by handling the
None case: use match or if let to check the result when setting
magic_context_acc, log an error that includes the MAGIC_CONTEXT_PUBKEY (and any
contextual tick id/slot if available), and skip this tick (return/continue)
instead of panicking; apply the same change to the identical get_account call in
the non-TUI ticker function so both paths log and gracefully skip when the
MagicContext account is missing.
In `@magicblock-tui/src/app.rs`:
- Around line 164-188: The FetchTransaction handling currently awaits
fetch_transaction_detail and blocks the event loop; change it to spawn the RPC
call (e.g., with tokio::spawn) so it runs in the background and send the result
back to the event loop over a channel similar to block_rx/tx_status_rx/log_rx.
Locate EventAction::FetchTransaction handling, replace the direct await of
fetch_transaction_detail(&rpc_url, &sig).await with spawning a task that calls
fetch_transaction_detail and then sends either the TransactionDetail (on
success) or an error-wrapped TransactionDetail (built using build_explorer_url
and the existing error shape) through the UI update channel; keep using
state.show_tx_detail only when consuming messages on the event loop side. Ensure
the spawned task clones needed state/rpc_url and handles errors by constructing
the same TransactionDetail shape before sending.
- Around line 297-317: The RPC POST currently uses reqwest::Client::new() and
.send().await with no timeout, which can hang the TUI; create the client with a
request timeout (e.g. use
reqwest::Client::builder().timeout(Duration::from_secs(10)).build().unwrap())
and replace Client::new() with that client (ensure you add use
std::time::Duration). This ensures the
.post(rpc_url).json(&request_body).send().await call returns an error on timeout
instead of stalling the event loop.
In `@magicblock-tui/src/logger.rs`:
- Around line 3-48: Replace the unbounded channel with a bounded
tokio::sync::mpsc::Sender to avoid unbounded buffer growth: change
TuiTracingLayer.tx type from UnboundedSender<LogEntry> to
tokio::sync::mpsc::Sender<LogEntry> in the struct and constructor
(TuiTracingLayer::new), update the code that creates the channel in app.rs to
use tokio::sync::mpsc::channel(capacity) and pass the Sender, and replace the
non-blocking send call in on_event with self.tx.try_send(entry) handling the
Err(TrySendError::Full|Closed) case by silently dropping the log; also update
the comment about send semantics to reflect that try_send() is what enforces
backpressure.
In `@magicblock-tui/src/ui.rs`:
- Around line 476-486: The truncation logic can underflow when popup_width is
<4; update the calculations to use saturating_sub so they never wrap: compute
max_chars = popup_width as usize.saturating_sub(4) and when deciding the
truncate index avoid subtracting 3 directly—either use
max_chars.saturating_sub(3) or guard with a condition like if max_chars > 3
before calling char_indices().nth; ensure the fallback uses safe behavior (e.g.,
take up to max_chars characters without appending "..." if max_chars <= 3) for
the variables and expressions around popup_width, max_chars, truncated, and the
char_indices()/nth call on detail.logs iteration.
In `@magicblock-tui/src/utils.rs`:
- Around line 7-18: The url_encode function allocates per character via
c.to_string() and format!; replace those with UTF-8 encoding into a stack buffer
and formatted writes to the existing String to avoid allocations—use
c.encode_utf8(&mut buf) to get the byte slice for the character and then, for
each byte, write the percent-encoded hex into encoded (either via
std::fmt::Write::write_fmt with format_args!("%02X", byte) or a small
byte-to-hex table to push two hex chars) instead of calling format! or
to_string(), keeping the rest of url_encode and its character-class match logic
intact.
GabrielePicco
requested review from
Dodecahedr0x,
bmuddha,
snawaz,
taco-paco and
thlorenz
1 participant
Summary
Screen.Recording.2026-01-25.at.14.08.46.mov
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Issues
Closes #620