Address comments

Author: minggangw

lib/action/client.js

@@ -70,6 +70,8 @@ class ActionClient extends Entity {
     this._pendingGoalRequests = new Map();
     this._pendingCancelRequests = new Map();
     this._pendingResultRequests = new Map();
+    this._filterGoalIds = new Set();
+    this._filterGoalIdBuffers = new Map();
 
     // Setup options defaults
     this._options = this._options || {};
@@ -509,25 +511,53 @@ class ActionClient extends Entity {
     this._pendingCancelRequests.delete(sequenceNumber);
   }
 
+  /**
+   * Disable feedback optimization and best-effort remove all tracked goal IDs
+   * from the native filter so the subscription reverts to unfiltered state.
+   * @ignore
+   * @param {string} reason - Warning message describing why optimization was disabled.
+   */
+  _disableFeedbackMsgOptimization(reason) {
+    this._enableFeedbackMsgOptimization = false;
+
+    // Best-effort remove every goal ID still in the native filter so that
+    // the subscription reverts to an unfiltered state and future feedback
+    // is not silently dropped.
+    for (const [uuid, goalIdBuf] of this._filterGoalIdBuffers) {
+      try {
+        rclnodejs.actionConfigureFeedbackSubFilterRemoveGoalId(
+          this.handle,
+          goalIdBuf
+        );
+      } catch {
+        // Intentionally swallowed — we are already disabling.
+      }
+    }
+    this._filterGoalIds.clear();
+    this._filterGoalIdBuffers.clear();
+
+    this._node
+      .getLogger()
+      .warn(
+        `${reason}\nFeedback message optimization is automatically disabled.`
+      );
+  }
+
   /**
    * Add a goal ID to the feedback subscription content filter.
    * @ignore
    * @param {object} goalId - The goal UUID message.
    */
   _feedbackSubFilterAddGoalId(goalId) {
     if (!this._enableFeedbackMsgOptimization) return;
+    const uuid = ActionUuid.fromMessage(goalId).toString();
+    const buf = Buffer.from(goalId.uuid);
     try {
-      rclnodejs.actionConfigureFeedbackSubFilterAddGoalId(
-        this.handle,
-        Buffer.from(goalId.uuid)
-      );
+      rclnodejs.actionConfigureFeedbackSubFilterAddGoalId(this.handle, buf);
+      this._filterGoalIds.add(uuid);
+      this._filterGoalIdBuffers.set(uuid, buf);
     } catch (e) {
-      this._enableFeedbackMsgOptimization = false;
-      this._node
-        .getLogger()
-        .warn(
-          `${e.message}\nFeedback message optimization is automatically disabled.`
-        );
+      this._disableFeedbackMsgOptimization(e.message);
     }
   }
 
@@ -538,18 +568,18 @@ class ActionClient extends Entity {
    */
   _feedbackSubFilterRemoveGoalId(goalId) {
     if (!this._enableFeedbackMsgOptimization) return;
+    const uuid = ActionUuid.fromMessage(goalId).toString();
+    if (!this._filterGoalIds.has(uuid)) return;
+    this._filterGoalIds.delete(uuid);
+    const buf = this._filterGoalIdBuffers.get(uuid);
+    this._filterGoalIdBuffers.delete(uuid);
     try {
       rclnodejs.actionConfigureFeedbackSubFilterRemoveGoalId(
         this.handle,
-        Buffer.from(goalId.uuid)
+        buf || Buffer.from(goalId.uuid)
       );
     } catch (e) {
-      this._enableFeedbackMsgOptimization = false;
-      this._node
-        .getLogger()
-        .warn(
-          `${e.message}\nFeedback message optimization is automatically disabled.`
-        );
+      this._disableFeedbackMsgOptimization(e.message);
     }
   }
 

test/test-subscription-content-filter.js

@@ -480,3 +480,36 @@ describe('subscription content-filtering', function () {
     done();
   });
 });
+
+describe('subscription isContentFilterSupported', function () {
+  this.timeout(30 * 1000);
+
+  beforeEach(async function () {
+    await rclnodejs.init();
+    this.node = new Node('cft_support_test_node');
+  });
+
+  afterEach(function () {
+    this.node.destroy();
+    rclnodejs.shutdown();
+  });
+
+  it('isContentFilterSupported returns boolean matching RMW capability', function (done) {
+    const typeclass = 'std_msgs/msg/Int16';
+    const subscription = this.node.createSubscription(
+      typeclass,
+      TOPIC,
+      (msg) => {}
+    );
+
+    const supported = subscription.isContentFilterSupported();
+    assert.strictEqual(typeof supported, 'boolean');
+
+    // isContentFilterSupported requires rolling; on older distros it returns false
+    const isRolling = DistroUtils.getDistroId() >= DistroUtils.DistroId.ROLLING;
+    const expectedSupported = isRolling && isContentFilteringSupported();
+    assert.strictEqual(supported, expectedSupported);
+
+    done();
+  });
+});