Skip to content

Conversation

@ndelangen
Copy link
Member

@ndelangen ndelangen commented Sep 17, 2025

What I did

We were using better-opn in place of open, but in the mean time open has moved to ESM-only, and improved a lot, so it's in fact now better.

This PR switches to the latest version of open.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

I ran nr storybook:ui in the monorepo, and my default browser in a new tab.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Summary by CodeRabbit

  • Bug Fixes

    • More reliable "open in browser" when starting the server: unified behavior, non-blocking attempts, and clearer error messages with guidance for CI/docker or headless environments.
  • Chores

    • Updated browser-launch dependency to a newer release and removed legacy dependency.
    • Cleaned up internal typings related to removed packages.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

Walkthrough

Removed better-opn, upgraded open, deleted ambient module declarations for both, and simplified the browser-opening utility to a single async open(address) call with try/catch; callers now handle the returned Promise (errors are caught where used).

Changes

Cohort / File(s) Summary
Dependency update
code/core/package.json
Removed better-opn; bumped open from ^8.4.0 to ^10.2.0.
Typings cleanup
code/core/src/core-server/typings.d.ts, code/core/src/typings.d.ts
Removed ambient module declarations for better-opn and open.
Browser open util refactor
code/core/src/core-server/utils/open-in-browser.ts, code/core/src/core-server/dev-server.ts
openInBrowser changed to async and now await open(address) with try/catch and standardized logging; removed default-browser detection and better-opn usage. dev-server now calls openInBrowser(...).catch(() => {}) to swallow open errors when appropriate.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller as Core Server / Dev Server
  participant Util as openInBrowser()
  participant Open as open (pkg)

  Caller->>Util: openInBrowser(address)
  activate Util
  Util->>Open: await open(address)
  alt resolved
    Open-->>Util: resolved
    Util-->>Caller: return
  else throws
    Open-->>Util: throws
    note right of Util #F9F0C1: catch error\nlog guidance for CI/docker
    Util-->>Caller: return (after logging)
  end
  deactivate Util
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I thump my paws—one call to start,
No forks, no guesses, just a single part.
Old typings trimmed, the meadow clears,
A hopeful hop, no tangled gears. 🐇✨

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch norbert/updates-2025-september-17

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ec0168 and 4d43dc3.

📒 Files selected for processing (1)
  • code/core/src/core-server/dev-server.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T07:32:14.477Z
Learnt from: ndelangen
PR: storybookjs/storybook#32484
File: code/core/src/core-server/utils/open-in-browser.ts:6-6
Timestamp: 2025-09-17T07:32:14.477Z
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/dev-server.ts
🧬 Code graph analysis (1)
code/core/src/core-server/dev-server.ts (1)
code/core/src/core-server/utils/open-in-browser.ts (1)
  • openInBrowser (6-16)
⏰ 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). (2)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/core-server/dev-server.ts (1)

124-127: LGTM! Error handling follows the expected pattern.

The addition of .catch() to handle potential browser opening failures is appropriate. This aligns with the fire-and-forget pattern for non-critical operations, as learned from previous feedback. The empty catch handler with a comment explaining the non-critical nature is exactly what's needed here.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title accurately reflects the primary change in the PR—upgrading the open package (and implicitly replacing better-opn)—and is concise and focused on the main dependency update.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

@nx-cloud
Copy link

nx-cloud bot commented Sep 17, 2025

View your CI Pipeline Execution ↗ for commit 4d43dc3

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 50s View ↗

☁️ Nx Cloud last updated this comment at 2025-09-17 07:53:06 UTC

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
code/core/src/core-server/utils/open-in-browser.ts (1)

6-15: Propagate success/failure or surface the underlying error.

Right now failures are logged but swallowed, giving callers no signal. Prefer returning a boolean or rethrowing after logging; also log the underlying error at debug level to aid diagnosis.

Apply this diff:

-export async function openInBrowser(address: string) {
+export async function openInBrowser(address: string): Promise<boolean> {
   try {
-    await open(address);
+    await open(address);
+    return true;
   } catch (error) {
-    logger.error(dedent`
+    logger.error(dedent`
         Could not open ${address} inside a browser. If you're running this command inside a
         docker container or on a CI, you need to pass the '--ci' flag to prevent opening a
         browser by default.
       `);
+    // Include details for troubleshooting without spamming normal logs
+    logger.debug(error);
+    return false;
   }
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b03fe2e and 0ec0168.

⛔ Files ignored due to path filters (1)
  • code/yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (4)
  • code/core/package.json (1 hunks)
  • code/core/src/core-server/typings.d.ts (0 hunks)
  • code/core/src/core-server/utils/open-in-browser.ts (1 hunks)
  • code/core/src/typings.d.ts (0 hunks)
💤 Files with no reviewable changes (2)
  • code/core/src/core-server/typings.d.ts
  • code/core/src/typings.d.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: Core Unit Tests, windows-latest

@storybook-app-bot
Copy link

storybook-app-bot bot commented Sep 17, 2025

Package Benchmarks

Commit: 4d43dc3, ran on 17 September 2025 at 07:42:56 UTC

The following packages have significant changes to their size or dependencies:

storybook

Before After Difference
Dependency count 48 43 🎉 -5 🎉
Self size 30.11 MB 30.04 MB 🎉 -78 KB 🎉
Dependency size 17.65 MB 17.30 MB 🎉 -346 KB 🎉
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 192 187 🎉 -5 🎉
Self size 886 KB 886 KB 🚨 +36 B 🚨
Dependency size 80.06 MB 79.63 MB 🎉 -424 KB 🎉
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 174 169 🎉 -5 🎉
Self size 35 KB 35 KB 0 B
Dependency size 76.49 MB 76.06 MB 🎉 -424 KB 🎉
Bundle Size Analyzer Link Link

create-storybook

Before After Difference
Dependency count 49 44 🎉 -5 🎉
Self size 1.55 MB 1.55 MB 0 B
Dependency size 47.76 MB 47.34 MB 🎉 -424 KB 🎉
Bundle Size Analyzer node node

@ndelangen ndelangen merged commit 63bb329 into next Sep 17, 2025
59 checks passed
@ndelangen ndelangen deleted the norbert/updates-2025-september-17 branch September 17, 2025 08:07
@github-actions github-actions bot mentioned this pull request Sep 17, 2025
16 tasks
@coderabbitai coderabbitai bot mentioned this pull request Oct 27, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants