Skip to content

fix(ui): usePreventLeave should not show alert for exceptions #12722

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JesperWe
Copy link
Contributor

@JesperWe JesperWe commented Jun 8, 2025

When using 3rd party custom components in an edit form there exists a possibility that a non-navigational click event will propagate through to payload.

In this case the findClosestAnchor function in usePreventLeave may find an anchor without href, resulting in the newUrlObj = new URL(newUrl) in isAnchorOfCurrentUrl throwing the exception:

TypeError: URL constructor: is not a valid URL.

As a result a native alert is shown to the user, with no real explanation as to what is going on. This is not a good experience.

I suggest moving it to a console log which is less "in your face" for users who do not know what to do about it anyway.

I discovered this while using a data grid component with a context menu. Clicking on menu items (which are <a> tags without href in this component) triggers the error.

(Another on-liner fix would ofc be to not attempt to create an URL object if there is no href if (anchor?.href) {, but I opted for this version since using alert() in production code is not a preferred practice anyway)

When using custom components there exists a possibility that the findClosestAnchor function in usePreventLeave does not find any anchor, in which case the newUrl = anchor.href will throw.

In this case an ugly native alert is shown to the user, with no real explanation as to what is going on. This is not a good experience.
@JesperWe
Copy link
Contributor Author

JesperWe commented Jun 8, 2025

@jacobsfletch could you have a look (asking since this is your code)?

@JarrodMFlesch
Copy link
Contributor

@JesperWe I am good with this. I would like to throw a try/catch inside the isAnchorOfCurrentUrl function as well if you could. The catch should just return false.

@JesperWe
Copy link
Contributor Author

JesperWe commented Jun 10, 2025

@JesperWe I am good with this. I would like to throw a try/catch inside the isAnchorOfCurrentUrl function as well if you could. The catch should just return false.

Like so @JarrodMFlesch ?

@JesperWe
Copy link
Contributor Author

Ping @JarrodMFlesch ... I would love to not have to package patch this 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants