Repository navigation
Conversation
- Added new test cases to verify onClick behavior with default prevention. - Refactored handleClick to ensure onClick is called before navigation. - Introduced utility functions for creating click events and retrieving onClick handlers.
📝 WalkthroughWalkthroughThe Link mock click handling was updated in both Next.js package variants to call ChangesLink click handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/frameworks/nextjs/src/export-mocks/link/index.test.tsx (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the public click path instead of
forwardRef.render.
(MockLink as any).render(props, null)relies on a non-public React wrapper field, and these new cases still miss the branches changed incode/frameworks/nextjs/src/export-mocks/link/index.tsxLines 30-33 and 43-51 because they never render/click thelegacyBehaviorpath or callpreventDefault()from the consumer handler. Please drive the component through an actual render and add a prevented-click case so thedefaultPreventedshort-circuit is covered too.Also applies to: 46-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/frameworks/nextjs/src/export-mocks/link/index.test.tsx` around lines 23 - 26, The link tests are bypassing the public click flow by calling MockLink.render directly, so the updated branches in MockLink are not being exercised. Update getAnchorOnClick and the related cases to render MockLink normally and trigger clicks through the actual anchor element, including a legacyBehavior path. Also add a test where the consumer click handler calls preventDefault so the defaultPrevented short-circuit in MockLink is covered.code/frameworks/nextjs-vite/src/export-mocks/link/index.test.tsx (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the public click path instead of
forwardRef.render.
(MockLink as any).render(props, null)relies on a non-public React wrapper field, and these new cases still miss the branches changed incode/frameworks/nextjs-vite/src/export-mocks/link/index.tsxLines 30-33 and 43-51 because they never render/click thelegacyBehaviorpath or callpreventDefault()from the consumer handler. Please drive the component through an actual render and add a prevented-click case so thedefaultPreventedshort-circuit is covered too.Also applies to: 46-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/frameworks/nextjs-vite/src/export-mocks/link/index.test.tsx` around lines 23 - 26, The link mock tests are using the non-public MockLink.render path, which bypasses the real click behavior in MockLink and misses the updated branches in the exported link mock. Update the tests to mount/render MockLink through its public component API and trigger clicks on the rendered anchor so the legacyBehavior path is exercised; also add a case where the consumer click handler calls preventDefault() to cover the defaultPrevented short-circuit in the click handler logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/frameworks/nextjs-vite/src/export-mocks/link/index.test.tsx`:
- Around line 23-26: The link mock tests are using the non-public
MockLink.render path, which bypasses the real click behavior in MockLink and
misses the updated branches in the exported link mock. Update the tests to
mount/render MockLink through its public component API and trigger clicks on the
rendered anchor so the legacyBehavior path is exercised; also add a case where
the consumer click handler calls preventDefault() to cover the defaultPrevented
short-circuit in the click handler logic.
In `@code/frameworks/nextjs/src/export-mocks/link/index.test.tsx`:
- Around line 23-26: The link tests are bypassing the public click flow by
calling MockLink.render directly, so the updated branches in MockLink are not
being exercised. Update getAnchorOnClick and the related cases to render
MockLink normally and trigger clicks through the actual anchor element,
including a legacyBehavior path. Also add a test where the consumer click
handler calls preventDefault so the defaultPrevented short-circuit in MockLink
is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12b79be4-cf40-4d31-9854-f686709b0fd6
📒 Files selected for processing (4)
code/frameworks/nextjs-vite/src/export-mocks/link/index.test.tsxcode/frameworks/nextjs-vite/src/export-mocks/link/index.tsxcode/frameworks/nextjs/src/export-mocks/link/index.test.tsxcode/frameworks/nextjs/src/export-mocks/link/index.tsx
Sidnioulz
changed the title
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Code LGTM, I tested it and it makes sense and behaves as intended. Thanks for that!
There is one bug, and I don't know if it's a regression or not: the linkAction does not actually do anything. We're supposed to call a mock function with a custom mockName that would appear in the actions panel, but nothing appears.
I created this story file to debug, and it did show me an action entry when I manually called an equivalent mock function inside my story:
import Link from 'next/link';
import type { PartialStoryFn, StoryContext } from 'storybook/internal/types';
import { fn } from 'storybook/test';
const FooCmp = () => (
<Link
href="/about"
onClick={(event) => {
console.log('Foo link clicked');
fn().mockName('This one will appear')(event)
// event.preventDefault();
}}
>
Foo
</Link>
);
export default {
component: FooCmp,
title: 'Debug Foo',
decorators: [
(storyFn: PartialStoryFn, context: StoryContext) => {
return storyFn();
},
],
};
export const Foo = {};Could you please find out why the framework's internal linkAction is having no effect when the event is not default prevented?
Ideally, could you please add stories to the template and an E2E test to ensure this does not regress?
|
Thanks for catching this! The |
- Updated loaders to log actions for next/link. - Added new test suite for next/link to verify link action logging on click. - Introduced a new story for Link component to trigger link action.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/frameworks/nextjs-vite/template/stories/Link.stories.tsx (1)
75-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
playinteraction for this new story.This new story is being used as coverage for the link-action path, but it does not exercise the click behavior itself. Please add a
playthat clicks the link and asserts the expected canvas behavior, then verify it with the Storybook Vitest config. As per coding guidelines,**/*.stories.tsx: “add or update<Component>.stories.tsxand cover each behavior withplayfunctions usingexpect,userEvent, andwithinfromstorybook/test” and “verifyplayassertions withvitest --config code/vitest.config.storybook.ts <story-file>.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/frameworks/nextjs-vite/template/stories/Link.stories.tsx` around lines 75 - 77, The new TriggersLinkAction story in Link.stories.tsx only renders the link and does not exercise the click path, so add a play function for this story that uses userEvent, within, and expect from storybook/test to click the Link and assert the expected canvas state or outcome after the interaction. Keep the behavior-specific assertion inside TriggersLinkAction so the story covers the link-action flow end to end, and then verify the play test with the Storybook Vitest config using code/vitest.config.storybook.ts for this story file.Source: Coding guidelines
code/frameworks/nextjs/template/stories/Link.stories.tsx (1)
75-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
playinteraction for this new story.This new story is being used as coverage for the link-action path, but it does not exercise the click behavior itself. Please add a
playthat clicks the link and asserts the expected canvas behavior, then verify it with the Storybook Vitest config. As per coding guidelines,**/*.stories.tsx: “add or update<Component>.stories.tsxand cover each behavior withplayfunctions usingexpect,userEvent, andwithinfromstorybook/test” and “verifyplayassertions withvitest --config code/vitest.config.storybook.ts <story-file>.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/frameworks/nextjs/template/stories/Link.stories.tsx` around lines 75 - 77, The new TriggersLinkAction story only renders the link and does not exercise the click path, so add a play function to this story that uses within, userEvent, and expect from storybook/test to click the Link and assert the expected canvas state after the action. Keep the fix localized to TriggersLinkAction in Link.stories.tsx, and then validate the interaction with the Storybook Vitest configuration by running the story file through the configured vitest command.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/frameworks/nextjs-vite/template/stories/Link.stories.tsx`:
- Around line 75-77: The new TriggersLinkAction story in Link.stories.tsx only
renders the link and does not exercise the click path, so add a play function
for this story that uses userEvent, within, and expect from storybook/test to
click the Link and assert the expected canvas state or outcome after the
interaction. Keep the behavior-specific assertion inside TriggersLinkAction so
the story covers the link-action flow end to end, and then verify the play test
with the Storybook Vitest config using code/vitest.config.storybook.ts for this
story file.
In `@code/frameworks/nextjs/template/stories/Link.stories.tsx`:
- Around line 75-77: The new TriggersLinkAction story only renders the link and
does not exercise the click path, so add a play function to this story that uses
within, userEvent, and expect from storybook/test to click the Link and assert
the expected canvas state after the action. Keep the fix localized to
TriggersLinkAction in Link.stories.tsx, and then validate the interaction with
the Storybook Vitest configuration by running the story file through the
configured vitest command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77f4c123-a15b-43f1-94f4-383c0787b4cc
📒 Files selected for processing (4)
code/core/src/actions/loaders.tscode/e2e-sandbox/framework-nextjs.spec.tscode/frameworks/nextjs-vite/template/stories/Link.stories.tsxcode/frameworks/nextjs/template/stories/Link.stories.tsx
github-project-automation
Bot
moved this from In Progress
to Done
in Core Team Projects
2 participants
Closes #35213
What I did
The mocked
next/link(used by@storybook/nextjsand@storybook/nextjs-vite) calledevent.preventDefault()before invoking the consumer-providedonClick. The realnext/linkdoes the opposite: it runs the consumer'sonClickfirst and only navigates (and prevents the browser default) if that handler hasn't already calledpreventDefault().Because the mock prevented default first, any handler that branches on
event.defaultPreventedreceived an already-prevented event. A concrete case is Headless UI'sButton as={Link}, whose merged click handler exits early when the event is already default-prevented - so the user'sonClicknever ran in Storybook, even though the same component works in a real Next.js app.Changes:
onClick(or thelegacyBehaviorchild'sonClick) before preventing default, mirroring the realnext/link.linkAction) when the event hasn't already been default-prevented, so a consumer that prevents default suppresses the mock navigation just like the real component.navigatelogic into a single helper used by both the default andlegacyBehaviorpaths. The behavior was previously duplicated in two places, which is why the bug existed in both.@storybook/nextjsand@storybook/nextjs-vite.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Added regression tests to the existing
index.test.tsxfor both packages:onClickis invoked before default is prevented (event.defaultPreventedisfalsewhen the handler runs).Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
yarn task --task sandbox --start-from auto --template nextjs/default-ts(ornextjs-vite/default-ts).next/linkwith anonClickhandler that branches onevent.defaultPrevented(for example a Headless UIButton as={Link}, or any link whoseonClickchecksevent.defaultPrevented).onClickruns. Before this change the handler was skipped because the event arrived already default-prevented.Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
next/linkmock so customonClickhandlers run before navigation is triggered.preventDefault()consistently in both standard and legacy click flows.next/linkmock test coverage for click ordering anddefaultPreventedbehavior.next/linkaction logging appears in the Actions panel.