fix(redirects): prevent circular redirect loop when a slug rename is reverted#1992
fix(redirects): prevent circular redirect loop when a slug rename is reverted#1992swissky wants to merge 1 commit into
Conversation
…reverted When a slug changes A -> B, an auto-redirect A -> B is created. Reverting the slug back (B -> A) previously created B -> A while chain collapsing turned the original redirect into the self-redirect A -> A, leaving the page at slug A unreachable. createAutoRedirect now deletes any redirect whose source matches the new (now live) URL after collapsing chains, and skips creating a redirect when old and new URL are identical. Fixes emdash-cms#1986
🦋 Changeset detectedLatest commit: deaabe9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
github-actions
Bot
added
area/core
size/M
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for the reported issue (#1986). The approach is sound: prevent the auto-redirect machinery from creating self-redirects when a slug rename is reverted by (1) deleting any redirect whose source is the now-live URL and (2) short-circuiting when the old and new URLs resolve to the same path. Both changes live in the correct place (RedirectRepository.createAutoRedirect), and the regression tests cover the exact failure modes described.
I checked:
- The full diff and the changed repository/test files.
- All callers of
createAutoRedirect: onlypackages/core/src/api/handlers/content.ts, which ignores the return value, so theRedirect | nullsignature change is safe internally and is not exposed via a public package export. - The redirect middleware: it performs a single hop, so any remaining self-redirect would loop; the fix removes the self-redirect before it can be reached.
- SQL safety: the new delete uses Kysely's parameterized builder (
source = ${newUrl}). - The
_emdash_redirectsschema: there is an index onsource, so the additional delete stays cheap. - The changeset: present, correctly names the
emdashpackage, and describes the user-visible fix.
No blocking issues or AGENTS.md violations found. The tests added are exactly the reproducing cases a TDD bug fix should include.
There was a problem hiding this comment.
This PR fixes the circular redirect loop reported in #1986 with a focused, architecture-appropriate change: it skips creating a redirect when the old and new URLs are identical, and after collapsing existing chains it deletes any redirect whose source is the newly-live URL. I read the diff, the redirect repository, the single handler caller (createSlugChangeRedirect), the redirect middleware, and the new tests.
The approach is sound because the redirect middleware runs before Astro content routes, so a redirect from a URL that now serves real content does shadow the page. The chain-collapsing logic and the early-return no-op are exactly the right places to intervene.
I did find one regression risk: the cleanup delete removes any redirect with source = newUrl, including manually created rules. The bug being fixed is caused by auto-redirects (auto = 1), and scoping the delete to auto-redirects is sufficient to break the loop. The current unscoped delete could silently remove user-created redirects that happen to share the restored URL, which is unexpected data loss during a slug change. The included tests only exercise auto-redirects, so they wouldn't catch this.
Otherwise the changeset is well-written, the return-type widening to Redirect | null is handled by the caller, and the regression tests cover the reported scenarios.
| // The new URL serves live content again — any redirect from it would | ||
| // shadow the page. This also removes the self-redirect that chain | ||
| // collapsing produces when a rename A → B is reverted (A → A). | ||
| await this.db.deleteFrom("_emdash_redirects").where("source", "=", newUrl).execute(); |
There was a problem hiding this comment.
[needs fixing] The cleanup delete removes every redirect with source = newUrl, including user-created manual redirects. The #1986 loop is produced by collapsed auto-redirects (auto = 1), so the safe, minimal fix is to scope this delete to auto-redirects only. Deleting manual redirects during an unrelated slug change is silent data loss.
| await this.db.deleteFrom("_emdash_redirects").where("source", "=", newUrl).execute(); | |
| // The new URL serves live content again — any auto-redirect from it would | |
| // shadow the page. This also removes the self-redirect that chain | |
| // collapsing produces when a rename A → B is reverted (A → A). | |
| await this.db | |
| .deleteFrom("_emdash_redirects") | |
| .where("source", "=", newUrl) | |
| .where("auto", "=", 1) | |
| .execute(); |
github-actions
Bot
added
review/approved
1 participant
What does this PR do?
Fixes the circular redirect loop reported in #1986. When a published item's slug is renamed A → B, an auto-redirect
A → Bis created. Reverting the slug back (B → A) createdB → A, while the chain-collapsing step turned the original redirect into the self-redirectA → A— leaving the page at slug A unreachable.createAutoRedirectnow:Rename chains stay collapsed: after
A → B → C → A, both/band/cpoint at/a, and/ahas no redirect.Closes #1986
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Regression tests in
redirect-repository.test.ts(fail onmain, pass with this fix):does not create a loop when a slug rename is reverted (#1986)does not create a loop when reverting after a rename chainis a no-op when old and new slug resolve to the same URL