♻️ (dmk): Handle PR comments

Author: aussedatlo

packages/device-management-kit/src/internal/device-session/model/DeviceSession.test.ts

@@ -314,15 +314,17 @@ describe("DeviceSession", () => {
       // Advance timers to trigger timeout
       vi.advanceTimersByTime(1100);
 
+      let caughtError: Error | undefined;
       try {
         await sendPromise;
-      } catch {
-        // Expected to timeout
+      } catch (error) {
+        caughtError = error as Error;
       }
 
       vi.useRealTimers();
 
       // then
+      expect(caughtError).toBeDefined();
       expect(mockIntentQueueService.enqueue).toHaveBeenCalled();
     });
   });
@@ -491,7 +493,7 @@ describe("DeviceSession", () => {
         execute: expect.any(Function),
       });
       expect(observable).toBeDefined();
-      expect(cancel).toBe(mockCancel);
+      expect(cancel).toBeTypeOf("function");
 
       // Verify observable emits values
       const values: unknown[] = [];
@@ -508,6 +510,10 @@ describe("DeviceSession", () => {
           requiredUserInteraction: "SignTransaction",
         },
       ]);
+
+      // Verify cancel is called
+      cancel();
+      expect(mockCancel).toHaveBeenCalled();
     });
 
     it("should provide correct internal API to device action", () => {

packages/device-management-kit/src/internal/device-session/model/DeviceSession.ts

@@ -227,51 +227,36 @@ export class DeviceSession {
     options: SendApduOptions = {
       isPolling: false,
       triggersDisconnection: false,
+      abortTimeout: undefined,
     },
   ): Promise<Either<DmkError, ApduResponse>> {
-    const abortTimeout = (options as SendApduOptions).abortTimeout;
-
-    const sendApduPromise = async () => {
-      this._logger.debug(`[exchange] => ${bufferToHexaString(rawApdu, false)}`);
-      const result = await this._connectedDevice.sendApdu(
-        rawApdu,
-        options.triggersDisconnection,
-        abortTimeout,
-      );
-
-      result
-        .ifRight((response: ApduResponse) => {
-          this._logger.debug(
-            `[exchange] <= ${bufferToHexaString(response.data, false)}${bufferToHexaString(response.statusCode, false)}`,
-          );
-          if (CommandUtils.isLockedDeviceResponse(response)) {
-            this._sessionEventDispatcher.dispatch({
-              eventName: SessionEvents.DEVICE_STATE_UPDATE_LOCKED,
-            });
-          } else {
-            this._sessionEventDispatcher.dispatch({
-              eventName: SessionEvents.DEVICE_STATE_UPDATE_CONNECTED,
-            });
-          }
-        })
-        .ifLeft(() => {
+    this._logger.debug(`[exchange] => ${bufferToHexaString(rawApdu, false)}`);
+    const result = await this._connectedDevice.sendApdu(
+      rawApdu,
+      options.triggersDisconnection,
+      options.abortTimeout,
+    );
+
+    return result
+      .ifRight((response: ApduResponse) => {
+        this._logger.debug(
+          `[exchange] <= ${bufferToHexaString(response.data, false)}${bufferToHexaString(response.statusCode, false)}`,
+        );
+        if (CommandUtils.isLockedDeviceResponse(response)) {
+          this._sessionEventDispatcher.dispatch({
+            eventName: SessionEvents.DEVICE_STATE_UPDATE_LOCKED,
+          });
+        } else {
           this._sessionEventDispatcher.dispatch({
-            eventName: SessionEvents.DEVICE_STATE_UPDATE_UNKNOWN,
+            eventName: SessionEvents.DEVICE_STATE_UPDATE_CONNECTED,
           });
+        }
+      })
+      .ifLeft(() => {
+        this._sessionEventDispatcher.dispatch({
+          eventName: SessionEvents.DEVICE_STATE_UPDATE_UNKNOWN,
         });
-      return result;
-    };
-
-    if (abortTimeout) {
-      return Promise.race([
-        sendApduPromise(),
-        new Promise<never>((_, reject) => {
-          setTimeout(() => reject(new SendApduTimeoutError()), abortTimeout);
-        }),
-      ]);
-    }
-
-    return sendApduPromise();
+      });
   }
 
   public sendCommand<Response, Args, ErrorStatusCodes>(
@@ -328,41 +313,28 @@ export class DeviceSession {
     command: Command<Response, Args, ErrorStatusCodes>,
     abortTimeout?: number,
   ): Promise<CommandResult<Response, ErrorStatusCodes>> {
-    const sendCommandPromise = async () => {
-      const apdu = command.getApdu();
-
-      const response = await this._unsafeInternalSendApdu(apdu.getRawApdu(), {
-        isPolling: false,
-        triggersDisconnection: command.triggersDisconnection ?? false,
-        abortTimeout,
-      });
-
-      return response.caseOf({
-        Left: (err) => {
-          this._logger.error("[sendCommand] error", { data: { err } });
-          throw err;
-        },
-        Right: (r) => {
-          const result = command.parseResponse(
-            r,
-            this._connectedDevice.deviceModel.id,
-          );
-          this._logger.debug("[sendCommand] result", { data: { result } });
-          return result;
-        },
-      });
-    };
+    const apdu = command.getApdu();
 
-    if (abortTimeout) {
-      return Promise.race([
-        sendCommandPromise(),
-        new Promise<never>((_, reject) => {
-          setTimeout(() => reject(new SendCommandTimeoutError()), abortTimeout);
-        }),
-      ]);
-    }
+    const response = await this._unsafeInternalSendApdu(apdu.getRawApdu(), {
+      isPolling: false,
+      triggersDisconnection: command.triggersDisconnection ?? false,
+      abortTimeout,
+    });
 
-    return sendCommandPromise();
+    return response.caseOf({
+      Left: (err) => {
+        this._logger.error("[sendCommand] error", { data: { err } });
+        throw err;
+      },
+      Right: (r) => {
+        const result = command.parseResponse(
+          r,
+          this._connectedDevice.deviceModel.id,
+        );
+        this._logger.debug("[sendCommand] result", { data: { result } });
+        return result;
+      },
+    });
   }
 
   public executeDeviceAction<
@@ -388,19 +360,25 @@ export class DeviceSession {
   >(
     deviceAction: DeviceAction<Output, Input, E, IntermediateValue>,
   ): ExecuteDeviceActionReturnType<Output, E, IntermediateValue> {
-    const { observable: o, cancel: c } = this._intentQueueService.enqueue({
-      type: "device-action",
-      execute: () => {
-        const { observable } =
-          this._unsafeInternalExecuteDeviceAction(deviceAction);
-        // The queue service will handle the subscription lifecycle
-        return observable;
-      },
-    });
+    let deviceActionCancel: (() => void) | undefined;
+
+    const { observable: o, cancel: queueCancel } =
+      this._intentQueueService.enqueue({
+        type: "device-action",
+        execute: () => {
+          const { observable, cancel } =
+            this._unsafeInternalExecuteDeviceAction(deviceAction);
+          deviceActionCancel = cancel;
+          return observable;
+        },
+      });
 
     return {
       observable: o,
-      cancel: c,
+      cancel: () => {
+        deviceActionCancel?.();
+        queueCancel();
+      },
     };
   }
 

packages/device-management-kit/src/internal/device-session/model/DeviceSessionStateHandler.ts

@@ -95,7 +95,7 @@ export class DeviceSessionStateHandler {
         this._pendingDeviceStatus = DeviceStatus.CONNECTED;
         return;
       case SessionEvents.NEW_STATE: {
-        // On new state, if an intent is successfull,
+        // On new state, if an intent is successful,
         // we should have a DEVICE_STATE_UPDATE_LOCKED or DEVICE_STATE_UPDATE_CONNECTED as pending status event
         // If not, we should still have a BUSY status as fallback waiting for the transport to disconnect
         const newDeviceStatus = this._pendingDeviceStatus;