UI: Fix args not preserved in isolation mode#35055
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIsolation mode tool now requests origin hrefs with inherited args enabled. A new e2e test verifies that opening a story in isolation mode preserves the story's updated argument values in both the isolated window URL and rendered content. ChangesIsolation Mode Tool + Test
Sequence Diagram(s)sequenceDiagram
participant Manager
participant API
participant Window
Manager->>API: getStoryHrefs(storyId, { base: 'origin', refId, inheritArgs: true })
API-->>Manager: originHrefs (previewHref includes inherited args)
Manager->>Window: window.open(originHrefs.previewHref, ...)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
Hi maintainers! This is my first contribution. I don't have permission to add labels. Could you please add the
The manual testing section is already included in the PR description. Thank you! |
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for the PR. Could you please add an E2E test to the test suite in core, that shows this bug is fixed?
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/e2e-tests/manager.spec.ts`:
- Line 252: The test's page.waitForURL call assumes spaces are encoded as '+'
(e.g. 'args=label:Hello+world') which can be '%20' in some environments; update
the wait/assertion around page.waitForURL to be encoding-agnostic by retrieving
the URL (via page.url() or by using page.waitForURL with a predicate), decoding
the query component (using decodeURIComponent on the query or parsing with
URLSearchParams), and then asserting that the decoded 'args' value equals or
includes 'label:Hello world' (or alternatively use a regex in page.waitForURL
that accepts both '\\+' and '%20' for the page.waitForURL call referencing the
same 'args=label:Hello+world' literal).
🪄 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: c033ebc3-9fe3-4b20-aaa3-d0959fa50896
📒 Files selected for processing (1)
code/e2e-tests/manager.spec.ts
|
@coderabbitai Fixed the URL encoding assumption. Now using |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat code/e2e-tests/manager.spec.ts | grep -A 50 -B 5 "isolation mode preserves\|Isolation mode\|decodeURIComponent\|inheritArgs\|args"Length of output: 3802
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@Sidnioulz Done! Added the E2E test. Also addressed the CodeRabbit feedback about URL encoding — now using |
When clicking 'Open in isolation mode', the args query parameter was not being passed to the iframe URL. This happened because getStoryHrefs defaults inheritArgs to isCurrentStory, which could be false in certain situations. Fixed by explicitly passing inheritArgs: true when calling getStoryHrefs from the isolation mode button. Fixes storybookjs#35047
Updated URL handling to include robust decoding logic for args.
21c4340 to
a964586
Compare
Sidnioulz
left a comment
There was a problem hiding this comment.
Could you please check the E2E test failures in CI? Thanks!
Replace waitForLoadState with waitForSelector to ensure button is rendered before proceeding.
|
@Sidnioulz Thanks for catching that. The issue was waitForLoadState() — it only waits for the HTML to load, not for the Storybook preview JS to render the story. Fixed by explicitly waiting for #storybook-root button to appear before asserting. |
- Replace strict 'button' tag selectors with generic text locators (`getByText`) to support multi-framework sandboxes (Lit, Angular, etc.). - Update isolation page wait condition to look for any child element under `#storybook-root` to prevent CI timeouts. - Clean up internal comments to maintain project linting rules.
|
@Sidnioulz Hi! I've fixed the E2E test failures. CI checks seem stuck — could you help re-trigger the workflow when you get a chance? Thanks! |
I'm on it! Thanks for the ping! |
|
@sijie-Z did you manage to run the E2E tests locally, by the way? Would you like some help / instructions to know how to run them locally on your machine? |
3f28ddf to
ed26d4e
Compare
|
Yep, just ran them. Looks like the test is failing because customQueryParams isn't updated yet when the button is clicked. I'll fix the test and push an update. |
|
@Sidnioulz I dug into this a bit more.
I also noticed that the isolation link's mapper only subscribes to Given that, I think the failing test may be exposing a real issue in how updated args are propagated to the isolation link, rather than being purely a timing issue in the test. Does that match your understanding, or am I missing another path that would cause the link to be recomputed? |
The isolation URL could be generated before customQueryParams had been updated from STORY_ARGS_UPDATED. Since getStoryHrefs() returns concrete URL strings, the generated href could become stale and miss recently updated args. Compute the href when the button is clicked so it always uses the latest query params from the store.
searchParams.get() already decodes percent-encoded sequences, and decodeURIComponent does not decode + to space, causing the assertion to fail on Storybook's URL encoding convention where spaces are represented as +.
|
Fixed and updated. The original isolation-mode issue is now resolved, and the related dev/e2e checks are passing. The remaining CircleCI failure appears to be unrelated to this change and comes from SvelteKit sandbox creation (No svelte.config.js or svelte.config.ts found). |
Sorry I missed your reply! Don't hesitate to come have a discussion on https://discord.gg/invite/storybook (channel I don't know this product area well enough. I can ask the rest of the team if you're stuck, but it looks like you've updated your PR since so I'm first gonna review that :) |
fixes #35047
What I did
Fixed the "Open in isolation mode" button to preserve the
argsquery parameter when opening the iframe URL. AddedinheritArgs: trueto thegetStoryHrefscall inshare.tsx.Manual testing
yarn task --task sandbox --start-from auto --template react-vite/default-tsSummary by CodeRabbit
Bug Fixes
Tests