GitHub Issue #1023: Add redirect() helper that uses core-safeRedirect #2021
GitHub Issue #1023: Add redirect() helper that uses core-safeRedirect #2021cnathe wants to merge 9 commits into
Conversation
| } | ||
|
|
||
| window.location.href = url.toString(); | ||
| redirect(url.toString()); |
There was a problem hiding this comment.
@labkey-alan let me know what you think of this change. This would force the usages of navigate() to be internal redirect/navigations only...but I assume that was the intent and all usages are internal URLs. If there is a need for an external redirect / navigate, it was likely using window.location.href directly, right?
labkey-alan
left a comment
There was a problem hiding this comment.
This works well in my testing and I am comfortable approving it as-is. However, there is one clear gap that we should consider addressing if we can, but I am also comfortable deferring as long as we document the problem somewhere.
We have several stragglers in our product that still use window.location.href = someAppURL.toHref(), and for the most part those can be converted without issue, however the current implementation of redirect will not work with the case in FolderManagementPage.tsx, when a user deletes the container they're currently in. I updated this usage locally and what happens is we delete the container, then navigate to the safeRedirect action for the container we just deleted, which 404's. We should probably put a comment in FolderManagementPage.tsx to document this issue. I think moving forward we may need to have redirect try to generate the safeRedirect URL for the target container.
Rationale
https://github.com/LabKey/internal-issues/issues/1023
In the related PR, we added usages of the core-safeRedirect action. This PR moves that into a redirect() helper that can be used instead of setting window.location.href.
Related Pull Requests
Changes