Skip to content

fix(core): Update with-routes to account for trailing slash or capitalizations when redirecting#2670

Merged
jordanarldt merged 1 commit intocanaryfrom
fix-redirects-issue
Nov 6, 2025
Merged

fix(core): Update with-routes to account for trailing slash or capitalizations when redirecting#2670
jordanarldt merged 1 commit intocanaryfrom
fix-redirects-issue

Conversation

@jordanarldt
Copy link
Copy Markdown
Contributor

@jordanarldt jordanarldt commented Nov 5, 2025

What/Why?

Update the with-routes middleware to account for the TRAILING_SLASH env var, as well as capitalized letters in 301 Redirects.

There was a reported issue where when using TRAILING_SLASH=false in the Catalyst environment variables, redirect loops could occur if you had a redirect going from /example-path -> /example-path/, or capitalizations like /some-url -> /some-URL

Testing

Tested locally, confirmed fix works.

Regression test results before changes:

image

Regression test results after changes:

image

Before fix

Dynamic 301 Redirect from /infinite-redirect-test to a product with the URL /infinite-redirect-test/ while TRAILING_SLASH=false

Screen.Recording.2025-11-05.at.4.54.51.PM.mov

Manual 301 Redirect from /redirect-test to /redirect-TEST

Screen.Recording.2025-11-05.at.4.55.46.PM.mov

After fix

Dynamic 301 Redirect from /infinite-redirect-test to a product with the URL /infinite-redirect-test/ while TRAILING_SLASH=false

Screen.Recording.2025-11-05.at.4.59.30.PM.mov

Manual 301 Redirect from /redirect-test to /redirect-TEST

Screen.Recording.2025-11-05.at.5.00.23.PM.mov

Migration

Copy changes from with-routes.ts using this PR as a reference

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Nov 5, 2025

🦋 Changeset detected

Latest commit: caa45fc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@bigcommerce/catalyst-core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented Nov 5, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
catalyst-b2b Ready Ready Preview Comment Nov 6, 2025 9:13pm
catalyst-canary Ready Ready Preview Comment Nov 6, 2025 9:13pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
catalyst Ignored Ignored Nov 6, 2025 9:13pm

@jordanarldt jordanarldt marked this pull request as ready for review November 5, 2025 23:01
@jordanarldt jordanarldt requested a review from a team November 5, 2025 23:01
Copy link
Copy Markdown
Contributor

@migueloller migueloller left a comment

Choose a reason for hiding this comment

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

Can we please get some regression tests for this fix @jordanarldt?

@jordanarldt
Copy link
Copy Markdown
Contributor Author

Can we please get some regression tests for this fix @jordanarldt?

@migueloller For sure. I'll get that added in the morning. We don't currently have any test fixtures for creating 301 redirects, so I'll need to create some test fixtures / api clients, and then add a new functional test suite. It may use up some of my capacity but we definitely should have some tests for this.

Comment thread core/middlewares/with-routes.ts
Copy link
Copy Markdown
Contributor

Do we not have the basics set up for spinning up Catalyst via Playwright and navigating to some URLs and then checking the response status code? If not, then that's definitely worth investing into!

Copy link
Copy Markdown
Contributor

Based on this existing test, it seems like we're already set up to test something like this.

https://github.com/bigcommerce/catalyst/blob/canary/core/tests/ui/e2e/not-found.spec.ts

Copy link
Copy Markdown
Contributor

@chanceaclark chanceaclark left a comment

Choose a reason for hiding this comment

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

Pretty much LMAO I have no idea what I was going to write here 😅

Comment thread core/middlewares/with-routes.ts Outdated
Comment thread core/middlewares/with-routes.ts
@jordanarldt
Copy link
Copy Markdown
Contributor Author

Based on this existing test, it seems like we're already set up to test something like this.

https://github.com/bigcommerce/catalyst/blob/canary/core/tests/ui/e2e/not-found.spec.ts

@migueloller The issue only replicates when you've created some specific 301 Redirects. In order to write a test for this, we need to be able to create the dynamic 301 redirects to a product via the test suites, which will require a fixture 👍

Copy link
Copy Markdown
Contributor

Ah I see. If we don't want to have to create the specific redirects for the BigCommerce API we could use Playwright's Mock APIs to mock the GraphQL request to include the relevant redirects?

Copy link
Copy Markdown
Contributor

I think a fixture would be ideal, by the way. But I wanted to provide an option in case that was prohibitively complicated.

Comment thread core/tests/fixtures/utils/api/redirects/index.ts
Copy link
Copy Markdown
Contributor

@migueloller migueloller left a comment

Choose a reason for hiding this comment

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

The tests are looking great! Thanks @jordanarldt 🙏🏻

@jordanarldt jordanarldt added this pull request to the merge queue Nov 6, 2025
Merged via the queue into canary with commit d5fbb73 Nov 6, 2025
11 checks passed
@jordanarldt jordanarldt deleted the fix-redirects-issue branch November 6, 2025 22:20
jamesqquick pushed a commit that referenced this pull request Feb 11, 2026
chanceaclark pushed a commit that referenced this pull request Apr 27, 2026
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