fix(wasm): relax EncHandler Send+Sync via MaybeSendSync, gate async_trait - #793
Merged
Merged
Conversation
…rait EncHandler is a public custom-enc extension point (BotBuilder::with_enc_handler, stored as Arc<dyn EncHandler> on Client), but unlike its sibling extension points EventHandler and SendContextResolver it hardcoded a Send + Sync supertrait and a plain async_trait with no wasm32 gate. The high-level crate builds for wasm32, where the client is intentionally !Send and a handler may capture !Send JS handles, so a wasm custom enc handler failed to compile on the supertrait bound. Mirror the established convention: supertrait becomes wacore::sync_marker::MaybeSendSync (Send + Sync on native, no bound on wasm32) and async_trait gets the dual cfg_attr(?Send) gate. Native behavior is unchanged: the blanket MaybeSendSync impl keeps dyn EncHandler Send + Sync, so the Arc<dyn EncHandler> cross-thread storage is untouched. Verified both the native build/tests and the wasm32 lib build.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe ChangesEncHandler Platform Compatibility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Look, this is a clean change. The key thing here is: does this actually work correctly on both wasm32 and native? You're using The concern I have is: make sure all the implementations of 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/types/enc_handler.rs (1)
54-55: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider matching the trait's conditional async_trait pattern in test code.
Look, the MockEncHandler impl uses unconditional
#[async_trait::async_trait], but the trait itself is now conditional. These tokio tests won't run on wasm32 anyway, so it's not breaking anything right now. But if we want this to scale and stay consistent, we should probably make the test impl match:+ #[cfg_attr(target_arch = "wasm32", async_trait::async_trait(?Send))] + #[cfg_attr(not(target_arch = "wasm32"), async_trait::async_trait)] - #[async_trait::async_trait] impl EncHandler for MockEncHandler {This way, if someone adds wasm32 tests later, things just work. Consistency matters when you're building something at scale.
🤖 Prompt for 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. In `@src/types/enc_handler.rs` around lines 54 - 55, The MockEncHandler impl uses an unconditional #[async_trait::async_trait] but the EncHandler trait is conditionally async; update the impl to use the same conditional attribute pattern as the trait (e.g., replace the unconditional #[async_trait::async_trait] above impl EncHandler for MockEncHandler with the matching cfg_attr/conditional form used on the EncHandler trait) so the test impl mirrors the trait's async_trait conditionalization and remains consistent across targets.
🤖 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.
Outside diff comments:
In `@src/types/enc_handler.rs`:
- Around line 54-55: The MockEncHandler impl uses an unconditional
#[async_trait::async_trait] but the EncHandler trait is conditionally async;
update the impl to use the same conditional attribute pattern as the trait
(e.g., replace the unconditional #[async_trait::async_trait] above impl
EncHandler for MockEncHandler with the matching cfg_attr/conditional form used
on the EncHandler trait) so the test impl mirrors the trait's async_trait
conditionalization and remains consistent across targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 21204dfe-36e3-483f-9c43-c21d227395e3
📒 Files selected for processing (1)
src/types/enc_handler.rs
Benchmark Results67 unchanged benchmark(s)
|
This was referenced Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
EncHandleris a public custom-enc extension point (BotBuilder::with_enc_handler, stored asArc<dyn EncHandler>onClient). Unlike its sibling extension pointsEventHandlerandSendContextResolver, it hardcoded aSend + Syncsupertrait and a plain#[async_trait]with no wasm32 gate.The high-level crate builds for wasm32, where the client is intentionally
!Sendand a handler may capture!SendJS handles. So a wasm custom enc handler failed to compile on the supertrait bound.This mirrors the established convention: the supertrait becomes
wacore::sync_marker::MaybeSendSync(which isSend + Syncon native, no bound on wasm32) andasync_traitgets the dualcfg_attr(..., async_trait(?Send))gate.Why
Unblocks a
!Send-handle-backed custom enc handler on the wasm port, consistent with howEventHandler(events.rs) andSendContextResolver(context.rs) already relax their bounds.Risk
None on native: the blanket
MaybeSendSyncimpl keepsdyn EncHandlerSend + Sync, so theArc<dyn EncHandler>cross-thread storage onClientis untouched. The onlyimpl EncHandlerblocks are test-only (native). Verified the native build plus tests AND the wasm32 lib build (cargo build -p whatsapp-rust --lib --release --target wasm32-unknown-unknown --no-default-features --features debug-diagnostics).