bug: BROWSER and BROWSER_ARGS env are not respected#34513
bug: BROWSER and BROWSER_ARGS env are not respected#34513ianzone wants to merge 4 commits intostorybookjs:nextfrom
Conversation
pass correct option to open()
There was a problem hiding this comment.
Pull request overview
Fixes Storybook’s browser-opening behavior so BROWSER/BROWSER_ARGS are passed through to the open() library correctly (closing #30553).
Changes:
- Updates
open()invocation to pass browser arguments viaapp: { name, arguments }instead of the legacy “array form”.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced legacy string-to-array coercion for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/core-server/utils/open-browser/opener.ts`:
- Line 192: The options object currently always sets app: { name: browser,
arguments: args } which produces an invalid shape when browser is undefined and
causes open() to reject silently; change the logic around the options
construction in opener.ts so that you only include the app property when browser
is a defined string (use browser and args to build app only when browser !=
null/undefined), otherwise create options without app (e.g., { wait:false,
url:true }); also ensure the open(url, options) rejection is not swallowed—catch
errors from open() and return false or propagate the error instead of always
returning true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a4b2d6d-bfb5-4e4c-bca3-4549504073df
📒 Files selected for processing (1)
code/core/src/core-server/utils/open-browser/opener.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/core-server/utils/open-browser/opener.ts`:
- Around line 192-196: Add a regression test in
code/core/src/core-server/utils/open-browser/opener.test.ts that exercises the
new BROWSER_ARGS path: set up the environment (or pass values) so the code path
that constructs the options object in opener.ts runs with a non-empty args
value, call the function that builds the options (the same entry used by
existing tests around lines 97-109), and assert that the returned options.app is
an object with name equal to the provided browser and arguments equal to the
expected args array/string; also include a test case for when browser is truthy
but no args are provided to ensure arguments is undefined or handled as before.
Ensure the test cleans up environment changes so it won’t affect other tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9036eee4-c550-4123-8fd6-7771db87d349
📒 Files selected for processing (1)
code/core/src/core-server/utils/open-browser/opener.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const options = { | ||
| app: browser ? { name: browser, arguments: args } : undefined, | ||
| wait: false, | ||
| url: true |
There was a problem hiding this comment.
Formatting in the new options object is inconsistent with surrounding code (missing trailing comma after url: true and missing comma in the closing brace block). With the repo formatter config (.oxfmtrc.json sets trailingComma: "es5") and existing style in this file (e.g. { stdio: 'inherit', }), this is likely to be auto-reformatted or fail lint/format checks.
| url: true | |
| url: true, |
| try { | ||
| const options = { app: browser, wait: false, url: true }; | ||
| const options = { | ||
| app: browser ? { name: browser, arguments: args } : undefined, | ||
| wait: false, | ||
| url: true |
There was a problem hiding this comment.
browserTarget originates from process.env.BROWSER (a string | undefined), but it’s cast to App | readonly App[] and then used to build { name: browser, arguments: args }. This wider typing obscures the actual contract and makes it easy to accidentally pass an App object/array as the name. Consider narrowing startBrowserProcess/browserTarget to string | undefined (and keep app construction strictly in terms of a browser name), which will also avoid needing type assertions here.
pass correct options to open()
Closes #30553
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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