Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -128,17 +128,17 @@ const restrictedImportPaths = [
{
name: 'sentry/views/insights/common/components/insightsTimeSeriesWidget',
message:
'Do not use this directly in your view component, see https://sentry.sentry.io/stories/?name=app%2Fviews%2Fdashboards%2Fwidgets%2FtimeSeriesWidget%2FtimeSeriesWidgetVisualization.stories.tsx&query=timeseries#deeplinking for more information',
'Do not use this directly in your view component, see https://sentry.sentry.io/stories/stories/shared/views/dashboards/widgets/timeserieswidget/timeserieswidgetvisualization#deeplinking for more information',
},
{
name: 'sentry/views/insights/common/components/insightsLineChartWidget',
message:
'Do not use this directly in your view component, see https://sentry.sentry.io/stories/?name=app%2Fviews%2Fdashboards%2Fwidgets%2FtimeSeriesWidget%2FtimeSeriesWidgetVisualization.stories.tsx&query=timeseries#deeplinking for more information',
'Do not use this directly in your view component, see https://sentry.sentry.io/stories/stories/shared/views/dashboards/widgets/timeserieswidget/timeserieswidgetvisualization#deeplinking for more information',
},
{
name: 'sentry/views/insights/common/components/insightsAreaChartWidget',
message:
'Do not use this directly in your view component, see https://sentry.sentry.io/stories/?name=app%2Fviews%2Fdashboards%2Fwidgets%2FtimeSeriesWidget%2FtimeSeriesWidgetVisualization.stories.tsx&query=timeseries#deeplinking for more information',
'Do not use this directly in your view component, see https://sentry.sentry.io/stories/stories/shared/views/dashboards/widgets/timeserieswidget/timeserieswidgetvisualization#deeplinking for more information',
},
];

Expand Down
15 changes: 6 additions & 9 deletions static/app/components/core/button/button.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,12 @@ import ButtonDocumentation from '!!type-loader!@sentry/scraps/button';

export const documentation = {
exports: {
module: ButtonDocumentation.exports.module,
exports: {
// Button has some exports that we don't want to document, strip them out until they are removed
...Object.fromEntries(
Object.entries(ButtonDocumentation.exports.exports).filter(
([key]) => key !== 'StyledButton' && key !== 'ButtonBar'
)
),
},
...ButtonDocumentation.exports,
exports: Object.fromEntries(
Object.entries(ButtonDocumentation.exports.exports)
.filter(([key]) => key !== 'StyledButton' && key !== 'ButtonBar')
.map(([key, value]) => [key, value.name])
),
},
props: {
...ButtonDocumentation.props,
Expand Down
1 change: 0 additions & 1 deletion static/app/components/core/button/linkButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ const StyledLinkButton = styled(
prop === 'external' ||
prop === 'replace' ||
prop === 'preventScrollReset' ||
prop === 'state' ||
(typeof prop === 'string' && isPropValid(prop)),
}
)<LinkButtonProps>`
Expand Down
7 changes: 1 addition & 6 deletions static/app/components/core/button/types.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type {LocationDescriptor, LocationState} from 'history';
import type {LocationDescriptor} from 'history';

import type {TooltipProps} from 'sentry/components/core/tooltip';

Expand Down Expand Up @@ -111,11 +111,6 @@ interface LinkButtonPropsWithTo extends BaseLinkButtonProps {
* Determines if the link should replace the current history entry.
*/
replace?: boolean;

/**
* The state to pass to the link.
*/
state?: LocationState | undefined;
}

// eslint-disable-next-line @typescript-eslint/naming-convention
Expand Down
4 changes: 2 additions & 2 deletions static/app/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -320,9 +320,9 @@ function buildRoutes(): RouteObject[] {
],
},
{
path: '/stories/:storyType?/:storySlug?/',
Copy link
Member Author

Choose a reason for hiding this comment

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

This meant that we could only ever have category/name stories, and couldn't use the same structure for stories that like components/replay/... See eslint.config.mjs to see the routing difference now

component: make(() => import('sentry/stories/view/index')),
path: '/stories/*',
withOrgPath: true,
component: make(() => import('sentry/stories/view/index')),
},
{
path: '/debug/notifications/:notificationSource?/',
Expand Down
89 changes: 79 additions & 10 deletions static/app/stories/view/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ import styled from '@emotion/styled';

import {Alert} from 'sentry/components/core/alert';
import LoadingIndicator from 'sentry/components/loadingIndicator';
import {StorySidebar} from 'sentry/stories/view/storySidebar';
import {useStoryRedirect} from 'sentry/stories/view/useStoryRedirect';
import {
StorySidebar,
useStoryBookFilesByCategory,
} from 'sentry/stories/view/storySidebar';
import {StoryTreeNode, type StoryCategory} from 'sentry/stories/view/storyTree';
import {useLocation} from 'sentry/utils/useLocation';
import OrganizationContainer from 'sentry/views/organizationContainer';
import RouteAnalyticsContextProvider from 'sentry/views/routeAnalyticsContextProvider';
Expand All @@ -16,13 +19,23 @@ import {StoryHeader} from './storyHeader';
import {useStoryDarkModeTheme} from './useStoriesDarkMode';
import {useStoriesLoader} from './useStoriesLoader';

export default function Stories() {
export function useStoryParams(): {storyCategory?: StoryCategory; storySlug?: string} {
const location = useLocation();
return isLandingPage(location) ? <StoriesLanding /> : <StoryDetail />;
// Match: /stories/:category/(one/optional/or/more/path/segments)
const match = location.pathname.match(/^\/stories\/([^/]+)\/(.+)/);
return {
storyCategory: match?.[1] as StoryCategory | undefined,
storySlug: match?.[2] ?? undefined,
};
}

function isLandingPage(location: ReturnType<typeof useLocation>) {
return /\/stories\/?$/.test(location.pathname) && !location.query.name;
export default function Stories() {
const location = useLocation();
return isLandingPage(location) && !location.query.name ? (
<StoriesLanding />
) : (
<StoryDetail />
);
}

function StoriesLanding() {
Expand All @@ -36,13 +49,37 @@ function StoriesLanding() {
}

function StoryDetail() {
useStoryRedirect();
const location = useLocation();
const {storyCategory, storySlug} = useStoryParams();
const stories = useStoryBookFilesByCategory();

let storyNode = getStoryFromParams(stories, {
category: storyCategory,
slug: storySlug,
});

const location = useLocation<{name: string; query?: string}>();
// If we don't have a story node, try to find it by the filesystem path
if (!storyNode && location.query.name) {
const nodes = Object.values(stories).flat();
const queue = [...nodes];

while (queue.length > 0) {
const node = queue.pop();
if (!node) break;

if (node.filesystemPath === location.query.name) {
storyNode = node;
break;
}

for (const key in node.children) {
queue.push(node.children[key]!);
}
}
}

const file = location.state?.storyPath ?? location.query.name;
const story = useStoriesLoader({
files: file ? [file] : [],
files: storyNode ? [storyNode.filesystemPath] : [],
});

return (
Expand Down Expand Up @@ -93,6 +130,38 @@ function StoriesLayout(props: PropsWithChildren) {
);
}

function isLandingPage(location: ReturnType<typeof useLocation>) {
return /^\/stories\/?$/.test(location.pathname);
}

function getStoryFromParams(
stories: ReturnType<typeof useStoryBookFilesByCategory>,
context: {category?: StoryCategory; slug?: string}
): StoryTreeNode | undefined {
const nodes = stories[context.category as keyof typeof stories] ?? [];

if (!nodes || nodes.length === 0) {
return undefined;
}

const queue = [...nodes];

while (queue.length > 0) {
const node = queue.pop();
if (!node) break;

if (node.slug === context.slug) {
return node;
}

for (const key in node.children) {
queue.push(node.children[key]!);
}
}

return undefined;
}

function GlobalStoryStyles() {
const theme = useTheme();
const darkTheme = useStoryDarkModeTheme();
Expand Down
22 changes: 13 additions & 9 deletions static/app/stories/view/landing/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const frontmatter = {
actions: [
{
children: 'Get Started',
to: '/stories?name=app/styles/colors.mdx',
to: '/stories/foundations/colors',
priority: 'primary',
},
{
Expand Down Expand Up @@ -93,8 +93,9 @@ export function StoryLanding() {
<CardGrid>
<Card
to={{
pathname: normalizeUrl(`/organizations/${organization.slug}/stories/`),
query: {name: 'app/styles/colors.mdx'},
pathname: normalizeUrl(
`/organizations/${organization.slug}/stories/foundations/colors`
),
}}
title="Color"
>
Expand All @@ -104,8 +105,9 @@ export function StoryLanding() {
</Card>
<Card
to={{
pathname: normalizeUrl(`/organizations/${organization.slug}/stories/`),
query: {name: 'app/icons/icons.stories.tsx'},
pathname: normalizeUrl(
`/organizations/${organization.slug}/stories/foundations/icons`
),
}}
title="Icons"
>
Expand All @@ -115,8 +117,9 @@ export function StoryLanding() {
</Card>
<Card
to={{
pathname: normalizeUrl(`/organizations/${organization.slug}/stories/`),
query: {name: 'app/styles/typography.stories.tsx'},
pathname: normalizeUrl(
`/organizations/${organization.slug}/stories/foundations/typography`
),
}}
title="Typography"
>
Expand All @@ -126,8 +129,9 @@ export function StoryLanding() {
</Card>
<Card
to={{
pathname: normalizeUrl(`/organizations/${organization.slug}/stories/`),
query: {name: 'app/styles/images.stories.tsx'},
pathname: normalizeUrl(
`/organizations/${organization.slug}/stories/foundations/images`
),
}}
title="Images"
>
Expand Down
4 changes: 2 additions & 2 deletions static/app/stories/view/storyExports.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ function StoryLayout() {
const {story} = useStory();
const [tab, setTab] = useQueryState(
'tab',
parseAsString.withOptions({history: 'push'})
parseAsString.withOptions({history: 'push'}).withDefault('usage')
);

return (
<Tabs value={tab ?? 'usage'} onChange={setTab}>
<Tabs value={tab} onChange={setTab}>
{isMDXStory(story) ? <MDXStoryTitle story={story} /> : null}
<StoryGrid>
<StoryContainer>
Expand Down
34 changes: 18 additions & 16 deletions static/app/stories/view/storyFooter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,15 @@ export function StoryFooter() {
const pagination = findPreviousAndNextStory(story, stories);
const organization = useOrganization();

const {state: prevState, ...prevTo} = pagination?.prev?.location ?? {};
const {state: nextState, ...nextTo} = pagination?.next?.location ?? {};

return (
<Flex align="center" justify="between" gap="xl">
{pagination?.prev && (
<Card
to={{
...prevTo,
pathname: normalizeUrl(
`/organizations/${organization.slug}${pagination.prev.location.pathname}`
`/organizations/${organization.slug}/stories/${pagination.prev.category}/${pagination.prev.slug}`
),
}}
state={prevState}
icon={<IconArrow direction="left" />}
>
<Text variant="muted" as="div">
Expand All @@ -46,12 +41,10 @@ export function StoryFooter() {
<Card
data-flip
to={{
...nextTo,
pathname: normalizeUrl(
`/organizations/${organization.slug}${nextTo.pathname}`
`/organizations/${organization.slug}/stories/${pagination.next.category}/${pagination.next.slug}`
),
}}
state={nextState}
icon={<IconArrow direction="right" />}
>
<Text variant="muted" as="div" align="right">
Expand All @@ -74,16 +67,25 @@ function findPreviousAndNextStory(
prev?: StoryTreeNode;
} | null {
const stories = Object.values(categories).flat();
const currentIndex = stories.findIndex(s => s.filesystemPath === story.filename);
const queue = [...stories];

while (queue.length > 0) {
const node = queue.pop();
if (!node) break;

if (node.filesystemPath === story.filename) {
return {
prev: queue[queue.length - 1] ?? undefined,
next: queue[queue.length + 1] ?? undefined,
Copy link

Choose a reason for hiding this comment

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

Bug: The findPreviousAndNextStory function uses queue[queue.length + 1], causing out-of-bounds access and next to be undefined. The DFS queue also lacks proper pagination order.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The findPreviousAndNextStory function attempts to retrieve the next story using queue[queue.length + 1]. This expression always results in an out-of-bounds array access, causing next to be undefined. Furthermore, the queue used in the DFS traversal does not maintain the sequential order of stories required for correct pagination, even if the index calculation were fixed. This prevents the "next story" navigation button from rendering.

💡 Suggested Fix

Re-implement the pagination logic in findPreviousAndNextStory to use a pre-computed, flat, and ordered list of all stories, similar to the previous working implementation, instead of relying on the DFS queue.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/stories/view/storyFooter.tsx#L79

Potential issue: The `findPreviousAndNextStory` function attempts to retrieve the next
story using `queue[queue.length + 1]`. This expression always results in an
out-of-bounds array access, causing `next` to be `undefined`. Furthermore, the `queue`
used in the DFS traversal does not maintain the sequential order of stories required for
correct pagination, even if the index calculation were fixed. This prevents the "next
story" navigation button from rendering.

Did we get this right? 👍 / 👎 to inform future reviews.

};
}

if (currentIndex === -1) {
return null;
for (const key in node.children) {
queue.push(node.children[key]!);
}
}

return {
prev: stories[currentIndex - 1] ?? undefined,
next: stories[currentIndex + 1] ?? undefined,
};
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Broken Pagination: Flatten Before Finding Neighbors

The pagination logic in findPreviousAndNextStory is broken. The function uses a queue with pop() to traverse nodes, but when it finds a match, it tries to access queue[queue.length - 1] for prev and queue[queue.length + 1] for next. Since the current node was just popped, queue[queue.length + 1] will always be undefined (out of bounds). Additionally, the queue-based depth-first traversal doesn't maintain linear story order like the original findIndex approach did, so even queue[queue.length - 1] won't correctly reference the previous story in display order. The function needs to flatten the tree structure into a linear array before finding prev/next stories.

Fix in Cursor Fix in Web

}

const Card = styled(LinkButton)`
Expand Down
13 changes: 5 additions & 8 deletions static/app/stories/view/storySearch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -220,14 +220,11 @@ function SearchComboBox(props: SearchComboBoxProps) {
if (!node) {
return;
}
const {state, ...to} = node.location;
navigate(
{
...to,
pathname: normalizeUrl(`/organizations/${organization.slug}${to.pathname}`),
},
{replace: true, state}
);
navigate({
pathname: normalizeUrl(
`/organizations/${organization.slug}/stories/${node.category}/${node.slug}`
),
});
};

const state = useComboBoxState({
Expand Down
Loading
Loading