-
Notifications
You must be signed in to change notification settings - Fork 34
Improve error handling for Studio updating process #928
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
Conversation
@nightnei I'm unsure if providing the different errors adds value here. Wouldn't displaying the current dialog be enough so users who launch the app from DMG or launch it from DMG and trigger an update see this dialog? Doesn't the current dialog catch those cases? In my tests, I'm getting this dialog when I try to launch Studio from DMG: |
@wojtekn
|
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.
@nightnei I see the point. Could we make the dialogs consistent with ones used during import/export errors in showErrorMessageBox
, so there is a title, generic message, and then the detailed one displayed? See #765 as a reference.
For example:
Error updating Studio
Studio can only update automatically from the Applications folder. Please move Studio to Applications and try again.
Studio is running from a disk image at: PATH
Error updating Studio
Studio can only update automatically from the Applications folder. Please move Studio to Applications and try again.
Studio is running from: PATH
Error updating Studio
Studio can only update from the writable Applications folder. Please check write permissions and try again.
Studio is running from: PATH
src/updates.ts
Outdated
return appPath.startsWith( '/Volumes/' ); | ||
} | ||
|
||
function handleReadOnlyVolumeError( err: Error ) { |
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.
Could we reduce responsibility by moving Sentry logging out of this function and renaming it to showReadOnlyVolumeError
?
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.
Also, I think we can merge it with showInstallLocationErrorNotice
as it seems like an unnecessary abstraction now.
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.
@wojtekn, added all changes proposed by you, makes sense 👍
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.
Thanks, looks good. Can we just improve formatting to make it consistent with push/pull error messages, what we do in showErrorMessageBox
?
It's not clear at first, but dialog title should go into message
prop and the content should go into detail
. Also, I propose to split our current detail
into two strings to make translation flow easier.
Before | After |
---|---|
![]() |
![]() |
src/updates.ts
Outdated
} else { | ||
message = sprintf( | ||
__( | ||
'Studio can only update from the writable Applications folder. Please check write permissions and try again.\n\nStudio is running from: %s' |
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.
Can we split those into two separate translatable messages?
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.
Oh, I didn't even pay attention to it, thanks Wojtek ❤️
Addressed feedback
@nightnei looks good, thanks for improving the dialog formatting. I tested it using the DMG downloaded from the build, but it didn't catch the disk image: |
Hm, I tested all cases before pushing the commit and this path is very weird... Btw, looks like different errors with paths are already useful - we got some exceptional thing 🙂 Hm, I will google this path |
@wojtekn I haven't managed to reproduce it, but I updated code to address |
Works fine now, thanks. |
Related issues
Proposed Changes
I tried to reproduce the issue, I messed up file system and I managed to reproduce the error only from inside of DMG file.
But necessary to mention:
Necessary to mention, that the error is thrown from Squirrel.Mac, so such issue is not specific to Studio and can be reprodicable for many different apps, so we can do not more then improving error handling.
So after different testings, I can confirm that update process work totally correctly in Studio 1.3.3 and almost latest macOS Sequoia 15.2.
What we can do (my proposed changes) - show more specific errors when it's running from inside DMG file. Also show errors to move the app to Applications folder in case if somebody accidentally encounter such error (I haven't managed to see this error) and as a last resort - show default message to check folder permissions and additionally in this case we will send Sentry error, to have move info about it, if somebody one day catch it somehow.
If this PR is merged - we can close the issue and ask the issue creator to try to reproduce it with Studio 1.3.4. So that we can catch this error in Sentry. But my assumption is that something is wrong specifically with installing Studio for that user, since during research of the error, I saw cases in some people, when just reinstalling an app fixed the issue. So I think that installing 1.3.4, after removing their version will fix it.
Testing Instructions
Apply the next diff:
npm run make
Test running Studio from inside of DMG file
/out/make
Studio-1.3.3-arm64.dmg
Test running Studio from Applications folder (It's literally unreal error, but it's exactly what encountered in this issue. My assumption that something is aliased/linked/etc, so with this error we will be sure, which path Studio sees)
/out/make
Studio-1.3.3-arm64.dmg
Test running Studio outside of Applications folder