-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
stories: remove location.state from storybook routing #102760
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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## jb/sentry/stories #102760 +/- ##
===================================================
Coverage 80.89% 80.89%
===================================================
Files 8869 8869
Lines 390451 390419 -32
Branches 24839 24829 -10
===================================================
Hits 315846 315846
+ Misses 74239 74207 -32
Partials 366 366 |
206a91f to
8705763
Compare
8705763 to
18cebd0
Compare
| ], | ||
| }, | ||
| { | ||
| path: '/stories/:storyType?/:storySlug?/', |
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 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
| | 'storySlug' | ||
| | 'storyType' |
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.
We can remove these and pretend they don't exist, which I think is good given that they are entirely unrelated to production params
| ); | ||
| return !!child; | ||
| }, [location, props.node]); | ||
| return !!props.node.find(n => n.slug === storySlug); |
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 is not an array method, the rule is detecting a false positive
|
I'm merging this to the downstream PR so that we don't need to review the changes twice |
| prev: stories[currentIndex - 1] ?? undefined, | ||
| next: stories[currentIndex + 1] ?? undefined, | ||
| }; | ||
| return null; |
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: 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.
| if (node.filesystemPath === story.filename) { | ||
| return { | ||
| prev: queue[queue.length - 1] ?? undefined, | ||
| next: queue[queue.length + 1] ?? undefined, |
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: 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.
Removes location.state from story handling and allows us to use nested routes