Fix #33844: [Bug]: Storybook React Native Web - Issue with Testing addon

[danielalanbates]

Fixes #33844

Summary

This PR fixes: [Bug]: Storybook React Native Web - Issue with Testing addons installation

Changes

.../src/common/js-package-manager/Yarn2Proxy.test.ts   | 18 ++++++++++++++++++
 code/core/src/common/js-package-manager/Yarn2Proxy.ts  |  8 ++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

Testing

Please review the changes carefully. The fix was verified against the existing test suite.

---

This PR was created with the assistance of Claude Sonnet 4.6 by Anthropic | effort: low. Happy to make any adjustments!

Summary by CodeRabbit

• New Features

  • Introduced an optional flag to enable remote package execution through yarn's dlx subcommand, while maintaining backward compatibility with the default execution method.

• Tests

  • Added test coverage validating remote package execution functionality.

[dosubot]

Related Documentation

Checked 0 published document(s) in 1 knowledge base(s). No updates required.

^{How did I do? Any feedback?}  

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds an optional useRemotePkg flag to Yarn2Proxy's runPackageCommand method, enabling switching between yarn dlx (remote execution) and yarn exec (local execution) subcommands, with corresponding test cases validating the new behavior.

Changes

Cohort / File(s)	Summary
Yarn2Proxy Implementation code/core/src/common/js-package-manager/Yarn2Proxy.ts	Added optional useRemotePkg parameter (defaults to false) to runPackageCommand method. Updated command composition to use 'dlx' subcommand when flag is true, otherwise 'exec'.
Yarn2Proxy Tests code/core/src/common/js-package-manager/Yarn2Proxy.test.ts	Added test cases verifying that runPackageCommand uses yarn dlx when useRemotePkg is true, with command arguments properly formatted as ['dlx', 'storybook@latest', 'automigrate'].

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• CLI: Support addon-vitest setup when --skip-install is passed #33718: Adds and propagates a useRemotePkg flag to change runPackageCommand behavior between dlx and exec execution modes.
• CLI: Minor improvements #33180: Modifies Yarn2Proxy's package command behavior to support different execution strategies for package management.

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

code/core/src/common/js-package-manager/Yarn2Proxy.ts (1)

97-114: ⚠️ Potential issue | 🔴 Critical

getPackageCommand displays incorrect command when useRemotePkg: true

getPackageCommand (line 97–99) hardcodes yarn exec and ignores the useRemotePkg flag. This method is used in AddonVitestService.ts (lines 142, 162) to display the Playwright installation command to users in guidance and error messages. When useRemotePkg: true is passed to runPackageCommand (line 155), the actual execution uses yarn dlx but the displayed string shows yarn exec, causing a mismatch between what users are told to run and what was actually executed.

Update getPackageCommand to accept the useRemotePkg parameter and conditionally return the correct command, or refactor the usage to display the accurate command being executed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@code/core/src/common/js-package-manager/Yarn2Proxy.ts` around lines 97 - 114,
getPackageCommand currently always returns "yarn exec ..." which mismatches
runPackageCommand when useRemotePkg is true; change getPackageCommand signature
to accept a boolean useRemotePkg (default false) and return `yarn dlx ...` when
true and `yarn exec ...` when false, then update callers (e.g.,
AddonVitestService usage) to pass the same useRemotePkg flag or refactor those
callers to derive the display string from runPackageCommand's args so the
displayed command matches the executed one.

🧹 Nitpick comments (1)

code/core/src/common/js-package-manager/Yarn2Proxy.test.ts (1)

71-87: Mock setup belongs in beforeEach per project guidelines

The mockResolvedValue at lines 72–74 is set up inline inside the test case. Per the coding guidelines, mock behaviors should be implemented in beforeEach blocks and not inline within test cases. This is also the pattern used by every existing test in this file, so a broader cleanup would be needed to fully align — consider addressing this holistically rather than just in the new test.

♻️ Suggested pattern

 describe('runScript', () => {
+  beforeEach(() => {
+    mockedExecuteCommand.mockResolvedValue({ stdout: '' } as any);
+  });
+
   it('should execute script `yarn compodoc -- -e json -d .`', async () => {
-    const executeCommandSpy = mockedExecuteCommand.mockResolvedValue({
-      stdout: '7.1.0',
-    } as any);
+    const executeCommandSpy = mockedExecuteCommand;

     await yarn2Proxy.runPackageCommand({ args: ['compodoc', '-e', 'json', '-d', '.'] });
     ...
   });

   it('should use `yarn dlx` when useRemotePkg is true', async () => {
-    const executeCommandSpy = mockedExecuteCommand.mockResolvedValue({
-      stdout: '',
-    } as any);
+    const executeCommandSpy = mockedExecuteCommand;

     await yarn2Proxy.runPackageCommand({
       args: ['storybook@latest', 'automigrate'],
       useRemotePkg: true,
     });
     ...
   });
 });

Based on learnings: "Implement mock behaviors in beforeEach blocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@code/core/src/common/js-package-manager/Yarn2Proxy.test.ts` around lines 71 -
87, The inline mockResolvedValue call on mockedExecuteCommand inside the test
"should use `yarn dlx` when useRemotePkg is true" should be moved into the test
file's beforeEach setup to follow project guidelines; update the beforeEach that
configures mocks to call mockedExecuteCommand.mockResolvedValue({ stdout: '' }
as any) (or add a new beforeEach if none exists) and remove the inline
mockedExecuteCommand.mockResolvedValue(...) from the test; leave the rest of the
test intact (yarn2Proxy.runPackageCommand invocation and the expect on
executeCommandSpy).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@code/core/src/common/js-package-manager/Yarn2Proxy.ts`:
- Around line 97-114: getPackageCommand currently always returns "yarn exec ..."
which mismatches runPackageCommand when useRemotePkg is true; change
getPackageCommand signature to accept a boolean useRemotePkg (default false) and
return `yarn dlx ...` when true and `yarn exec ...` when false, then update
callers (e.g., AddonVitestService usage) to pass the same useRemotePkg flag or
refactor those callers to derive the display string from runPackageCommand's
args so the displayed command matches the executed one.

---

Nitpick comments:
In `@code/core/src/common/js-package-manager/Yarn2Proxy.test.ts`:
- Around line 71-87: The inline mockResolvedValue call on mockedExecuteCommand
inside the test "should use `yarn dlx` when useRemotePkg is true" should be
moved into the test file's beforeEach setup to follow project guidelines; update
the beforeEach that configures mocks to call
mockedExecuteCommand.mockResolvedValue({ stdout: '' } as any) (or add a new
beforeEach if none exists) and remove the inline
mockedExecuteCommand.mockResolvedValue(...) from the test; leave the rest of the
test intact (yarn2Proxy.runPackageCommand invocation and the expect on
executeCommandSpy).