Fetch macOS native location permission to improve user experience#70833
Conversation
|
@QichenZhu @carlosmiceli Can you let me know your thoughts on this? I believe this will provide a much better user experience than having a timeout. |
Codecov Report❌ Patch coverage is
... and 71 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@ShridharGoel can we split into some commits? There's a lot of changes for a single commit and it's hard to distinguish the "building blocks" of this PR. |
|
There's also failing test and missing coverage, not sure if that changes your solution. |
|
Please ignore this CodeCov comment for now. It's something new that we're testing out and it shouldn't commented that yet. Updated a config and it shouldn't comment again. |
|
I'll split this into multiple commits. |
ShridharGoel
force-pushed
the
desktopLocation
branch
3 times, most recently
from
7f225bc to
d4b2950
Compare
melvin-bot
Bot
requested review from
QichenZhu
and removed request for
a team
|
@QichenZhu Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@ShridharGoel, we have a process to add new libraries like we did for heic-to: https://expensify.slack.com/archives/C01GTK53T8Q/p1744200665946799, but I want to check first if you were able to build a universal app? I could only build an x64 app on my Intel Mac, not sure what environment setup is needed. |
|
Yes, |
|
@QichenZhu Can you check if you are able to build the universal app on main branch? |
@ShridharGoel, yes. I think it's because this PR introduces some native code from node-mac-permissions. |
|
@QichenZhu Since universal builds are not needed generally in local setup, do you think it matters if local arm64 universal builds don't work on Intel macs? |
I'm fine with it as long as it doesn't break CI/CD. @carlosmiceli, can you trigger an ad-hoc build to verify? |
|
🚧 @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.
|
@ShridharGoel, we need to sort out build failure.
|
ShridharGoel
force-pushed
the
desktopLocation
branch
from
d4b2950 to
555d140
Compare
|
|
ShridharGoel
force-pushed
the
desktopLocation
branch
2 times, most recently
from
cb78ec1 to
e45b476
Compare
|
@carlosmiceli Can we try the adhoc build once again? |
|
Update: Waiting for an adhoc build here. |
|
@carlosmiceli @AndrewGable Gentle bump for the adhoc build, thanks. |
|
Just now getting to this, will proceed with the adhoc build. |
|
Running workflow https://github.com/Expensify/App/actions/runs/18474069684 |
|
@ShridharGoel here's teh build: #71362 (comment) |
|
@carlosmiceli Thanks. @QichenZhu Can you test it when possible? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-native.mp4Android: mWeb Chromeandroid-web.webmiOS: HybridAppios-native.mp4iOS: mWeb Safariios-web.movMacOS: Chrome / Safarimac-web.movMacOS: Desktopmac-desktop.mov |
|
@ShridharGoel, I understand this PR focuses on desktop, but it's recommended to test on all platforms and include screenshots/videos. |
|
The adhoc build works well in my testing 👍 How's the new library request going? |
|
I think @AndrewGable is handling the library request internally. |
|
@AndrewGable is OOO but the request to add node-mac-permissions is approved |
|
@carlosmiceli Please take a look when possible. |
| x64ArchFiles: '**/node_modules/node-mac-permissions/bin/**', | ||
| extendInfo: { | ||
| CFBundleIconName: getMacBundleIconName(), | ||
| NSLocationWhenInUseUsageDescription: 'This app uses location to help you track distance expenses.', |
There was a problem hiding this comment.
I hate to open up a can of worms, but how is this translated when you have the app set to Spanish for example?
There was a problem hiding this comment.
Looks like we'll need language specific InfoPlist.strings files if we want to support translations. I'll add for en and es.
There was a problem hiding this comment.
Talked to @roryabraham 1:1 and he suggested we just stick with English only for now.
| const status = getAuthStatus('location'); | ||
|
|
||
| switch (status) { | ||
| case 'authorized': |
There was a problem hiding this comment.
These feel like great uses for constants
| ships support. | ||
| ``` | ||
|
|
||
| - Upstream PR/issue: N/A |
There was a problem hiding this comment.
Do you mean we should be creating an issue in node-abi ?
There was a problem hiding this comment.
An issue and/or a PR, yes.
There was a problem hiding this comment.
Just checked, newer versions of node-abi have support for Electron 37. So, we'll need to update our node-abi version, and then this patch should not be needed. It seems that we are currently getting node-abi dependency via other dependencies.
├─┬ electron-builder@26.0.19
│ └─┬ app-builder-lib@26.0.19
│ └─┬ @electron/rebuild@3.7.2
│ └── node-abi@3.65.0
└─┬ react-fast-pdf@1.0.29
└─┬ pdfjs-dist@4.8.69
└─┬ canvas@3.1.0
└─┬ prebuild-install@7.1.3
└── node-abi@3.65.0 deduped
I'll add an override to use 3.78.0
AndrewGable
left a comment
AndrewGable
left a comment
There was a problem hiding this comment.
Looks good, all yours @carlosmiceli
|
|
||
| - name: Setup Python for node-gyp | ||
| id: setup-python | ||
| uses: actions/setup-python@v5 |
There was a problem hiding this comment.
Immutable action refs only please:
| uses: actions/setup-python@v5 | |
| # v6.0.0 | |
| uses: actions/setup-python@18566f86b301499665bd3eb1a2247e0849c64fa5 |
There was a problem hiding this comment.
Thanks for the comment and quick fix.
@ShridharGoel, this one is already done in #72823. Can you follow up on the other comments asap?
There was a problem hiding this comment.
Yes, will be done today.
| id: setup-python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' |
There was a problem hiding this comment.
Let's also add a pip cache:
| python-version: '3.12' | |
| python-version: '3.12' | |
| cache: 'pip' |
| python-version: '3.12' | ||
|
|
||
| - name: Ensure setuptools for node-gyp | ||
| run: python -m pip install --upgrade pip setuptools |
There was a problem hiding this comment.
Can we add a requirements.txt for and use that to codify and pin python dependencies? Then this would just be pip install -r requirements.txt
There was a problem hiding this comment.
Also, why do we need python -m ...? The docs make it look like setup-python should add pip to the $PATH just fine: https://github.com/actions/setup-python?tab=readme-ov-file#caching-packages-dependencies
| 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 }} |
There was a problem hiding this comment.
This shouldn't be necessary, since it looks like actions/setup-python is already adding the specified version of python to the front of the $PATH
There was a problem hiding this comment.
Same comments in deploy.yml apply throughout this file as well
| type MacPermissionsModule = { | ||
| getAuthStatus?: (authType: AuthType) => PermissionType | 'not determined'; | ||
| }; | ||
| const macPermissionsModule = (await import('node-mac-permissions')) as MacPermissionsModule | {default: MacPermissionsModule}; |
There was a problem hiding this comment.
Do we need to import this every time the event callback is called?
I think it would make more sense to lift macPermissionsModule to the global scope and either:
- synchronously import it once on app startup (probably simplest, and it's unlikely it would have a big impact on TTI, but you'd have to measure 🤷🏼), or
- check if it's defined in the callback, and only if it's not then dynamically import it, but only once per app startup, not once per callback execution
There was a problem hiding this comment.
Also, I'm not sure what kind of tree-shaking we have in place for the Electron main process, but if it's possible to import only getAuthStatus that would probably be better than importing the whole module.
| import type {GetCurrentPosition} from './getCurrentPosition.types'; | ||
| import {GeolocationErrorCode} from './getCurrentPosition.types'; | ||
|
|
||
| const makeError = (code: (typeof GeolocationErrorCode)[keyof typeof GeolocationErrorCode], message: string) => ({ |
There was a problem hiding this comment.
Use ValueOf from type-fest for improved readability:
| const makeError = (code: (typeof GeolocationErrorCode)[keyof typeof GeolocationErrorCode], message: string) => ({ | |
| const makeError = (code: ValueOf<typeof GeolocationErrorCode>, message: string) => ({ |
| if (typeof window !== 'undefined' && window.electron?.invoke) { | ||
| window.electron | ||
| .invoke('check-location-permission') | ||
| .then((permissionStatus: unknown) => { |
There was a problem hiding this comment.
Move the type out of desktop/main.ts into it's own file, then import it both in desktop/main.ts and here, then you can have strongly typed location permission status.
| doGeoRequest(); | ||
| }) | ||
| .catch(() => { | ||
| doGeoRequest(); // Fallback to direct geolocation |
There was a problem hiding this comment.
I'm confused by this. In desktop/main.ts you fallback on permission denied, but in here you seem to fallback on trying to access location anyways?
There was a problem hiding this comment.
Changed this to denied
| doGeoRequest(); // Fallback to direct geolocation | ||
| }); | ||
|
|
||
| return; // handled via IPC |
There was a problem hiding this comment.
When this returns, everything after that point runs through the IPC response
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@ShridharGoel sorry, I didn't get to finish my review before this was merged. Can you please create a follow-up to address these review comments? |
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.2.33-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.33-4 🚀
|
7 participants
Explanation of Change
Fetch macOS native location permission to improve user experience.
Fixed Issues
$ #70014
PROPOSAL: #70014 (comment)
Tests
Preconditions: disable location services for Expensify Desktop app in MacOS System settings (Location services)
QA Steps
Same as tests.
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))npm run compress-svg)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
LocationFix.mov
Android: mWeb Chrome
Screen.Recording.2025-10-14.at.8.14.59.PM.mov
iOS: Native
<Was facing Mapbox API issue because of which the map screen wasn't loading>
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.16.Plus.-.2025-10-14.at.20.08.01.mov
MacOS: Chrome / Safari
Screen.Recording.2025-10-14.at.7.48.03.PM.mov
MacOS: Desktop
DesktopLocation.mov