fix: correctly merge query params when preview-url contains query string#34685
fix: correctly merge query params when preview-url contains query string#34685aayushbaluni wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe change refactors ChangesURL Resolution and Query Parameter Handling
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/manager-api/modules/url.ts (1)
290-304:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMerge
queryParamsinto theURLobject instead of appending them.This still duplicates keys that already exist in
PREVIEW_URL. For example,PREVIEW_URL='.../?custom=true'plusqueryParams: { custom: 'false' }produces both values, soURLSearchParams.get('custom')still resolves to the old one. The fix should overwrite preview-side params onpreviewUrlObject.searchParamsbefore serializing the final href.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/modules/url.ts` around lines 290 - 304, The preview href is built by string-concatenating query strings which preserves existing PREVIEW_URL keys instead of overwriting them; replace that string concat with a URL object merge: construct a URL (e.g., new URL(previewHrefPrefix + previewPathQuery + previewHash) or new URL(previewHrefPrefix) with previewPathQuery applied), then iterate over the provided queryParams (and any computed params like customPreviewParams/argsParam/globalsParam) and call previewUrlObject.searchParams.set(key, value) to overwrite existing keys on previewUrlObject.searchParams, then serialize previewHref from previewUrlObject.toString(); update the return that sets previewHref (and keep managerHref logic unchanged) and ensure you reference previewHrefPrefix, previewPathQuery, previewHash, customPreviewParams, argsParam, globalsParam, and previewHref when making these changes.
🧹 Nitpick comments (1)
code/core/src/manager-api/tests/url.test.js (1)
507-524: ⚡ Quick winAdd a relative
PREVIEW_URLcase here too.This only covers the absolute branch, but the original repro also uses relative overrides like
iframe.html?foo=bar. A companion assertion for a relative preview URL would cover theisAbsolutePreviewUrl === falsepath that now does the extra resolve/strip-origin work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager-api/tests/url.test.js` around lines 507 - 524, The test only exercises the absolute PREVIEW_URL branch; add a companion test that sets global.PREVIEW_URL to a relative value like 'iframe.html?custom=true', calls initURL (same usage as the existing test), calls api.getStoryHrefs('test--story'), parses previewHref and asserts the same expectations (search param 'custom' === 'true', 'id' === 'test--story', 'viewMode' === 'story', and no duplicate '?' in previewHref), and clean up by deleting global.PREVIEW_URL; this will exercise the isAbsolutePreviewUrl === false path and the resolve/strip-origin logic in initURL/getStoryHrefs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@code/core/src/manager-api/modules/url.ts`:
- Around line 290-304: The preview href is built by string-concatenating query
strings which preserves existing PREVIEW_URL keys instead of overwriting them;
replace that string concat with a URL object merge: construct a URL (e.g., new
URL(previewHrefPrefix + previewPathQuery + previewHash) or new
URL(previewHrefPrefix) with previewPathQuery applied), then iterate over the
provided queryParams (and any computed params like
customPreviewParams/argsParam/globalsParam) and call
previewUrlObject.searchParams.set(key, value) to overwrite existing keys on
previewUrlObject.searchParams, then serialize previewHref from
previewUrlObject.toString(); update the return that sets previewHref (and keep
managerHref logic unchanged) and ensure you reference previewHrefPrefix,
previewPathQuery, previewHash, customPreviewParams, argsParam, globalsParam, and
previewHref when making these changes.
---
Nitpick comments:
In `@code/core/src/manager-api/tests/url.test.js`:
- Around line 507-524: The test only exercises the absolute PREVIEW_URL branch;
add a companion test that sets global.PREVIEW_URL to a relative value like
'iframe.html?custom=true', calls initURL (same usage as the existing test),
calls api.getStoryHrefs('test--story'), parses previewHref and asserts the same
expectations (search param 'custom' === 'true', 'id' === 'test--story',
'viewMode' === 'story', and no duplicate '?' in previewHref), and clean up by
deleting global.PREVIEW_URL; this will exercise the isAbsolutePreviewUrl ===
false path and the resolve/strip-origin logic in initURL/getStoryHrefs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b05e0993-cef6-41e1-aa61-228acdf415b4
📒 Files selected for processing (2)
code/core/src/manager-api/modules/url.tscode/core/src/manager-api/tests/url.test.js
|
Hi @aayushbaluni, Due to a recent high volume of unreviewed AI-generated PRs, we are requesting verification and proof that the implemented fix actually works. Please provide a simple GIF/Video or image of how the fix works, optimally with before-and-after comparisons. Thank you for your understanding! |
|
Closing due to inactivity |
github-project-automation
Bot
moved this from Human verification
to Done
in Core Team Projects
2 participants
Problem
When
--preview-urlincludes a query string (e.g.,http://localhost:6007/?custom=true), Storybook 10.2+ appends additional parameters with a second?instead of&, producing a malformed iframe URL.Change
Use proper URL/query string merging when building the iframe src so existing query parameters from the preview URL are preserved and new parameters are appended with
&.Fixes #34615.
Made with Cursor
Summary by CodeRabbit