fix(core): Update logic to use base path instead of full pathname #33686
Conversation
JSMike
force-pushed
the
33548-fix-base-path
branch
from
47bfeed to
92069d1
Compare
📝 WalkthroughWalkthroughChanges URL canonicalization logic in refs manager by replacing path segment removal method: instead of stripping '/iframe.html' and trailing slashes, the code now uses regex pattern Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@code/core/src/manager-api/modules/refs.ts`:
- Around line 84-85: The normalization currently builds localFull/sourceFull
from localOrigin+localPathname and then strips the last path segment with
.replace(/\/[^\/]*$/,''), which collapses a trailing slash form
(https://host/storybook/) to https://host/storybook but turns
https://host/storybook into https://host; change the logic in the localFull and
sourceFull construction to normalize by removing only a trailing slash (e.g.,
replace(/\/$/,'')) or, better, use the URL API to join origin+pathname and then
trim a single trailing slash so both https://host/storybook and
https://host/storybook/ normalize to the same value; update the expressions that
compute localFull and sourceFull (and any callers that rely on them)
accordingly.
| const localFull = `${localOrigin + localPathname}`.replace(/\/[^\/]*$/, ''); | ||
| const sourceFull = `${sourceOrigin + sourcePathname}`.replace(/\/[^\/]*$/, ''); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n code/core/src/manager-api/modules/refs.ts | head -100Repository: storybookjs/storybook
Length of output: 3877
🏁 Script executed:
# Check where getSourceType is used
rg "getSourceType" code/core/src/manager-api/modules/refs.ts -B 2 -A 2
# Search for tests of this function
fd "refs" code/core --type f -name "*.test.ts" -o -name "*.spec.ts" | head -5Repository: storybookjs/storybook
Length of output: 529
🏁 Script executed:
# Find test files for refs
fd "refs" code/core -name "*test*" -o -name "*spec*"
# Search for usage of getSourceType
rg "getSourceType" code/core --type ts --type tsxRepository: storybookjs/storybook
Length of output: 323
🏁 Script executed:
# Find test files for refs module
find code/core -name "*refs*test*" -o -name "*refs*spec*"
# Search for getSourceType usage with correct rg syntax
rg "getSourceType" code/coreRepository: storybookjs/storybook
Length of output: 1135
🏁 Script executed:
cat code/core/src/manager-api/tests/refs.test.tsRepository: storybookjs/storybook
Length of output: 34831
🏁 Script executed:
# Let me trace through the regex behavior with concrete examples
python3 << 'EOF'
import re
# Test the regex behavior
regex = r'/[^/]*$'
test_cases = [
'https://storybook.js.org/storybook/iframe.html',
'https://storybook.js.org/storybook/',
'https://storybook.js.org/storybook',
'https://external.com/storybook/',
'https://external.com/storybook',
]
for url in test_cases:
result = re.sub(regex, '', url)
print(f"{url:50} → {result}")
EOFRepository: storybookjs/storybook
Length of output: 151
🏁 Script executed:
# Let's manually verify the regex behavior with simple shell commands
echo "Testing regex replacement /\/[^\/]*$/:"
echo ""
echo "Input: https://storybook.js.org/storybook/iframe.html"
echo "https://storybook.js.org/storybook/iframe.html" | sed 's/\/[^\/]*$//'
echo ""
echo "Input: https://storybook.js.org/storybook/"
echo "https://storybook.js.org/storybook/" | sed 's/\/[^\/]*$//'
echo ""
echo "Input: https://storybook.js.org/storybook"
echo "https://storybook.js.org/storybook" | sed 's/\/[^\/]*$//'
echo ""
echo "Input: https://external.com/storybook/"
echo "https://external.com/storybook/" | sed 's/\/[^\/]*$//'Repository: storybookjs/storybook
Length of output: 411
🏁 Script executed:
# Check if tests cover the scenario: location with trailing slash vs ref without trailing slash
grep -n "https://storybook.js.org/storybook'" code/core/src/manager-api/tests/refs.test.ts | head -20Repository: storybookjs/storybook
Length of output: 166
🏁 Script executed:
# Check the context around those line numbers to understand what's being tested
sed -n '140,170p' code/core/src/manager-api/tests/refs.test.tsRepository: storybookjs/storybook
Length of output: 1352
🏁 Script executed:
# Let me trace the exact scenario the review comment mentions
# Location: /storybook/ (trailing slash)
# Source ref: /storybook (no trailing slash, no filename)
echo "Scenario from review comment:"
echo "Manager location: https://storybook.js.org/storybook/ (with trailing slash)"
echo "Ref source: https://storybook.js.org/storybook (no trailing slash, no filename)"
echo ""
echo "After regex /\/[^\/]*$/:"
echo "localFull: https://storybook.js.org/storybook/ →" $(echo 'https://storybook.js.org/storybook/' | sed 's/\/[^\/]*$//')
echo "sourceFull: https://storybook.js.org/storybook →" $(echo 'https://storybook.js.org/storybook' | sed 's/\/[^\/]*$//')
echo ""
echo "Result: These DO NOT MATCH, so getSourceType() returns 'external' (INCORRECT)"
echo ""
echo "This edge case is NOT covered in the test suite."Repository: storybookjs/storybook
Length of output: 563
The base-path normalization logic has a trailing-slash edge case that could cause refs to be misclassified.
The regex at lines 84–85 removes the last path segment including its slash. This means:
https://host/storybook/normalizes tohttps://host/storybookhttps://host/storybooknormalizes tohttps://host
If a ref is configured without a trailing slash (e.g., https://host/storybook) but the manager runs at a path with a trailing slash (e.g., https://host/storybook/), they will normalize to different values and the ref will be incorrectly classified as external instead of local. This scenario is not covered by the current test suite.
🤖 Prompt for AI Agents
In `@code/core/src/manager-api/modules/refs.ts` around lines 84 - 85, The
normalization currently builds localFull/sourceFull from
localOrigin+localPathname and then strips the last path segment with
.replace(/\/[^\/]*$/,''), which collapses a trailing slash form
(https://host/storybook/) to https://host/storybook but turns
https://host/storybook into https://host; change the logic in the localFull and
sourceFull construction to normalize by removing only a trailing slash (e.g.,
replace(/\/$/,'')) or, better, use the URL API to join origin+pathname and then
trim a single trailing slash so both https://host/storybook and
https://host/storybook/ normalize to the same value; update the expressions that
compute localFull and sourceFull (and any callers that rely on them)
accordingly.
There was a problem hiding this comment.
I tested with refs with both trailing and non-trailing and both worked, the ref adds /index.html to the link to external storybook properly in both cases.
| } | ||
|
|
||
| const pathname = location.pathname || '/'; | ||
| const pathname = location.pathname?.replace(/[^\/]*$/, '') || '/'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and read the relevant section
cat -n code/core/src/manager-api/modules/url.ts | head -280 | tail -50Repository: storybookjs/storybook
Length of output: 2462
🏁 Script executed:
# Let's also check the full context around line 250
cat -n code/core/src/manager-api/modules/url.ts | sed -n '240,270p'Repository: storybookjs/storybook
Length of output: 1518
🏁 Script executed:
# Search for how managerBase and previewBase are computed
grep -n "managerBase\|previewBase" code/core/src/manager-api/modules/url.tsRepository: storybookjs/storybook
Length of output: 519
🏁 Script executed:
# Look for tests related to url module
fd -e "test\|spec" -p "url" code/Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Search for tests related to getStoryHrefs or url.ts
find code -name "*.test.*" -o -name "*.spec.*" | xargs grep -l "getStoryHrefs\|url" 2>/dev/null | head -20Repository: storybookjs/storybook
Length of output: 1078
🏁 Script executed:
# Look at git history for this specific line/function
cd code/core/src/manager-api/modules && git log -p --all -S "replace(/\[^\/\]\*\$/," -- url.ts | head -150Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Check if there are any comments or documentation about pathname handling
grep -n -B5 -A5 "pathname" code/core/src/manager-api/modules/url.ts | head -50Repository: storybookjs/storybook
Length of output: 799
🏁 Script executed:
# Search for any existing issue discussions or comments about trailing slashes
grep -rn "trailing slash\|bare.*path\|directory.*path" code/core/src/manager-api/ 2>/dev/nullRepository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Let's examine the url.test.js file to see what tests exist
cat -n code/core/src/manager-api/tests/url.test.jsRepository: storybookjs/storybook
Length of output: 19528
🏁 Script executed:
# Let's also search for any references to pathname without trailing slash
grep -rn "pathname" code/core/src/manager-api/modules/url.tsRepository: storybookjs/storybook
Length of output: 311
🏁 Script executed:
# Check if there are integration tests or other references to getStoryHrefs
grep -rn "getStoryHrefs" code --include="*.test.*" --include="*.spec.*"Repository: storybookjs/storybook
Length of output: 1901
Preserve directory-like paths without file extensions.
Line 250 strips the last path segment unconditionally, collapsing /storybook to / or /index.html to /. This affects both managerBase and previewBase. To handle paths correctly, distinguish between actual files (with extensions) and directory-like paths—preserve directory paths by either keeping them intact or ensuring a trailing slash, while only stripping file segments.
💡 Suggested fix
- const pathname = location.pathname?.replace(/[^\/]*$/, '') || '/';
+ const rawPathname = location.pathname || '/';
+ const pathname = rawPathname.endsWith('/')
+ ? rawPathname
+ : /\.[^/]+$/.test(rawPathname)
+ ? rawPathname.replace(/[^/]*$/, '')
+ : `${rawPathname}/`;There was a problem hiding this comment.
this feedback is also assuming that a directory wouldn't have a period. ie: /my-storybook/1.0, from my experience the trailing slash has always been required and caused other file path issues if you navigate to a path not ending in /
JSMike
force-pushed
the
33548-fix-base-path
branch
2 times, most recently
from
6273404 to
a61919a
Compare
|
Thanks for contributing! Looks like you took a very similar approach to the one I took here. Sorry I hadn't seen this PR when I fixed my own mess, otherwise I would've attributed you. |
|
no worries @ghengeveld, I can fixup my PR, this still has changes to |
Co-authored-by: ia319 <linglingmeng876@gmail.com>
JSMike
force-pushed
the
33548-fix-base-path
branch
from
a61919a to
8ea6033
Compare
|
@ghengeveld I've updated and synced with |
2 participants
Closes #33548
What I did
Updated url.ts and ref.ts to use base path instead of full pathname. This solved a logic error when finding relative files that had assumed the pathname didn't end with
/PR based on #33624
I've added @ia319 as co-author to the commit.
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!
Basic test:
yarn startAdvanced with refs:
yarn task --task sandbox --start-from auto --template react-vite/default-tsyarn task --task sandbox --start-from auto --template lit-vite/default-tsnpm run build-storybooknpx http-server . --cors --port 8080npm run build-storybooknpx http-server . --cors --port 8081Documentation
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
✏️ Tip: You can customize this high-level summary in your review settings.