Skip to content

Conversation

@dannyhw
Copy link
Member

@dannyhw dannyhw commented Sep 26, 2025

Closes #
related to this:
storybookjs/react-native#795

What I did

Fixes an issue where react native storybook was crashing due to references to the document triggered by emotion web being loaded in storybook/theming

since this code isn't actually executed by RN then just moving it to be lazy loaded is enough to fix the crash.

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

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

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 pull request has been released as version 0.0.0-pr-32572-sha-c9578275. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information
Published version 0.0.0-pr-32572-sha-c9578275
Triggered by @yannbf
Repository storybookjs/storybook
Branch dannyhw/feat/rn-fix-openeditor-document-reference-error
Commit c9578275
Datetime Fri Sep 26 18:25:01 UTC 2025 (1758911101)
Workflow run 18046073542

To request a new release of this pull request, mention the @storybookjs/core team.

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

Summary by CodeRabbit

  • Refactor
    • Theme colors are now loaded during initialization rather than upfront, improving startup responsiveness.
    • Notification and error styling remain unchanged and continue to display correctly.
    • More robust theming when styling assets become available later.
    • No changes to public interfaces or user workflows.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 26, 2025

Walkthrough

Replaced the static top-level import of color from storybook/theming with a dynamic import('storybook/theming') inside the init routine, awaited before use. Notification rendering continues to reference color.negative from the dynamically loaded module. No exported/public API signatures were changed.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant M as Manager Module (open-in-editor)
  participant T as storybook/theming
  participant U as Notification UI

  Note over M: Dynamic import replaces previous static import

  M->>M: init()
  rect rgba(200, 220, 255, 0.25)
    note right of M: New/changed interaction (async)
    M->>T: import('storybook/theming')
    activate T
    T-->>M: { color }
    deactivate T
  end
  M->>U: render notification using color.negative

  opt Async timing
    Note over M,U: Notification rendering awaits theming module
  end
Loading

Suggested reviewers

  • ndelangen

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Title Check ✅ Passed The title clearly specifies the affected platform (React Native) and succinctly describes the main issue being resolved (“document reference error” in the open-in-editor module), offering enough context for reviewers to understand the primary change at a glance. It is concise, focused on the core fix, and avoids extraneous details or vague wording.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dannyhw/feat/rn-fix-openeditor-document-reference-error

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.

@dannyhw
Copy link
Member Author

dannyhw commented Sep 26, 2025

If someone could trigger a canary version then I could test to see if it works fully though I have already tested somewhat by editing in node_modules

@nx-cloud
Copy link

nx-cloud bot commented Sep 26, 2025

View your CI Pipeline Execution ↗ for commit 3c8cf0c

Command Status Duration Result
nx run-many -t check -c production --parallel=7 ✅ Succeeded 1s View ↗
nx run-many -t build -c production --parallel=3 ✅ Succeeded 3s View ↗

☁️ Nx Cloud last updated this comment at 2025-09-28 01:04:03 UTC

@dannyhw dannyhw force-pushed the dannyhw/feat/rn-fix-openeditor-document-reference-error branch from 589deeb to c957827 Compare September 26, 2025 17:59
@dannyhw
Copy link
Member Author

dannyhw commented Sep 26, 2025

the canary works for react native but something seems to be broken for the normal sandboxes

image

@dannyhw
Copy link
Member Author

dannyhw commented Sep 27, 2025

testing locally I found that changing the import to be relative fixes the bundling issue

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Would be nice to understand the relative import better, but otherwise LGTM

@shilman shilman changed the title React Native: Fix document reference error in open-in-editor when importing manager-api React Native: Fix document reference error in open-in-editor Sep 28, 2025
@shilman shilman merged commit fadeabc into next Sep 28, 2025
59 of 64 checks passed
@shilman shilman deleted the dannyhw/feat/rn-fix-openeditor-document-reference-error branch September 28, 2025 01:01
@github-actions github-actions bot mentioned this pull request Sep 28, 2025
16 tasks
@yannbf yannbf added the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Sep 29, 2025
@yannbf yannbf removed the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Oct 1, 2025
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.

4 participants