Skip to content

fix(apple): actually show user-friendly alert messages#8282

Merged
jamilbk merged 7 commits into
mainfrom
fix/actually-alert-error-type
Feb 28, 2025
Merged

fix(apple): actually show user-friendly alert messages#8282
jamilbk merged 7 commits into
mainfrom
fix/actually-alert-error-type

Conversation

@jamilbk

@jamilbk jamilbk commented Feb 27, 2025

Copy link
Copy Markdown
Member

Before, we would receive an NSError object and the type-matching wouldn't take effect at all, causing the default alert to show every time. This solves that by introducing a UserFriendlyError protocol which is more robust against the two main Error and NSError variants.

@vercel

vercel Bot commented Feb 27, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 28, 2025 5:23am

@jamilbk jamilbk force-pushed the fix/actually-alert-error-type branch from 7e3745c to a754d84 Compare February 27, 2025 02:10
Base automatically changed from fix/prevent-installing-vpn-configuration-twice to main February 28, 2025 04:51
@MainActor
struct macOSAlert { // swiftlint:disable:this type_name
static func show(for error: Error) {
let message = (error as UserFriendlyError).userMessage() ?? "\(error)"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this means try to cast and if cast fails, fallback to printing the raw error?

Would it be better to define the parameter of the function to enforce at compile-time that it needs to implement the UserFriendlyError protocol? That seems more robust because you'll get a compile error any time you want to show an alert for something that doesn't implement the protocol.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we need this to handle any error. Since we bubble up errors from external frameworks through here, we won't know in advance what they can be.

We picked a couple error types to make friendlier here because they are likely to happen if the user doesn't enable permissions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And? Just implement the protocol for these errors. Or make a separate function that shows a generic error.

The type-system is trying to help you here by flagging all the parts of the code where you are not displaying a user-friendly error dialog.

@jamilbk jamilbk Feb 28, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know what errors could be raised. Ther's potentially dozens or hundreds from the various calls from the various frameworks we use.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is the issue that when catching the errors, we don't know their type?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean - implement the protocol for Error and NSError?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct we don't know in advance what error types are caught

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know what errors could be raised. Ther's potentially dozens or hundreds from the various calls from the various frameworks we use.

Ah I thought that upon catch, the compiler enumerates what errors it may through but I guess it doesn't?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean - implement the protocol for Error and NSError?

That is also an option yeah. What I am planning to do in the Rust GUI client is: Show a generic error like "Oh, we've encountered an expected error. If this persists, contact your administrator." but log the detailed error to Sentry.

@jamilbk jamilbk added this pull request to the merge queue Feb 28, 2025
Merged via the queue into main with commit ab7e805 Feb 28, 2025
@jamilbk jamilbk deleted the fix/actually-alert-error-type branch February 28, 2025 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants