Fixed: handle dialog closure case in confirmation flow (#195)

* fix(gator-permissions-snap): handle dialog closure case in confirmation flow

When the confirmation dialog is closed (result is null), the promise should resolve with false
to indicate that the user did not grant confirmation. This adds proper handling for
the dialog closure scenario and includes a corresponding test case.

This change ensures that the confirmation dialog properly handles the case where
the user closes the dialog without making a decision, returning false instead of
undefined or other unexpected values. The test case verifies this behavior.

Author: mj-kiwi

packages/gator-permissions-snap/src/core/confirmation.tsx

@@ -68,17 +68,13 @@ export class ConfirmationDialog {
     const interfaceId = this.#interfaceId;
 
     const isConfirmationGranted = new Promise<boolean>((resolve, reject) => {
-      // cleanup can't be defined before the click handlers, so cannot be const
-      // eslint-disable-next-line prefer-const
-      let cleanup: () => Promise<void>;
-
       const { unbind: unbindGrantButtonClick } = this.#userEventDispatcher.on({
         elementName: ConfirmationDialog.#grantButton,
         eventType: UserInputEventType.ButtonClickEvent,
         interfaceId,
         handler: async () => {
-          await cleanup();
-
+          await this.#cleanup();
+          this.#interfaceId = undefined;
           resolve(true);
         },
       });
@@ -88,38 +84,18 @@ export class ConfirmationDialog {
         eventType: UserInputEventType.ButtonClickEvent,
         interfaceId,
         handler: async () => {
-          await cleanup();
-
+          await this.#cleanup();
+          this.#interfaceId = undefined;
           resolve(false);
         },
       });
 
-      cleanup = async () => {
-        unbindGrantButtonClick();
-        unbindCancelButtonClick();
-
-        // clear our stored unbind handler reference
-        this.#unbindHandlers = undefined;
-
-        try {
-          await this.#snaps.request({
-            method: 'snap_resolveInterface',
-            params: {
-              id: interfaceId,
-              value: {},
-            },
-          });
-        } catch (error) {
-          const reason = error as Error;
-          reject(reason);
-        }
-      };
-
       // store hooks so we can close/reject programmatically on error
       this.#unbindHandlers = () => {
         unbindGrantButtonClick();
         unbindCancelButtonClick();
       };
+
       this.#decisionReject = reject;
 
       // we don't await this, because we only want to present the dialog, and
@@ -131,6 +107,14 @@ export class ConfirmationDialog {
             id: interfaceId,
           },
         })
+        .then(async (result) => {
+          // Should resolve with false when dialog is closed.
+          if (result === null) {
+            await this.#cleanup(false);
+
+            resolve(false);
+          }
+        })
         .catch((error) => {
           const reason = error as Error;
           reject(reason);
@@ -142,6 +126,33 @@ export class ConfirmationDialog {
     };
   }
 
+  /**
+   * Clean up event handlers and optionally resolve the interface.
+   * @param resolveInterface - Whether to resolve the interface. Defaults to true.
+   */
+  async #cleanup(resolveInterface = true): Promise<void> {
+    // Unbind any listeners to avoid leaks
+    if (this.#unbindHandlers) {
+      try {
+        this.#unbindHandlers();
+      } catch {
+        // ignore
+      } finally {
+        this.#unbindHandlers = undefined;
+      }
+    }
+
+    if (resolveInterface && this.#interfaceId) {
+      await this.#snaps.request({
+        method: 'snap_resolveInterface',
+        params: {
+          id: this.#interfaceId,
+          value: {},
+        },
+      });
+    }
+  }
+
   #buildConfirmation(): JSX.Element {
     return (
       <Container>
@@ -203,30 +214,11 @@ export class ConfirmationDialog {
       return;
     }
 
-    // Unbind any listeners to avoid leaks
-    if (this.#unbindHandlers) {
-      try {
-        this.#unbindHandlers();
-      } catch {
-        // ignore
-      } finally {
-        this.#unbindHandlers = undefined;
-      }
-    }
+    // Clean up handlers and resolve interface
+    await this.#cleanup(true);
 
-    try {
-      await this.#snaps.request({
-        method: 'snap_resolveInterface',
-        params: {
-          id: this.#interfaceId,
-          value: {},
-        },
-      });
-    } catch (error) {
-      // If closing fails, still reject with the original reason
-    } finally {
-      this.#interfaceId = undefined;
-    }
+    // Clear interface ID after cleanup
+    this.#interfaceId = undefined;
 
     if (this.#decisionReject) {
       this.#decisionReject(reason);

packages/gator-permissions-snap/test/core/confirmation.test.ts

@@ -140,6 +140,25 @@ describe('ConfirmationDialog', () => {
       expect(result).toStrictEqual({ isConfirmationGranted: false });
     });
 
+    it('should resolve with false when dialog is closed', async () => {
+      // Simulate dialog closure
+      mockSnaps.request.mockResolvedValueOnce(null);
+
+      const awaitingUserDecision =
+        confirmationDialog.displayConfirmationDialogAndAwaitUserDecision();
+
+      expect(mockUserEventDispatcher.on).toHaveBeenCalledTimes(2);
+      expect(mockUnbindFunctions).toHaveLength(2);
+
+      const result = await awaitingUserDecision;
+
+      mockUnbindFunctions.forEach((mockUnbindFn) => {
+        expect(mockUnbindFn).toHaveBeenCalledTimes(1);
+      });
+
+      expect(result).toStrictEqual({ isConfirmationGranted: false });
+    });
+
     it('should clean up event listeners after decision', async () => {
       const awaitingUserDecision =
         confirmationDialog.displayConfirmationDialogAndAwaitUserDecision();