From adf155d40e03293a095a789285543bdd3951aefb Mon Sep 17 00:00:00 2001
From: Pierre Le Fevre <hello@pierrelf.com>
Date: Sun, 22 Mar 2026 15:41:47 +0100
Subject: [PATCH] Review fixes: remove redundant reaction, use named constants,
 add allSettled test

- Remove orphaned add_reaction call in promise_native_resolve (only
  chain_promise is needed for pending thenables)
- Expose PROMISE_FULFILLED, PROMISE_REJECTED, and PROMISE_RESULT_KEY
  as pub constants and use them in drain_microtasks instead of magic
  numbers
- Add test_promise_all_settled test for Promise.allSettled coverage

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 crates/js/src/builtins.rs | 18 +++++-------------
 crates/js/src/vm.rs       | 26 ++++++++++++++++++++------
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/crates/js/src/builtins.rs b/crates/js/src/builtins.rs
index 4834f52..6023146 100644
--- a/crates/js/src/builtins.rs
+++ b/crates/js/src/builtins.rs
@@ -4667,12 +4667,12 @@ fn weakset_proto_delete(args: &[Value], ctx: &mut NativeContext) -> Result<Value
 
 /// Promise states.
 const PROMISE_PENDING: f64 = 0.0;
-const PROMISE_FULFILLED: f64 = 1.0;
-const PROMISE_REJECTED: f64 = 2.0;
+pub const PROMISE_FULFILLED: f64 = 1.0;
+pub const PROMISE_REJECTED: f64 = 2.0;
 
 /// Hidden property keys for Promise objects.
 const PROMISE_STATE_KEY: &str = "__promise_state__";
-const PROMISE_RESULT_KEY: &str = "__promise_result__";
+pub const PROMISE_RESULT_KEY: &str = "__promise_result__";
 const PROMISE_REACTIONS_KEY: &str = "__promise_reactions__";
 const PROMISE_REACTION_COUNT_KEY: &str = "__promise_reaction_count__";
 
@@ -4987,16 +4987,8 @@ fn promise_native_resolve(args: &[Value], ctx: &mut NativeContext) -> Result<Val
             let reason = promise_get_prop(ctx.gc, value_ref, PROMISE_RESULT_KEY);
             reject_promise_internal(ctx.gc, promise_ref, reason);
         } else {
-            // Pending thenable: register a reaction to propagate settlement.
-            add_reaction(
-                ctx.gc,
-                value_ref,
-                Value::Undefined, // identity — will be handled by microtask drain
-                Value::Undefined,
-            );
-            // Actually, we need to resolve/reject promise_ref when value_ref settles.
-            // Add reactions that call __Promise_resolve/__Promise_reject on promise_ref.
-            // For simplicity, store a direct chain.
+            // Pending thenable: chain so that when value_ref settles,
+            // promise_ref settles the same way.
             chain_promise(ctx.gc, value_ref, promise_ref);
         }
     } else {
diff --git a/crates/js/src/vm.rs b/crates/js/src/vm.rs
index 4951787..ca4a6f9 100644
--- a/crates/js/src/vm.rs
+++ b/crates/js/src/vm.rs
@@ -868,9 +868,9 @@ impl Vm {
                                                 crate::builtins::promise_get_prop_pub(
                                                     &self.gc,
                                                     parent_ref,
-                                                    "__promise_result__",
+                                                    crate::builtins::PROMISE_RESULT_KEY,
                                                 );
-                                            if parent_state == 1.0 {
+                                            if parent_state == crate::builtins::PROMISE_FULFILLED {
                                                 crate::builtins::resolve_promise_internal(
                                                     &mut self.gc,
                                                     chained,
@@ -903,22 +903,23 @@ impl Vm {
                                                 let state = crate::builtins::promise_state_pub(
                                                     &self.gc, val_ref,
                                                 );
-                                                if state == 1.0 {
+                                                if state == crate::builtins::PROMISE_FULFILLED {
                                                     let r = crate::builtins::promise_get_prop_pub(
                                                         &self.gc,
                                                         val_ref,
-                                                        "__promise_result__",
+                                                        crate::builtins::PROMISE_RESULT_KEY,
                                                     );
                                                     crate::builtins::resolve_promise_internal(
                                                         &mut self.gc,
                                                         chained,
                                                         r,
                                                     );
-                                                } else if state == 2.0 {
+                                                } else if state == crate::builtins::PROMISE_REJECTED
+                                                {
                                                     let r = crate::builtins::promise_get_prop_pub(
                                                         &self.gc,
                                                         val_ref,
-                                                        "__promise_result__",
+                                                        crate::builtins::PROMISE_RESULT_KEY,
                                                     );
                                                     crate::builtins::reject_promise_internal(
                                                         &mut self.gc,
@@ -5423,4 +5424,17 @@ mod tests {
             v => panic!("expected 99, got {v:?}"),
         }
     }
+
+    #[test]
+    fn test_promise_all_settled() {
+        match eval_global(
+            "Promise.allSettled([Promise.resolve(1), Promise.reject('err')]).then(function(v) { result = v[0].status + ',' + v[1].status; });",
+            "result",
+        )
+        .unwrap()
+        {
+            Value::String(s) => assert_eq!(s, "fulfilled,rejected"),
+            v => panic!("expected 'fulfilled,rejected', got {v:?}"),
+        }
+    }
 }
-- 
2.51.2