Core: Honor BROWSER shell scripts before xdg-open #33292
Conversation
| value.toLowerCase().endsWith('.sh') | ||
| ) { | ||
| action = Actions.SCRIPT; |
There was a problem hiding this comment.
Attn: this will cause a shell script to be passed on to a Node runtime. It will most likely cause bugs for other users.
There was a problem hiding this comment.
Yes, definitely, that was the bug though, right? #32949 My thought was that the reporter has that setup intentionally so that something happens in addition to the browser running.
There was a problem hiding this comment.
They want it to be called, but probably not by a Node runtime. Rather, by a Shell runtime. xdg-open will not run in Node.
I believe the way to handle this would be to add an Actions type for shell and to spawn a shell interpreter. Which leads to the question: what operating systems are expected to have .sh? Should POSIX be assumed? Should the presence of bash be assumed?
There was a problem hiding this comment.
Good questions. They weren't specific with the use-case, I was thinking the user would deal with the actual running of the script and the things within the script. I bet storybook doesn't really want to be in the business of figuring out OS's/shells that kind of thing.
If we do want to go down that route then I feel like we could decide things like what's assumed, Though actually spawning off a node script, or trying to, seems like pretty good 'best effort' for something that is not the main purpose of storybook.
There was a problem hiding this comment.
Agree that passing .sh to Node would be wrong; we either need a shell branch or drop .sh from this path. Since Storybook likely doesn’t want to manage cross-platform shell execution, I propose we support Node-run scripts only and remove .sh here.
There was a problem hiding this comment.
If the goal of the issue is to support xdg-open users, who rely on FreeDesktop.org standards (including most Linux and some BSD users), then surely it's fine to restrict .sh execution to POSIX compliant systems e.g. Linux, BSD, Mac and WSL. We can just call bash like we call node (even though users could prefer another JS interpreter).
There was a problem hiding this comment.
Oh wait, so I said 'thumbs up' but I thought of something else. This is for what I would call 'xdg-open'-like users in that they don't have xdg-open but rather some script, that presumably acts in the same way. I'm still going to implement it to work POSIX systems.
📝 WalkthroughWalkthroughRefactored the browser-opening utility to support shell script execution (.sh files) alongside existing Node scripts. Introduced BrowserEnvError for environment-related errors, centralized script execution event handling, expanded BROWSER environment variable interpretation for multiple script types (.js, .mjs, .cjs, .ts, .sh), enhanced control flow with platform checks, and added comprehensive test coverage. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant openBrowser
participant EnvCheck as BROWSER Interpretation
participant executeNode as executeNodeScript
participant executeShell as executeShellScript
participant BrowserProc as Browser Process
Caller->>openBrowser: openBrowser(startUrl)
openBrowser->>EnvCheck: Determine browserTarget from BROWSER env
alt BROWSER points to .js/.mjs/.cjs/.ts
EnvCheck->>openBrowser: SCRIPT
openBrowser->>executeNode: executeNodeScript(script, url)
executeNode->>executeNode: spawn node with script & url
executeNode->>Caller: return true
else BROWSER points to .sh (on Unix-like)
EnvCheck->>openBrowser: SHELL_SCRIPT
openBrowser->>executeShell: executeShellScript(script, url)
executeShell->>executeShell: spawn /bin/sh with script & url
executeShell->>Caller: return true
else BROWSER points to .sh (on Windows)
EnvCheck->>openBrowser: SHELL_SCRIPT
openBrowser->>BrowserProc: open(url, options)
BrowserProc->>Caller: proceed with open
else BROWSER is standard browser name
EnvCheck->>openBrowser: BROWSER
openBrowser->>BrowserProc: open(url, {app: BROWSER})
BrowserProc->>Caller: spawn browser process
else BROWSER not set or missing
EnvCheck->>openBrowser: Error (BrowserEnvError)
openBrowser->>Caller: throw BrowserEnvError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/core-server/utils/open-browser/opener.ts (1)
50-65: Function name is misleading for shell script support.The function name
executeNodeScriptaccurately reflects that it spawns scripts using Node.js (process.execPath), but the changes at lines 37-43 now route shell scripts here. If shell script support is added, this function should either:
- Be renamed to reflect generic script execution (e.g.,
executeScript)- Detect file extension and dispatch to appropriate runtime
- Remain Node-specific while shell scripts are handled by a separate function
This comment relates to the critical issue raised at lines 37-43.
🧹 Nitpick comments (1)
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts (1)
35-44: Add test coverage for .mjs and .cjs extensions.The changes in opener.ts added support for .mjs and .cjs extensions, but only .sh is tested here. Since .mjs and .cjs are valid Node.js module formats that should work with the current implementation, add test cases to verify they execute correctly via
executeNodeScript.Example additional tests:
it('executes .mjs script specified via BROWSER', () => { process.env.BROWSER = '/tmp/browser.mjs'; openBrowser('http://localhost:6006/'); expect(vi.mocked(spawn)).toHaveBeenCalledWith( process.execPath, ['/tmp/browser.mjs', 'http://localhost:6006/'], { stdio: 'inherit' } ); expect(vi.mocked(open)).not.toHaveBeenCalled(); }); it('executes .cjs script specified via BROWSER', () => { process.env.BROWSER = '/tmp/browser.cjs'; openBrowser('http://localhost:6006/'); expect(vi.mocked(spawn)).toHaveBeenCalledWith( process.execPath, ['/tmp/browser.cjs', 'http://localhost:6006/'], { stdio: 'inherit' } ); expect(vi.mocked(open)).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts(1 hunks)code/core/src/core-server/utils/open-browser/opener.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
code/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports
Files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
🧠 Learnings (15)
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Follow the spy mocking rules defined in `.cursor/rules/spy-mocking.mdc` for consistent mocking patterns with Vitest
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Write meaningful unit tests that actually import and call the functions being tested
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
| vi.mock('open', () => ({ default: vi.fn() })); | ||
| vi.mock('cross-spawn', () => ({ default: vi.fn() })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add spy: true option to mock calls.
Per coding guidelines, all package mocks in Vitest tests must use the spy: true option for consistent mocking patterns.
Apply this diff:
-vi.mock('open', () => ({ default: vi.fn() }));
-vi.mock('cross-spawn', () => ({ default: vi.fn() }));
+vi.mock('open', { spy: true });
+vi.mock('cross-spawn', { spy: true });Then update the beforeEach mock implementations at lines 24-26 to configure the spied functions:
beforeEach(() => {
vi.resetAllMocks();
process.env = { ...originalEnv, BROWSER: '/tmp/browser.sh' };
process.argv = ['node', 'test'];
Object.defineProperty(process, 'platform', { value: 'linux' });
vi.mocked(open).mockResolvedValue({} as unknown as Awaited<ReturnType<typeof open>>);
const child = { on: vi.fn() } as unknown as ChildProcess;
vi.mocked(spawn).mockReturnValue(child);
});As per coding guidelines for Vitest spy mocking.
🤖 Prompt for AI Agents
In code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts around
lines 10-11 and updating beforeEach at lines 24-26, the mocks for 'open' and
'cross-spawn' need the spy: true option and the test setup must configure the
spied functions; change vi.mock calls to include { spy: true }, then in
beforeEach replace the previous mock implementations with calls that configure
the spied functions: use vi.mocked(open).mockResolvedValue(...) and
vi.mocked(spawn).mockReturnValue(...) after resetting mocks and setting
process.env/argv/platform as shown in the review comment. Ensure types are
preserved (cast return values as needed) and remove any direct vi.fn()
replacements so the spy-based mocks are used.
code/core/src/core-server/utils/open-browser/__tests__/opener.test.ts
Outdated
Show resolved
Hide resolved
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
In addition to the CR changes I also moved the test file to be collocated with it's test file to be consistent with other test files. |
|
View your CI Pipeline Execution ↗ for commit e71775a ☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
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/core-server/utils/open-browser/opener.ts (1)
83-93: Replace console.log with node-logger.Per coding guidelines, Node.js code should use the server-side logger from
storybook/internal/node-loggerinstead ofconsole.log.+import { logger } from 'storybook/internal/node-logger';Then update the handler:
function attachCloseHandler(child: ReturnType<typeof spawn>, scriptPath: string) { child.on('close', (code) => { if (code !== 0) { - console.log(); - console.log(picocolors.red('The script specified as BROWSER environment variable failed.')); - console.log(`${picocolors.cyan(scriptPath)} exited with code ${code}.`); - console.log(); + logger.error( + `The script specified as BROWSER environment variable failed.\n${picocolors.cyan(scriptPath)} exited with code ${code}.` + ); return; } }); }
🧹 Nitpick comments (2)
code/core/src/core-server/utils/open-browser/opener.ts (2)
39-39: Use ES import for consistency.The file uses ES module imports elsewhere. Consider importing
fsat the top of the file for consistency with the codebase style.import { execSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; import { join } from 'node:path';Then update line 39:
- const version = require('fs').readFileSync('/proc/version', 'utf8').toLowerCase(); + const version = readFileSync('/proc/version', 'utf8').toLowerCase();
217-225: Clarify fallback behavior for shell scripts on Windows.When
canRunShellis false (native Windows), line 224 passes the.shpath tostartBrowserProcessas the browser app. This relies on theopenlibrary to handle it, which may have unpredictable results on Windows (could open in a text editor, fail silently, or prompt for an application).Consider either:
- Adding a warning/log when falling back on Windows to inform users their shell script won't execute as intended, or
- Returning
falseto indicate the browser couldn't be opened as configured.case Actions.SHELL_SCRIPT: { if (!value) { throw new BrowserEnvError('BROWSER environment variable is not set.'); } if (canRunShell) { return executeShellScript(value, url); } + logger.warn( + 'Shell scripts cannot be executed natively on Windows. Falling back to default browser behavior.' + ); return startBrowserProcess(browserTarget, url, args); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/core-server/utils/open-browser/opener.test.ts(1 hunks)code/core/src/core-server/utils/open-browser/opener.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/core-server/utils/open-browser/opener.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/core-server/utils/open-browser/opener.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/core-server/utils/open-browser/opener.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/utils/open-browser/opener.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/core-server/utils/open-browser/opener.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/core-server/utils/open-browser/opener.ts
🧠 Learnings (3)
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Follow existing patterns and conventions in the Storybook codebase
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-09-17T07:32:14.512Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/src/core-server/utils/open-in-browser.ts:6-6
Timestamp: 2025-09-17T07:32:14.512Z
Learning: When reviewing async functions that perform side effects like opening browsers, `.catch()` is preferred over `await` if the operation should be non-blocking. This prevents unhandled Promise rejections while keeping the operation as fire-and-forget.
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (3)
code/core/src/core-server/utils/open-browser/opener.ts (3)
58-68: LGTM!The extension detection now correctly differentiates between Node.js scripts (
.js,.mjs,.cjs) and shell scripts (.sh), addressing the original issue where shell scripts were incorrectly handled.
95-111: LGTM!The separation of
executeNodeScriptandexecuteShellScriptis clean. Using/bin/shis appropriate for POSIX systems, and thecanRunShellguard inopenBrowserensures this isn't called on native Windows.
72-81: Verify error code uniqueness forCORE_SERVERcategory.The error class is well-structured, but the hardcoded
code: 1should be verified to ensure it doesn't conflict with otherCORE_SERVERerror codes elsewhere in the codebase. Check that this code value is either unique within the category or follows the established pattern for error code allocation in this category.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/core-server/utils/open-browser/opener.ts (1)
8-8:isWslis effectively unused for real WSL and adds complexity tocanRunShellBecause
canRunShellis computed asprocess.platform !== 'win32' || isWsl(),isWsl()is only ever evaluated whenprocess.platform === 'win32'. In real WSL,process.platformis'linux', so the left side is alreadytrueandisWsl()is never consulted. That means:
- All non‑Windows platforms (including WSL) already satisfy
canRunShellviaprocess.platform !== 'win32'.isWsl()currently only runs on true Windows, where/proc/versiontypically doesn’t exist and the env flags aren’t set, so it just returnsfalseafter a failed read.Functionally this still does what you want (shell scripts only on non‑win32), but the WSL detection logic is redundant and can be confusing, especially given the WSL‑specific tests. I’d either:
- Simplify to
const canRunShell = process.platform !== 'win32';and dropisWsland its tests, or- If you truly need WSL detection, rework
canRunShellandisWslalong the usual pattern (isWslcheckingprocess.platform === 'linux'plus/proc/version/env heuristics) so that WSL is actually identified by that helper.This is a behavioral no‑op today but would reduce surprise for future maintainers.
How does Node.js report `process.platform` when running inside Windows Subsystem for Linux (WSL), and what’s the typical pattern libraries use to implement an `isWsl` helper?Also applies to: 30-41, 198-221
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/core-server/utils/open-browser/opener.test.ts(1 hunks)code/core/src/core-server/utils/open-browser/opener.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/opener.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/opener.test.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/opener.test.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/opener.test.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/core-server/utils/open-browser/opener.tscode/core/src/core-server/utils/open-browser/opener.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/core/src/core-server/utils/open-browser/opener.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/core-server/utils/open-browser/opener.test.ts
code/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports
Files:
code/core/src/core-server/utils/open-browser/opener.test.ts
🧠 Learnings (17)
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-09-17T07:32:14.512Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/src/core-server/utils/open-in-browser.ts:6-6
Timestamp: 2025-09-17T07:32:14.512Z
Learning: When reviewing async functions that perform side effects like opening browsers, `.catch()` is preferred over `await` if the operation should be non-blocking. This prevents unhandled Promise rejections while keeping the operation as fire-and-forget.
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Follow the spy mocking rules defined in `.cursor/rules/spy-mocking.mdc` for consistent mocking patterns with Vitest
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Write meaningful unit tests that actually import and call the functions being tested
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Test files should follow the naming pattern `*.test.ts`, `*.test.tsx`, `*.spec.ts`, or `*.spec.tsx`
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
🧬 Code graph analysis (1)
code/core/src/core-server/utils/open-browser/opener.test.ts (1)
code/core/src/core-server/utils/open-browser/opener.ts (1)
openBrowser(197-229)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/core/src/core-server/utils/open-browser/opener.ts (1)
11-19: Structured error handling and logging for script execution look goodUsing a dedicated
BrowserEnvError, centralizedattachCloseHandler, andlogger.errorfor non‑zero exit codes keeps failures from BROWSER scripts visible without breaking the main server flow. The boolean return contract ofexecuteNodeScript/executeShellScriptand the non‑blockingopen(url, options).catch(() => {})pattern instartBrowserProcessare consistent with existing behavior and prior guidance for fire‑and‑forget browser launches. As per coding guidelines, usingstorybook/internal/node-loggerinstead ofconsoleis also correct here.Also applies to: 69-89, 91-107
code/core/src/core-server/utils/open-browser/opener.test.ts (1)
3-12: Good, focused coverage of BROWSER script behaviors with consistent mockingThe test suite cleanly isolates BROWSER script handling across Node script extensions and shell-script scenarios, and the mocks for
openandcross-spawnfollow the project’s Vitest spy‑mocking rules (vi.mock(..., { spy: true }), central setup inbeforeEach, andvi.mocked()usage). This gives solid regression coverage for the newSCRIPT/SHELL_SCRIPTflows without over‑specifying internal details. Based on learnings, this aligns well with the repo’s testing and mocking guidelines.Also applies to: 18-29, 37-138
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/core-server/utils/open-browser/opener.ts (1)
185-213: openBrowser control flow now properly prioritizes scripts and shells before xdg-open/openThe updated switch cleanly routes:
SCRIPT→ Node script viaexecuteNodeScriptSHELL_SCRIPT→/bin/shon non‑Windows, or legacy browser-opening behavior on WindowsBROWSER/default → existingstartBrowserProcesspathThis satisfies the regression’s requirement: honor script-like BROWSER values (including
.sh) before falling back to mechanisms like xdg-open. One minor follow-up you might consider (not blocking) is updating the function JSDoc to mention shell scripts and the possibility ofBrowserEnvErrorbeing thrown, since it currently only references node.js scripts and boolean returns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/core-server/utils/open-browser/opener.test.ts(1 hunks)code/core/src/core-server/utils/open-browser/opener.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/core-server/utils/open-browser/opener.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/core-server/utils/open-browser/opener.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/core-server/utils/open-browser/opener.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/utils/open-browser/opener.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/core-server/utils/open-browser/opener.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/core-server/utils/open-browser/opener.ts
🧠 Learnings (3)
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-09-17T07:32:14.512Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/src/core-server/utils/open-in-browser.ts:6-6
Timestamp: 2025-09-17T07:32:14.512Z
Learning: When reviewing async functions that perform side effects like opening browsers, `.catch()` is preferred over `await` if the operation should be non-blocking. This prevents unhandled Promise rejections while keeping the operation as fire-and-forget.
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (5)
code/core/src/core-server/utils/open-browser/opener.ts (5)
8-19: Imports and logger usage look consistent with server-side guidelinesUsing
storybook/internal/node-loggerfor this Node.js utility matches the logging guidance forcode/**/*.{ts,tsx,js,jsx,mjs}and keeps output consistent with the rest of core-server.
23-54: BROWSER env classification for script vs shell looks correct but clearly POSIX-focusedThe extended
Actionsenum andgetBrowserEnvlogic cleanly distinguish JS-family scripts (.js/.mjs/.cjs) from shell scripts (.sh), while preserving the defaultBROWSERbehavior for all other values. This matches the linked issue’s intent: treat script-like BROWSER paths specially before falling back to the normal browser-launch flow, and only treat.shas a shell script when explicitly named as such.
56-65: BrowserEnvError is a reasonable specialization for env-related failuresWrapping these failures in a dedicated
BrowserEnvErrorthat extendsStorybookError(withcategory: 'CORE_SERVER') is a good way to make browser-env issues explicit and distinguishable from other errors.
87-94: Shell execution via/bin/shgated bycanRunShellis aligned with the POSIX-only goalUsing
/bin/shas the interpreter for.shBROWSER values on non‑Windows platforms is a pragmatic, portable choice and matches the discussion about limiting shell support to POSIX/WSL environments; the Windows case correctly falls back to the existing browser path instead of trying to run a shell script directly.
147-149: Non-blocking open with guardedcatchmatches existing async-side-effect guidanceThe parameterless
catchblocks here keep the behavior simple: errors from the AppleScript attempt and fromopen(url, options)are swallowed so browser-opening remains best-effort and non-fatal, whileopen(...).catch(() => {})avoids unhandled promise rejections. This is consistent with the “fire-and-forget” guidance for open-browser flows.Also applies to: 171-176
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks @robbchar! Even though we were all looking at the wrong thing initially, there are a bunch of improvements in your PR that I'd definitely like to see merged.
See my comments on the issue and my other PR for full context :) I think we can focus your PR on adding tests and making JS handling more robust, and let the other one handle xdg-open and shell scripting.
| }); | ||
|
|
||
| it('executes a node script when BROWSER points to a JS file', () => { | ||
| process.env.BROWSER = '/tmp/browser.js'; |
There was a problem hiding this comment.
@ndelangen do we have an env mocking library already? I've used mockedEnv in the past. I'm concerned we're not cleaning up env here so this could lead to future tests on the same runner behaving differently.
|
|
||
| function executeShellScript(scriptPath: string, url: string) { | ||
| const extraArgs = process.argv.slice(2); | ||
| const child = spawn('/bin/sh', [scriptPath, ...extraArgs, url], { |
There was a problem hiding this comment.
| const child = spawn('/bin/sh', [scriptPath, ...extraArgs, url], { | |
| const child = spawn('sh', [scriptPath, ...extraArgs, url], { |
POSIX does not mandate use of /bin/, but Node.js spawn should lookup commands through $PATH, ensuring that sh is found regardless of install location.
| value.toLowerCase().endsWith('.js') || | ||
| value.toLowerCase().endsWith('.mjs') || | ||
| value.toLowerCase().endsWith('.cjs') |
There was a problem hiding this comment.
This is good and should be kept. We could even extend to .ts for Node 24+.
| return executeShellScript(value, url); | ||
| } | ||
| return startBrowserProcess(browserTarget, url, args); | ||
| } |
There was a problem hiding this comment.
If I understand correctly what was going on with the reported bug, xdg-open used to gracefully handle Shell scripts (likely via the shebang), which is why we didn't need that.
I don't mind more explicit shell script handling but I don't know if we still need it. Another approach would be to rename Actions.BROWSER to Actions.OPEN and keep the code paths joined as they used to be before 10.0.
@ndelangen WDYT?
valentinpalkovic
changed the title
robbchar
force-pushed
the
fix/browser-script-bypass-xdg
branch
from
e78fb9f to
ae5c60d
Compare
robbchar
force-pushed
the
fix/browser-script-bypass-xdg
branch
from
ae5c60d to
e74681b
Compare
…'/bin/sh', handle error event)
robbchar
force-pushed
the
fix/browser-script-bypass-xdg
branch
from
e74681b to
6fea2b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/core-server/utils/open-browser/opener.test.ts (1)
63-74: Consider adding test coverage for.tsextension.The implementation supports
.tsfiles for Node.js script execution (in anticipation of Node 24+ native TypeScript support), but there's no test case for it. Adding a test would ensure this path is covered.Add a test case after the
.cjstest:it('executes a node script when BROWSER points to a TS file', () => { process.env.BROWSER = '/tmp/browser.ts'; openBrowser('http://localhost:6006/'); expect(vi.mocked(spawn)).toHaveBeenCalledWith( process.execPath, ['/tmp/browser.ts', 'http://localhost:6006/'], { stdio: 'inherit' } ); expect(vi.mocked(open)).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/core-server/utils/open-browser/opener.test.ts(1 hunks)code/core/src/core-server/utils/open-browser/opener.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/core/src/core-server/utils/open-browser/opener.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/core/src/core-server/utils/open-browser/opener.test.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/core-server/utils/open-browser/opener.test.tscode/core/src/core-server/utils/open-browser/opener.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/core-server/utils/open-browser/opener.test.tscode/core/src/core-server/utils/open-browser/opener.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/core-server/utils/open-browser/opener.test.tscode/core/src/core-server/utils/open-browser/opener.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/core-server/utils/open-browser/opener.test.tscode/core/src/core-server/utils/open-browser/opener.ts
code/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports
Files:
code/core/src/core-server/utils/open-browser/opener.test.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/core-server/utils/open-browser/opener.test.tscode/core/src/core-server/utils/open-browser/opener.ts
🧠 Learnings (21)
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Write meaningful unit tests that actually import and call the functions being tested
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Follow the spy mocking rules defined in `.cursor/rules/spy-mocking.mdc` for consistent mocking patterns with Vitest
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{test,spec}.{ts,tsx,js,jsx} : Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Test files should follow the naming pattern `*.test.ts`, `*.test.tsx`, `*.spec.ts`, or `*.spec.tsx`
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.tscode/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking without the `spy: true` option in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/core-server/utils/open-browser/opener.test.ts
📚 Learning: 2025-09-17T07:32:14.512Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/src/core-server/utils/open-in-browser.ts:6-6
Timestamp: 2025-09-17T07:32:14.512Z
Learning: When reviewing async functions that perform side effects like opening browsers, `.catch()` is preferred over `await` if the operation should be non-blocking. This prevents unhandled Promise rejections while keeping the operation as fire-and-forget.
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to **/*.{ts,tsx} : Enable TypeScript strict mode
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use client-side logger from 'storybook/internal/client-logger' for browser code
Applied to files:
code/core/src/core-server/utils/open-browser/opener.ts
🧬 Code graph analysis (2)
code/core/src/core-server/utils/open-browser/opener.test.ts (1)
code/core/src/core-server/utils/open-browser/opener.ts (1)
openBrowser(190-222)
code/core/src/core-server/utils/open-browser/opener.ts (1)
code/core/src/server-errors.ts (1)
StorybookError(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (8)
code/core/src/core-server/utils/open-browser/opener.ts (6)
10-17: LGTM on imports.The logger import from
storybook/internal/node-loggercorrectly follows the coding guidelines for Node.js code, andStorybookErroris properly imported for the custom error class.
41-52: LGTM on extended BROWSER detection.The expanded extension detection properly routes Node.js scripts (.js, .mjs, .cjs, .ts) to
Actions.SCRIPTand shell scripts (.sh) toActions.SHELL_SCRIPT. The case-insensitive matching is appropriate.
56-65: LGTM on custom error class.
BrowserEnvErrorproperly extendsStorybookErrorwith appropriate category and provides clear error messaging for BROWSER configuration issues.
67-82: LGTM on centralized event handling.This properly addresses the concern about unhandled spawn errors by attaching both
errorandcloselisteners. The centralized approach keeps logging consistent across script execution paths.
93-100: LGTM on shell script execution.Using
'sh'instead of'/bin/sh'correctly follows POSIX conventions—Node.jsspawnwill look up the shell interpreter via$PATH, ensuring portability across different Unix-like systems whereshmay not be at/bin/sh.
206-214: LGTM on SHELL_SCRIPT handling with Windows fallback.The platform check appropriately routes shell scripts to direct execution on POSIX systems and falls back to
startBrowserProcesson Windows, which delegates to the OS's handling. This maintains the fire-and-forget behavior.code/core/src/core-server/utils/open-browser/opener.test.ts (2)
10-11: LGTM on mock setup.Using
vi.mock()with{ spy: true }correctly follows the coding guidelines for Vitest mocking patterns.
18-35: LGTM on test setup and cleanup.The use of
vi.spyOn(process, 'platform', 'get')correctly addresses the past review concern about platform mocking. Mock implementations are properly placed inbeforeEach, andvi.restoreAllMocks()ensures cleanup.
| expect(vi.mocked(spawn)).toHaveBeenCalledWith( | ||
| '/bin/sh', | ||
| ['/tmp/findAHandler.sh', 'http://localhost:6006/'], | ||
| { stdio: 'inherit' } | ||
| ); |
There was a problem hiding this comment.
Test expects /bin/sh but implementation uses sh.
The implementation at opener.ts line 95 uses spawn('sh', ...) (without path prefix) per past review guidance on POSIX compliance. This test will fail because it expects /bin/sh.
Apply this diff to align with the implementation:
expect(vi.mocked(spawn)).toHaveBeenCalledWith(
- '/bin/sh',
+ 'sh',
['/tmp/findAHandler.sh', 'http://localhost:6006/'],
{ stdio: 'inherit' }
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(vi.mocked(spawn)).toHaveBeenCalledWith( | |
| '/bin/sh', | |
| ['/tmp/findAHandler.sh', 'http://localhost:6006/'], | |
| { stdio: 'inherit' } | |
| ); | |
| expect(vi.mocked(spawn)).toHaveBeenCalledWith( | |
| 'sh', | |
| ['/tmp/findAHandler.sh', 'http://localhost:6006/'], | |
| { stdio: 'inherit' } | |
| ); |
🤖 Prompt for AI Agents
In code/core/src/core-server/utils/open-browser/opener.test.ts around lines 96
to 100, the test asserts the spawn call used '/bin/sh' but the implementation
calls 'sh'; update the test expectation to match the implementation by replacing
'/bin/sh' with 'sh' in the toHaveBeenCalledWith assertion so the mocked spawn
check matches the actual call signature.
|
@Sidnioulz I'm sorry but I'm a bit confused should I be doing something like this: if (hasNodeShebang(file)) {
spawn(process.execPath, [file])
} else {
open(file)
}? thanks |
4 participants
Closes #32949
What I did
Modified the code to allow for more script types other than just .js. (As a disclaimer I can't test this in a reproduction in my current environment, Windows Home...)
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
As I said previously I can't test this in my local environment, Windows Home... I used Github Codespaces to repro the bug I just can't easily run against a fix.
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.tsMake 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
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.