refactor: replace RefCell with UnsafeCell for cached attributes and content in InternalBinaryNode

Author: jlucaso1

src/binary.rs

@@ -1,6 +1,6 @@
 use js_sys::{Array, Object, Uint8Array};
 use std::borrow::Cow;
-use std::cell::RefCell;
+use std::cell::UnsafeCell;
 use std::mem;
 use std::rc::Rc;
 use wacore_binary::{
@@ -120,16 +120,29 @@ pub struct InternalBinaryNode {
     _owned_data: Rc<[u8]>,
     node_ref: NodeRef<'static>,
     #[wasm_bindgen(skip)]
-    cached_attrs: RefCell<Option<Attrs>>,
+    cached_attrs: UnsafeCell<Option<Attrs>>,
     #[wasm_bindgen(skip)]
-    cached_content: RefCell<Option<Content>>,
+    cached_content: UnsafeCell<Option<Content>>,
 }
 
 impl InternalBinaryNode {
     #[inline(always)]
     fn node_ref(&self) -> &NodeRef<'static> {
         &self.node_ref
     }
+
+    #[inline]
+    fn convert_attrs(attrs: &[(Cow<'_, str>, ValueRef<'_>)]) -> Attrs {
+        let obj = Object::new();
+        for (k, v) in attrs.iter() {
+            let js_value = match v.as_str() {
+                Some(s) => JsValue::from_str(s),
+                None => JsValue::from_str(&v.to_string()),
+            };
+            let _ = js_sys::Reflect::set(&obj, &JsValue::from_str(k), &js_value);
+        }
+        obj.unchecked_into()
+    }
 }
 
 #[wasm_bindgen]
@@ -183,72 +196,63 @@ impl InternalBinaryNode {
 
     #[wasm_bindgen(getter)]
     pub fn attrs(&self) -> Attrs {
-        let mut cached = self.cached_attrs.borrow_mut();
-        if cached.is_none() {
-            let attrs = &self.node_ref().attrs;
-
-            let obj = Object::new();
-            for (k, v) in attrs.iter() {
-                let js_value = match v.as_str() {
-                    Some(s) => JsValue::from_str(s),
-                    None => JsValue::from_str(&v.to_string()),
-                };
-                let _ = js_sys::Reflect::set(&obj, &JsValue::from_str(k), &js_value);
-            }
-
-            *cached = Some(obj.unchecked_into());
+        // SAFETY: WASM is single-threaded
+        let cached = unsafe { &mut *self.cached_attrs.get() };
+        if let Some(attrs) = cached.as_ref() {
+            return attrs.clone();
         }
 
-        cached
-            .as_ref()
-            .expect("Cached attributes should be populated before access")
-            .clone()
-            .unchecked_into()
+        let attrs = Self::convert_attrs(&self.node_ref().attrs);
+        *cached = Some(attrs.clone());
+        attrs
     }
 
     #[wasm_bindgen(setter)]
     pub fn set_attrs(&self, new_attrs: Attrs) {
-        *self.cached_attrs.borrow_mut() = Some(new_attrs);
+        // SAFETY: WASM is single-threaded
+        unsafe { *self.cached_attrs.get() = Some(new_attrs) };
     }
 
     #[wasm_bindgen(getter)]
     pub fn content(&self) -> Option<Content> {
-        let mut cached = self.cached_content.borrow_mut();
-        if cached.is_none() {
-            match self.node_ref().content.as_deref() {
-                Some(NodeContentRef::Bytes(bytes)) => {
-                    let bytes_ref = bytes.as_ref();
-                    let u8arr = Uint8Array::new_with_length(bytes_ref.len() as u32);
-                    u8arr.copy_from(bytes_ref);
-                    *cached = Some(u8arr.unchecked_into());
-                }
-                Some(NodeContentRef::String(s)) => {
-                    *cached = Some(JsValue::from_str(s).unchecked_into());
-                }
-                Some(NodeContentRef::Nodes(nodes)) => {
-                    let arr = Array::new_with_length(nodes.len() as u32);
-                    for (i, node_ref) in nodes.iter().enumerate() {
-                        let child = InternalBinaryNode {
-                            _owned_data: Rc::clone(&self._owned_data),
-                            node_ref: node_ref.clone(),
-                            cached_attrs: RefCell::new(None),
-                            cached_content: RefCell::new(None),
-                        };
-                        arr.set(i as u32, child.into());
-                    }
-                    *cached = Some(arr.unchecked_into());
+        // SAFETY: WASM is single-threaded
+        let cached = unsafe { &mut *self.cached_content.get() };
+        if let Some(content) = cached.as_ref() {
+            return Some(content.clone());
+        }
+
+        let result: Option<Content> = match self.node_ref().content.as_deref() {
+            Some(NodeContentRef::Bytes(bytes)) => {
+                let bytes_ref = bytes.as_ref();
+                let u8arr = Uint8Array::new_with_length(bytes_ref.len() as u32);
+                u8arr.copy_from(bytes_ref);
+                Some(u8arr.unchecked_into())
+            }
+            Some(NodeContentRef::String(s)) => Some(JsValue::from_str(s).unchecked_into()),
+            Some(NodeContentRef::Nodes(nodes)) => {
+                let arr = Array::new_with_length(nodes.len() as u32);
+                for (i, node_ref) in nodes.iter().enumerate() {
+                    let child = InternalBinaryNode {
+                        _owned_data: Rc::clone(&self._owned_data),
+                        node_ref: node_ref.clone(),
+                        cached_attrs: UnsafeCell::new(None),
+                        cached_content: UnsafeCell::new(None),
+                    };
+                    arr.set(i as u32, child.into());
                 }
-                None => *cached = Some(JsValue::undefined().unchecked_into()),
+                Some(arr.unchecked_into())
             }
-        }
-        cached
-            .as_ref()
-            .map(|v| v.clone().unchecked_into::<Content>())
+            None => None,
+        };
+
+        *cached = result.clone();
+        result
     }
 
     #[wasm_bindgen(setter)]
     pub fn set_content(&self, new_content: Content) {
-        *self.cached_content.borrow_mut() = Some(new_content);
+        // SAFETY: WASM is single-threaded
+        unsafe { *self.cached_content.get() = Some(new_content) };
     }
 }
 
@@ -281,7 +285,7 @@ pub fn decode_node(data: Vec<u8>) -> Result<InternalBinaryNode, JsValue> {
     Ok(InternalBinaryNode {
         _owned_data: owned_data,
         node_ref,
-        cached_attrs: RefCell::new(None),
-        cached_content: RefCell::new(None),
+        cached_attrs: UnsafeCell::new(None),
+        cached_content: UnsafeCell::new(None),
     })
 }

test/binary.test.ts

@@ -233,17 +233,15 @@ test("should allow attrs mutation and reassignment", () => {
   expect(roundtrip.attrs.hello).toBe("world");
 });
 
-// Add to the describe("Binary Marshalling") block
 test("should allow content mutation and reassignment", () => {
   const original: BinaryNode = {
     tag: "patch",
     attrs: {},
-    content: [{ tag: "item", attrs: {}, content: "old" }], // Array of children
+    content: [{ tag: "item", attrs: {}, content: "old" }],
   };
   const binary = encodeNode(original);
   const node = decodeNode(binary);
 
-  // Mutation: Push to array
   (node.content as BinaryNode[]).push({
     tag: "new-item",
     attrs: {},
@@ -252,18 +250,15 @@ test("should allow content mutation and reassignment", () => {
   expect((node.content as BinaryNode[]).length).toBe(2);
   expect((node.content as BinaryNode[])[1].content).toBe("added");
 
-  // Reassignment: Replace with string
   node.content = "replaced with string";
   expect(typeof node.content).toBe("string");
   expect(node.content).toBe("replaced with string");
 
-  // Reassignment: Back to array
   node.content = [
     { tag: "final", attrs: {}, content: new Uint8Array([1, 2, 3]) },
   ];
   expect((node.content as BinaryNode[])[0].content).toBeInstanceOf(Uint8Array);
 
-  // Round-trip re-encode (mutations persist)
   const reencoded = encodeNode(node);
   const roundtrip = decodeNode(reencoded);
   expect(Array.isArray(roundtrip.content)).toBe(true);
@@ -273,47 +268,42 @@ test("should allow content mutation and reassignment", () => {
   );
 });
 
-// Edge: Binary content mutation (e.g., Uint8Array slicing, but rare in Baileys)
 test("should handle binary content reassignment", () => {
   const binContent = new Uint8Array([10, 20, 30]);
   const node: BinaryNode = { tag: "enc", attrs: {}, content: binContent };
   const binary = encodeNode(node);
   const decoded = decodeNode(binary);
 
-  // Reassign to new bytes
   decoded.content = new Uint8Array([40, 50, 60]);
   expect(decoded.content).toEqual(new Uint8Array([40, 50, 60]));
 
-  // Re-encode
   const reencoded = encodeNode(decoded);
   const roundtrip = decodeNode(reencoded);
   expect(roundtrip.content).toEqual(new Uint8Array([40, 50, 60]));
 });
+
 test("should handle non-string attribute values during encoding (lenient conversion)", () => {
-  // Test node with mixed attr types
   const original: BinaryNode = {
     tag: "test-node",
     attrs: {
       // @ts-ignore
-      id: 123, // Number -> "123"
+      id: 123,
       // @ts-ignore
-      active: true, // Boolean -> "true"
+      active: true,
       // @ts-ignore
-      inactive: false, // Boolean -> "false"
-      version: "1.0", // String (unchanged)
+      inactive: false,
+      version: "1.0",
       // @ts-ignore
-      foo: null, // Null -> skipped (not present)
+      foo: null,
       // @ts-ignore
-      bar: undefined, // Undefined -> skipped (not present)
-      // Note: Objects in attrs will be coerced via toString(), but test simple cases
+      bar: undefined,
     },
     content: "test content",
   };
 
   const binary = encodeNode(original);
   const decoded = decodeNode(binary);
 
-  // Verify encoded attrs are all strings, with correct conversions
   const attrs = decoded.attrs;
   expect(attrs).toBeDefined();
   expect(typeof attrs.id).toBe("string");
@@ -323,15 +313,13 @@ test("should handle non-string attribute values during encoding (lenient convers
   expect(attrs.inactive).toBe("false");
   expect(attrs.version).toBe("1.0");
 
-  // Skipped values should be absent
   expect(attrs.foo).toBeUndefined();
   expect(attrs.bar).toBeUndefined();
 
-  // Round-trip: Re-encode and check persistence (non-strings become strings)
   const reencoded = encodeNode(decoded);
   const roundtrip = decodeNode(reencoded);
   const roundtripAttrs = roundtrip.attrs;
-  expect(roundtripAttrs.id).toBe("123"); // Persists as string
+  expect(roundtripAttrs.id).toBe("123");
   expect(roundtripAttrs.active).toBe("true");
 });
 
@@ -350,7 +338,7 @@ test("should skip empty string values in attrs during encoding", () => {
   const decoded = decodeNode(binary);
 
   const attrs = decoded.attrs;
-  expect(attrs.emptyStr).toBeUndefined(); // Skipped (empty)
-  expect(attrs.whitespace).toBeUndefined(); // Skipped (after trim? Wait, code uses unwrap_or_default(), so "" -> skipped if empty)
-  expect(attrs.valid).toBe("ok"); // Preserved
+  expect(attrs.emptyStr).toBeUndefined();
+  expect(attrs.whitespace).toBeUndefined();
+  expect(attrs.valid).toBe("ok");
 });

ts/index.ts

@@ -2,7 +2,6 @@ import { initSync } from "../pkg/whatsapp_rust_bridge.js";
 
 import { base64Wasm } from "./macro.js" with { type: "macro" };
 
-// Web-compatible base64 decoding (works in both Node.js and browsers)
 function base64ToUint8Array(base64: string): Uint8Array {
   const binaryString = atob(base64);
   const bytes = new Uint8Array(binaryString.length);