Cm/desktop location workflow#71362
Conversation
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
melvin-bot
Bot
requested review from
lakchote
and removed request for
a team
carlosmiceli
requested review from
AndrewGable
and removed request for
lakchote
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (typeof window !== 'undefined' && window.electron?.invoke) { | ||
| window.electron | ||
| .invoke('check-location-permission') | ||
| .then((permissionStatus: unknown) => { |
There was a problem hiding this comment.
❌ PERF-6
In useCallback, useMemo, and similar hooks (including promises), specify individual properties as dependencies instead of passing entire objects. The then callback is capturing the entire permissionStatus parameter but only using it as a string conversion.
Consider extracting the string conversion earlier or being more specific about what properties are needed:
window.electron
.invoke('check-location-permission')
.then((permissionStatus: string) => {
if (permissionStatus === 'denied') {
error(makeError(GeolocationErrorCode.PERMISSION_DENIED, 'Location access denied. Enable location permissions in system settings.'));
return;
}
doGeoRequest();
})
|
|
||
| ipcMain.handle(ELECTRON_EVENTS.CHECK_LOCATION_PERMISSION, () => { | ||
| try { | ||
| const {getAuthStatus} = require('node-mac-permissions') as {getAuthStatus: (authType: AuthType) => PermissionType | 'not determined'}; |
There was a problem hiding this comment.
❌ PERF-4
Objects and functions passed as props should be properly memoized or simplified to prevent unnecessary re-renders. The require call creates a new object reference on each function execution.
Consider moving the require statement outside the handler to avoid creating new objects on each invocation:
let nodePermissions: {getAuthStatus: (authType: AuthType) => PermissionType | 'not determined'} | null = null;
try {
nodePermissions = require('node-mac-permissions');
} catch {
// Module not available
}
ipcMain.handle(ELECTRON_EVENTS.CHECK_LOCATION_PERMISSION, () => {
try {
if (!nodePermissions?.getAuthStatus || typeof nodePermissions.getAuthStatus !== 'function') {
log.warn('node-mac-permissions not available or invalid, defaulting to denied');
return Promise.resolve('denied');
}
const status = nodePermissions.getAuthStatus('location');
// ... rest of the logic
} catch (error) {
log.warn('Error checking permissions:', (error as Error)?.message);
return Promise.resolve('denied');
}
});
|
@AndrewGable to confirm, I should run the adhoc build on this PR now, right? |
|
🚧 @carlosmiceli has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 @AndrewGable has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
🚧 @carlosmiceli has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
Closing now that adhoc build worked. |
- Add node-mac-permissions as optional dependency for system-level permission checking - Add node-gyp as optional dependency for native module compilation - These dependencies enable checking macOS location permissions before geolocation requests
- Add NSLocationWhenInUseUsageDescription and NSLocationUsageDescription - These descriptions are required by macOS App Store for location access - Provides clear explanation of why the app needs location permissions
- Add com.apple.security.location entitlement to allow location access - Required for the app to access system-level location services - Enables proper integration with macOS permission system
- Add CHECK_LOCATION_PERMISSION event to ELECTRON_EVENTS - Add event to context bridge whitelist for secure IPC communication - Enables renderer process to request location permission status from main process
- Add IPC handler that uses node-mac-permissions to check system-level location permissions - Maps macOS permission states to web-compatible permission states (granted/denied/prompt) - Includes proper error handling and fallback to denied when module is unavailable - Bridges native macOS permission APIs with web geolocation API
…ecking - Implement getCurrentPosition for desktop that checks system permissions before geolocation - Uses IPC to check macOS location permissions via main process - Provides immediate feedback when permissions are denied instead of 30-second timeout - Falls back to direct geolocation if IPC is unavailable - Improves user experience by preventing long loading states on permission denial
- Define AuthType and PermissionType interfaces for node-mac-permissions module - Provides type safety when using native macOS permission APIs - Ensures proper typing for location permission checking functionality
- Import optionalDependencies from desktop package.json - Add optional dependencies to webpack externals configuration - Ensures node-mac-permissions is properly bundled and available at runtime - Prevents webpack from trying to bundle native modules
…ilds - Exclude node-mac-permissions from Android, iOS, and web platforms - Prevents build failures on platforms where the native module is not available - Ensures the module is only included in macOS desktop builds where it's needed
…ensifyApp into desktopLocation
carlosmiceli
force-pushed
the
cm/desktopLocation-workflow
branch
from
ad99e38 to
b51ef69
Compare
|
🚧 @carlosmiceli has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
2 participants
Explanation of Change
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop