Add functionality for UI

[mihneagrigore]

This pull request introduces several changes to the p2p-transfer project to improve compatibility with WebRTC relay transport, update toolchain requirements, and refactor error handling in the node connection logic. The most significant updates include switching to a local version of the iroh library with custom features, moving the Rust toolchain to nightly, initializing a Tokio runtime in main.rs, and updating protocol handling to use more specific error types.

Dependency and toolchain updates:

• Changed the iroh dependency in Cargo.toml to use a local path and disabled default features, allowing for more controlled and customizable builds.
• Updated the Rust toolchain in rust-toolchain to use the nightly channel instead of a specific stable version, enabling access to newer Rust features.

Runtime and transport improvements:

• Added explicit creation and entry of a Tokio runtime in main.rs to ensure async operations function correctly in the application.
• Modified the EchoNode spawning logic in node.rs to bind the endpoint using TransportMode::WebrtcRelay, enabling WebRTC relay transport for peer-to-peer connections.

Protocol handler refactoring:

• Updated the protocol handler and connection methods in node.rs to use std::result::Result<(), AcceptError> instead of the generic anyhow::Result, providing more precise error handling for connection acceptance. [1] [2] [3]
• Refactored the connect function in node.rs to simplify task spawning and event sending logic, and corrected the type of bytes_received for consistency.

[Copilot]

Pull request overview

This pull request adds peer-to-peer networking functionality to enable WebRTC-based file transfers through a UI. The changes include upgrading to a local iroh library build, switching to the Rust nightly toolchain, initializing a Tokio runtime for async operations, and implementing comprehensive UI controls for connection management.

Key Changes:

• Switched from stable Rust 1.82 to nightly toolchain and migrated to a local iroh dependency with custom features
• Added Tokio runtime initialization in main.rs to support async operations in the eframe application
• Refactored error handling in node.rs to use specific AcceptError types instead of generic anyhow::Result
• Implemented complete UI for accepting and receiving connections with terminal log viewing capabilities

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
p2p-transfer/Cargo.toml	Updated iroh dependency to use local path with disabled default features
p2p-transfer/rust-toolchain	Changed Rust toolchain from stable 1.82 to nightly channel
p2p-transfer/src/main.rs	Added Tokio runtime initialization for async operation support
p2p-transfer/src/node.rs	Updated protocol handlers to use specific AcceptError types, refactored connect function to simplify task spawning, and enabled WebRTC relay transport mode
p2p-transfer/src/app.rs	Added comprehensive UI implementation including connection state management, node spawning, terminal logs viewing, and receive dialog functionality
Cargo.lock	Extensive dependency updates reflecting the local iroh build and added WebRTC-related dependencies

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[radumarias]

• move controls to the top
• show terminal at the bottom like 1/3 of screen height
• keep terminal panel fixed size, and with a scrollbar like now, now it increases when the text is large
• share creates an url like https://wesha.rs/ and you parse from there on receive. This will then also work in webapp, as it will start the actual app in the browser

[radumarias]

╭─── Claude Code v2.0.69 ───────────────────────────────────────────────────────────────────────────────────────╮
│ │ Tips for getting started │
│ Welcome back! │ Run /init to create a CLAUDE.md file with instructions for Claude │
│ │ ───────────────────────────────────────────────────────────────── │
│ * ▐▛███▜▌ * │ Recent activity │
│ * ▝▜█████▛▘ * │ No recent activity │
│ * ▘▘ ▝▝ * │ │
│ │ │
│ Opus 4.5 · Claude API │ │
│ ~/dev/radumarias/web-sync/syncoxiders │ │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

PR Review Summary

Commit: 8eebd2c - Add support for WASM
Files Changed: p2p-transfer/src/app.rs (+341, -79)

---

Critical Issues (3 found)

Agent	Issue	Location
code-reviewer	Inconsistent duplicate detection keys - shared_files uses path, shared_files_data uses name	Lines 1436-1450
silent-failure-hunter	Silent lock failures in add_file_to_share() - .lock().ok().and_then() chains silently fail with no user feedback	Lines 1413-1417
silent-failure-hunter	Silent fallback to empty file list in start_accepting() - if lock fails, node starts with no files	Lines 310-313

---

Important Issues (8 found)

Agent	Issue	Location
code-reviewer	shared_files_data not cleared in stop_accepting() - memory accumulation	Lines 535-573
code-reviewer	shareable_url not cleared when stopping	stop_accepting()
silent-failure-hunter	Silent lock failures in WASM file picker - 4 independent locks can fail silently	Lines 193-204
silent-failure-hunter	download_file_wasm() silent failure - nested if let with no error handling	Lines 1339-1366
silent-failure-hunter	Multiple .unwrap() calls on web APIs that can panic	Lines 152-155, 164, 175, 1355, 1543-1545
silent-failure-hunter	reconnect_for_files() silent early return with no logging	Lines 949-956
silent-failure-hunter	Console-only error logging for WASM - users never see errors	Throughout file
code-reviewer	Potential deadlock risk with nested lock acquisitions	Lines 1729-1746

---

Suggestions (9 found)

Agent	Suggestion	Impact
code-simplifier	Add MutexExt trait for set(), update(), get_cloned() methods	~100 lines saved
code-simplifier	Create platform logging abstraction to reduce #[cfg] duplication	~50 lines saved
code-simplifier	Extract common event handling logic between WASM/native	~150 lines saved
code-simplifier	Create clear_picked_file() method	4 duplications removed
code-simplifier	Create update_status_and_repaint() helper	Cleaner status updates
code-simplifier	Extract connection closed handler	3 duplications removed
silent-failure-hunter	Implement user-visible error notification system (toasts)	Better UX
silent-failure-hunter	Replace .lock().ok().and_then() with explicit error handling	Better debugging
silent-failure-hunter	Use expect() with descriptive messages instead of .unwrap()	Clearer panics

---

Strengths

• Arc/Mutex patterns correctly used for shared state across async boundaries
• #[cfg(target_arch = "wasm32")] attributes are properly placed
• Platform-specific code paths are consistent
• WASM file handling properly stores data in memory since filesystem isn't available
• Good use of spawn_local for WASM async operations

---

Recommended Action Plan

1. Fix Critical Issues First

A. Fix inconsistent deduplication keys (Lines 1436-1450):
// Change from checking by path to checking by name for consistency
if !files.iter().any(|(n, _, _)| n == &name) {

B. Add logging for lock failures (Lines 1413-1417):
let file_info = {
let name_result = self.picked_file_name.lock();
let path_result = self.picked_file_path.lock();
let size_result = self.picked_file_size.lock();

  if name_result.is_err() || path_result.is_err() || size_result.is_err() {
      web_sys::console::error_1(&"Internal error: failed to access file state".into());
      return;
  }
  // ... rest of logic

};

C. Log when falling back to empty file list (Lines 310-313):
let files_to_share = match shared_files_data.lock() {
Ok(files_data) => files_data.clone(),
Err(e) => {
web_sys::console::error_1(&format!("Warning: Failed to access files: {}", e).into());
Vec::new()
}
};

2. Address Important Issues

• Clear shareable_url in stop_accepting()
• Clear shared_files_data in stop_accepting() or document why it persists
• Add error handling to download_file_wasm() with user notification
• Replace .unwrap() with expect() or proper error handling in web API calls

3. Consider Suggestions

The code simplification suggestions from the simplifier agent could reduce the codebase by 400-450 lines while improving maintainability. This is optional but recommended for long-term maintenance.

---

Re-run Review After Fixes

After addressing critical and important issues, run:
/pr-review-toolkit:review-pr errors code
to verify the error handling improvements.

────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

 fix the critical issues (tab to accept)
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
? for shortcuts 52204 tokens

[radumarias]

Overview

This PR turns p2p-transfer from a stub into an actually functional P2P transfer app:

• Migrates iroh to the anchalshivank/iroh#feature/webrtc-wasm-support fork (with default-features = false) and adds [patch] overrides for n0-watcher, netwatch, portmapper, both at the workspace and crate level.
• Adopts TransportMode::WebrtcRelay so the same node code runs in browser and native.
• Adds default-members = ["p2p-transfer"] to the workspace.
• Bumps the toolchain 1.82 → nightly.
• Bootstraps a Tokio runtime in main.rs before eframe::run_native.
• Replaces tokio::sync::broadcast with async_channel::unbounded.
• Defines a real wire format on top of Echo: length-prefixed filename, length-prefixed data, then per-file (name | data_len | total_chunks | blake3_hash | (chunk_idx | size | bytes)*) framing in 256 KiB chunks. Receiver verifies the per-file BLAKE3 hash.
• Introduces TransferEvent::{FileStart, ChunkReceived, FileComplete} and chunk-by-chunk file writes via OpenOptions + seek in the UI.
• Adds a new blob_store module: a cross-platform BLAKE3 BlobStore and a native-only bao submodule wrapping bao_tree::PreOrderMemOutboard.
• Adds 11 unit/integration tests in src/tests.rs.

The direction is right and the protocol design is sensible. Several issues below would need to be addressed before this can land.

---

Critical

EchoNode::subscribe_accept_events is dead code (node.rs:200)

pub fn subscribe_accept_events(&self) -> Receiver<AcceptEvent> {
    let (tx, rx) = unbounded();
    let _main_sender = self.accept_events.clone();
    rx
}

A new channel pair is created, the sender is dropped, and the unrelated _main_sender clone is bound and immediately discarded. The returned receiver will yield nothing forever. The previous broadcast::Sender::subscribe() semantics were lost. Either:

• Wire tx into a forwarding task spawned from the main accept_events source, or
• Keep the original receiver as a field and hand out a clone (only works because you switched to async_channel, which supports multi-consumer), or
• Delete the method.

Right now it's a footgun for anyone hooking up the UI later.

Accept-side events are silently dropped (node.rs:44, 84, 112)

let (event_sender, _event_receiver) = unbounded();

The receiver is dropped at every spawn_* site, but event_sender.try_send(...) on an unbounded channel succeeds even with no consumer — every AcceptEvent::{Accepted, Echoed, Closed} is enqueued and immediately leaked / GC'd. Consequently the seeder UI gets zero feedback about who connected, how much was sent, or when the connection closed. The receiver side is fine (it consumes ConnectEvent from connect()), so this only manifests on the sharing side. Probably the root cause of any "the seeder UI doesn't update" bugs.

Receiver allocates from attacker-controlled length (node.rs, connect)

let data_len = u64::from_le_bytes(data_len_buf);
...
let mut all_data = Vec::with_capacity(data_len as usize);

data_len is wire data. A peer can pin you with u64::MAX and OOM/abort. Same issue exists for name_len (u32) and the per-chunk chunk_size. Add a hard upper bound (e.g. MAX_FILE_SIZE: u64 = 16 * 1024 * 1024 * 1024) and bail out before allocating. Validate data_len <= total_chunks * CHUNK_SIZE and chunk_size <= CHUNK_SIZE too.

---

High-impact

Tokio runtime lifetime in main.rs

let runtime = tokio::runtime::Runtime::new().expect(...);
let _guard = runtime.enter();

EnterGuard only attaches the runtime to the current thread. Egui frame callbacks run on the main thread, so direct tokio::spawn from update() works — but anything eframe schedules on a worker (e.g. persistence, state restore on some backends) calling into your code will panic with "no reactor running." Safer pattern: runtime.block_on(async { ... }) around the eframe run_native call (it doesn't need to run on the tokio thread, just inside an entered context), or store runtime.handle().clone() and use Handle::spawn. At minimum, document the constraint near the spawn sites.

Bao "merkle verification" doesn't actually use the merkle tree

BaoBlob::verify_chunk is:

let expected = &self.data[start as usize..end as usize];
chunk_data == expected

That's a memcmp against the locally-stored blob — no outboard is consulted, no bao_tree::io::sync::DecodeResponseIter or similar. The whole "BitTorrent-like merkle verification" abstraction is decorative right now: it can only verify chunks if you already have the full blob, which defeats the purpose. Either wire up real outboard-based verification (bao_tree::io::sync::Outboard::root + verifying iterator) or omit the module from this PR and add it when there's a real consumer.

Online tests are tautological / never fail

In src/tests.rs:

match result {
    Ok(Ok(node)) => { ... assertions ... }
    Ok(Err(e)) => panic!(...),
    Err(_) => {}             // timeout: silent pass
}

if received_data == test_data {
    assert_eq!(received_data, test_data);
}

The "transfer" test can only assert equality when equality already holds — if the transfer fails or returns wrong bytes, the test passes anyway. Combined with timeouts swallowing all network errors, these online_test_* cases will never fail in CI, never alert you to regressions. Either:

• Mark them #[ignore] and run via cargo test -- --ignored in a separate "integration" job, AND
• Make the assertion non-conditional inside the matched branch, AND
• Treat timeout as failure when RUN_ONLINE_TESTS=1 is set.

---

Medium

Wire format has no version / magic / overall length

The receiver reads 4-byte filename length straight off the stream. A protocol mismatch shows up as opaque read_exact "early eof". Add a 4-byte magic + 1-byte version at the top of the stream so future format changes are detectable. Cheap insurance.

Mutex::lock().ok() everywhere swallows poison

BlobStore, BaoStore, and the egui state all use the lock().ok().map(...).unwrap_or_default() idiom. A poisoned mutex (panic mid-write) becomes "store looks empty" with no diagnostic. At minimum, log on Err. Consider parking_lot::Mutex (no poisoning), since you're not relying on poison-as-an-API.

Lots of .unwrap() in app.rs

Sampled instances:

• node_guard.unwrap() and node_guard.as_ref().unwrap() (app.rs:1283, 1288, 1062)
• web_sys::window().unwrap().set_timeout_with_callback_and_timeout_and_arguments_0(...).unwrap() (app.rs:1768-1770)
• files.is_err() || files.as_ref().unwrap().is_empty() (app.rs:1875)

A panic in egui's frame closure tears down the whole frame; in WASM it kills the runtime. if let Ok(...) / if let Some(...) is one extra line. The node_guard ones are the most fragile because you've already locked the mutex — swap for if let Some(node) = node_guard.as_ref().

println! mixed with log::info!

Roughly 30 println! calls were added (e.g. app.rs:487, 495, 511, 527, 542, ...). They bypass env_logger's filtering and won't show up in the WASM console at all (only web_sys::console::log_* does). Convert to log::info!/log::error!; you already use these elsewhere.

Sender side uses try_send on accept events; receiver side uses await send

Echo::handle_connection_0 uses event_sender.try_send(...) (since the channel may have no consumer). But connect() uses event_sender.send(...).await?, which is correct because the consumer exists. Just flag the asymmetry — once you fix the dropped-receiver issue above, switch the seeder side to await send too, otherwise a slow UI consumer can lose events under bursty traffic.

---

Low / nits

• Toolchain bump to nightly without justification: the previous 1.82 was stable. Is there a specific feature in use? If not, prefer the latest stable; if there is, document it (e.g. in rust-toolchain comment or CLAUDE.md).
• [patch] URLs use branch = "...": a force-push to either fork breaks everyone's builds. Pin rev = "..." until you control the upstreams.
• Formatting glitch: }impl Echo { on a single line in node.rs:295. cargo fmt --check (run by ./check.sh) will reject this.
• Trailing newlines missing: Cargo.toml and node.rs both end without a newline.
• Duplicate re-exports: lib.rs re-exports BaoBlob/BaoStore/BaoReceiver/BLOCK_SIZE even though blob_store.rs already does. Pick one.
• mod hex: a hand-rolled hex codec in 30 lines is fine, but hex is already in the dep tree transitively. Drop and use hex::encode/decode if you want to shrink the file.
• BlobCollection::from_share_string failure mode: a stray comma ("a,,b") makes the whole parse error. Either skip empty parts or document that the format is strict.
• No test coverage for BaoReceiver: add_chunk out-of-range, progress, assemble_and_verify (success and hash-mismatch) are all untested, and that's the only piece doing actual verification.
• #[allow(refining_impl_trait)] on ProtocolHandler::accept: required by the iroh fork's signature; OK, just confirming the suppression is intentional and not papering over a future incompatibility when iroh's trait stabilizes.

---

Summary

The protocol design and module split are solid; the BLAKE3 verification on the receiver is a real improvement. Blockers before merge: the two dead-channel bugs (subscribe_accept_events, dropped accept receiver), the unbounded allocation from wire-supplied data_len, the tautological online tests, and the formatting/println! cleanup so ./check.sh passes. The Bao module is half-done and should either be finished or trimmed back so it doesn't suggest a guarantee it doesn't provide.