Conversation
|
Review of code: Risk-Ordered Findings
(Note: dependency mismatch/derive-deftly conflict is already fixed by pinning |
alltheseas
force-pushed
the
feature/tor-routing
branch
from
096c48c to
f00bea3
Compare
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds cross-platform Tor support: desktop runtime-managed Tor client with SOCKS proxy, Android JNI/native Arti integration, runtime-configurable WebSocket transport via SOCKS, HTTP fetching over SOCKS, UI settings to toggle Tor, and build tooling to compile Arti for Android. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Notedeck App
participant TorMgr as TorManager
participant Runtime as TorRuntime
participant RelayPool as RelayPool
participant Network as Network Layer
App->>TorMgr: new(data_path) / set_enabled(true)
TorMgr->>Runtime: spawn background thread (run_tor_runtime)
Runtime->>Runtime: build config, bootstrap Tor, start SOCKS proxy
Runtime-->>TorMgr: ReadyState::Ready(port)
App->>TorMgr: poll()
TorMgr-->>App: status = Running(port)
App->>App: update_relay_transport()
App->>RelayPool: configure_transport(Socks(proxy), wakeup)
RelayPool->>RelayPool: update ws_options
RelayPool->>RelayPool: reconnect relays using ws_options
App->>Network: http_fetch(url, socks_proxy)
Network->>Network: http_req_via_socks(url, socks_proxy)
Network->>Runtime: connect via SOCKS5
Runtime->>Network: forward data
Network-->>App: HyperHttpResponse
sequenceDiagram
participant User as User
participant UI as Settings UI
participant App as Notedeck App
participant TorMgr as TorManager
User->>UI: toggle Tor on
UI->>App: emit SettingsAction::ToggleTor(true)
App->>TorMgr: set_enabled(true)
TorMgr->>Runtime: spawn/start Tor
Runtime-->>TorMgr: ReadyState::Ready(port)
App->>App: update_relay_transport() -> configure RelayPool to use SOCKS
UI-->>User: show Tor status Running
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 11
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Line 127: The Cargo.toml currently uses a local path dependency for ewebsock
("ewebsock = { path = \"vendor/ewebsock\" }"); replace this with a git-based
dependency that points to our fork that includes the SOCKS5 changes by updating
the ewebsock entry to reference the fork's git URL (and optional branch or
revision) so the project no longer depends on vendored source; ensure the
dependency key remains "ewebsock" and include the fork's repo URL and
branch/commit in the entry (and remove the path attribute).
In `@crates/notedeck_columns/src/ui/settings.rs`:
- Around line 545-550: The TorStatus::Failed(err) branch currently exposes the
raw internal error to users; instead, log the detailed error (e.g., using
tracing::debug or tracing::error with err) and return a generic, localized
user-facing label via the tr! macro (using self.note_context.i18n) such as "Tor
status: Connection failed" or a mapped, friendly string; ensure you keep tracing
of err for diagnostics but do not interpolate err into the displayed
translation.
In `@crates/notedeck/src/tor.rs`:
- Around line 38-47: The stop method on TorHandle currently calls handle.join()
which can block indefinitely if the Tor runtime ignores the shutdown signal;
modify TorHandle::stop to send the shutdown via self.shutdown.take() as is, then
poll the thread (self.thread.take() / handle) for a bounded timeout (e.g., 5s)
using handle.is_finished() in a loop with short sleeps, logging a warning if the
timeout elapses, and only call handle.join() if is_finished() is true to avoid
blocking forever; reference TorHandle, stop, shutdown, thread, join, and
is_finished when implementing this change.
- Around line 218-232: Ready signal is sent too early: move sending
ReadyState::Ok (ready_tx_clone.send) to after the proxy is confirmed listening
rather than immediately after tor.create_bootstrapped(); update proxy::run_proxy
(or its call site) so it reports successful bind before
returning/continuing—e.g., add a bind-complete oneshot or change run_proxy to
return a Result that resolves when the SOCKS port is bound, then send
ReadyState::Ok only after that confirmation (use the existing listen/Listen and
proxy_future/shutdown_rx flow to await bind success before sending).
- Line 19: Add docstrings to the private helper functions build_client_config()
and path_to_string() describing their purpose, inputs, outputs, and any
important behavior (e.g., build_client_config() constructs the tor client
configuration including the default SOCKS port and error handling;
path_to_string() converts a Path/PathBuf to a String and how it handles
invalid/unicode paths). Update the function comments immediately above each
function with concise Rust-style /// doc comments that follow existing project
tone and mention any special cases (like the hardcoded 9150 port and that
binding failures are reported via TorStatus::Failed).
In `@vendor/ewebsock/.cargo-ok`:
- Line 1: The repository currently vendors ewebsock (presence of
vendor/ewebsock/.cargo-ok) without justification; either stop vendoring and
reference a maintained fork in Cargo.toml (add the fork crate reference and
remove the vendored directory), or if vendoring is unavoidable, add a clear
justification comment/README next to vendor/ewebsock/.cargo-ok explaining why a
fork/published crate cannot be used (e.g., upstream unresponsive, required
unpublished patches, licensing constraints), list the specific changes that
required vendoring, and document maintenance expectations and contact for future
updates; update Cargo metadata accordingly so maintainers can see the rationale.
In `@vendor/ewebsock/Cargo.toml.orig`:
- Around line 1-75: This patch vendors the ewebsock crate (package "ewebsock")
which violates the guideline; push your SOCKS5 changes to a public/private fork
of the ewebsock repository and update the Cargo.toml dependency to reference
that fork via a git dependency (e.g., set ewebsock in Cargo.toml to use git =
"https://.../your-fork.git" and optionally rev = "..." or tag) instead of
keeping the copy under vendor/, and if vendoring is absolutely unavoidable add a
short justification and link to the upstream issue/PR in the repo docs or a
VENDORING.md explaining why a fork was not possible.
In `@vendor/ewebsock/src/native_tungstenite_tokio.rs`:
- Around line 80-83: The on_event callback return value (used at the
WsEvent::Opened call and the reader closure) currently ignores
ControlFlow::Break and merely logs a warning; update the logic so
ControlFlow::Break is honored: have the on_event(...) result be checked and, if
it indicates Break, break out of the reader loop / return from the async reader
task immediately instead of continuing, or change the callback signature to
return Result<(), E> (or bool) and propagate that result so callers can stop
processing; adjust handling around the on_event(WsEvent::Opened) call and the
reader async closure so the Break/Err path causes an immediate exit rather than
continuing.
- Around line 159-172: The test test_connect_tokio attempts to open a real
WebSocket to example.com which has no WebSocket endpoint and will fail; either
convert the test to use a local/mock WebSocket server (spawn a tokio-based
echo/mock server and call crate::connect against its address) or mark the test
as an ignored/integration test by adding the appropriate test attribute (e.g.,
#[ignore]) and a comment referencing crate::connect and test_connect_tokio so CI
won't run a failing external connection; implement whichever approach is
appropriate for your test suite.
In `@vendor/ewebsock/src/native_tungstenite.rs`:
- Around line 200-223: The code currently panics on WsMessage::Unknown in the
send loop; replace the panic with a non-panicking error-handling path: when
matching WsMessage::Unknown(_) in the match inside the loop that handles
rx.try_recv(), log a warning or construct and return an Err with a clear message
(e.g., "cannot send WsMessage::Unknown") instead of panic!, ensuring you follow
the surrounding function's Result error type (the same style used for socket
send errors); keep existing cleanup behavior (socket.close/flush) if you return
an Err so resources are cleaned up consistently.
In `@vendor/ewebsock/src/web.rs`:
- Around line 32-47: The send() method in web.rs currently panics for
WsMessage::Ping, WsMessage::Pong and other unknown variants; update the match in
send() (inside the send method) to handle WsMessage::Ping and WsMessage::Pong
instead of falling through to the panic: add explicit arms for WsMessage::Ping
and WsMessage::Pong that either translate them to an appropriate send call on
socket (e.g., send a small binary/frame or text payload if your websocket
backend requires) or safely no-op/log the event with log::debug!/log::warn! so
they don't panic, and add a final arm for WsMessage::Unknown that logs and
returns an error instead of panicking; reference the send function and the
WsMessage enum variants when making the change.
🧹 Nitpick comments (18)
vendor/ewebsock/Cargo.toml (1)
90-96: Version ranges may affect build reproducibility.The tungstenite dependencies use version ranges (
>=0.23, <=0.24), which could lead to different builds depending on when dependencies are resolved.Since this is a vendored crate, consider pinning to exact versions for more predictable builds, or document which specific version was tested.
crates/notedeck_chrome/src/chrome.rs (2)
919-942: Tor indicator implementation follows egui idioms correctly.The
Areapositioning, layering withOrder::Tooltip, and non-blocking rendering are all appropriate. The subtle background blending provides good visual integration.Consider adding click affordance (as noted in PR comments) to allow users to retry on failure or access Tor settings:
💡 Optional: Add click-to-action affordance
let response = egui::Frame::new() // ... existing frame setup ... .show(area_ui, |ui| { // ... existing icon painting ... }); // Make clickable for retry/settings navigation if response.response.clicked() { // Could emit an action to navigate to Settings or retry Tor } response.response.on_hover_text(style.tooltip);
968-979: Consider sanitizing error messages in the Failed tooltip.The error message from
err.as_str()is displayed directly in the tooltip. Per PR comments, internal Arti error strings may be cryptic or expose implementation details. Consider mapping common errors to user-friendly messages while logging the raw error for debugging.💡 Optional: User-friendly error mapping
TorStatus::Failed(err) => { let user_msg = match err.as_str() { s if s.contains("timeout") => "Connection timed out", s if s.contains("network") => "Network error", _ => "Connection failed", }; Some(TorIndicatorStyle { // ... tooltip: tr!(i18n, "Tor routing failed: {error}", ..., error = user_msg), }) }crates/notedeck/Cargo.toml (1)
56-60: Target-gate Tor dependencies for clarity and to prevent potential transitive dependency issues on unsupported platforms.The tor module already has proper conditional compilation (main implementation excluded on Android/wasm with a stub provided), however, the Arti dependencies in Cargo.toml lack explicit target gating. While Cargo correctly handles this when all consuming code is gated with
#[cfg], explicitly gating these dependencies in Cargo.toml makes the intent clearer and preemptively avoids potential issues from transitive dependencies that may not support these platforms.♻️ Suggested target-specific dependencies
[target.'cfg(not(any(target_os = "android", target_arch = "wasm32")))'.dependencies] arti = { workspace = true } arti-client = { workspace = true } tor-config = { workspace = true } tor-config-path = { workspace = true } tor-rtcompat = { workspace = true }crates/notedeck/src/persist/settings_handler.rs (1)
32-41: Consider adding#[serde(default)]for backward compatibility.When users upgrade from a version without
use_tor, their existingsettings.jsonwon't contain this field. Without#[serde(default)]on either the field or struct, deserialization will fail (Line 130-138 falls back to defaults, but this may cause unnecessary error logging).Consider annotating the struct or field:
Option 1: Default at struct level (recommended for future fields)
-#[derive(Serialize, Deserialize, PartialEq, Clone)] +#[derive(Serialize, Deserialize, PartialEq, Clone, Default)] +#[serde(default)] pub struct Settings {Option 2: Default at field level
+ #[serde(default)] pub use_tor: bool,crates/enostr/src/relay/mod.rs (1)
231-234: Note:update_optionsdoes not trigger reconnection.This method updates stored options without reconnecting. Callers must explicitly call
connect()afterward if they want changes to take effect immediately. This is the current design (as seen inRelayPool::configure_transport), but consider documenting this behavior.📝 Optional: Add docstring clarifying reconnection requirement
+ /// Updates the connection options for this relay. + /// + /// Note: This does not trigger a reconnection. Call `connect()` afterward + /// for the new options to take effect. pub fn update_options(&mut self, options: Options) { self.options = options; }vendor/ewebsock/src/native_tungstenite_tokio.rs (4)
23-27: Cloning sender and spawning per-send is inefficient.Each
send()clones theSenderand spawns a new Tokio task. This adds overhead compared to the blocking version innative_tungstenite.rswhich usestx.send(msg).ok()synchronously. Consider usingtry_send()for non-blocking behavior without spawning:♻️ Suggested improvement
pub fn send(&mut self, msg: WsMessage) { - if let Some(tx) = self.tx.clone() { - tokio::spawn(async move { tx.send(msg).await }); + if let Some(tx) = &self.tx { + if let Err(err) = tx.try_send(msg) { + log::warn!("Failed to send WebSocket message: {err}"); + } } }
93-93: Consider returning an error instead of panicking onWsMessage::Unknown.Panicking in library code can be disruptive. Consider filtering out or logging
Unknownmessages instead:♻️ Suggested alternative
- WsMessage::Unknown(_) => panic!("You cannot send WsMessage::Unknown"), + WsMessage::Unknown(_) => { + log::warn!("Ignoring attempt to send WsMessage::Unknown"); + return; + }Note: This would require restructuring the stream mapping slightly.
139-139: Consider making the channel buffer size configurable.The hardcoded buffer of 1000 messages may be excessive for typical WebSocket usage. Consider deriving this from
Optionsor using a smaller default.
148-151: Unnecessaryurl.clone()- theurlis already moved into the async block.♻️ Suggested fix
tokio::spawn(async move { - ws_connect_async(url.clone(), options, outgoing_messages_stream, on_event).await; + ws_connect_async(url, options, outgoing_messages_stream, on_event).await; log::debug!("WS connection finished."); });vendor/ewebsock/src/web.rs (2)
69-74: Options are silently ignored in WASM implementation.The
_ignored_optionsparameter suggests intentional omission, but this differs from native behavior. Consider logging when non-default options are passed, or documenting this limitation in the public API.
88-88:Sendbound onRcis misleading for single-threaded WASM.
Rc<dyn Send + Fn...>has aSendbound that's meaningless in WASM's single-threaded context. This likely comes from theEventHandlertype alias for API consistency with native targets. Consider using a cfg-specific type alias if this causes confusion.crates/notedeck/src/tor.rs (3)
201-240: Consider documenting the Tor runtime lifecycle and error paths.The function handles multiple error scenarios but lacks docstrings explaining the expected behavior. Per coding guidelines, ensure docstring coverage for added code.
📝 Suggested documentation
+ /// Launch the blocking Arti task inside the runtime and wait for shutdown. + /// + /// # Lifecycle + /// 1. Creates the Tor runtime and bootstraps the client + /// 2. Sends `ReadyState::Ok` once bootstrap completes + /// 3. Starts the SOCKS proxy on the specified port + /// 4. Runs until shutdown signal or proxy error + /// + /// # Errors + /// Returns an error string if runtime creation, bootstrap, or proxy fails. + /// Errors are also sent via `ready_tx` for the manager to handle. fn run_tor_runtime(
291-293:set_enabledreturnsOk(())on unsupported platforms, which may be misleading.Returning
Ok(())when Tor is unsupported could lead callers to believe Tor was successfully enabled. Consider returning an error or checkingis_supported()in callers:♻️ Alternative approach
pub fn set_enabled(&mut self, _enabled: bool) -> Result<(), String> { - Ok(()) + Err("Tor is not supported on this platform".to_owned()) }Or document that callers should check
is_supported()first.
267-274:TorStatusenum is duplicated across platform-specific modules.The enum definition appears in both the main module (lines 57-63) and the fallback (lines 268-274). Consider moving it outside the
innermodules to avoid duplication:♻️ Suggested refactor
/// Overall Tor connection status used throughout the UI. #[derive(Clone, Debug)] pub enum TorStatus { Disabled, Starting, Running { socks_port: u16 }, Failed(String), Unsupported, } #[cfg(all(not(target_arch = "wasm32"), not(target_os = "android")))] mod inner { use super::TorStatus; // ... rest of implementation } #[cfg(any(target_arch = "wasm32", target_os = "android"))] mod inner { use super::TorStatus; // ... rest of implementation }crates/notedeck_columns/src/ui/settings.rs (1)
94-102: Toggle state may briefly desync ifset_enabledfails.The settings are updated before checking if Tor enablement succeeds. On failure, the setting is reverted, but the UI toggle may have already reflected the change. Consider checking support and enablement before updating settings:
♻️ Suggested ordering
Self::ToggleTor(enabled) => { - settings.set_use_tor(enabled); if notedeck::TorManager::is_supported() { if let Err(err) = tor.set_enabled(enabled) { tracing::error!("failed to toggle tor: {err}"); - settings.set_use_tor(!enabled); + } else { + settings.set_use_tor(enabled); } + } else { + settings.set_use_tor(enabled); } }vendor/ewebsock/src/native_tungstenite.rs (2)
483-495: Silent catch-all may skip timeout on other TLS backends.The
_ => {}arm silently ignores anyMaybeTlsStreamvariants not explicitly handled. Ifnative-tlsfeature is ever enabled, timeouts won't be set for those connections. Consider logging a warning or adding explicit handling.Proposed improvement
#[cfg(feature = "tls")] MaybeTlsStream::Rustls(s) => { s.get_mut() .set_read_timeout(value) .map_err(|err| format!("failed to set read timeout: {err}"))?; } - _ => {} + #[cfg(feature = "native-tls")] + MaybeTlsStream::NativeTls(s) => { + s.get_mut() + .set_read_timeout(value) + .map_err(|err| format!("failed to set read timeout: {err}"))?; + } + _ => { + log::warn!("Unable to set read timeout on unknown stream type"); + } };
500-506: Test depends on external network; may fail in CI.This test connects to
ws://example.com, which requires network access and may fail intermittently or in sandboxed CI environments. Consider using a local mock server or marking this as an ignored integration test.Suggestion
#[test] +#[ignore] // Requires network access fn test_connect() { let options = crate::Options::default(); // see documentation for more options let (mut sender, _receiver) = crate::connect("ws://example.com", options).unwrap(); sender.send(crate::WsMessage::Text("Hello!".into())); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.cargo/config.tomlCargo.tomlcrates/enostr/src/relay/mod.rscrates/enostr/src/relay/pool.rscrates/notedeck/Cargo.tomlcrates/notedeck/src/app.rscrates/notedeck/src/context.rscrates/notedeck/src/lib.rscrates/notedeck/src/persist/settings_handler.rscrates/notedeck/src/tor.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck_columns/src/nav.rscrates/notedeck_columns/src/ui/settings.rsvendor/ewebsock/.cargo-okvendor/ewebsock/.cargo_vcs_info.jsonvendor/ewebsock/Cargo.tomlvendor/ewebsock/Cargo.toml.origvendor/ewebsock/README.mdvendor/ewebsock/src/lib.rsvendor/ewebsock/src/native_tungstenite.rsvendor/ewebsock/src/native_tungstenite_tokio.rsvendor/ewebsock/src/tungstenite_common.rsvendor/ewebsock/src/web.rs
🧰 Additional context used
📓 Path-based instructions (4)
crates/notedeck*/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/notedeck*/src/**/*.rs: UseAppContextto access mutable handles to services (database, caches, relay pool, account state, localization, settings, wallet) rather than accessing global state
Never use Mutexes in UI paths; the render loop must never block. UseRc<RefCell<>>for single-threaded interior mutability,Arc<tokio::sync::RwLock<>>for cross-thread sharing, andpoll_promise::Promisefor async results
Wrap async work inpoll_promise::Promise, check withpromise.ready()orpromise.ready_mut()each frame—never block the render loop
Usetokio::sync::RwLockinstead ofArc<Mutex<>>for cross-thread sharing in Notedeck
Persist state viaTimedSerializer::try_saveto avoid blocking the frame; batch mutations withSettingsHandler::update_batch
Wrap user-facing strings withtr!ortr_plural!macros for localization and runpython3 scripts/export_source_strings.pyafter changing strings
UseJobPoolfor CPU-bound work and return results viatokio::sync::oneshotwrapped in Promises; usetokio::spawn()for network I/O and relay sync
Mark performance-critical functions with#[profiling::function]for visibility in the puffin profiler
Usetracingmacros for structured logging andprofilingscopes where hot paths exist
Prefer early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers ofifstatements (Nevernesting principle)
Global variables are not allowed in this codebase, even if they are thread local; state should be managed in a struct that is passed in as reference
Ensure docstring coverage for any code added or modified
Avoid Mutexes in Notedeck code; preferpoll_promise::Promisefor async results,Rc<RefCell<>>for single-threaded interior mutability, ortokio::sync::RwLockwhen cross-thread sharing is truly necessary
Never block the render loop; usePromise::ready()for non-blocking result checks. Offload CPU-heavy work to...
Files:
crates/notedeck/src/lib.rscrates/notedeck/src/persist/settings_handler.rscrates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck/src/tor.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck_columns/src/nav.rs
crates/notedeck*/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use strict Rust 2021 edition; ensure edition-lints are strict and clippy
disallowed_methodsis denied at crate root to enforce API hygiene
Files:
crates/notedeck/src/lib.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Do not vendor code; in Cargo.toml replace the existing URL with the fork that includes the new code. If vendoring is absolutely necessary, present the case why no other options are feasible
Files:
Cargo.toml
crates/notedeck*/src/app.rs
📄 CodeRabbit inference engine (AGENTS.md)
Implement the
Apptrait withupdate(&mut self, &mut AppContext, &mut egui::Ui) -> AppResponseto drive egui rendering and signal high-level actions
Files:
crates/notedeck/src/app.rs
🧠 Learnings (17)
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Prefer module-level organization over monolithic files; each feature (accounts, decks, timelines, media) should live in its own module tree
Applied to files:
crates/notedeck/src/lib.rscrates/notedeck/Cargo.tomlcrates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Tests should live alongside modules (e.g., in test submodules), often using `#[tokio::test]` when async behavior is involved
Applied to files:
crates/notedeck/src/lib.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Ensure docstring coverage for any code added or modified
Applied to files:
crates/notedeck/src/lib.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `tokio::sync::RwLock` instead of `Arc<Mutex<>>` for cross-thread sharing in Notedeck
Applied to files:
crates/notedeck/src/lib.rscrates/notedeck/Cargo.tomlcrates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/ui/settings.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/lib.rs : Use strict Rust 2021 edition; ensure edition-lints are strict and clippy `disallowed_methods` is denied at crate root to enforce API hygiene
Applied to files:
crates/notedeck/src/lib.rscrates/notedeck/Cargo.tomlvendor/ewebsock/Cargo.tomlcrates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/ui/settings.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Global variables are not allowed in this codebase, even if they are thread local; state should be managed in a struct that is passed in as reference
Applied to files:
crates/notedeck/src/lib.rscrates/notedeck/src/context.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/ui/settings.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Avoid Mutexes in Notedeck code; prefer `poll_promise::Promise` for async results, `Rc<RefCell<>>` for single-threaded interior mutability, or `tokio::sync::RwLock` when cross-thread sharing is truly necessary
Applied to files:
crates/notedeck/Cargo.tomlcrates/notedeck/src/app.rscrates/notedeck_columns/src/ui/settings.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `JobPool` for CPU-bound work and return results via `tokio::sync::oneshot` wrapped in Promises; use `tokio::spawn()` for network I/O and relay sync
Applied to files:
crates/enostr/src/relay/pool.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Persist state via `TimedSerializer::try_save` to avoid blocking the frame; batch mutations with `SettingsHandler::update_batch`
Applied to files:
crates/notedeck/src/persist/settings_handler.rscrates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to Cargo.toml : Do not vendor code; in Cargo.toml replace the existing URL with the fork that includes the new code. If vendoring is absolutely necessary, present the case why no other options are feasible
Applied to files:
vendor/ewebsock/Cargo.tomlvendor/ewebsock/.cargo-ok
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `AppContext` to access mutable handles to services (database, caches, relay pool, account state, localization, settings, wallet) rather than accessing global state
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck_columns/src/nav.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/app.rs : Implement the `App` trait with `update(&mut self, &mut AppContext, &mut egui::Ui) -> AppResponse` to drive egui rendering and signal high-level actions
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck_columns/src/nav.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Embrace egui idioms in UI code: builder chains, closures returning `Response`, `ui.vertical`/`horizontal` for layout
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck_columns/src/nav.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Never use Mutexes in UI paths; the render loop must never block. Use `Rc<RefCell<>>` for single-threaded interior mutability, `Arc<tokio::sync::RwLock<>>` for cross-thread sharing, and `poll_promise::Promise` for async results
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/ui/settings.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `tracing` macros for structured logging and `profiling` scopes where hot paths exist
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_columns/src/ui/settings.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : For animations (GIFs, video), track `repaint_at` timestamps and only request repaints when necessary; avoid spinning every frame
Applied to files:
crates/notedeck_chrome/src/chrome.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Wrap user-facing strings with `tr!` or `tr_plural!` macros for localization and run `python3 scripts/export_source_strings.py` after changing strings
Applied to files:
crates/notedeck_columns/src/ui/settings.rs
🧬 Code graph analysis (11)
crates/enostr/src/relay/mod.rs (2)
crates/enostr/src/relay/pool.rs (3)
new(114-121)new(143-150)url(53-58)vendor/ewebsock/src/lib.rs (1)
connect_with_wakeup(242-250)
vendor/ewebsock/src/web.rs (3)
vendor/ewebsock/src/native_tungstenite.rs (6)
drop(31-33)send(40-44)close(49-54)forget(57-60)ws_receive_impl(63-76)ws_connect_impl(148-167)vendor/ewebsock/src/native_tungstenite_tokio.rs (6)
drop(14-16)send(23-27)close(32-37)forget(40-43)ws_receive_impl(155-157)ws_connect_impl(129-135)vendor/ewebsock/src/lib.rs (1)
new(91-93)
vendor/ewebsock/src/native_tungstenite_tokio.rs (3)
vendor/ewebsock/src/tungstenite_common.rs (1)
into_requester(20-32)vendor/ewebsock/src/native_tungstenite.rs (4)
send(40-44)close(49-54)forget(57-60)ws_connect_impl(148-167)vendor/ewebsock/src/lib.rs (1)
connect(222-226)
crates/notedeck_chrome/src/chrome.rs (1)
crates/notedeck/src/tor.rs (2)
status(89-91)status(287-289)
crates/notedeck/src/app.rs (1)
crates/notedeck/src/tor.rs (2)
is_supported(85-87)is_supported(283-285)
crates/notedeck/src/tor.rs (2)
crates/notedeck/src/storage/file_storage.rs (1)
path(42-44)crates/notedeck/src/app.rs (1)
new(188-339)
vendor/ewebsock/src/tungstenite_common.rs (1)
vendor/ewebsock/src/lib.rs (1)
default(165-176)
crates/notedeck_columns/src/ui/settings.rs (2)
crates/notedeck/src/tor.rs (2)
is_supported(85-87)is_supported(283-285)crates/notedeck/src/ui.rs (1)
richtext_small(5-10)
vendor/ewebsock/src/lib.rs (3)
vendor/ewebsock/src/native_tungstenite.rs (2)
ws_connect_impl(148-167)ws_receive_impl(63-76)vendor/ewebsock/src/native_tungstenite_tokio.rs (2)
ws_connect_impl(129-135)ws_receive_impl(155-157)vendor/ewebsock/src/web.rs (2)
ws_connect_impl(70-191)ws_receive_impl(65-67)
vendor/ewebsock/src/native_tungstenite.rs (1)
vendor/ewebsock/src/tungstenite_common.rs (1)
into_requester(20-32)
crates/notedeck_columns/src/nav.rs (1)
crates/notedeck/src/tor.rs (2)
is_supported(85-87)is_supported(283-285)
🪛 LanguageTool
vendor/ewebsock/README.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 775 characters long)
Context: ...](https://github.com/rerun-io/ewebsock) 
🔇 Additional comments (40)
.cargo/config.toml (1)
4-6: Verify:[env]section applies globally, not just to aarch64-linux-gnu.The
[env]section is a top-level Cargo configuration that sets these environment variables for all targets, not only foraarch64-unknown-linux-gnu. Cargo does not support target-scoped[env]sections.If global SQLite bundling is intentional (the PR mentions Windows builds also need this), consider adding a clarifying comment or moving this section away from the target-specific block to avoid confusion:
[target.aarch64-unknown-linux-gnu] linker = "aarch64-linux-gnu-gcc" +# Bundle SQLite for all targets to ensure consistent builds [env] LIBSQLITE3_SYS_BUNDLING = "1" LIBSQLITE3_SYS_STATIC = "1"If bundling was intended only for specific cross-compilation scenarios, you'll need a different approach (e.g., conditional logic in a
build.rsor CI-level environment variables).crates/notedeck/src/context.rs (2)
32-32: LGTM! TorManager correctly integrated into AppContext.The addition follows the established pattern of exposing services as mutable references through
AppContext, which aligns with the coding guidelines for service access.Consider adding a brief doc comment for the
torfield to maintain consistency with documentation practices.
1-5: Import addition is appropriate.
TorManageris correctly imported alongside other service types, maintaining the existing import organization.vendor/ewebsock/README.md (1)
1-42: Vendored upstream documentation.This README is part of the vendored ewebsock crate. The static analysis hint about exclamation marks is a false positive—these are standard markdown badge URLs.
The vendoring concern has been raised on the
.cargo-okfile.vendor/ewebsock/Cargo.toml (1)
264-264: Good:unsafe_code = "deny"enforced.This aligns with the crate's "unsafe forbidden" policy and provides strong safety guarantees.
vendor/ewebsock/.cargo_vcs_info.json (1)
1-6: Good: VCS info preserved for traceability.The SHA reference (
636d3912e1d2e7389f343774a1ad8f8ec77df085) enables tracking the exact upstream commit this vendor snapshot was taken from, which is essential for future updates or security audits.Cargo.toml (1)
93-97: LGTM on Arti dependencies with pinned revision.Using a specific git revision for the Arti crates ensures reproducible builds. Since Arti is still evolving, pinning to a known-working commit (as mentioned in PR comments, this resolves the derive-deftly conflict) is appropriate.
Consider documenting in a code comment or README why this specific revision was chosen (e.g., "pinned to fix derive-deftly 1.5 compatibility") to help future maintainers understand if/when to update.
crates/notedeck/src/lib.rs (2)
39-39: LGTM on module organization.The
tormodule is correctly placed in its own module tree, following the codebase convention. The alphabetical ordering is maintained.
92-92: Public re-exports look appropriate.Re-exporting
TorManagerandTorStatusat the crate root follows the established pattern for other public types. Based on learnings, ensure thetormodule itself has adequate docstring coverage for the exported types.crates/notedeck_chrome/src/chrome.rs (6)
68-73: LGTM on Tor color palette constants.The color constants follow a clear naming convention and appear to match the Tor brand palette. Using module-level constants is appropriate for these static values.
270-272: LGTM on Tor status retrieval placement.The status check and indicator display are correctly placed after app action processing, ensuring the indicator reflects the current state. The
status()call appears to be a non-blocking read.
891-901: LGTM on indicator styling types.The
TorIndicatorStylestruct cleanly encapsulates all visual properties, and using an enum forOverlayStyleallows future extensibility (e.g., adding pulsing animation forStartingstate).
903-916: LGTM on color blending implementation.The linear interpolation with proper clamping is correct. Using
from_rgba_unmultipliedis appropriate since the input colors are in unmultiplied format.
991-992: Hiding the indicator forUnsupportedis reasonable.Returning
NoneforTorStatus::Unsupportedhides the badge entirely on platforms where Tor isn't available (Android, wasm). Per PR comments, consider providing guidance in Settings UI for alternatives (e.g., Orbot on Android) rather than silently hiding the feature.
995-1050: LGTM on onion icon painting.The procedural drawing creates a recognizable onion shape with proper layering. The geometry calculations using radius-based offsets ensure the icon scales correctly with the allocated rect.
crates/notedeck_columns/src/nav.rs (3)
41-44: LGTM on Tor import addition.The
TorManagerimport from thenotedeckcrate is appropriate for integrating Tor status and support checks into the navigation and settings flow.
530-537: LGTM on settings action processing.The additional parameters (
ctx.torandui.ctx()) passed toprocess_settings_actioncorrectly propagate Tor manager context, enabling the settings handler to manage Tor state. This follows the coding guideline of usingAppContextto access services.
646-655: LGTM on SettingsView Tor integration.Passing
ctx.tor.status()andTorManager::is_supported()toSettingsViewenables proper conditional UI rendering based on platform support and current Tor state.crates/notedeck/src/persist/settings_handler.rs (1)
197-207: LGTM on Tor setting accessors.The
set_use_toranduse_tormethods follow the established pattern of other settings accessors. The setter persists viatry_save_settings()which usesTimedSerializer::try_saveto avoid blocking the frame, consistent with coding guidelines.vendor/ewebsock/src/tungstenite_common.rs (2)
1-17: LGTM on WebSocketConfig conversion.The
From<Options>implementation correctly mapsmax_incoming_frame_sizeto tungstenite'smax_frame_size, with proper handling ofusize::MAXas unbounded (None). Other options fields are appropriately handled elsewhere in the connection flow.
19-32: LGTM on request builder helper.The
into_requesterfunction cleanly constructs aClientRequestBuilderwith headers and subprotocols from options. The builder pattern with chainedwith_headerandwith_sub_protocolcalls is idiomatic.crates/enostr/src/relay/mod.rs (3)
151-157: LGTM on Options field addition.Adding the
optionsfield toRelayenables dynamic transport configuration, which is essential for the Tor routing feature.
183-200: LGTM on Relay::new signature extension.The extended signature properly accepts and stores
Options, cloning for the connection call sinceconnect_with_wakeuptakes ownership. This design allows the relay to reconnect with the same options later.
218-225: LGTM on connect using stored options.Using
self.options.clone()ensures reconnections use the current configuration, enabling transport changes to take effect on reconnect.crates/enostr/src/relay/pool.rs (4)
10-18: LGTM on imports.The conditional imports for
TransportandArcare correctly gated for platforms that support Tor (non-wasm32, non-android).
128-133: LGTM on ws_options field.The
ws_optionsfield centralizes WebSocket configuration, enabling consistent options across all relays in the pool.
322-326: LGTM on options propagation.New relays are created with the pool's current
ws_options, ensuring they inherit the configured transport (direct or SOCKS proxy).
350-366: Partial relay reconnection failures are silently logged.When
configure_transportis called (e.g., enabling Tor), relays that fail to reconnect are logged but the method continues. This is resilient but could leave users unaware that some relays are still using direct connections.Per PR reviewer comments, consider whether partial failures should be surfaced to the UI or whether the status should indicate "some relays failed to switch transport."
The current behavior is acceptable for an initial implementation, but verify this aligns with the desired UX for transport switching. Consider whether
configure_transportshould return a result indicating partial success/failure.vendor/ewebsock/src/native_tungstenite_tokio.rs (1)
39-43: LGTM!The
forgetimplementation correctly prevents the connection from being closed when the sender is dropped, matching the pattern innative_tungstenite.rs.vendor/ewebsock/src/web.rs (2)
91-148: LGTM!The callback handling follows standard WASM patterns with proper use of
.forget()to keep closures alive. The blob-to-ArrayBuffer conversion via FileReader is a reasonable approach for handling binary data.
193-199: LGTM!The
close_sockethelper properly handles errors and provides appropriate logging.crates/notedeck/src/tor.rs (1)
110-138: LGTM!The
poll()implementation correctly usestry_recv()for non-blocking status updates, which is appropriate for the UI render loop. Status transitions are handled properly.crates/notedeck_columns/src/ui/settings.rs (2)
499-563: LGTM!The
network_sectionimplementation follows egui idioms with proper layout, usestr!macros for all user-facing strings, and cleanly handles the unsupported case with an early return.
136-153: LGTM!The
SettingsViewconstructor properly accepts and stores the new Tor-related fields.crates/notedeck/src/app.rs (5)
133-136: LGTM!Polling Tor status every frame with
tor.poll()is appropriate since it uses non-blockingtry_recv(). The transport update is correctly cfg-gated for supported platforms.
218-226: LGTM!Tor initialization properly checks
is_supported()and the user setting before enabling, with appropriate error handling and setting revert on failure.
384-405: LGTM!The
app_context()method correctly exposesTorManagerviaAppContext, following the established pattern for service access.
86-88: LGTM!The new
torandws_transportfields are correctly added with appropriate cfg-gating for platform support.
431-449: This review comment is incorrect. Theconfigure_transportimplementation atcrates/enostr/src/relay/pool.rs:351-366explicitly reconnects all existing relay connections when the transport is updated. It iterates through all relays, callsupdate_options()with the new transport settings, and then callsconnect()to reconnect each relay with the new configuration. The log message"failed to reconnect relay"confirms reconnection is intentional.Likely an incorrect or invalid review comment.
vendor/ewebsock/src/lib.rs (1)
1-281: Well-structured cross-platform WebSocket API.The vendored library provides a clean abstraction over platform-specific WebSocket implementations with proper conditional compilation for native vs. WASM targets. The API surface is well-documented, and the SOCKS proxy support through the
Transportenum enables the Tor routing feature cleanly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| let control = on_event(WsEvent::Opened); | ||
| if control.is_break() { | ||
| log::warn!("ControlFlow::Break not implemented for the tungstenite tokio backend"); | ||
| } |
There was a problem hiding this comment.
ControlFlow::Break is ignored; consider implementing or documenting the limitation.
The on_event callback can return ControlFlow::Break but the code only logs a warning and continues. The reader's async {} return (line 121) means it never actually breaks out of the loop. If this is intentional, consider returning Result<(), ()> from the callback instead to avoid API confusion.
Also applies to: 118-121
🤖 Prompt for AI Agents
In `@vendor/ewebsock/src/native_tungstenite_tokio.rs` around lines 80 - 83, The
on_event callback return value (used at the WsEvent::Opened call and the reader
closure) currently ignores ControlFlow::Break and merely logs a warning; update
the logic so ControlFlow::Break is honored: have the on_event(...) result be
checked and, if it indicates Break, break out of the reader loop / return from
the async reader task immediately instead of continuing, or change the callback
signature to return Result<(), E> (or bool) and propagate that result so callers
can stop processing; adjust handling around the on_event(WsEvent::Opened) call
and the reader async closure so the Break/Err path causes an immediate exit
rather than continuing.
vendor/ewebsock/src/web.rs
Outdated
| pub fn send(&mut self, msg: WsMessage) { | ||
| if let Some(socket) = &mut self.socket { | ||
| let result = match msg { | ||
| WsMessage::Binary(data) => { | ||
| socket.set_binary_type(web_sys::BinaryType::Blob); | ||
| socket.send_with_u8_array(&data) | ||
| } | ||
| WsMessage::Text(text) => socket.send_with_str(&text), | ||
| unknown => { | ||
| panic!("Don't know how to send message: {unknown:?}"); | ||
| } | ||
| }; | ||
| if let Err(err) = result.map_err(string_from_js_value) { | ||
| log::error!("Failed to send: {err:?}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ping and Pong messages are not handled and will panic.
The send() method only handles Binary and Text, but panics on Ping, Pong, and Unknown. The native implementations support Ping/Pong. Consider handling these or documenting the limitation:
♻️ Suggested handling
WsMessage::Binary(data) => {
socket.set_binary_type(web_sys::BinaryType::Blob);
socket.send_with_u8_array(&data)
}
WsMessage::Text(text) => socket.send_with_str(&text),
+ WsMessage::Ping(_) | WsMessage::Pong(_) => {
+ log::debug!("Ping/Pong not supported in WASM WebSocket");
+ return;
+ }
unknown => {
panic!("Don't know how to send message: {unknown:?}");
}🤖 Prompt for AI Agents
In `@vendor/ewebsock/src/web.rs` around lines 32 - 47, The send() method in web.rs
currently panics for WsMessage::Ping, WsMessage::Pong and other unknown
variants; update the match in send() (inside the send method) to handle
WsMessage::Ping and WsMessage::Pong instead of falling through to the panic: add
explicit arms for WsMessage::Ping and WsMessage::Pong that either translate them
to an appropriate send call on socket (e.g., send a small binary/frame or text
payload if your websocket backend requires) or safely no-op/log the event with
log::debug!/log::warn! so they don't panic, and add a final arm for
WsMessage::Unknown that logs and returns an error instead of panicking;
reference the send function and the WsMessage enum variants when making the
change.
f00bea3 to
c3176be
Compare
|
MacOS |
android
|
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java`:
- Line 14: DEFAULT_SOCKS_PORT is hardcoded to 9150 which may collide with Tor
Browser; modify TorManager to detect and handle port conflicts in
startSocksProxy: instead of always using DEFAULT_SOCKS_PORT, probe for an
available port (e.g., try binding a ServerSocket to ports starting at 9150 and
incrementing) or accept a configurable port parameter, and if the chosen port is
already in use throw/log a clear, non-silent error that includes the conflicting
port and suggestion to pick another port; update references to
DEFAULT_SOCKS_PORT and the startSocksProxy method to use the resolved available
port or the provided parameter.
In `@crates/notedeck_columns/src/ui/settings.rs`:
- Around line 101-117: The UI mutates Settings.use_tor immediately via the
toggle_value widget causing a desync if tor.set_enabled(...) (handled by the
ToggleTor action) fails; change the UI to not directly mutate the Settings
struct on click but instead: keep the toggle's displayed state bound to
settings.use_tor, emit the ToggleTor action when clicked (do not call
settings.set_use_tor() from the widget), and in the ToggleTor handler (where
tor.set_enabled(enabled) is called) update settings.set_use_tor(enabled) only on
Ok, and on Err explicitly revert any UI-visible state if needed (e.g., re-emit a
state update or call settings.set_use_tor(previous_value>) so the widget
reflects the real state; refer to the ToggleTor enum variant, tor.set_enabled,
settings.set_use_tor, the toggle_value widget, the Settings struct and the
SettingsHandler when applying the change.
In `@crates/notedeck/Cargo.toml`:
- Around line 57-58: The keyring dependency causes WASM/Android build failures;
move the keyring = { workspace = true } entry out of the main [dependencies] and
into a platform-gated section such as [target.'cfg(all(not(target_arch =
"wasm32"), not(target_os = "android")))'.dependencies] so it is only compiled
for non-wasm/non-android targets, or replace it with a WASM-compatible
implementation and conditionally #[cfg(...)] use that in code paths like app.rs,
media/network.rs, and tor.rs.
In `@crates/notedeck/src/media/network.rs`:
- Around line 297-301: The status code parsing silently falls back to 0 on
malformed status lines (status_line -> status_code) which bypasses the redirect
check and can hide errors; update the parser in network.rs to treat parse
failures as an error instead of using unwrap_or(0): attempt to extract and parse
the status token from status_line, and if missing or parse fails
return/propagate an appropriate Err (or Result::Err) from the surrounding
function (rather than assigning 0), so callers can handle malformed responses
and the redirect check ((300..400).contains(&status_code)) won't operate on an
invalid value.
- Around line 39-46: The comment on http_fetch is misleading: http_req actually
returns Unsupported on wasm32/android so HTTP is entirely unsupported there (not
just SOCKS proxy). Update the comment above pub async fn http_fetch(...) to
state that HTTP requests are not supported on wasm32/android (e.g., "HTTP
requests are not supported on these platforms; http_req returns Unsupported") or
else implement a platform-specific alternative using native APIs; reference the
http_fetch wrapper and the http_req implementation to ensure the comment or
alternative accurately reflects that full HTTP is unsupported on these targets.
In `@tools/arti-build/build-arti.sh`:
- Around line 124-127: The STRIP path logic in build-arti.sh only checks
darwin-x86_64 and linux-x86_64 and misses Apple Silicon; update the STRIP
resolution (the STRIP variable assignment and its subsequent if fallback) to
also test the darwin-aarch64 (or darwin-arm64 if your NDK uses that naming)
prebuilt path under $ANDROID_NDK_HOME/toolchains/llvm/prebuilt before falling
back to the linux path so llvm-strip is found on M1/M2 Macs.
🧹 Nitpick comments (18)
crates/enostr/src/relay/pool.rs (1)
350-366: Consider returning reconnection results for better error visibility.The current implementation logs reconnection failures but doesn't inform the caller which relays failed. This aligns with the PR comment concern about "transport switch leaves existing relay sockets direct"—if
connect()fails, that relay remains on the old transport.Consider returning a summary of failures or at least a boolean indicating partial success:
♻️ Suggested enhancement
#[cfg(not(target_arch = "wasm32"))] pub fn configure_transport( &mut self, transport: Transport, wakeup: Arc<dyn Fn() + Send + Sync>, -) { +) -> Vec<String> { + let mut failed_relays = Vec::new(); self.ws_options.transport = transport; for relay in &mut self.relays { if let PoolRelay::Websocket(wsr) = relay { wsr.relay.update_options(self.ws_options.clone()); let wake = wakeup.clone(); if let Err(err) = wsr.relay.connect(move || (*wake)()) { error!("failed to reconnect relay {}: {err}", wsr.relay.url); + failed_relays.push(wsr.relay.url.to_string()); } } } + failed_relays }This would allow the UI layer to show which relays couldn't switch to Tor, addressing the reviewer's concern about toggle desync.
crates/notedeck_chrome/android/app/build.gradle (1)
23-25: Consider enabling minification for release builds.
minifyEnabled falsein release builds increases APK size and exposes more code to reverse engineering. Unless there's a specific reason (e.g., debugging production issues), consider enabling it:release { - minifyEnabled false + minifyEnabled true + proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro' }crates/notedeck/src/jni_cache.rs (1)
18-33: Global state used for JNI cache - document the exception.The coding guidelines state "Global variables are not allowed in this codebase." While this
static JNI_CACHEis necessary for JNI interop (Java needs a stable entry point to call into Rust), consider adding a module-level doc comment explicitly documenting why this is an exception to the rule.The manual
Send + Syncimplementations appear correct based on the safety justification, but this is a sensitive pattern.Based on learnings, global variables are prohibited. Please confirm this JNI use case is an acceptable exception, or consider whether the cache could be managed differently (e.g., passed through JNI calls).
tools/arti-build/src/lib.rs (2)
497-500: Consider explicit shutdown for cleaner connection termination.When one direction of the bidirectional copy completes, the other direction is abruptly dropped. For cleaner TCP termination, consider explicitly shutting down the write half of the remaining connection.
Optional improvement for graceful shutdown
tokio::select! { - _ = client_to_tor => {} - _ = tor_to_client => {} + result = client_to_tor => { + if result.is_ok() { + let _ = tor_write.shutdown().await; + } + } + result = tor_to_client => { + if result.is_ok() { + let _ = client_write.shutdown().await; + } + } }
58-78: Nested lock acquisition in log_to_java could deadlock.The
log_to_javafunction acquiresLOG_CALLBACKlock while it could potentially be called from code that already holds theSTATElock (e.g., during error handling in initialize). While the current call sites appear safe, this pattern is fragile.Consider using
try_lock()instead oflock()for the log callback to avoid potential deadlock:fn log_to_java(message: &str) { if let Some(callback_lock) = LOG_CALLBACK.get() { - if let Ok(callback_guard) = callback_lock.lock() { + if let Ok(callback_guard) = callback_lock.try_lock() {crates/notedeck/src/media/network.rs (2)
377-388: Silent failure on malformed chunk size.If the chunk size line is malformed (not valid hex),
unwrap_or(0)treats it as end-of-body. This could silently return an incomplete response instead of erroring.Consider returning an error on parse failure
- let chunk_size = usize::from_str_radix(size_line.trim(), 16).unwrap_or(0); - - if chunk_size == 0 { - break; - } + let size_str = size_line.trim(); + // Empty line or "0" indicates end of chunks + if size_str.is_empty() || size_str == "0" { + break; + } + let chunk_size = usize::from_str_radix(size_str, 16) + .map_err(|_| HyperHttpError::InvalidRedirectLocation)?; // Consider adding a dedicated error variant
198-267: Duplicate redirect handling logic in HTTPS/HTTP branches.The redirect handling code (lines 219-235 and 245-261) is nearly identical between the HTTPS and HTTP branches. Consider extracting the redirect handling into a helper or restructuring to reduce duplication.
This is a maintainability nit - the current code is correct and functional.
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorProvider.java (1)
1-57: Clean interface design for Tor provider abstraction.The interface is well-structured with clear Javadoc documentation. Consider adding Android's
@Nullableannotation togetSocksProxy()return type andsetLogCallback()parameter to make the nullability contract explicit for consumers.import androidx.annotation.Nullable; // ... `@Nullable` String getSocksProxy(); // ... void setLogCallback(`@Nullable` TorLogCallback callback);crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/NativeTorProvider.java (1)
60-75: Minor inconsistency:getSocksProxy()queries native instead of using cachedsocksPort.
startSocksProxy()stores the port insocksPortfield (line 37), butgetSocksProxy()queriesArtiNative.getSocksPort()instead of using the cached value. This is actually safer since it reflects the true native state, but the localsocksPortfield becomes partially redundant. Consider either:
- Removing the
socksPortfield entirely and always querying native, or- Using
socksPorthere for consistency (only if you trust the local state)The current approach (querying native) is the safer choice for correctness.
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java (2)
70-86: Directory creation failures are silently ignored.
mkdirs()can fail (permissions, full disk), but the return value is discarded. This may cause a confusing failure inprovider.initialize()downstream.♻️ Suggested improvement
// Create directories - cacheDir.mkdirs(); - stateDir.mkdirs(); + if (!cacheDir.mkdirs() && !cacheDir.exists()) { + Log.e(TAG, "Failed to create cache directory: " + cacheDir); + return false; + } + if (!stateDir.mkdirs() && !stateDir.exists()) { + Log.e(TAG, "Failed to create state directory: " + stateDir); + return false; + }
64-90:start()is not thread-safe.While
getInstance()is synchronized,start()is not. Concurrent calls could race on theinitializedflag, potentially callingprovider.initialize()twice.♻️ Consider synchronizing start/stop
- public boolean start(int socksPort) { + public synchronized boolean start(int socksPort) {Or use a more granular lock if synchronizing the entire method is too coarse.
crates/notedeck_chrome/src/chrome.rs (4)
920-930: Missing docstrings on new types.Per coding guidelines, new code should have docstring coverage. Consider adding brief documentation for
TorIndicatorStyleandOverlayStyle.📝 Suggested documentation
+/// Style configuration for the Tor status indicator badge. struct TorIndicatorStyle { + /// Primary badge color base: Color32, + /// Secondary/highlight color accent: Color32, + /// Color for the onion stem/sprout sprout: Color32, + /// Optional overlay (e.g., slash for disabled state) overlay: Option<OverlayStyle>, + /// Localized tooltip text tooltip: String, } +/// Visual overlay styles for the Tor indicator. enum OverlayStyle { + /// Diagonal slash overlay with specified color Slash(Color32), }
932-945: Consider usingColor32::lerpor add docstring.
egui::Color32has alerpmethod that does the same thing. If you prefer the custom implementation for clarity, please add a docstring explaining the function.
947-971: Indicator is hover-only; no click affordance.The badge shows a tooltip on hover but has no click handler for retry or detailed info. This was noted in PR comments as a potential UX gap. Consider whether a click action (e.g., navigate to settings, retry connection) would improve usability.
997-1008: Error tooltip exposes raw error string.The
TorStatus::Failed(err)branch interpolateserr.as_str()directly into the tooltip. While less visible than main UI text, consider whether internal Arti error strings are appropriate for end users.♻️ Alternative: generic error with logged details
TorStatus::Failed(err) => Some(TorIndicatorStyle { base: TOR_ALERT, accent: TOR_AMBER, sprout: TOR_AMBER, overlay: Some(OverlayStyle::Slash(Color32::from_rgb(250, 226, 226))), - tooltip: tr!( - i18n, - "Tor routing failed: {error}", - "Tooltip when Tor routing has failed", - error = err.as_str() - ), + tooltip: { + tracing::debug!("Tor failure details: {}", err.as_str()); + tr!( + i18n, + "Tor routing failed. Check settings to retry.", + "Tooltip when Tor routing has failed" + ) + }, }),crates/notedeck_columns/src/ui/settings.rs (1)
574-640: LGTM with minor suggestion: Network section implementation.Good use of
settings_group, proper localization withtr!, and clean status mapping. TheTorStatus::Failedhandling correctly logs details while showing a user-friendly message.One minor enhancement per PR comments: the
Unsupportedmessage could suggest alternatives like Orbot on Android.💡 Optional: Suggest alternatives for unsupported platforms
TorStatus::Unsupported => tr!( self.note_context.i18n, - "Tor is not available on this platform.", + "Tor is not available on this platform. Consider using Orbot or a system-wide VPN.", "Status label when Tor is unsupported" ),crates/notedeck/src/tor.rs (2)
513-515: Consider logging directory creation failures.Silently ignoring
create_dir_allerrors could mask permission issues or disk space problems. While the Java side also creates these directories (per theTorManager.javasnippet), consider at least logging warnings:♻️ Suggested improvement
// Create directories - let _ = std::fs::create_dir_all(&self.cache_dir); - let _ = std::fs::create_dir_all(&self.state_dir); + if let Err(e) = std::fs::create_dir_all(&self.cache_dir) { + tracing::warn!("Failed to create tor cache dir: {e}"); + } + if let Err(e) = std::fs::create_dir_all(&self.state_dir) { + tracing::warn!("Failed to create tor state dir: {e}"); + }
478-494: Potential status desync if Java-side Tor crashes after startup.The
poll()method only monitors state duringStarting. OnceRunningis reached, there's no health check to detect if the Java Tor runtime crashes or becomes unresponsive. The PR objectives highlight this "toggle desync" concern.Consider periodically verifying the SOCKS port remains accessible, or having the Java side expose a health/status method that can be polled:
♻️ Possible approach
pub fn poll(&mut self) { if self.jni_failed { return; } match &self.status { TorStatus::Starting => { if let Some(port) = self.get_socks_port() { if port > 0 { self.status = TorStatus::Running { socks_port: port as u16, }; } } } TorStatus::Running { .. } => { // Periodically verify Tor is still running if let Some(port) = self.get_socks_port() { if port == 0 { tracing::warn!("Tor appears to have stopped"); self.status = TorStatus::Failed("Tor stopped unexpectedly".to_string()); } } } _ => {} } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktools/arti-build/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.beads/issues.jsonl.cargo/config.toml.gitignoreCargo.tomlMakefilecrates/enostr/src/relay/mod.rscrates/enostr/src/relay/pool.rscrates/notedeck/Cargo.tomlcrates/notedeck/src/app.rscrates/notedeck/src/context.rscrates/notedeck/src/imgcache.rscrates/notedeck/src/jni_cache.rscrates/notedeck/src/lib.rscrates/notedeck/src/media/gif.rscrates/notedeck/src/media/network.rscrates/notedeck/src/media/static_imgs.rscrates/notedeck/src/persist/settings_handler.rscrates/notedeck/src/tor.rscrates/notedeck_chrome/android/app/build.gradlecrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/ArtiNative.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/NativeTorProvider.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/StubTorProvider.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorLogCallback.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorProvider.javacrates/notedeck_chrome/src/chrome.rscrates/notedeck_columns/src/nav.rscrates/notedeck_columns/src/ui/settings.rstools/arti-build/ARTI_VERSIONtools/arti-build/Cargo.tomltools/arti-build/build-arti.shtools/arti-build/src/lib.rs
✅ Files skipped from review due to trivial changes (3)
- tools/arti-build/ARTI_VERSION
- .beads/issues.jsonl
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/notedeck/src/persist/settings_handler.rs
- .cargo/config.toml
🧰 Additional context used
📓 Path-based instructions (4)
crates/notedeck*/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/notedeck*/src/**/*.rs: UseAppContextto access mutable handles to services (database, caches, relay pool, account state, localization, settings, wallet) rather than accessing global state
Never use Mutexes in UI paths; the render loop must never block. UseRc<RefCell<>>for single-threaded interior mutability,Arc<tokio::sync::RwLock<>>for cross-thread sharing, andpoll_promise::Promisefor async results
Wrap async work inpoll_promise::Promise, check withpromise.ready()orpromise.ready_mut()each frame—never block the render loop
Usetokio::sync::RwLockinstead ofArc<Mutex<>>for cross-thread sharing in Notedeck
Persist state viaTimedSerializer::try_saveto avoid blocking the frame; batch mutations withSettingsHandler::update_batch
Wrap user-facing strings withtr!ortr_plural!macros for localization and runpython3 scripts/export_source_strings.pyafter changing strings
UseJobPoolfor CPU-bound work and return results viatokio::sync::oneshotwrapped in Promises; usetokio::spawn()for network I/O and relay sync
Mark performance-critical functions with#[profiling::function]for visibility in the puffin profiler
Usetracingmacros for structured logging andprofilingscopes where hot paths exist
Prefer early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers ofifstatements (Nevernesting principle)
Global variables are not allowed in this codebase, even if they are thread local; state should be managed in a struct that is passed in as reference
Ensure docstring coverage for any code added or modified
Avoid Mutexes in Notedeck code; preferpoll_promise::Promisefor async results,Rc<RefCell<>>for single-threaded interior mutability, ortokio::sync::RwLockwhen cross-thread sharing is truly necessary
Never block the render loop; usePromise::ready()for non-blocking result checks. Offload CPU-heavy work to...
Files:
crates/notedeck/src/context.rscrates/notedeck/src/imgcache.rscrates/notedeck/src/media/gif.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/lib.rscrates/notedeck/src/tor.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/media/static_imgs.rscrates/notedeck/src/jni_cache.rscrates/notedeck/src/media/network.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rs
crates/notedeck*/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use strict Rust 2021 edition; ensure edition-lints are strict and clippy
disallowed_methodsis denied at crate root to enforce API hygiene
Files:
crates/notedeck/src/lib.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Do not vendor code; in Cargo.toml replace the existing URL with the fork that includes the new code. If vendoring is absolutely necessary, present the case why no other options are feasible
Files:
Cargo.toml
crates/notedeck*/src/app.rs
📄 CodeRabbit inference engine (AGENTS.md)
Implement the
Apptrait withupdate(&mut self, &mut AppContext, &mut egui::Ui) -> AppResponseto drive egui rendering and signal high-level actions
Files:
crates/notedeck/src/app.rs
🧠 Learnings (18)
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `AppContext` to access mutable handles to services (database, caches, relay pool, account state, localization, settings, wallet) rather than accessing global state
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/app.rs : Implement the `App` trait with `update(&mut self, &mut AppContext, &mut egui::Ui) -> AppResponse` to drive egui rendering and signal high-level actions
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `tokio::sync::RwLock` instead of `Arc<Mutex<>>` for cross-thread sharing in Notedeck
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/lib.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Global variables are not allowed in this codebase, even if they are thread local; state should be managed in a struct that is passed in as reference
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Embrace egui idioms in UI code: builder chains, closures returning `Response`, `ui.vertical`/`horizontal` for layout
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck_columns/src/nav.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/lib.rs : Use strict Rust 2021 edition; ensure edition-lints are strict and clippy `disallowed_methods` is denied at crate root to enforce API hygiene
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/lib.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Never use Mutexes in UI paths; the render loop must never block. Use `Rc<RefCell<>>` for single-threaded interior mutability, `Arc<tokio::sync::RwLock<>>` for cross-thread sharing, and `poll_promise::Promise` for async results
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `tracing` macros for structured logging and `profiling` scopes where hot paths exist
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Persist state via `TimedSerializer::try_save` to avoid blocking the frame; batch mutations with `SettingsHandler::update_batch`
Applied to files:
crates/notedeck/src/context.rscrates/notedeck_columns/src/ui/settings.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Prefer module-level organization over monolithic files; each feature (accounts, decks, timelines, media) should live in its own module tree
Applied to files:
crates/notedeck/src/context.rscrates/notedeck/src/lib.rscrates/notedeck/src/app.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : For animations (GIFs, video), track `repaint_at` timestamps and only request repaints when necessary; avoid spinning every frame
Applied to files:
crates/notedeck/src/imgcache.rscrates/notedeck/src/media/gif.rscrates/notedeck_chrome/src/chrome.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Ensure docstring coverage for any code added or modified
Applied to files:
crates/notedeck_chrome/src/chrome.rscrates/notedeck/src/tor.rscrates/notedeck/src/app.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Avoid Mutexes in Notedeck code; prefer `poll_promise::Promise` for async results, `Rc<RefCell<>>` for single-threaded interior mutability, or `tokio::sync::RwLock` when cross-thread sharing is truly necessary
Applied to files:
crates/notedeck/src/lib.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Tests should live alongside modules (e.g., in test submodules), often using `#[tokio::test]` when async behavior is involved
Applied to files:
crates/notedeck/src/lib.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Never block the render loop; use `Promise::ready()` for non-blocking result checks. Offload CPU-heavy work to `JobPool` or `tokio::spawn()`, returning results via channels or Promises
Applied to files:
crates/notedeck/src/tor.rscrates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Wrap user-facing strings with `tr!` or `tr_plural!` macros for localization and run `python3 scripts/export_source_strings.py` after changing strings
Applied to files:
crates/notedeck_columns/src/ui/settings.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to Cargo.toml : Do not vendor code; in Cargo.toml replace the existing URL with the fork that includes the new code. If vendoring is absolutely necessary, present the case why no other options are feasible
Applied to files:
Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `JobPool` for CPU-bound work and return results via `tokio::sync::oneshot` wrapped in Promises; use `tokio::spawn()` for network I/O and relay sync
Applied to files:
crates/notedeck/src/app.rscrates/enostr/src/relay/pool.rs
🧬 Code graph analysis (10)
crates/notedeck/src/context.rs (1)
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java (1)
TorManager(12-134)
crates/notedeck/src/imgcache.rs (2)
crates/notedeck/src/media/gif.rs (1)
set_http_config(129-131)crates/notedeck/src/media/static_imgs.rs (1)
set_http_config(47-49)
crates/notedeck/src/media/gif.rs (2)
crates/notedeck/src/imgcache.rs (5)
new(37-47)new(114-139)new(290-300)set_http_config(50-53)set_http_config(308-310)crates/notedeck/src/media/static_imgs.rs (2)
new(38-44)set_http_config(47-49)
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/NativeTorProvider.java (1)
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/ArtiNative.java (1)
ArtiNative(6-83)
crates/notedeck/src/tor.rs (3)
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java (1)
TorManager(12-134)crates/notedeck/src/app.rs (1)
new(198-360)crates/notedeck/src/jni_cache.rs (2)
with_jni(152-166)find_class(106-148)
crates/notedeck_columns/src/ui/settings.rs (2)
crates/notedeck/src/tor.rs (3)
is_supported(128-130)is_supported(380-382)is_supported(452-455)crates/notedeck/src/ui.rs (1)
richtext_small(5-10)
crates/notedeck/src/media/static_imgs.rs (2)
crates/notedeck/src/imgcache.rs (5)
new(37-47)new(114-139)new(290-300)set_http_config(50-53)set_http_config(308-310)crates/notedeck/src/media/gif.rs (2)
new(120-126)set_http_config(129-131)
tools/arti-build/src/lib.rs (1)
crates/notedeck/src/jobs/job_pool.rs (1)
oneshot(52-52)
crates/notedeck/src/app.rs (1)
crates/notedeck/src/tor.rs (6)
is_supported(128-130)is_supported(380-382)is_supported(452-455)socks_proxy(153-159)socks_proxy(392-394)socks_proxy(470-476)
crates/enostr/src/relay/mod.rs (1)
crates/enostr/src/relay/pool.rs (2)
new(114-121)new(143-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (Windows) / run
- GitHub Check: Test (Linux) / run
- GitHub Check: Test (macOS) / run
- GitHub Check: Rustfmt + Clippy
- GitHub Check: Check (android)
🔇 Additional comments (63)
crates/enostr/src/relay/mod.rs (2)
156-156: LGTM! Clean integration of connection options.The
Optionsfield is correctly stored on theRelaystruct and propagated throughnew(). Cloning on connect is appropriate sinceewebsock::connect_with_wakeuptakes ownership.Also applies to: 184-199
218-234: LGTM! Proper separation of option update and reconnection.The
connectmethod correctly uses the stored options, andupdate_optionsappropriately only updates the configuration without triggering reconnection—giving callers explicit control over when to reconnect.crates/enostr/src/relay/pool.rs (3)
10-18: LGTM! Appropriate conditional imports.The
TransportandArcimports are correctly gated for non-wasm32 targets, aligning with the platform-specific Tor support.
128-133: LGTM! Clean field addition and initialization.The
ws_optionsfield is properly encapsulated as private and initialized with sensible defaults.Also applies to: 141-150
322-326: LGTM! New relays inherit pool's transport configuration.Passing
self.ws_options.clone()ensures newly added relays use the current transport settings (e.g., SOCKS proxy for Tor).tools/arti-build/Cargo.toml (1)
1-44: LGTM! Well-structured Android Arti build crate.The crate is properly configured for building a cdylib targeting Android:
- Git dependencies use the official Arti repository with a pinned revision matching the workspace
- Feature selection (rustls over native-tls) avoids OpenSSL cross-compilation complexity
- Release profile is appropriately optimized for mobile binary size
Cargo.toml (2)
106-110: TLS backend inconsistency between desktop and Android builds.Line 110 specifies
native-tlsfortor-rtcompat(desktop), whiletools/arti-build/Cargo.tomlusesrustlsfor Android. This is likely intentional (avoiding OpenSSL cross-compilation on Android), but worth documenting to prevent confusion during maintenance.Additionally, verify that both TLS backends are tested, as they may have different certificate validation behaviors.
140-141: ewebsock fork usage is now properly configured.The dependency correctly uses a git-based fork reference instead of vendoring, addressing the previous review concern. Ensure this fork is maintained or upstream changes are periodically merged.
crates/notedeck/Cargo.toml (1)
57-73: Target-specific Tor dependencies correctly configured.The platform gating (
not(wasm32), not(android)) properly restricts full Arti runtime to desktop platforms, aligning with the PR's design where Android/WASM use stub modules.Makefile (2)
46-50: Well-structured Tor-enabled Android build target.The
android-tortarget correctly:
- Chains
arti-arm64andjnidependencies- Adds appropriate logcat filters for Tor-related logging (
NativeTorProvider,TorManager)- Maintains consistency with the non-Tor
androidtarget
17-19: The target namearti-arm64is accurate and not misleading. The--releaseflag inbuild-arti.shexplicitly restricts the build to ARM64 only (line 12 and lines 32-35 showRELEASE_TARGETScontains onlyarm64-v8a:aarch64-linux-android). No changes needed.Likely an incorrect or invalid review comment.
tools/arti-build/build-arti.sh (1)
1-56: LGTM!The script structure, argument parsing, and prerequisite checks are well-implemented with proper error handling via
set -eand clear usage documentation.crates/notedeck/src/jni_cache.rs (2)
106-148: LGTM!The
find_classimplementation correctly handles:
- Exception clearing before and after JNI calls
- Local reference creation for thread safety
- Proper error propagation with descriptive messages
150-176: LGTM!The
with_jnihelper correctly attaches the current thread to the JVM, and the non-Android stubs provide appropriate no-op behavior for cross-platform compilation.tools/arti-build/src/lib.rs (1)
220-247: Stop function doesn't clear the Tor client.The
stopfunction clearsproxy_task,shutdown_tx, andsocks_port, but leavesclientintact. This meansisInitialized()will still returntrueafter stop, and subsequentstartSocksProxycalls will reuse the old client. Verify this is the intended behavior.If the intent is to allow restart without re-bootstrap, this is fine. If not, consider also clearing the client:
state.socks_port = None; + // Optionally clear client to force re-initialization + // state.client = None; log_to_java("SOCKS proxy stopped");crates/notedeck/src/media/network.rs (1)
156-181: LGTM on URL parsing and path handling.The URL parsing correctly handles query strings and empty paths, defaulting to "/" when needed.
crates/notedeck/src/context.rs (1)
33-33: LGTM!Adding
TorManagertoAppContextfollows the established pattern for accessing services through the context struct, consistent with the coding guidelines. This ensures Tor state is managed through the proper service architecture rather than global state.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.java (2)
34-36: LGTM!The native method declaration for JNI classloader cache initialization is properly defined as static and private, which is appropriate for internal JNI bridging.
166-175: LGTM!The JNI classloader initialization is correctly placed after
super.onCreate()(which loads the native library) and wrapped with appropriate error handling. TheUnsatisfiedLinkErrorcatch ensures the app doesn't crash if the native method isn't available, which aligns with the PR's note that Android uses a stub module.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorLogCallback.java (1)
1-13: LGTM!Clean, well-documented callback interface following Java conventions. The single-method design is appropriate for log forwarding from the native Tor layer.
crates/notedeck/src/imgcache.rs (3)
4-4: LGTM!Import correctly added to support HTTP configuration propagation.
48-53: LGTM!The
set_http_configmethod correctly propagates the HTTP configuration to both static and animated image caches. Theclone()on the first call is appropriate since the config needs to be passed to both caches. Good docstring coverage.
307-310: LGTM!Clean delegation to the underlying
TexturesCache, with appropriate documentation. This provides the expected API surface for configuring HTTP behavior (e.g., SOCKS proxy for Tor) on theImagesfacade.crates/notedeck/src/lib.rs (4)
16-16: LGTM!JNI cache module correctly declared as public for cross-crate access from the Android layer.
40-40: LGTM!Tor module correctly declared as public, enabling Tor integration across the crate boundary.
68-69: LGTM!
HttpConfigis appropriately re-exported alongside other media types, enabling consumers to configure HTTP behavior (e.g., SOCKS proxy for Tor routing) without deep module imports.
94-94: LGTM!
TorManagerandTorStatusare correctly re-exported at the crate root, making the Tor integration types accessible to dependent crates (notedeck_chrome,notedeck_columns).crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/StubTorProvider.java (1)
1-48: LGTM!Clean null object pattern implementation that allows the codebase to gracefully handle missing native Tor support. All methods consistently return appropriate "not available" values (
false,null), and the class is well-documented.Per the PR reviewer comments, consider providing user-facing guidance (e.g., suggesting Orbot as an alternative) when
isSupported()returnsfalseon Android. This is a UX improvement that could be addressed in follow-up work rather than blocking this PR.crates/notedeck/src/media/gif.rs (2)
116-131: LGTM! HTTP config integration follows established patterns.The
http_configfield and setter align with the pattern instatic_imgs.rs. The implementation correctly propagates HTTP configuration (including SOCKS proxy for Tor) through the async network fetch path without blocking the render loop.
216-231: Async fetch correctly integrates SOCKS proxy configuration.The
http_fetchcall properly extracts the SOCKS proxy from config. One minor note: the error message at line 228 may expose internal HTTP error details to users if surfaced in UI. Consider whether this error propagates to user-visible contexts.crates/notedeck_columns/src/nav.rs (3)
40-43: TorManager correctly imported and accessed via AppContext.The import and usage pattern follows the coding guidelines by accessing Tor state through
AppContextrather than global state.
589-597: Settings action handler correctly wired with Tor context.The
ctx.torreference is appropriately passed toprocess_settings_action, enabling Tor toggle functionality in settings.
711-719: SettingsView receives Tor status for UI rendering.The view construction correctly receives both runtime status (
ctx.tor.status()) and platform capability (TorManager::is_supported()), enabling appropriate UI presentation for Tor toggle state.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/ArtiNative.java (2)
6-18: JNI library loading with proper error capture.The static initializer correctly handles
UnsatisfiedLinkErrorand captures the error message for diagnostic purposes. This pattern allows graceful degradation when the native library isn't available.
56-61: Note:getSocksPort()returns int, converted to String in provider.The native method returns
int(or -1 if not running), whileTorProvider.getSocksProxy()returnsString. The conversion happens inNativeTorProvider.getSocksProxy()(lines 66-70), which is appropriate separation of concerns.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/NativeTorProvider.java (2)
8-11: Implementation setup looks good.The TAG constant and socksPort tracking field are appropriately scoped.
12-44: Defensive programming with library guards and exception handling.All methods properly check
ArtiNative.isLibraryLoaded()before invoking native methods, and wrap calls in try-catch blocks. This ensures graceful handling of native library failures.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java (1)
92-134: LGTM: Clean delegation pattern.The remaining methods correctly delegate to the provider. Good documentation coverage.
crates/notedeck_chrome/src/chrome.rs (3)
75-80: LGTM: Tor color palette constants.The color constants follow the Tor visual identity (purple, lavender, etc.) and provide clear semantic naming for different states.
1024-1079: LGTM: Custom onion icon painting.The vector drawing logic for the Tor onion badge is well-structured with clear geometric calculations. The overlay slash provides good visual feedback for disabled/error states.
278-280: LGTM: Integration point for Tor indicator.The indicator is correctly rendered after the app content update, using
AppContextto access Tor status as per coding guidelines.crates/notedeck/src/media/static_imgs.rs (4)
25-29: LGTM: HttpConfig struct for proxy configuration.Clean, minimal struct for HTTP configuration.
CloneandDefaultderives are appropriate for the use case.
46-49: LGTM: HttpConfig setter with documentation.The setter follows the same pattern as
AnimatedImgTexCache::set_http_config(per gif.rs snippets), maintaining consistency across the codebase.
97-114: LGTM: Config propagation to async fetch.The
http_configis correctly cloned before being moved into the async closure, avoiding borrow checker issues while ensuring the fetch uses the current configuration.
146-161: LGTM: SOCKS proxy integration in network fetch.The proxy address is correctly passed to
http_fetch. Usingas_deref()is idiomatic for convertingOption<String>toOption<&str>.crates/notedeck_columns/src/ui/settings.rs (3)
38-38: LGTM: New SettingsAction variant for Tor toggle.The
ToggleTor(bool)variant is appropriately added to handle the network setting change.
123-129: LGTM: SettingsView extended with Tor state.The new fields cleanly separate the current status (for display) from the support flag (for conditional rendering).
823-828: LGTM: Network section integrated into settings UI.The section is correctly placed in the UI flow with consistent spacing.
crates/notedeck/src/app.rs (7)
30-33: LGTM: Platform-gated imports for transport types.Correct use of
cfgattributes to importWsTransportandSocksOptionsonly on non-wasm32 targets.
89-91: LGTM: Tor fields added to Notedeck struct.The
TorManagerandWsTransportfields are appropriately added with correct platform gating.
230-241: LGTM: Tor initialization with error recovery.Good pattern: attempt to enable Tor based on persisted settings, and revert the setting if initialization fails. This prevents startup loops where Tor always fails but settings say it should be enabled.
481-493: LGTM: Platform-specific crypto provider installation.Using ring on desktop for rustls is standard. The no-op on Android/WASM is acceptable since those platforms have their own crypto implementations or don't need TLS for this use case.
405-427: LGTM: TorManager exposed via AppContext.Per coding guidelines, services should be accessed through
AppContext. Addingtor: &mut self.torfollows this pattern correctly.
453-478: No changes needed. The transport switch properly reconnects existing relay sockets by callingconnect()on each relay after updating transport options. New WebSocket connections are established that use the updated transport configuration, replacing the old connections.
143-146: No issue found—tor.poll()is non-blocking.All platform-specific implementations use only non-blocking operations: the non-wasm32 variant checks a channel with
try_recv(), the wasm32 variant is an empty stub, and the Android variant performs a simple JNI state query viagetSocksPort(). Calling from the render loop is safe.crates/notedeck/src/tor.rs (8)
1-35: LGTM! Well-structured imports and constants with appropriate documentation.The use of
std::sync::mpscwithtry_recv()in the poll method is a good choice for non-blocking status checks from the UI thread. TheReadyStateenum cleanly encapsulates bootstrap completion signaling.
58-94: LGTM! Shutdown timeout properly implemented.The
stop()method correctly handles the shutdown timeout case, and theDropimplementation ensures cleanup. The abandoned thread will eventually terminate when the Arti runtime shuts down, which is an acceptable tradeoff to prevent blocking the main thread indefinitely.
96-125: LGTM! Clean type definitions with appropriate encapsulation.
TorStatusisClone + Debugfor easy UI consumption, whileTorManagercorrectly avoidsClonesince it owns a thread handle. NoMutexusage aligns with the coding guidelines.
161-193: LGTM! Non-blocking poll implementation follows UI guidelines.The
poll()method correctly usestry_recv()for non-blocking status checks, which aligns with the "never block the render loop" coding guideline. All channel states (Ok, Err, Empty, Disconnected) are properly handled with appropriate status transitions and cleanup.
285-301: Port verification before ready signal is well implemented.The
tokio::select!withbiasedcorrectly prioritizes proxy errors, and the port verification addresses the past review concern about premature ready signaling.Consider: The 100ms delay before verification may be tight on heavily loaded systems. If flaky startup reports emerge, consider adding a small retry loop or increasing the delay.
360-401: LGTM! Minimal wasm32 stub with compatible API surface.The stub correctly returns
Unsupportedstatus andis_supported() = false, allowing calling code to handle platform differences gracefully.
657-658: LGTM! Clean public API re-exports.The consistent API surface across all platform implementations allows consuming code to work uniformly without platform-specific conditionals.
336-345: Clarify whether stream isolation is required.The
build_client_config()function configures only storage directories and uses Arti's default stream isolation behavior, which allows circuits to be shared across streams. If the PR objectives or security requirements call for stream isolation (to prevent shared circuits and relay correlation), explicit configuration is needed: eitherTorClient::isolated_client()orStreamPrefswith anIsolationToken. Confirm whether this is a requirement before implementing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java
Outdated
Show resolved
Hide resolved
| let status_code: u16 = status_line | ||
| .split_whitespace() | ||
| .nth(1) | ||
| .and_then(|s| s.parse().ok()) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
Status code parse failure returns 0, which is not handled.
If the status line is malformed, unwrap_or(0) sets status_code to 0. This value doesn't match the redirect check (300..400).contains(&status_code) but could cause unexpected behavior. Consider returning an error for malformed responses.
Proposed fix
let status_code: u16 = status_line
.split_whitespace()
.nth(1)
.and_then(|s| s.parse().ok())
- .unwrap_or(0);
+ .ok_or_else(|| HyperHttpError::Uri)?; // Consider a dedicated MalformedResponse variant📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let status_code: u16 = status_line | |
| .split_whitespace() | |
| .nth(1) | |
| .and_then(|s| s.parse().ok()) | |
| .unwrap_or(0); | |
| let status_code: u16 = status_line | |
| .split_whitespace() | |
| .nth(1) | |
| .and_then(|s| s.parse().ok()) | |
| .ok_or_else(|| HyperHttpError::Uri)?; |
🤖 Prompt for AI Agents
In `@crates/notedeck/src/media/network.rs` around lines 297 - 301, The status code
parsing silently falls back to 0 on malformed status lines (status_line ->
status_code) which bypasses the redirect check and can hide errors; update the
parser in network.rs to treat parse failures as an error instead of using
unwrap_or(0): attempt to extract and parse the status token from status_line,
and if missing or parse fails return/propagate an appropriate Err (or
Result::Err) from the surrounding function (rather than assigning 0), so callers
can handle malformed responses and the redirect check
((300..400).contains(&status_code)) won't operate on an invalid value.
| let nmethods = buf[1] as usize; | ||
| let mut methods = vec![0u8; nmethods]; | ||
| stream | ||
| .read_exact(&mut methods) | ||
| .await | ||
| .map_err(|e| anyhow!("Failed to read methods: {}", e))?; | ||
|
|
||
| // Send response - no auth required | ||
| stream | ||
| .write_all(&[0x05, 0x00]) | ||
| .await | ||
| .map_err(|e| anyhow!("Failed to send auth response: {}", e))?; |
There was a problem hiding this comment.
SOCKS5 authentication negotiation doesn't verify client supports no-auth.
The code reads the client's supported methods but doesn't verify that 0x00 (no authentication) is in the list before responding with [0x05, 0x00]. Per RFC 1928, if no acceptable method is found, the server should respond with 0xFF.
Proposed fix to validate auth method
let nmethods = buf[1] as usize;
let mut methods = vec![0u8; nmethods];
stream
.read_exact(&mut methods)
.await
.map_err(|e| anyhow!("Failed to read methods: {}", e))?;
- // Send response - no auth required
- stream
- .write_all(&[0x05, 0x00])
- .await
- .map_err(|e| anyhow!("Failed to send auth response: {}", e))?;
+ // Verify client supports no-auth (0x00)
+ if !methods.contains(&0x00) {
+ stream.write_all(&[0x05, 0xFF]).await?;
+ return Err(anyhow!("Client doesn't support no-auth method"));
+ }
+
+ // Send response - no auth required
+ stream
+ .write_all(&[0x05, 0x00])
+ .await
+ .map_err(|e| anyhow!("Failed to send auth response: {}", e))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let nmethods = buf[1] as usize; | |
| let mut methods = vec![0u8; nmethods]; | |
| stream | |
| .read_exact(&mut methods) | |
| .await | |
| .map_err(|e| anyhow!("Failed to read methods: {}", e))?; | |
| // Send response - no auth required | |
| stream | |
| .write_all(&[0x05, 0x00]) | |
| .await | |
| .map_err(|e| anyhow!("Failed to send auth response: {}", e))?; | |
| let nmethods = buf[1] as usize; | |
| let mut methods = vec![0u8; nmethods]; | |
| stream | |
| .read_exact(&mut methods) | |
| .await | |
| .map_err(|e| anyhow!("Failed to read methods: {}", e))?; | |
| // Verify client supports no-auth (0x00) | |
| if !methods.contains(&0x00) { | |
| stream.write_all(&[0x05, 0xFF]).await?; | |
| return Err(anyhow!("Client doesn't support no-auth method")); | |
| } | |
| // Send response - no auth required | |
| stream | |
| .write_all(&[0x05, 0x00]) | |
| .await | |
| .map_err(|e| anyhow!("Failed to send auth response: {}", e))?; |
|
Signed-off-by: alltheseas <[email protected]>
Add ewebsock dependency with SOCKS5 proxy support for routing WebSocket connections through Tor. Uses git dependency pointing to fork with transport abstraction layer. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Signed-off-by: alltheseas <[email protected]>
Signed-off-by: alltheseas <[email protected]>
Force bundled sqlite3 builds on Windows and other platforms to ensure consistent behavior and avoid system library version issues. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Address CodeRabbit feedback for the Tor integration: - Add shutdown timeout (5s) to prevent indefinite blocking when stopping the Tor runtime. Uses polling with is_finished() instead of blocking join() to allow timeout handling. - Verify SOCKS port is actually listening before signaling ready. Previously the ready signal was sent after bootstrap but before confirming the proxy was accepting connections. - Add comprehensive docstrings to all public types, methods, and helper functions explaining their purpose and behavior. - Add documentation for the hardcoded port 9150, noting it may conflict with Tor Browser. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: alltheseas <[email protected]>
- Fix toggle desync: check Tor support before attempting toggle and only update settings if the operation succeeds - Hide internal error details from users: log errors at debug level and show generic "Connection failed" message in UI - Add clippy allow for too_many_arguments on process_settings_action Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: alltheseas <[email protected]>
Remove the `not(target_os = "android")` condition from the Transport import and configure_transport method, allowing Android builds to use custom transports for WebSocket connections. This is required for routing WebSocket traffic through Tor's SOCKS proxy on Android. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add a global JNI cache that stores the JavaVM and ClassLoader for cross-thread JNI access on Android. This is necessary because FindClass() only works on the main thread, so we cache the ClassLoader to enable JNI calls from background threads where Tor operations run. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add build-tor-android.sh script that compiles the arti-mobile crate for Android targets (arm64-v8a, armeabi-v7a, x86, x86_64). The script creates the Cargo package structure and uses cargo-ndk to build shared libraries that provide the native Tor implementation. Supports Intel Mac, Apple Silicon, and Linux build hosts. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add Java classes that provide the Android-side Tor integration: - TorProvider: Interface for Tor service abstraction - ArtiTorProvider: Native implementation using Arti via JNI - TorManager: Manages Tor lifecycle and provides SOCKS proxy info These classes are called from Rust via JNI to bootstrap and control the Tor connection on Android. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add Android-specific Tor implementation that communicates with the Java TorManager class via JNI. The Rust TorManager wraps JNI calls to start/stop Tor and retrieve SOCKS proxy information. Also export jni_cache module from lib.rs for Android builds. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Update build configuration for Android Tor support: - Makefile: Add android-tor target to build Arti native libraries - build.gradle: Configure jniLibs directory for native libraries - MainActivity: Initialize JNI cache and TorManager on startup Co-Authored-By: Claude Opus 4.5 <[email protected]>
Implement http_req_via_socks() function that performs HTTP requests through a SOCKS5 proxy for Tor routing. Features: - SOCKS5 connection via tokio-socks - TLS support via tokio-rustls with webpki-roots - HTTP/1.1 protocol with chunked transfer encoding - Redirect handling (up to 5 redirects) - Proper host header for virtual hosting Add workspace dependencies: tokio-socks, tokio-rustls, webpki-roots Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add HttpConfig struct that carries SOCKS proxy configuration through the media loading pipeline: - StaticImgTexCache: Store and use HttpConfig for image fetching - AnimatedImgTexCache: Store and use HttpConfig for GIF fetching - TexturesCache: Propagate HttpConfig to both static and animated - Images: Propagate HttpConfig through the cache hierarchy This allows media fetching to be routed through Tor when enabled. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Update update_relay_transport() to also configure the HTTP proxy for media loading when Tor state changes. When Tor is enabled, both WebSocket connections and HTTP image fetches are routed through the SOCKS proxy. Co-Authored-By: Claude Opus 4.5 <[email protected]>
alltheseas
force-pushed
the
feature/tor-routing
branch
from
1326e0e to
05efa1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java`:
- Around line 64-90: The start() method reads and writes the shared initialized
flag without synchronization which can race if called concurrently; make start()
thread-safe by synchronizing the critical section (or using a dedicated lock)
that checks initialized, creates cacheDir/stateDir, calls
provider.initialize(...), and sets initialized to true so directory creation and
provider.initialize(...) happen at most once; keep the outer logging and the
final call to provider.startSocksProxy(socksPort) outside or inside the
synchronized block as appropriate, but ensure the initialized check/assignment
and provider.initialize(...) are atomic.
- Around line 74-77: Check the boolean return values of cacheDir.mkdirs() and
stateDir.mkdirs() in TorManager and, if a call returns false and the directory
does not already exist, log a warning (including the directory absolute path and
that this may cause initialize() to fail) so failures due to permissions or IO
are visible; keep the existing flow otherwise (don’t change initialize()
behavior), and use the project’s logging mechanism (e.g., Log.w/TAG or existing
logger) to emit the warning.
In `@crates/notedeck_chrome/src/chrome.rs`:
- Around line 997-1008: The TorStatus::Failed branch currently injects the
internal Arti error (err.as_str()) into the user-facing tooltip inside the
TorIndicatorStyle created in the TorStatus::Failed arm; change this to a
user-friendly or summarized message instead of the raw error (e.g., use tr! with
a generic string like "Tor routing failed. Please check your settings or try
reconnecting" or map a few common err variants to short, non-sensitive
summaries) and update the tr! call used for tooltip so it no longer interpolates
err.as_str() but returns the safe message.
In `@crates/notedeck/src/media/network.rs`:
- Around line 384-388: The code currently treats a malformed chunk size as zero
by using unwrap_or(0) when parsing size_line into chunk_size, which silently
ends body reading; change the parsing to propagate an error instead of
defaulting to 0 (e.g., replace usize::from_str_radix(size_line.trim(),
16).unwrap_or(0) with a fallible parse that returns Err on failure or maps the
ParseIntError into the function's error type), and ensure the surrounding
function (the chunked-body reader that uses chunk_size and size_line) returns
that error instead of breaking/continuing.
In `@crates/notedeck/src/tor.rs`:
- Around line 513-515: The directory creation calls currently swallow errors
(let _ = std::fs::create_dir_all(&self.cache_dir); let _ =
std::fs::create_dir_all(&self.state_dir);), which can hide failures that cause
the subsequent JNI initialize() to fail; change these to propagate or log errors
instead: call std::fs::create_dir_all(...) and handle the Result (use ? to
return the error from the enclosing function or map_err to convert into your
crate's error type) for both self.cache_dir and self.state_dir, and only call
JNI initialize() after both creations succeed (or at minimum log the path and
error via your logger before proceeding).
- Around line 486-491: The code currently casts the JNI-returned port to u16
directly in get_socks_port() -> TorStatus::Running which can produce wrong
values for negative or >65535 ports; update the block around get_socks_port() so
you validate the returned port is within 1..=65535 (or 0 if you allow 0) before
converting, e.g., use an explicit range check or TryFrom<i32> to u16, and only
set self.status = TorStatus::Running { socks_port: port as u16 } when the check
passes; if the port is out of range, do not cast—handle it by setting an
appropriate non-running status (e.g., TorStatus::Stopped or TorStatus::Error)
and add a log/warning indicating the invalid JNI port value.
♻️ Duplicate comments (6)
crates/notedeck/Cargo.toml (1)
57-58: Keyring dependency remains ungated despite platform concerns.The previous review flagged that
keyringshould be moved to a platform-specific section since it doesn't support WASM. The dependency is still in the main[dependencies]block. If WASM builds are planned, this will cause compilation failures.Verify if WASM builds are actually targeted and if keyring needs platform gating:
#!/bin/bash # Check if there's WASM-specific handling or if keyring usage is already gated in code rg -n "target_arch.*wasm|cfg.*wasm" crates/notedeck/src --type rust | head -20 # Check keyring usage pattern rg -n "use.*keyring|keyring::" crates/notedeck/src --type rust -B2 -A2 | head -30tools/arti-build/src/lib.rs (1)
384-395: SOCKS5 authentication negotiation doesn't verify client supports no-auth.The code reads the client's supported methods but doesn't verify that
0x00(no authentication) is in the list before responding with[0x05, 0x00]. Per RFC 1928, if no acceptable method is found, the server should respond with0xFF.Proposed fix to validate auth method
let nmethods = buf[1] as usize; let mut methods = vec![0u8; nmethods]; stream .read_exact(&mut methods) .await .map_err(|e| anyhow!("Failed to read methods: {}", e))?; + // Verify client supports no-auth (0x00) + if !methods.contains(&0x00) { + stream.write_all(&[0x05, 0xFF]).await?; + return Err(anyhow!("Client doesn't support no-auth method")); + } + // Send response - no auth required stream .write_all(&[0x05, 0x00]) .await .map_err(|e| anyhow!("Failed to send auth response: {}", e))?;crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java (1)
14-14: Fixed SOCKS port 9150 may collide with Tor Browser.This was flagged in a previous review. Tor Browser uses port 9150 by default. If both run concurrently,
startSocksProxywill fail or produce undefined behavior. Consider probing for an available port or reporting a clear error when the port is occupied.crates/notedeck_columns/src/ui/settings.rs (1)
594-602: Potential UI desync:toggle_valuemutates settings before action is processed.The
toggle_valuewidget (line 596) mutatesself.settings.use_torimmediately when clicked. Iftor.set_enabled()fails in the action handler (lines 110-115), the localSettingsstruct retains the toggled value even though the operation failed. TheSettingsHandlerwon't persist it, but the UI will show the incorrect state until the view is reconstructed.Consider using a checkbox that doesn't directly mutate the settings, or revert the settings value on failure:
🔧 Option A: Use checked state without direct mutation
- if ui - .toggle_value( - &mut self.settings.use_tor, - RichText::new(label).text_style(NotedeckTextStyle::Small.text_style()), - ) - .changed() - { - action = Some(SettingsAction::ToggleTor(self.settings.use_tor)); - } + let mut checked = self.settings.use_tor; + if ui + .toggle_value( + &mut checked, + RichText::new(label).text_style(NotedeckTextStyle::Small.text_style()), + ) + .changed() + { + // Don't mutate settings here - let the action handler do it + action = Some(SettingsAction::ToggleTor(checked)); + }🔧 Option B: Revert in action handler on failure
Self::ToggleTor(enabled) => { if !notedeck::TorManager::is_supported() { tracing::warn!("Tor toggle requested but not supported on this platform"); + settings.set_use_tor(!enabled); // Revert the toggle return route_action; } match tor.set_enabled(enabled) { Ok(()) => settings.set_use_tor(enabled), Err(err) => { tracing::error!("failed to toggle tor: {err}"); + settings.set_use_tor(!enabled); // Revert the toggle } } }crates/notedeck/src/media/network.rs (2)
38-46: Misleading comment: HTTP is entirely unsupported on Android/WASM, not just SOCKS proxy.The comment states "SOCKS proxy not yet supported for HTTP" but
http_reqreturnsHyperHttpError::Unsupportedon these platforms, meaning all HTTP requests fail. Update the comment to accurately reflect that HTTP requests are not supported on Android/WASM platforms.Suggested fix
/// Stub for Android/WASM #[cfg(any(target_arch = "wasm32", target_os = "android"))] pub async fn http_fetch( url: &str, _socks_proxy: Option<&str>, ) -> Result<HyperHttpResponse, HyperHttpError> { - // On Android/WASM, SOCKS proxy not yet supported for HTTP + // HTTP requests not supported on Android/WASM platforms http_req(url).await }
297-301: Malformed status line silently falls back to 0, masking protocol errors.If the status line is malformed or missing,
status_codebecomes 0, which bypasses the redirect check and proceeds to read the body as if the request succeeded. This could mask HTTP protocol errors.Suggested fix
let status_code: u16 = status_line .split_whitespace() .nth(1) .and_then(|s| s.parse().ok()) - .unwrap_or(0); + .ok_or_else(|| HyperHttpError::Uri)?; // Or add a MalformedResponse variant
🧹 Nitpick comments (11)
tools/arti-build/src/lib.rs (2)
469-500: Bidirectional copy silently drops the losing task's result.When one direction completes or errors,
tokio::select!cancels the other branch without checking if an error occurred. Ifclient_to_torfails with an error, it's silently ignored whentor_to_clientwins the race, or vice versa.Consider using tokio::try_join! or handling both results
- tokio::select! { - _ = client_to_tor => {} - _ = tor_to_client => {} - } + // Use select but log errors from the losing side + tokio::select! { + result = client_to_tor => { + if let Err(e) = result { + log_to_java(&format!("client_to_tor error: {}", e)); + } + } + result = tor_to_client => { + if let Err(e) = result { + log_to_java(&format!("tor_to_client error: {}", e)); + } + } + }Alternatively, for a cleaner shutdown where both sides complete:
let _ = tokio::join!(client_to_tor, tor_to_client);
464-467: Success response reports dummy bind address instead of actual.The SOCKS5 success response sends
127.0.0.1:0as the bound address. While most clients ignore this for CONNECT commands, RFC 1928 expects the actual bound address. For a local proxy this is minor but could confuse strict clients.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorLogCallback.java (1)
6-12: Consider adding@FunctionalInterfaceannotation.Since this interface has exactly one abstract method and is intended to be used as a callback (likely with lambdas), adding the
@FunctionalInterfaceannotation would provide compile-time validation and clearer intent.Suggested change
/** * Callback interface for receiving Tor log messages. */ +@FunctionalInterface public interface TorLogCallback { /** * Called when a log message is received from the Tor client. * * `@param` message The log message */ void onLog(String message); }crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorProvider.java (1)
7-57: LGTM! Well-designed provider interface.The interface provides a clean abstraction for swapping between native Arti and stub implementations. The method signatures are clear and the Javadoc is comprehensive.
One optional improvement: consider adding Android nullability annotations (
@NullableforgetSocksProxy()return value and@NonNullfor string parameters) to improve null-safety for callers.crates/notedeck/src/lib.rs (1)
16-16: Consider adding#[cfg(target_os = "android")]to thejni_cachemodule declaration.The
jni_cachemodule is Android-specific JNI code. While the module file already gates all real implementations behind#[cfg(target_os = "android")], moving this attribute to the module declaration inlib.rswould make the Android-only nature explicit at the crate level and prevent accidental imports of Android-specific functions on other platforms.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/ArtiNative.java (1)
41-82: Consider documenting the precondition for native method calls.Callers must check
isLibraryLoaded()before invoking native methods to avoidUnsatisfiedLinkError. While this is standard JNI practice, adding a class-level Javadoc note about this precondition would improve API clarity.📝 Suggested documentation addition
/** * JNI bindings to the Arti Tor client native library. + * + * <p><b>Important:</b> Callers must verify {`@link` `#isLibraryLoaded`()} returns + * {`@code` true} before invoking any native methods, otherwise an + * {`@link` UnsatisfiedLinkError} will be thrown.</p> */ public class ArtiNative {crates/notedeck/src/media/static_imgs.rs (1)
25-29: LGTM! Clean HttpConfig definition.The
HttpConfigstruct withCloneandDefaultderives is well-designed for thread-safe propagation. Thesocks_proxyfield correctly usesOption<String>to support both direct and proxied connections.Consider adding
#[derive(Debug)]for easier logging/debugging of HTTP configuration state:📝 Optional enhancement
/// Configuration for HTTP requests, including optional SOCKS proxy. -#[derive(Clone, Default)] +#[derive(Clone, Default, Debug)] pub struct HttpConfig { pub socks_proxy: Option<String>, }crates/notedeck/src/jni_cache.rs (1)
18-33: Global staticJNI_CACHEmay conflict with coding guidelines.Per coding guidelines: "Global variables are not allowed in this codebase, even if they are thread local; state should be managed in a struct that is passed in as reference."
However, for JNI initialization this pattern is likely necessary since:
- Java calls
initClassLoaderbefore Rust has control- The ClassLoader must be available from any thread
- There's no practical way to pass this state through the call stack from Java
The
Send + Syncimplementations are correctly justified in the comments. If the team accepts this as a necessary exception for JNI interop, consider adding a comment explaining why this global is acceptable despite the guideline.crates/notedeck/src/app.rs (1)
481-493: Consider clarifying the no-opinstall_cryptocomment.The comment "Crypto not needed on mobile/web platforms" may be misleading since Android still uses TLS for relay connections. Consider clarifying that the crypto provider is either handled differently on these platforms or that this specific initialization isn't required.
📝 Suggested comment improvement
/// No-op on Android/WASM - crypto handled by platform or not needed #[cfg(any(target_arch = "wasm32", target_os = "android"))] pub fn install_crypto() { - // Crypto not needed on mobile/web platforms + // On Android, TLS is handled via native platform APIs or tokio-rustls + // picks up a default provider. On WASM, the browser handles TLS. }crates/notedeck/src/media/network.rs (1)
216-267: Consider extracting redirect handling to reduce duplication.The HTTPS (lines 219-240) and HTTP (lines 245-266) branches contain identical redirect handling logic. This could be refactored by extracting the redirect handling after
do_http_requestreturns.Suggested refactor
loop { // Connect through SOCKS proxy let stream = Socks5Stream::connect(socks_proxy, (current_host.as_str(), current_port)) .await .map_err(|e| HyperHttpError::Hyper(Box::new(e)))?; let mut stream = stream.into_inner(); + let (content_type, bytes, redirect) = if current_is_https { // If HTTPS, wrap with TLS - if current_is_https { let mut root_store = rustls::RootCertStore::empty(); root_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); // ... TLS setup ... let mut tls_stream = connector.connect(server_name, stream).await...; - let (content_type, bytes, redirect) = - do_http_request(&mut tls_stream, ¤t_host, ¤t_path).await?; - - if let Some(location) = redirect { - // ... redirect handling ... - } - return Ok(HyperHttpResponse { content_type, bytes }); + do_http_request(&mut tls_stream, ¤t_host, ¤t_path).await? } else { - let (content_type, bytes, redirect) = - do_http_request(&mut stream, ¤t_host, ¤t_path).await?; - - if let Some(location) = redirect { - // ... identical redirect handling ... - } - return Ok(HyperHttpResponse { content_type, bytes }); + do_http_request(&mut stream, ¤t_host, ¤t_path).await? + }; + + // Unified redirect handling + if let Some(location) = redirect { + if redirects >= MAX_REDIRECTS { + return Err(HyperHttpError::TooManyRedirects); + } + let (new_host, new_port, new_path, new_is_https) = parse_redirect_location(...)?; + // ... update state ... + redirects += 1; + continue; } + return Ok(HyperHttpResponse { content_type, bytes }); }crates/notedeck/src/tor.rs (1)
478-494: Consider adding a bootstrap timeout for the Starting state.If the JNI
getSocksPort()keeps returning 0 (e.g., bootstrap stalls), the status remainsStartingindefinitely with no feedback to the user. Consider adding a timeout that transitions toFailedafter a reasonable period.Suggested approach
Track when
Startingstate began and transition toFailedafter a timeout (e.g., 60-120 seconds):pub struct TorManager { status: TorStatus, // ... existing fields ... /// Timestamp when Starting state began, for timeout detection starting_since: Option<std::time::Instant>, } pub fn poll(&mut self) { // ... existing checks ... if matches!(self.status, TorStatus::Starting) { // Check for timeout if let Some(started) = self.starting_since { if started.elapsed() > Duration::from_secs(120) { self.status = TorStatus::Failed("Tor bootstrap timed out".to_string()); self.starting_since = None; return; } } if let Some(port) = self.get_socks_port() { if port > 0 { self.status = TorStatus::Running { socks_port: port as u16 }; self.starting_since = None; } } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktools/arti-build/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.cargo/config.toml.gitattributes.gitignoreCargo.tomlMakefilecrates/enostr/src/relay/mod.rscrates/enostr/src/relay/pool.rscrates/notedeck/Cargo.tomlcrates/notedeck/src/app.rscrates/notedeck/src/context.rscrates/notedeck/src/imgcache.rscrates/notedeck/src/jni_cache.rscrates/notedeck/src/lib.rscrates/notedeck/src/media/gif.rscrates/notedeck/src/media/network.rscrates/notedeck/src/media/static_imgs.rscrates/notedeck/src/persist/settings_handler.rscrates/notedeck/src/tor.rscrates/notedeck_chrome/android/app/build.gradlecrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/ArtiNative.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/NativeTorProvider.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/StubTorProvider.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorLogCallback.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.javacrates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorProvider.javacrates/notedeck_chrome/src/chrome.rscrates/notedeck_columns/src/nav.rscrates/notedeck_columns/src/ui/settings.rstools/arti-build/ARTI_VERSIONtools/arti-build/Cargo.tomltools/arti-build/build-arti.shtools/arti-build/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
🚧 Files skipped from review as they are similar to previous changes (7)
- tools/arti-build/Cargo.toml
- tools/arti-build/ARTI_VERSION
- crates/notedeck/src/context.rs
- crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/NativeTorProvider.java
- crates/notedeck_chrome/android/app/build.gradle
- tools/arti-build/build-arti.sh
- .gitignore
🧰 Additional context used
📓 Path-based instructions (4)
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Do not vendor code; in Cargo.toml replace the existing URL with the fork that includes the new code. If vendoring is absolutely necessary, present the case why no other options are feasible
Files:
Cargo.toml
crates/notedeck*/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/notedeck*/src/**/*.rs: UseAppContextto access mutable handles to services (database, caches, relay pool, account state, localization, settings, wallet) rather than accessing global state
Never use Mutexes in UI paths; the render loop must never block. UseRc<RefCell<>>for single-threaded interior mutability,Arc<tokio::sync::RwLock<>>for cross-thread sharing, andpoll_promise::Promisefor async results
Wrap async work inpoll_promise::Promise, check withpromise.ready()orpromise.ready_mut()each frame—never block the render loop
Usetokio::sync::RwLockinstead ofArc<Mutex<>>for cross-thread sharing in Notedeck
Persist state viaTimedSerializer::try_saveto avoid blocking the frame; batch mutations withSettingsHandler::update_batch
Wrap user-facing strings withtr!ortr_plural!macros for localization and runpython3 scripts/export_source_strings.pyafter changing strings
UseJobPoolfor CPU-bound work and return results viatokio::sync::oneshotwrapped in Promises; usetokio::spawn()for network I/O and relay sync
Mark performance-critical functions with#[profiling::function]for visibility in the puffin profiler
Usetracingmacros for structured logging andprofilingscopes where hot paths exist
Prefer early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers ofifstatements (Nevernesting principle)
Global variables are not allowed in this codebase, even if they are thread local; state should be managed in a struct that is passed in as reference
Ensure docstring coverage for any code added or modified
Avoid Mutexes in Notedeck code; preferpoll_promise::Promisefor async results,Rc<RefCell<>>for single-threaded interior mutability, ortokio::sync::RwLockwhen cross-thread sharing is truly necessary
Never block the render loop; usePromise::ready()for non-blocking result checks. Offload CPU-heavy work to...
Files:
crates/notedeck/src/imgcache.rscrates/notedeck/src/jni_cache.rscrates/notedeck/src/media/gif.rscrates/notedeck_columns/src/ui/settings.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/media/static_imgs.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rscrates/notedeck/src/media/network.rscrates/notedeck/src/persist/settings_handler.rscrates/notedeck/src/lib.rscrates/notedeck/src/tor.rs
crates/notedeck*/src/app.rs
📄 CodeRabbit inference engine (AGENTS.md)
Implement the
Apptrait withupdate(&mut self, &mut AppContext, &mut egui::Ui) -> AppResponseto drive egui rendering and signal high-level actions
Files:
crates/notedeck/src/app.rs
crates/notedeck*/src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use strict Rust 2021 edition; ensure edition-lints are strict and clippy
disallowed_methodsis denied at crate root to enforce API hygiene
Files:
crates/notedeck/src/lib.rs
🧠 Learnings (18)
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to Cargo.toml : Do not vendor code; in Cargo.toml replace the existing URL with the fork that includes the new code. If vendoring is absolutely necessary, present the case why no other options are feasible
Applied to files:
Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : For animations (GIFs, video), track `repaint_at` timestamps and only request repaints when necessary; avoid spinning every frame
Applied to files:
crates/notedeck/src/imgcache.rscrates/notedeck/src/media/gif.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `JobPool` for CPU-bound work and return results via `tokio::sync::oneshot` wrapped in Promises; use `tokio::spawn()` for network I/O and relay sync
Applied to files:
crates/enostr/src/relay/pool.rscrates/notedeck/src/app.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Embrace egui idioms in UI code: builder chains, closures returning `Response`, `ui.vertical`/`horizontal` for layout
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck_columns/src/nav.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Wrap user-facing strings with `tr!` or `tr_plural!` macros for localization and run `python3 scripts/export_source_strings.py` after changing strings
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `AppContext` to access mutable handles to services (database, caches, relay pool, account state, localization, settings, wallet) rather than accessing global state
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/lib.rs : Use strict Rust 2021 edition; ensure edition-lints are strict and clippy `disallowed_methods` is denied at crate root to enforce API hygiene
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `tokio::sync::RwLock` instead of `Arc<Mutex<>>` for cross-thread sharing in Notedeck
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rscrates/notedeck/Cargo.tomlcrates/notedeck/src/lib.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Avoid Mutexes in Notedeck code; prefer `poll_promise::Promise` for async results, `Rc<RefCell<>>` for single-threaded interior mutability, or `tokio::sync::RwLock` when cross-thread sharing is truly necessary
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Persist state via `TimedSerializer::try_save` to avoid blocking the frame; batch mutations with `SettingsHandler::update_batch`
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck_columns/src/nav.rscrates/notedeck/Cargo.tomlcrates/notedeck/src/persist/settings_handler.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/app.rs : Implement the `App` trait with `update(&mut self, &mut AppContext, &mut egui::Ui) -> AppResponse` to drive egui rendering and signal high-level actions
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Global variables are not allowed in this codebase, even if they are thread local; state should be managed in a struct that is passed in as reference
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Never use Mutexes in UI paths; the render loop must never block. Use `Rc<RefCell<>>` for single-threaded interior mutability, `Arc<tokio::sync::RwLock<>>` for cross-thread sharing, and `poll_promise::Promise` for async results
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck_chrome/src/chrome.rscrates/notedeck/src/app.rscrates/notedeck/Cargo.toml
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Use `tracing` macros for structured logging and `profiling` scopes where hot paths exist
Applied to files:
crates/notedeck_columns/src/ui/settings.rscrates/notedeck/src/app.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Prefer module-level organization over monolithic files; each feature (accounts, decks, timelines, media) should live in its own module tree
Applied to files:
crates/notedeck/src/app.rscrates/notedeck_columns/src/nav.rscrates/notedeck/Cargo.tomlcrates/notedeck/src/lib.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Ensure docstring coverage for any code added or modified
Applied to files:
crates/notedeck/src/app.rscrates/notedeck/Cargo.tomlcrates/notedeck/src/tor.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Never block the render loop; use `Promise::ready()` for non-blocking result checks. Offload CPU-heavy work to `JobPool` or `tokio::spawn()`, returning results via channels or Promises
Applied to files:
crates/notedeck/src/app.rscrates/notedeck/src/tor.rs
📚 Learning: 2026-01-05T20:25:35.921Z
Learnt from: CR
Repo: damus-io/notedeck PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-05T20:25:35.921Z
Learning: Applies to crates/notedeck*/src/**/*.rs : Tests should live alongside modules (e.g., in test submodules), often using `#[tokio::test]` when async behavior is involved
Applied to files:
crates/notedeck/Cargo.toml
🧬 Code graph analysis (8)
crates/notedeck/src/imgcache.rs (2)
crates/notedeck/src/media/gif.rs (1)
set_http_config(129-131)crates/notedeck/src/media/static_imgs.rs (1)
set_http_config(47-49)
crates/notedeck/src/media/gif.rs (2)
crates/notedeck/src/imgcache.rs (5)
new(37-47)new(114-139)new(290-300)set_http_config(50-53)set_http_config(308-310)crates/notedeck/src/media/static_imgs.rs (2)
new(38-44)set_http_config(47-49)
crates/notedeck_columns/src/ui/settings.rs (2)
crates/notedeck/src/tor.rs (3)
is_supported(128-130)is_supported(380-382)is_supported(452-455)crates/notedeck/src/ui.rs (1)
richtext_small(5-10)
crates/notedeck_chrome/src/chrome.rs (1)
crates/notedeck/src/tor.rs (3)
status(133-135)status(384-386)status(457-459)
crates/notedeck/src/media/static_imgs.rs (2)
crates/notedeck/src/imgcache.rs (5)
new(37-47)new(114-139)new(290-300)set_http_config(50-53)set_http_config(308-310)crates/notedeck/src/media/gif.rs (2)
new(120-126)set_http_config(129-131)
crates/notedeck/src/app.rs (1)
crates/notedeck/src/tor.rs (6)
is_supported(128-130)is_supported(380-382)is_supported(452-455)socks_proxy(153-159)socks_proxy(392-394)socks_proxy(470-476)
crates/notedeck_columns/src/nav.rs (1)
crates/notedeck/src/tor.rs (3)
is_supported(128-130)is_supported(380-382)is_supported(452-455)
crates/notedeck/src/lib.rs (1)
crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java (1)
TorManager(12-134)
🔇 Additional comments (65)
.cargo/config.toml (1)
4-6:[env]section applies globally, not just to aarch64-unknown-linux-gnu.In TOML,
[env]is a new top-level section that closes the previous[target.aarch64-unknown-linux-gnu]block. These environment variables will apply to all cargo builds, not only cross-compilation for that target.If this is intentional (to ensure consistent SQLite bundling across all platforms), please disregard. If target-specific behavior was intended, note that Cargo does not support
[target.<triple>.env]—you would need to set these variables externally when invoking the cross-compilation command, or use a build script with conditional logic.crates/enostr/src/relay/mod.rs (4)
151-157: LGTM!The addition of the private
optionsfield to store websocket configuration is appropriate. Keeping it private with access viaupdate_optionsmaintains proper encapsulation.
184-200: LGTM!The updated
Relay::newcorrectly accepts and stores the options, cloning when passing toconnect_with_wakeupsince the method takes ownership. Storing the original options enables consistent reconnection behavior.
218-225: LGTM!Using
self.options.clone()inconnectensures that reconnection attempts preserve the configured transport (e.g., SOCKS proxy for Tor). This is essential for maintaining consistent routing behavior across disconnects.
231-234: LGTM!The
update_optionsmethod provides a clean way to update the stored options. The separation of concerns (updating options vs. triggering reconnection) is appropriate, with the caller responsible for reconnection when needed.crates/enostr/src/relay/pool.rs (4)
10-18: LGTM!The imports are appropriately organized with
TransportandArcgated behind#[cfg(not(target_arch = "wasm32"))]since SOCKS proxy transport is only available on native platforms.
128-149: LGTM!The
ws_optionsfield is properly added toRelayPooland initialized toOptions::default(), ensuring new pools start with direct connections until explicitly configured otherwise.
322-326: LGTM!Passing
self.ws_options.clone()toRelay::newensures that newly added relays inherit the current transport configuration (e.g., Tor proxy if enabled).
350-366: Implementation correctly reconnects all Websocket relays on transport change.This addresses the concern about existing relay sockets remaining direct after enabling Tor—only Websocket relays are reconnected, which is correct since Multicast relays use UDP multicast and don't require transport configuration. The options are updated before reconnect, so if reconnection fails, subsequent automatic reconnect attempts (via
keepalive_ping) will use the new transport. The error is logged, which is appropriate.Likely an incorrect or invalid review comment.
Cargo.toml (2)
140-141: Git-based dependency correctly replaces vendored code.The
ewebsockdependency now uses a git fork URL with a specific revision, addressing the previous review concern about vendoring. This aligns with the coding guidelines.
106-110: Arti dependencies properly configured with consistent pinned revision.All five Arti-related crates are sourced from the official torproject GitLab repository and pinned to the same commit revision (
624cdc9ec220124692c369f96ecb3cc72d9bbb66), ensuring reproducible builds. The required features (experimental-api,tokio,native-tls) are appropriately enabled. The configuration follows the guideline of using git URLs instead of vendoring code.Makefile (2)
13-23: Well-structured build targets for Arti.The new Makefile targets provide clear workflows for building Arti on Android:
arti: Full build for all architecturesarti-arm64: ARM64-only release build for faster development iterationarti-clean: Cleanup targetThe structure is clean and the comments are helpful.
46-50: Android Tor target properly integrates with build pipeline.The
android-tortarget correctly:
- Depends on
arti-arm64to ensure the native library is built- Adds appropriate logcat filters (
NativeTorProvider,TorManager) for debugging Tor-related issuescrates/notedeck/Cargo.toml (1)
59-73: Arti/TLS dependencies correctly gated for desktop platforms.The target-specific section properly excludes WASM and Android from Arti-related dependencies, which aligns with the PR notes that Android and WASM use stub modules instead of the native Arti runtime.
tools/arti-build/src/lib.rs (3)
1-6: Good module documentation.Clear documentation explaining the purpose of the crate and its functionality.
25-56: Global mutable state with Mutex is acceptable for this JNI bridge.While Notedeck crates prefer avoiding Mutexes, this is a standalone Android JNI library (
tools/arti-build) that bridges Java and Rust. The global state pattern is common for JNI libraries where the Java side controls lifecycle. The use ofOnceLock<Mutex<>>provides thread-safe initialization.
116-138:block_onwill block the JNI calling thread—ensure this is never called from Android's main/UI thread.The
runtime.block_on()call on line 119 blocks whatever thread invokes the JNI method. IfArtiNative.initialize()is called from the main/UI thread, it will cause ANR (Application Not Responding) during network-dependent Tor bootstrap.Currently, no active call sites to
TorManager.start()orNativeTorProvider.initialize()are visible in the Java codebase. If these are used, they must be invoked from a background thread (e.g., viaThread,Executor, or coroutines). If this initialization is triggered from Rust or through undocumented code paths, ensure the calling context is a worker thread, not the main thread.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/MainActivity.java (2)
34-36: Native method declaration for JNI classloader cache.The
initClassLoadernative method enables Rust code to cache the Java ClassLoader for later JNI calls. This is a standard pattern for JNI code that needs to find classes from native threads.
166-175: Correct initialization sequence for JNI classloader.The implementation correctly:
- Calls
initClassLoaderaftersuper.onCreate()which loads the native library- Wraps in try/catch to handle cases where the native method isn't available
- Logs success/failure for debugging
The comment clearly explains the timing requirements.
crates/notedeck/src/persist/settings_handler.rs (2)
44-45: Consistentserde(default)usage for backward compatibility.Using
#[serde(default)]ensures that existing settings files without theuse_torfield will deserialize correctly with the default value offalse. This is the correct approach for adding new optional settings.
218-228: LGTM! Methods follow established patterns.The
set_use_toranduse_tormethods are consistent with other settings accessors in this file. The persistence viatry_savecorrectly avoids blocking the render loop per coding guidelines.crates/notedeck/src/imgcache.rs (2)
48-53: LGTM! Clean HTTP config propagation.The method correctly propagates the HTTP configuration to both static and animated image caches. Cloning for
static_imageand moving foranimatedis an efficient pattern. The docstring adequately describes the purpose.
307-310: LGTM! Consistent delegation to textures cache.The
Images::set_http_configmethod properly delegates toTexturesCache, maintaining a clean layered architecture for HTTP config propagation.crates/notedeck/src/lib.rs (3)
40-40: Good module organization for Tor feature.Adding
toras a dedicated public module aligns with the coding guideline to prefer module-level organization where each feature lives in its own module tree.
68-70: LGTM! Appropriate public re-exports.Re-exporting
HttpConfigfromstatic_imgsat the crate root provides convenient access for consumers configuring HTTP/SOCKS proxy settings for media fetching.
94-94: LGTM! Clean public API surface for Tor.Re-exporting
TorManagerandTorStatusat crate root makes the Tor API easily accessible to dependent crates likenotedeck_chromeandnotedeck_columns.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/StubTorProvider.java (1)
1-48: LGTM! Clean stub implementation.The stub provider correctly implements the
TorProviderinterface with appropriate no-op behavior and return values (falsefor operations,nullfor proxy address). This provides a safe fallback for builds without native Tor support.crates/notedeck_columns/src/nav.rs (3)
42-43: LGTM! Proper Tor context wiring through navigation.The
TorManagerimport and usage correctly follows theAppContextpattern for accessing services. Thectx.torreference is appropriately passed through the navigation layer.
711-719: LGTM! SettingsView correctly receives Tor state.The
tor.status()andTorManager::is_supported()calls are non-blocking and appropriately pass Tor state to the settings UI for rendering the toggle and status indicators.
589-597: LGTM! Settings action processing wired for Tor control.The
ctx.torparameter enables the settings action handler to control Tor state (enable/disable toggle), following the establishedAppContextservice access pattern.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/ArtiNative.java (1)
10-18: Good error handling for library loading.The static initializer gracefully catches
UnsatisfiedLinkErrorand stores the error message for diagnostic purposes, preventing crashes on devices without native support.crates/notedeck/src/media/gif.rs (2)
113-131: LGTM! Consistent HttpConfig integration.The
http_configfield andset_http_configsetter mirror the pattern inStaticImgTexCache, maintaining consistency across image cache types. The default initialization ensures backward compatibility.
176-191: LGTM! Proper HttpConfig propagation to async fetch.The
http_configis correctly cloned before being moved into the async task, andsocks_proxy.as_deref()properly convertsOption<String>toOption<&str>for thehttp_fetchcall. Error handling is appropriately in place.Also applies to: 216-231
crates/notedeck/src/media/static_imgs.rs (2)
31-49: LGTM! Proper cache integration with HttpConfig.The
http_configfield with its setter follows the pattern established across image caches and includes appropriate docstring coverage as per coding guidelines.
146-161: LGTM! Network fetch properly uses HttpConfig.The
http_fetchintegration withsocks_proxy.as_deref()correctly enables SOCKS proxy support for static image loading. Error handling is consistent with the animated image fetch path.crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java (1)
36-47: LGTM on singleton accessor and passthrough methods.The singleton pattern with synchronized
getInstance()and the delegation methods (stop(),getSocksProxy(),isSupported(),isRunning(),setLogCallback()) are well-documented and correctly delegate to the underlying provider.Also applies to: 92-134
crates/notedeck_chrome/src/chrome.rs (6)
75-80: LGTM on Tor palette constants.The Tor brand color constants are well-defined and provide semantic naming for the indicator states.
920-930: LGTM on style data structures.
TorIndicatorStyleandOverlayStylecleanly encapsulate per-status visual properties.
932-945: LGTM onblend_colorshelper.Correctly implements linear interpolation with proper clamping for all RGBA channels.
947-971: Floating Tor indicator implementation looks good.The badge correctly anchors to the top-right, uses
Order::Tooltipfor proper layering, and provides hover feedback via tooltip. Per PR comments, you may want to consider adding click affordance in the future (e.g., click to retry on failure or show details), but the current hover-tooltip approach is functional.
1024-1079: LGTM onpaint_tor_onionrendering.The onion icon is rendered with proper layering: filled base, concentric accent strokes, sprout lines, and conditional slash overlay. The calculations are correct and the rendering approach is clean.
278-280: LGTM on Tor indicator integration.The indicator is correctly placed after app actions are processed, queries status via a non-blocking clone, and passes localization context for tooltips.
crates/notedeck_columns/src/ui/settings.rs (5)
8-11: LGTM on imports andToggleToraction variant.Clean additions following the existing
SettingsActionpattern.Also applies to: 38-38
101-117: Correct error handling inToggleToraction.The handler properly checks
is_supported()before toggling, only updates persistent settings on success, and logs errors. This addresses the UI desync concern from the previous review.
123-165: LGTM onSettingsViewstruct additions.The
tor_statusandtor_supportedfields are correctly added and initialized via the constructor.
604-636: LGTM on status label display with localization.All status variants correctly use
tr!()for localization. TheFailedcase now logs the error detail and shows a user-friendly message, addressing the previous review comment.Per PR comments, consider adding guidance for Android users when
Unsupported(e.g., "Consider using Orbot for Tor support"), but this is optional.
823-828: LGTM onnetwork_sectionintegration in UI flow.The network section is correctly integrated between storage and keys sections, following the existing action propagation pattern.
crates/notedeck/src/jni_cache.rs (4)
1-16: LGTM on module documentation and imports.The docstring clearly explains the Android ClassLoader problem this module solves, and imports are correctly cfg-gated for Android-only compilation.
35-86: LGTM oninitClassLoaderJNI entry point.The function correctly handles:
- Re-initialization guard (line 48)
- Null classloader validation (line 53)
- JavaVM extraction with error handling
- GlobalRef creation to prevent garbage collection
- OnceLock race condition handling (lines 81-85)
Good defensive programming with comprehensive error logging.
101-148: LGTM onfind_classimplementation.Correctly implements cross-thread class loading:
- Class name format conversion (lines 111-112)
- Defensive exception clearing before/after JNI calls
- Proper local reference creation from GlobalRef for thread safety
- Clear error messages with class name context
The
SAFETYcomment at line 146 adequately justifies theJObject -> JClassconversion.
150-177: LGTM onwith_jniand non-Android stubs.
with_jnicorrectly attaches the current thread to the JVM before executing the closure. The non-Android stubs appropriately no-op, withis_initialized()returningtruesince JNI isn't needed on those platforms.crates/notedeck/src/app.rs (5)
10-11: LGTM on Tor-related imports and struct fields.Clean additions following existing patterns. The
ws_transportfield is correctly cfg-gated for non-wasm32 platforms, and no Mutexes are introduced.Also applies to: 30-33, 89-91
230-241: LGTM on TorManager initialization.Correct initialization sequence:
- Load settings first
- Create TorManager with data path
- Only enable if both supported and configured
- Revert settings on failure to prevent desync
- Start with
WsTransport::Directuntil Tor is ready
143-145: LGTM on frame update integration.
tor.poll()is correctly called each frame to check async status changes.update_relay_transport()is guarded by change detection to avoid unnecessary work.
423-423: LGTM ontorreference inAppContext.Follows the established pattern for service access via context, per coding guidelines.
453-478: Relay transport reconfiguration correctly reconnects existing sockets.The
pool.configure_transport()method iterates through all WebSocket relays, updates their transport options, and explicitly reconnects them viarelay.connect(). Existing connections will use the new transport (Socks or Direct) immediately when reconfigured, not on next reconnect. The implementation is correct.crates/notedeck/src/media/network.rs (4)
1-24: LGTM!Platform-conditional imports are correctly gated, and the 20MB body limit is reasonable for media content.
48-154: LGTM!The desktop HTTP implementation properly handles HTTPS with certificate fallback, redirect following with limits, and body size enforcement both from headers and during streaming.
412-449: LGTM!Redirect location parsing correctly handles absolute URLs, relative paths, and query string preservation.
460-504: LGTM!Error enum is well-structured with clear Display messages, and the
Unsupportedvariant properly indicates platform limitations.crates/notedeck/src/tor.rs (5)
20-28: LGTM!Constants are well-documented with clear explanation of port 9150 choice and potential conflict with Tor Browser. The shutdown timeout addresses the blocking concern from PR comments.
58-94: LGTM!The shutdown mechanism properly addresses the indefinite blocking concern with a timeout, and the Drop implementation ensures cleanup.
161-193: LGTM!The
poll()method correctly usestry_recv()for non-blocking status checks, adhering to the coding guideline of never blocking the render loop.
285-301: LGTM!The port verification logic addresses the past review comment about premature ready signaling. The proxy is given time to bind, verified to be accepting connections, and only then is the ready signal sent.
360-401: LGTM!The WASM stub correctly indicates Tor is unsupported on this platform with appropriate no-op implementations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| public boolean start(int socksPort) { | ||
| if (!provider.isSupported()) { | ||
| Log.w(TAG, "Tor is not supported on this build"); | ||
| return false; | ||
| } | ||
|
|
||
| if (!initialized) { | ||
| File cacheDir = new File(context.getCacheDir(), "tor"); | ||
| File stateDir = new File(context.getFilesDir(), "tor"); | ||
|
|
||
| // Create directories | ||
| cacheDir.mkdirs(); | ||
| stateDir.mkdirs(); | ||
|
|
||
| Log.i(TAG, "Initializing Tor (cache: " + cacheDir + ", state: " + stateDir + ")"); | ||
|
|
||
| if (!provider.initialize(cacheDir.getAbsolutePath(), stateDir.getAbsolutePath())) { | ||
| Log.e(TAG, "Failed to initialize Tor"); | ||
| return false; | ||
| } | ||
|
|
||
| initialized = true; | ||
| } | ||
|
|
||
| Log.i(TAG, "Starting SOCKS proxy on port " + socksPort); | ||
| return provider.startSocksProxy(socksPort); | ||
| } |
There was a problem hiding this comment.
start() method is not thread-safe.
While getInstance() is synchronized, start() is not. If called concurrently from multiple threads, the initialized flag check (line 70) and assignment (line 85) could race, potentially causing double initialization. Consider synchronizing start() or using a lock.
🔧 Suggested fix
- public boolean start(int socksPort) {
+ public synchronized boolean start(int socksPort) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public boolean start(int socksPort) { | |
| if (!provider.isSupported()) { | |
| Log.w(TAG, "Tor is not supported on this build"); | |
| return false; | |
| } | |
| if (!initialized) { | |
| File cacheDir = new File(context.getCacheDir(), "tor"); | |
| File stateDir = new File(context.getFilesDir(), "tor"); | |
| // Create directories | |
| cacheDir.mkdirs(); | |
| stateDir.mkdirs(); | |
| Log.i(TAG, "Initializing Tor (cache: " + cacheDir + ", state: " + stateDir + ")"); | |
| if (!provider.initialize(cacheDir.getAbsolutePath(), stateDir.getAbsolutePath())) { | |
| Log.e(TAG, "Failed to initialize Tor"); | |
| return false; | |
| } | |
| initialized = true; | |
| } | |
| Log.i(TAG, "Starting SOCKS proxy on port " + socksPort); | |
| return provider.startSocksProxy(socksPort); | |
| } | |
| public synchronized boolean start(int socksPort) { | |
| if (!provider.isSupported()) { | |
| Log.w(TAG, "Tor is not supported on this build"); | |
| return false; | |
| } | |
| if (!initialized) { | |
| File cacheDir = new File(context.getCacheDir(), "tor"); | |
| File stateDir = new File(context.getFilesDir(), "tor"); | |
| // Create directories | |
| cacheDir.mkdirs(); | |
| stateDir.mkdirs(); | |
| Log.i(TAG, "Initializing Tor (cache: " + cacheDir + ", state: " + stateDir + ")"); | |
| if (!provider.initialize(cacheDir.getAbsolutePath(), stateDir.getAbsolutePath())) { | |
| Log.e(TAG, "Failed to initialize Tor"); | |
| return false; | |
| } | |
| initialized = true; | |
| } | |
| Log.i(TAG, "Starting SOCKS proxy on port " + socksPort); | |
| return provider.startSocksProxy(socksPort); | |
| } |
🤖 Prompt for AI Agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java`
around lines 64 - 90, The start() method reads and writes the shared initialized
flag without synchronization which can race if called concurrently; make start()
thread-safe by synchronizing the critical section (or using a dedicated lock)
that checks initialized, creates cacheDir/stateDir, calls
provider.initialize(...), and sets initialized to true so directory creation and
provider.initialize(...) happen at most once; keep the outer logging and the
final call to provider.startSocksProxy(socksPort) outside or inside the
synchronized block as appropriate, but ensure the initialized check/assignment
and provider.initialize(...) are atomic.
| // Create directories | ||
| cacheDir.mkdirs(); | ||
| stateDir.mkdirs(); | ||
|
|
There was a problem hiding this comment.
Unchecked mkdirs() return values.
The mkdirs() calls don't check their return values. If directory creation fails (e.g., permissions issues), the subsequent initialize() call will fail with a potentially confusing error. Consider logging a warning if directory creation fails.
🔧 Suggested improvement
- cacheDir.mkdirs();
- stateDir.mkdirs();
+ if (!cacheDir.mkdirs() && !cacheDir.exists()) {
+ Log.w(TAG, "Failed to create cache directory: " + cacheDir);
+ }
+ if (!stateDir.mkdirs() && !stateDir.exists()) {
+ Log.w(TAG, "Failed to create state directory: " + stateDir);
+ }🤖 Prompt for AI Agents
In
`@crates/notedeck_chrome/android/app/src/main/java/com/damus/notedeck/tor/TorManager.java`
around lines 74 - 77, Check the boolean return values of cacheDir.mkdirs() and
stateDir.mkdirs() in TorManager and, if a call returns false and the directory
does not already exist, log a warning (including the directory absolute path and
that this may cause initialize() to fail) so failures due to permissions or IO
are visible; keep the existing flow otherwise (don’t change initialize()
behavior), and use the project’s logging mechanism (e.g., Log.w/TAG or existing
logger) to emit the warning.
| TorStatus::Failed(err) => Some(TorIndicatorStyle { | ||
| base: TOR_ALERT, | ||
| accent: TOR_AMBER, | ||
| sprout: TOR_AMBER, | ||
| overlay: Some(OverlayStyle::Slash(Color32::from_rgb(250, 226, 226))), | ||
| tooltip: tr!( | ||
| i18n, | ||
| "Tor routing failed: {error}", | ||
| "Tooltip when Tor routing has failed", | ||
| error = err.as_str() | ||
| ), | ||
| }), |
There was a problem hiding this comment.
Raw error still exposed in Failed tooltip.
The TorStatus::Failed(err) branch interpolates the internal error string into the user-facing tooltip (line 1006: error = err.as_str()). While the settings.rs file was updated to show a generic message, this tooltip still exposes internal Arti errors. Consider showing a user-friendly message here as well, or at least summarizing common errors.
🔧 Suggested approach
TorStatus::Failed(err) => Some(TorIndicatorStyle {
base: TOR_ALERT,
accent: TOR_AMBER,
sprout: TOR_AMBER,
overlay: Some(OverlayStyle::Slash(Color32::from_rgb(250, 226, 226))),
- tooltip: tr!(
- i18n,
- "Tor routing failed: {error}",
- "Tooltip when Tor routing has failed",
- error = err.as_str()
- ),
+ tooltip: {
+ tracing::debug!("Tor routing failed: {err}");
+ tr!(
+ i18n,
+ "Tor routing failed. Check Settings for details.",
+ "Tooltip when Tor routing has failed"
+ )
+ },
}),🤖 Prompt for AI Agents
In `@crates/notedeck_chrome/src/chrome.rs` around lines 997 - 1008, The
TorStatus::Failed branch currently injects the internal Arti error
(err.as_str()) into the user-facing tooltip inside the TorIndicatorStyle created
in the TorStatus::Failed arm; change this to a user-friendly or summarized
message instead of the raw error (e.g., use tr! with a generic string like "Tor
routing failed. Please check your settings or try reconnecting" or map a few
common err variants to short, non-sensitive summaries) and update the tr! call
used for tooltip so it no longer interpolates err.as_str() but returns the safe
message.
| let chunk_size = usize::from_str_radix(size_line.trim(), 16).unwrap_or(0); | ||
|
|
||
| if chunk_size == 0 { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Malformed chunk size silently terminates body reading.
If the chunk size line contains invalid hex, unwrap_or(0) treats it as end-of-body, potentially truncating the response. Consider returning an error for malformed chunked encoding.
Suggested fix
- let chunk_size = usize::from_str_radix(size_line.trim(), 16).unwrap_or(0);
+ let chunk_size = usize::from_str_radix(size_line.trim(), 16)
+ .map_err(|_| HyperHttpError::Uri)?; // Or add a MalformedChunk variant
if chunk_size == 0 {
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let chunk_size = usize::from_str_radix(size_line.trim(), 16).unwrap_or(0); | |
| if chunk_size == 0 { | |
| break; | |
| } | |
| let chunk_size = usize::from_str_radix(size_line.trim(), 16) | |
| .map_err(|_| HyperHttpError::Uri)?; | |
| if chunk_size == 0 { | |
| break; | |
| } |
🤖 Prompt for AI Agents
In `@crates/notedeck/src/media/network.rs` around lines 384 - 388, The code
currently treats a malformed chunk size as zero by using unwrap_or(0) when
parsing size_line into chunk_size, which silently ends body reading; change the
parsing to propagate an error instead of defaulting to 0 (e.g., replace
usize::from_str_radix(size_line.trim(), 16).unwrap_or(0) with a fallible parse
that returns Err on failure or maps the ParseIntError into the function's error
type), and ensure the surrounding function (the chunked-body reader that uses
chunk_size and size_line) returns that error instead of breaking/continuing.
| if let Some(port) = self.get_socks_port() { | ||
| if port > 0 { | ||
| self.status = TorStatus::Running { | ||
| socks_port: port as u16, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Validate port range before casting to u16.
The port as u16 cast on line 489 can produce incorrect values if the JNI returns a negative number or a value > 65535. Consider adding range validation.
Suggested fix
if let Some(port) = self.get_socks_port() {
- if port > 0 {
+ if port > 0 && port <= u16::MAX as i32 {
self.status = TorStatus::Running {
socks_port: port as u16,
};
}
}🤖 Prompt for AI Agents
In `@crates/notedeck/src/tor.rs` around lines 486 - 491, The code currently casts
the JNI-returned port to u16 directly in get_socks_port() -> TorStatus::Running
which can produce wrong values for negative or >65535 ports; update the block
around get_socks_port() so you validate the returned port is within 1..=65535
(or 0 if you allow 0) before converting, e.g., use an explicit range check or
TryFrom<i32> to u16, and only set self.status = TorStatus::Running { socks_port:
port as u16 } when the check passes; if the port is out of range, do not
cast—handle it by setting an appropriate non-running status (e.g.,
TorStatus::Stopped or TorStatus::Error) and add a log/warning indicating the
invalid JNI port value.
| // Create directories | ||
| let _ = std::fs::create_dir_all(&self.cache_dir); | ||
| let _ = std::fs::create_dir_all(&self.state_dir); |
There was a problem hiding this comment.
Directory creation errors are silently ignored.
If directory creation fails, the subsequent JNI initialize() call may fail with a confusing error. Consider propagating the error or at least logging it.
Suggested fix
// Create directories
- let _ = std::fs::create_dir_all(&self.cache_dir);
- let _ = std::fs::create_dir_all(&self.state_dir);
+ if let Err(e) = std::fs::create_dir_all(&self.cache_dir) {
+ tracing::warn!("Failed to create Tor cache directory: {}", e);
+ }
+ if let Err(e) = std::fs::create_dir_all(&self.state_dir) {
+ tracing::warn!("Failed to create Tor state directory: {}", e);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create directories | |
| let _ = std::fs::create_dir_all(&self.cache_dir); | |
| let _ = std::fs::create_dir_all(&self.state_dir); | |
| // Create directories | |
| if let Err(e) = std::fs::create_dir_all(&self.cache_dir) { | |
| tracing::warn!("Failed to create Tor cache directory: {}", e); | |
| } | |
| if let Err(e) = std::fs::create_dir_all(&self.state_dir) { | |
| tracing::warn!("Failed to create Tor state directory: {}", e); | |
| } |
🤖 Prompt for AI Agents
In `@crates/notedeck/src/tor.rs` around lines 513 - 515, The directory creation
calls currently swallow errors (let _ =
std::fs::create_dir_all(&self.cache_dir); let _ =
std::fs::create_dir_all(&self.state_dir);), which can hide failures that cause
the subsequent JNI initialize() to fail; change these to propagate or log errors
instead: call std::fs::create_dir_all(...) and handle the Result (use ? to
return the error from the enclosing function or map_err to convert into your
crate's error type) for both self.cache_dir and self.state_dir, and only call
JNI initialize() after both creations succeed (or at minimum log the path and
error via your logger before proceeding).
Move keyring back to the platform-gated dependencies section to prevent WASM and Android build failures. The keyring crate doesn't support these platforms. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Change "relay traffic" to "network traffic" since we route both WebSocket relay connections and HTTP media fetches through Tor. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Instead of hardcoding port 9150 (which conflicts with Tor Browser), automatically probe for an available port starting from 9150. This avoids silent failures when another application is using the port. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add comprehensive test coverage for: - TorManager state machine (Disabled -> Starting -> Running -> Disabled) - Enable/disable idempotency - Platform-specific behavior (desktop vs WASM) - HttpConfig for SOCKS proxy configuration - StaticImgTexCache proxy configuration Tests verify correct state transitions without requiring actual Tor network connectivity, making them suitable for CI environments. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Address three critical issues in Tor implementation: 1. [P1] Fix traffic leak during Tor bootstrap: - Add `should_allow_connections()` method to TorManager - Block relay connections during Starting state - Skip accounts.update() when Tor is bootstrapping - Prevents IP leaks before SOCKS proxy is ready (tor.rs:161-169, app.rs:147-151) 2. [P1] Fix runtime failure detection after bootstrap: - Monitor thread health in poll() after bootstrap completes - Detect when runtime thread exits unexpectedly - Update status to Failed if thread terminates (tor.rs:207-217) 3. [P2] Fix UI blocking during Tor shutdown: - Make stop() non-blocking by spawning cleanup thread - Send shutdown signal immediately without waiting - Background thread handles join with timeout - Prevents 5s UI freeze when disabling Tor (tor.rs:58-92) Add tests for new should_allow_connections() behavior. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
1 participant
🧅 Experimental Tor Branch 🧅
Built with rust Arti
https://github.com/zydou/arti
Puffin profile + top right icon showing Tor is disconnected
Puffin profile + top right icon showing Tor is connected
OS limitations
Linus, macOS + Windows run the full TorManager (crates/notedeck/src/tor.rs:1-
259), so the desktop builds spawn the bundled Arti runtime in a background
thread, expose the SOCKS proxy on 127.0.0.1:, and toggle the VPN-style
badge plus relay transport automatically. No platform gating is in place for
those OSes.
that stub module). On macOS/Windows/Linux the Tor toggle, status polling,
and WebSocket transport switching all operate the same way.
Summary
in Settings → Network, wiring the toggle through SettingsAction so enabling/
disabling Tor reconfigures relay sockets without restarting the app.
patched ewebsock that supports SOCKS5 handshakes—enabling all relay traffic
to transparently route through Tor when the toggle is on.
in the top-right corner with color/iconography for “connected,” “starting,”
“disabled,” and “error,” mirroring the official Tor palette to make the
state immediately visible.
Testing
through Connected → Disabled and verifying relays flip between direct &
SOCKS options).
Summary by CodeRabbit
New Features
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.