Skip to content

Fix base element href include localhost #271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Aug 11, 2025

Conversation

stephanie56
Copy link
Member

@stephanie56 stephanie56 commented Aug 8, 2025

Issue: #AP-6057

What Changed

When the app use a base url, it's usually a path like /some/path. But the DOM snapshot output that we are getting localhost value. This PR remove the localhost value if there is any, if the base url doesn't include localhost, return whatever value as it is.

How to test

  1. Test with chromatic review
    Before fix: https://6595c5968c95bd964ba59382-ukxkpmtgaw.chromatic.com/?path=/story/archiving-assets-assets-from-css-urls-are-archived--snapshot-1&globals=viewport:w1000h660
    After fix: https://6595c5968c95bd964ba59382-wxgdisaoqj.chromatic.com/?path=/story/archiving-assets-assets-from-css-urls-are-archived--snapshot-1&globals=viewport:w1000h660
  2. Test locally:
  • pull the main branch and add a base element <base href="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/" /> to this page test-server/fixtures/asset-paths/css-urls.html
  • run yarn test:cypress, then yarn archive-storybook:cypress to observe this story assets-from-css-urls-are-archived--snapshot. You can notice some images are missing.
  • pull this branch and run the same steps, all the images are loaded.
  • check the html of the story, the base url should be / instead of http://localhost300.

@stephanie56 stephanie56 force-pushed the stephanie/ap-6057-svgs-not-loading-2 branch from 75b4245 to d17472e Compare August 8, 2025 16:37
Copy link

codecov bot commented Aug 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.73%. Comparing base (87c85ab) to head (8bc8d59).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #271      +/-   ##
==========================================
+ Coverage   95.64%   95.73%   +0.09%     
==========================================
  Files          14       14              
  Lines         390      399       +9     
  Branches       67       69       +2     
==========================================
+ Hits          373      382       +9     
  Misses         17       17              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@stephanie56 stephanie56 marked this pull request as ready for review August 8, 2025 17:47
@stephanie56 stephanie56 force-pushed the stephanie/ap-6057-svgs-not-loading-2 branch from 9127448 to 1876eb8 Compare August 11, 2025 14:47
@stephanie56 stephanie56 changed the title Fix base ref include localhost bug Fix base element href include localhost Aug 11, 2025
Copy link
Contributor

@andrewortwein andrewortwein left a comment

Choose a reason for hiding this comment

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

Looks good, but I left a comment that I'd like to resolve before approving.

@stephanie56 stephanie56 force-pushed the stephanie/ap-6057-svgs-not-loading-2 branch from 40d1594 to 599df78 Compare August 11, 2025 15:45
@andrewortwein andrewortwein self-requested a review August 11, 2025 15:47
Copy link
Contributor

@andrewortwein andrewortwein left a comment

Choose a reason for hiding this comment

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

Thanks for adding a separate test for this. Looks great! 👍🏻

@stephanie56 stephanie56 requested a review from tevanoff August 11, 2025 16:57
@stephanie56 stephanie56 merged commit 72c08fe into main Aug 11, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants