ci(wasm): guard whatsapp-rust wasm32 build and fix two #668 regressions - #671
Conversation
#668 added two uses that only compile on native and broke the wasm32 build that whatsapp-rust-bridge relies on. The `tokio` unresolved-module error (E0433) aborts name resolution before type-checking, so it masked a second regression behind it. 1. message.rs called `tokio::time::timeout` directly, which is absent under `--no-default-features` (wasm). Use the agnostic `wacore::runtime::timeout` over `self.runtime`, matching request/handshake/media_reupload. 2. the new `original_message_resolver` field holds a `dyn OriginalMessageResolver`, which is intentionally `?Send` on wasm (JS handles), making `Client` `!Send`. `SendContextResolver` and `EventHandler` required `Send + Sync` unconditionally, breaking every impl on wasm. Relax them to a conditional `wacore::sync_marker::MaybeSendSync` (still `Send + Sync` on native via a blanket impl, transparent there; dropped on wasm). Add `.github/workflows/wasm.yml` building the lib for wasm32 in release, mirroring the bridge's flags, so this class of regression fails CI.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds WASM32 target support by introducing a conditional synchronization marker trait, applying it across the trait hierarchy to relax thread-safety requirements on WASM, integrating runtime-aware timeout abstractions, and configuring CI/CD and dependency management for WASM builds. ChangesWASM32 Support with Conditional Synchronization
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Runtime as wacore::runtime
participant Resolver as msg_secret_resolver
Client->>Runtime: timeout(&*self.runtime, duration, lookup)
Runtime->>Resolver: poll lookup future
Resolver-->>Runtime: Ok(Some(secret)) / Ok(None) / Err
Runtime-->>Client: propagate result (Some/None/timeout warning)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 150-154: The getrandom dependency is configured with feature
"wasm_js" but the target gate uses target_arch = "wasm32", which is too broad
and will force wasm_js for non-browser wasm (e.g., wasm32-wasi) and can break
builds; change the target predicate in Cargo.toml to the specific
browser/unknown wasm triple(s) you support (e.g., cfg(target =
"wasm32-unknown-unknown")) or enumerate supported targets, or alternatively
document that this crate only supports browser wasm; also note that getrandom's
"wasm_js" is usually intended for binaries/tests because it pulls in
wasm-bindgen/js-sys if this crate is a library and consider moving the wasm_js
selection to binary-specific Cargo.toml sections instead of the library
dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e67ae5a9-c15e-49c9-88d3-ec1e945c1395
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/wasm.ymlCargo.tomlsrc/message.rswacore/src/client/context.rswacore/src/lib.rswacore/src/sync_marker.rswacore/src/types/events.rs
cfg(target_arch = "wasm32") also matched wasm32-wasi/emscripten, forcing the browser backend (and wasm-bindgen/js-sys) onto targets that have their own getrandom backend. Gate to the unknown-unknown triple per getrandom's guidance; it's the only target without a default backend, where wasm_js is the sole valid choice anyway.
Benchmark Results67 unchanged benchmark(s)
|
What
Adds a CI job that builds the
whatsapp-rustlib forwasm32-unknown-unknownin release, and fixes two regressions from #668 that broke that build (the onewhatsapp-rust-bridgeconsumes).The catch: the
tokiounresolved-module error (E0433) aborts name resolution before type-checking, so it masked a second wasm regression sitting right behind it. The bridge build only ever surfaced thetokioerror.Fix 1 —
tokio::time::timeoutinmessage.rs.resolve_msg_secret_via_appcalledtokio::time::timeoutdirectly, which doesn't exist under--no-default-features(notokio-runtimefeature → notokiodep). Swapped for the runtime-agnosticwacore::runtime::timeout(&*self.runtime, ...), the same helper already used byrequest.rs,handshake.rs,flush_scope.rsandmedia_reupload.rs. Thematcharms are unchanged (it returnsResult<T, Elapsed>, same shape astokio's).Fix 2 —
Send + Syncbounds vs the wasm?Sendresolver. #668 addedoriginal_message_resolver: Option<Arc<dyn OriginalMessageResolver>>. That trait is, by design,?Sendon wasm (documented inmsg_secret.rs— it may be backed by!SendJS handles), which makesClient!Send/!Syncon wasm. ButSendContextResolverandEventHandlerrequiredSend + Syncunconditionally, so all 6impls (context_impl.rs,send.rs,bot.rs) failed to compile on wasm. Introducedwacore::sync_marker::MaybeSendSync— a conditional bound that isSend + Syncon native (via a blanket impl over everySend + Synctype) and empty on wasm — and switched both supertraits to it.Why
whatsapp-rust-bridgecompileswhatsapp-rustto wasm32 with--no-default-features --features debug-diagnostics. Nothing in this repo's CI exercised that target, so a single accidental commit could (and did) break downstream wasm builds without turning anything red here. The newwasm.ymljob mirrors the bridge's flags so this class of regression fails CI instead of reaching a consumer.Compatibility note
The
MaybeSendSyncrelaxation is transparent on native: a transitive supertraitSend + Syncstill propagates todyn SendContextResolver/dyn EventHandler(verified with a standalone PoC —Box<dyn _>: Sendstill holds), so no native consumer loses a bound. It only relaxes wasm. The crate also gains agetrandomdependency gated tocfg(target_arch = "wasm32")(enables thewasm_jsbackend feature so the lib builds standalone for wasm); it is a no-op on native targets.Tests
cargo build -p whatsapp-rust --lib --release --target wasm32-unknown-unknown --no-default-features --features debug-diagnostics(withRUSTFLAGS='--cfg getrandom_backend="wasm_js"') — clean (was 1 error before fix 1, 6 errors after, 0 now)cargo build --workspace --exclude e2e-tests(native) — cleancargo clippy -p wacore -p whatsapp-rust --all-targets(native) — cleancargo fmt --all