Merge pull request Expensify#72891 from ShridharGoel/desktopUpdates

roryabraham · web-flow · commit 502cafd61b81 · 2025-10-20T15:22:11.000-07:00
Follow up for location improvements in desktop
diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml
@@ -305,9 +305,10 @@ jobs:
         with:
           python-version: '3.12'
           cache: 'pip'
+          cache-dependency-path: desktop/requirements.txt
 
       - name: Ensure setuptools for node-gyp
-        run: python -m pip install --upgrade pip setuptools
+        run: pip install --upgrade -r desktop/requirements.txt
 
       - name: Load Desktop credentials from 1Password
         id: load-credentials
@@ -332,7 +333,6 @@ jobs:
           AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
           GCP_GEOLOCATION_API_KEY: ${{ secrets.GCP_GEOLOCATION_API_KEY_PRODUCTION }}
           S3_BUCKET: ${{ github.ref == 'refs/heads/production' && vars.PRODUCTION_S3_BUCKET || vars.STAGING_S3_BUCKET }}
-          PYTHON: ${{ steps.setup-python.outputs.python-path }}
 
       - name: Upload desktop sourcemaps artifact
         # v4
diff --git a/.github/workflows/testBuild.yml b/.github/workflows/testBuild.yml
@@ -192,9 +192,10 @@ jobs:
         with:
           python-version: '3.12'
           cache: 'pip'
+          cache-dependency-path: desktop/requirements.txt
 
       - name: Ensure setuptools for node-gyp
-        run: python -m pip install --upgrade pip setuptools
+        run: pip install --upgrade -r desktop/requirements.txt
 
       - name: Load Desktop credentials from 1Password
         id: load-credentials
@@ -226,7 +227,6 @@ jobs:
           AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
           AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
           GCP_GEOLOCATION_API_KEY: ${{ secrets.GCP_GEOLOCATION_API_KEY_STAGING }}
-          PYTHON: ${{ steps.setup-python.outputs.python-path }}
 
   web:
     name: Build and deploy Web
diff --git a/cspell.json b/cspell.json
@@ -767,7 +767,8 @@
         "Dtype",
         "Areport",
         "mple",
-        "Selec"
+        "Selec",
+        "setuptools"
     ],
     "ignorePaths": [
         "src/languages/de.ts",
diff --git a/desktop/main.ts b/desktop/main.ts
@@ -14,6 +14,8 @@ import CONFIG from '@src/CONFIG';
 import CONST from '@src/CONST';
 import IntlStore from '@src/languages/IntlStore';
 import type {TranslationPaths} from '@src/languages/types';
+import type {LocationPermissionState} from '@src/libs/getCurrentPosition/locationPermission';
+import {LOCATION_PERMISSION_STATES} from '@src/libs/getCurrentPosition/locationPermission';
 import type PlatformSpecificUpdater from '@src/setup/platformSetup/types';
 import type {Locale} from '@src/types/onyx';
 import type {CreateDownloadQueueModule, DownloadItem} from './createDownloadQueue';
@@ -32,11 +34,53 @@ const MAC_PERMISSION_STATUSES = {
     NOT_DETERMINED: 'not determined',
 } as const;
 
-const LOCATION_PERMISSION_STATES = {
-    GRANTED: 'granted',
-    DENIED: 'denied',
-    PROMPT: 'prompt',
-} as const;
+type MacPermissionsModule = {
+    getAuthStatus?: (authType: AuthType) => PermissionType | typeof MAC_PERMISSION_STATUSES.NOT_DETERMINED;
+};
+
+type MacGetAuthStatus = MacPermissionsModule['getAuthStatus'];
+
+let macGetAuthStatusPromise: Promise<MacGetAuthStatus | undefined> | undefined;
+
+const logMacPermissionsWarning = (message: string, error?: unknown) => {
+    if (error instanceof Error) {
+        log.warn(message, error.message);
+    } else if (typeof error === 'string') {
+        log.warn(message, error);
+    } else {
+        log.warn(message);
+    }
+};
+
+const loadMacGetAuthStatus = async (): Promise<MacGetAuthStatus | undefined> => {
+    if (!macGetAuthStatusPromise) {
+        if (process.platform !== 'darwin') {
+            macGetAuthStatusPromise = Promise.resolve<MacGetAuthStatus | undefined>(undefined);
+        } else {
+            try {
+                macGetAuthStatusPromise = Promise.resolve(((await import('node-mac-permissions')) as MacPermissionsModule).getAuthStatus);
+            } catch (error: unknown) {
+                logMacPermissionsWarning('node-mac-permissions not available, defaulting to denied:', error);
+                return undefined;
+            }
+        }
+    }
+
+    return macGetAuthStatusPromise;
+};
+
+const resolveLocationPermissionStatus = (status: PermissionType | typeof MAC_PERMISSION_STATUSES.NOT_DETERMINED): LocationPermissionState => {
+    switch (status) {
+        case MAC_PERMISSION_STATUSES.AUTHORIZED:
+            return LOCATION_PERMISSION_STATES.GRANTED;
+        case MAC_PERMISSION_STATUSES.NOT_DETERMINED:
+            return LOCATION_PERMISSION_STATES.PROMPT;
+        case MAC_PERMISSION_STATUSES.DENIED:
+        case MAC_PERMISSION_STATUSES.RESTRICTED:
+        default:
+            return LOCATION_PERMISSION_STATES.DENIED;
+    }
+};
 
 // Setup google api key in process environment, we are setting it this way intentionally. It is required by the
 // geolocation api (window.navigator.geolocation.getCurrentPosition) to work on desktop.
@@ -390,34 +434,16 @@ const mainWindow = (): Promise<void> => {
                 });
 
                 ipcMain.handle(ELECTRON_EVENTS.CHECK_LOCATION_PERMISSION, async () => {
+                    const getAuthStatus = await loadMacGetAuthStatus();
+
+                    if (!getAuthStatus) {
+                        return LOCATION_PERMISSION_STATES.DENIED;
+                    }
+
                     try {
-                        type MacPermissionsModule = {
-                            getAuthStatus?: (authType: AuthType) => PermissionType | 'not determined';
-                        };
-                        const macPermissionsModule = (await import('node-mac-permissions')) as MacPermissionsModule | {default: MacPermissionsModule};
-                        const macPermissions: MacPermissionsModule = ('default' in macPermissionsModule ? macPermissionsModule.default : macPermissionsModule) ?? {};
-                        const {getAuthStatus} = macPermissions;
-
-                        if (!getAuthStatus || typeof getAuthStatus !== 'function') {
-                            log.warn('node-mac-permissions not available or invalid, defaulting to denied');
-                            return LOCATION_PERMISSION_STATES.DENIED;
-                        }
-
-                        const status = getAuthStatus('location');
-
-                        switch (status) {
-                            case MAC_PERMISSION_STATUSES.AUTHORIZED:
-                                return LOCATION_PERMISSION_STATES.GRANTED;
-                            case MAC_PERMISSION_STATUSES.DENIED:
-                            case MAC_PERMISSION_STATUSES.RESTRICTED:
-                                return LOCATION_PERMISSION_STATES.DENIED;
-                            case MAC_PERMISSION_STATUSES.NOT_DETERMINED:
-                                return LOCATION_PERMISSION_STATES.PROMPT;
-                            default:
-                                return LOCATION_PERMISSION_STATES.DENIED;
-                        }
+                        return resolveLocationPermissionStatus(getAuthStatus('location'));
                     } catch (error) {
-                        log.warn('node-mac-permissions not available, defaulting to denied:', (error as Error)?.message);
+                        log.warn('node-mac-permissions threw while checking location permission, defaulting to denied:', (error as Error)?.message);
                         return LOCATION_PERMISSION_STATES.DENIED;
                     }
                 });
diff --git a/desktop/requirements.txt b/desktop/requirements.txt
@@ -0,0 +1 @@
+setuptools==80.9.0
diff --git a/src/libs/getCurrentPosition/index.desktop.ts b/src/libs/getCurrentPosition/index.desktop.ts
@@ -1,7 +1,10 @@
+import type {ValueOf} from 'type-fest';
 import type {GetCurrentPosition} from './getCurrentPosition.types';
 import {GeolocationErrorCode} from './getCurrentPosition.types';
+import type {LocationPermissionState} from './locationPermission';
+import {LOCATION_PERMISSION_STATES} from './locationPermission';
 
-const makeError = (code: (typeof GeolocationErrorCode)[keyof typeof GeolocationErrorCode], message: string) => ({
+const makeError = (code: ValueOf<typeof GeolocationErrorCode>, message: string) => ({
     code,
     message,
     PERMISSION_DENIED: GeolocationErrorCode.PERMISSION_DENIED,
@@ -10,12 +13,22 @@ const makeError = (code: (typeof GeolocationErrorCode)[keyof typeof GeolocationE
     NOT_SUPPORTED: GeolocationErrorCode.NOT_SUPPORTED,
 });
 
+const isLocationPermissionState = (status: unknown): status is LocationPermissionState =>
+    typeof status === 'string' && Object.values(LOCATION_PERMISSION_STATES).includes(status as LocationPermissionState);
+
 const getCurrentPosition: GetCurrentPosition = (success, error, options) => {
     const doGeoRequest = () => {
         try {
             navigator.geolocation.getCurrentPosition(success, error, options);
-        } catch (e) {
-            error(makeError(GeolocationErrorCode.POSITION_UNAVAILABLE, String(e ?? 'Geolocation call failed')));
+        } catch (caughtError) {
+            let reason = 'Geolocation call failed';
+            if (caughtError instanceof Error) {
+                reason = caughtError.message;
+            } else if (typeof caughtError === 'string') {
+                reason = caughtError;
+            }
+
+            error(makeError(GeolocationErrorCode.POSITION_UNAVAILABLE, reason));
         }
     };
 
@@ -24,17 +37,20 @@ const getCurrentPosition: GetCurrentPosition = (success, error, options) => {
         window.electron
             .invoke('check-location-permission')
             .then((permissionStatus: unknown) => {
-                const status = String(permissionStatus);
+                if (!isLocationPermissionState(permissionStatus)) {
+                    error(makeError(GeolocationErrorCode.PERMISSION_DENIED, 'Unable to verify location permissions. Enable location access and try again.'));
+                    return;
+                }
 
-                if (status === 'denied') {
+                if (permissionStatus === LOCATION_PERMISSION_STATES.DENIED) {
                     error(makeError(GeolocationErrorCode.PERMISSION_DENIED, 'Location access denied. Enable location permissions in system settings.'));
                     return;
                 }
 
                 doGeoRequest();
             })
             .catch(() => {
-                doGeoRequest(); // Fallback to direct geolocation
+                error(makeError(GeolocationErrorCode.PERMISSION_DENIED, 'Unable to verify location permissions. Enable location access and try again.'));
             });
 
         return; // handled via IPC
diff --git a/src/libs/getCurrentPosition/locationPermission.ts b/src/libs/getCurrentPosition/locationPermission.ts
@@ -0,0 +1,12 @@
+import type {ValueOf} from 'type-fest';
+
+const LOCATION_PERMISSION_STATES = {
+    GRANTED: 'granted',
+    DENIED: 'denied',
+    PROMPT: 'prompt',
+} as const;
+
+type LocationPermissionState = ValueOf<typeof LOCATION_PERMISSION_STATES>;
+
+export type {LocationPermissionState};
+export {LOCATION_PERMISSION_STATES};
