-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update dependencies on desktop package file to fix vulnerabilities #30186
Conversation
@aimane-chnaif 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] |
@aimane-chnaif I need you to check that I won't be breaking the electron updater that checks for uploads on desktop. I think most of the major changes were aimed at Windows so it does not apply, but we need to test it. I did so and it seems it's working fine, but we need to double-check. |
I see |
Ahh no sorry, I added it because I was testing it earlier, I will remove it. |
@aimane-chnaif Friendly bump! |
Thanks for the bump. On it now |
Reviewer Checklist
Screenshots/Videos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
I am seeing below error in console but I think it's expected for local build
[2023-10-26 17:01:59.167] [info] Checking for update
[2023-10-26 17:01:59.723] [info] Generated new staging user ID: 4686d03d-e917-5107-9e1e-8e27b62f3c5d
[2023-10-26 17:02:02.875] [error] Error: Error: Cannot find latest-mac.yml in the latest release artifacts (https://github.com/Expensify/App/releases/download/1.3.91-6/latest-mac.yml): HttpError: 404
@tylerkaraszewski 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] |
🎯 @aimane-chnaif, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #30453. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/tylerkaraszewski in version: 1.3.93-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀
|
🚀 Deployed to staging by https://github.com/tylerkaraszewski in version: 1.3.94-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/tylerkaraszewski in version: 1.3.94-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀
|
Details
Bumping to one of the latest versions to fix security vulnerabilities. I know quite the jump, I read the changelogs and I think we should be fine since most breaking changes are for Windows.
CHANGELOGS!! Checked the PRs for comments about breaking changes:
electron-updater
cache directory on macOS electron-userland/electron-builder#7032Fixed Issues
$ https://github.com/Expensify/Expensify/issues/298613
PROPOSAL:
Tests
npm run ios
npm run android
npm run web
npm run desktop
Let's also make sure that the desktop build succeeds.
config/electronBuilder.config.js
npm run desktop-build
dist/Chat.dmg
, and install the app. Run it and make sure it works.~/Library/Logs/react-native-chat/main.log
. Inspect them by any means you prefer and search for the log line "Checking for update". This indicates that the auto-updater is running. Because this is a local production build, you'll likely see a 404 error against a query to Github's servers immediately after. That's expected for a local production build.Offline tests
QA Steps
Check for Updates
in the system menuUpdate Not Available
should show up.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop