-
Notifications
You must be signed in to change notification settings - Fork 25
DEVPROD-4193: Redirect to project identifier on Project Health page #2290
Conversation
2 failed and 3 flaky tests on run #16252 ↗︎
Details:
Review all test suite changes for PR #2290 ↗︎ |
if (projectFromUrl && projectFromUrl !== Cookies.get(CURRENT_PROJECT)) { | ||
if ( | ||
projectFromUrl && | ||
!validateObjectId(projectFromUrl) && |
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.
Great improvement! I think it's possible this has a large impact on the issue altogether.
const { identifier } = projectData.project; | ||
const currentUrl = location.pathname.concat(location.search); | ||
const redirectPathname = currentUrl.replace(project, identifier); | ||
navigate(redirectPathname); |
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.
This could be overkill, but maybe adding an analytics event would be helpful to track whether we're still seeing this behavior frequently after the cookie change.
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.
for sure! I think the only clickable way to get to an ID link now is from the legacy UI 🫠 (DEVPROD-5194)
}, | ||
}); | ||
|
||
return { isRedirecting: needsRedirect && loading }; |
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.
This works great but would you mind explaining what && loading
does? Just curious
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.
in theory it should never really happen, but it handles the case where someone visits /commits/{repoId}
! Without && loading
, isRedirecting
will keep being true even though the repo could never redirect to an identifier
(loading && !versions) || !projectIdentifier || isResizing | ||
(loading && !versions) || | ||
!projectIdentifier || | ||
isRedirecting || |
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.
[nit] Is this maybe overkill since we check loading
for a function that is skipped if we're redirecting?
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.
good question, I checked it manually, and it looks like the screen will flicker without isRedirecting
😔
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.
wahoo!
failing tests on Hosts page seems unrelated |
DEVPROD-4193
Description
This PR creates a new hook called
useProjectRedirect
which handles the redirect. I made it a bit general since I think it might be reused for other pages.I also slightly tweaked the logic of how the
CURRENT_PROJECT
cookie is set so that it is never set to an Object ID.Testing