-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(stories): support pretty URLs #95365
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
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #95365 +/- ##
==========================================
- Coverage 77.05% 72.32% -4.73%
==========================================
Files 10493 10493
Lines 605919 605915 -4
Branches 23704 23703 -1
==========================================
- Hits 466887 438252 -28635
- Misses 136609 165240 +28631
Partials 2423 2423 |
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.
Bug: Story Redirect Hook Fails for URL Formats
The useStoryRedirect
hook fails to redirect correctly due to two issues:
- It attempts to access
category
andtopic
as query parameters (location.query
) instead of path parameters, preventing redirects for new-style URLs (e.g.,/stories/foundations/colors
). - An early return condition
if (location.state?.storyPath ?? location.query.name)
incorrectly bails out whenlocation.query.name
is present, blocking redirects from legacy URLs (e.g.,/stories/?name=...
) to the new format.
static/app/stories/view/useStoryRedirect.tsx#L21-L69
sentry/static/app/stories/view/useStoryRedirect.tsx
Lines 21 to 69 in f67384f
export function useStoryRedirect() { | |
const location = useLocation<StoryQuery>(); | |
const navigate = useNavigate(); | |
const stories = useStoryBookFilesByCategory(); | |
useEffect(() => { | |
// If we already have a `storyPath` in state, bail out | |
if (location.state?.storyPath ?? location.query.name) { | |
return; | |
} | |
if (!location.pathname.startsWith('/stories')) { | |
return; | |
} | |
const story = getStoryMeta(location.query, stories); | |
if (!story) { | |
return; | |
} | |
if (story.category === 'shared') { | |
navigate( | |
{pathname: `/stories/`, search: `?name=${encodeURIComponent(story.path)}`}, | |
{replace: true, state: {storyPath: story.path}} | |
); | |
} else { | |
navigate( | |
{pathname: `/stories/${story.category}/${kebabCase(story.label)}`}, | |
{replace: true, state: {storyPath: story.path}} | |
); | |
} | |
}, [location, navigate, stories]); | |
} | |
type StoryCategory = keyof ReturnType<typeof useStoryBookFilesByCategory>; | |
interface StoryMeta { | |
category: StoryCategory; | |
label: string; | |
path: string; | |
} | |
function getStoryMeta( | |
query: StoryQuery, | |
stories: ReturnType<typeof useStoryBookFilesByCategory> | |
) { | |
if (query.name) { | |
return legacyGetStoryMetaFromQuery(query, stories); | |
} | |
if (query.category && query.topic) { | |
return getStoryMetaFromQuery(query, stories); | |
} | |
return undefined; |
Was this report helpful? Give feedback by reacting with 👍 or 👎
This PR implements a new routing structure for `/stories`. Closes DE-193. Previously, all `/stories` routes were access via a query param with their full path structure such as `?name=app/components/collapsePanel.stories.tsx`. With this implementation, it was difficult to navigate via URL without understanding our internal file structure (which can also be subject to change). To improve routing ergonomics and stability, the foundations and core components now have permalinks like `/stories/foundations/colors` and `/stories/core/button`. These are automatically picked up by a client redirect, which makes them backwards-compatible with the existing URL structure. For shared components/hooks/etc, the existing `?name=app/components/collapsePanel.stories.tsx` URL structure is maintained. | Type | Before | After | |------|--------|--------| |foundations| `/stories/?name=app/styles/colors.mdx` | `/stories/foundations/colors` | |core component| `/stories?name=app/components/core/button/index.mdx` | `/stories/core/button` | |shared| `/stories/?name=app/utils/useOrganization.stories.tsx` | `/stories/?name=app/utils/useOrganization.stories.tsx` |
Follow-up to #95365. Previously, legacy URLs would not be redirected. Now, legacy URLs force a redirect. Previously, the sidebar and search pointed to the legacy URLs, which required a redirect. Now, the sidebar and search leverage the new URLs and pass along `state` info.
Follow-up to #95365. Previously, legacy URLs would not be redirected. Now, legacy URLs force a redirect. Previously, the sidebar and search pointed to the legacy URLs, which required a redirect. Now, the sidebar and search leverage the new URLs and pass along `state` info.
This PR implements a new routing structure for
/stories
. Closes DE-193.Previously, all
/stories
routes were access via a query param with their full path structure such as?name=app/components/collapsePanel.stories.tsx
. With this implementation, it was difficult to navigate via URL without understanding our internal file structure (which can also be subject to change).To improve routing ergonomics and stability, the foundations and core components now have permalinks like
/stories/foundations/colors
and/stories/core/button
. These are automatically picked up by a client redirect, which makes them backwards-compatible with the existing URL structure.For shared components/hooks/etc, the existing
?name=app/components/collapsePanel.stories.tsx
URL structure is maintained./stories/?name=app/styles/colors.mdx
/stories/foundations/colors
/stories?name=app/components/core/button/index.mdx
/stories/core/button
/stories/?name=app/utils/useOrganization.stories.tsx
/stories/?name=app/utils/useOrganization.stories.tsx