fix(redirects): fire for unauthenticated visitors#817
fix(redirects): fire for unauthenticated visitors#817ascorbic merged 8 commits intoemdash-cms:mainfrom
Conversation
The redirect middleware bailed when `locals.emdash.db` was missing, which is the intentional state for public visitors -- so 301/302 rules from `_emdash_redirects` only fired for logged-in admins, edit-mode sessions and preview tokens. WordPress migration redirects, manual rewrites and the `Auto: slug change` rows did nothing for real traffic, and `hits` / `_emdash_404_log` stayed at zero. Falls back to `getDb()` (ALS-aware -- returns the per-request scoped session when one is active, the singleton otherwise) when `locals.emdash.db` is absent. Same accessor the loader and template helpers use, so the public render boundary stays minimal. Adds a regression test covering exact and pattern matches on the public-visitor branch, plus the existing authenticated branch and the "no db configured" fallback.
🦋 Changeset detectedLatest commit: 8c45d47 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@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-forms
@emdash-cms/plugin-webhook-notifier
commit: |
github-actions
Bot
added
size/L
query-count changed
Query-count snapshot changes7 routes changed, total Δ +7 queries. D1
Comparing snapshot files between base and head. Updated automatically on each push. |
|
Thanks @all3f0r1, the correctness fix is right. Snapshot shows +1 query on every public request. That's because Before merging, can we extend the cache to cover exact matches too? Roughly:
Cold isolate: 1 query loads both. Warm: 0 queries. Empty-redirect sites: empty Map, no probe needed. Happy to take a follow-up look once it's pushed. |
One query loads both kinds at cold-start; warm requests issue zero queries. Exact rules indexed by source path in a Map (O(1) lookup), pattern rules pre-compiled into an array. Empty-redirect sites cache an empty Map + array, eliminating the per-request probe. Addresses ascorbic's review on emdash-cms#817: snapshot was showing +1 query on every public request because findExactMatch ran uncached while pattern lookups already went through the module-level cache. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Alright, I applied your request the cleanest way I could come up with. LMK if it's up to your standard. |
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Fix is correct and well-motivated. Public branch has anonScoped.db in ALS via runWithContext (middleware.ts:357), so getDb() resolves to the per-request scoped Kysely on the public render path -- exactly the same accessor the loader and template helpers use. Invalidation set is complete (api/redirects/index.ts, [id].ts, content.ts:650 for slug-change auto-redirects, seed/apply.ts:683). Cache redesign in 572ba2e correctly addresses the +1/route warm-path cost the workflow flagged.
A few low-severity notes inline -- none blocking. Mostly observations on the cold-isolate trade-off and the pre-existing module-scope singleton pattern.
One extra note (couldn't anchor to the relevant line because it falls outside the diff hunks): the outer try { ... } catch { return next(); } at redirect.ts:113-116 swallows every error from findAllEnabled / recordHit / log404 / next() and silently bypasses redirects. The comment justifies this as "redirects table doesn't exist yet (pre-migration)" but in practice it also masks transient DB errors and bugs -- and post-cache-redesign, one bad row that throws inside setCachedRedirects (e.g. a compilePattern regression on a malformed pattern source) will cause every public request to silently bypass redirects until the isolate restarts, since the cache was never populated. Not a regression introduced by this PR -- the old code had the same silent catch -- but the blast radius grew because the cache now batches all rules. Consider narrowing to isMissingTableError(error) (used elsewhere in the codebase per AGENTS.md) and at least console.error("[emdash] redirect middleware error:", error) for the rest. Follow-up rather than blocking.
| * null = not yet populated, object = cached. | ||
| */ | ||
| let cachedPatternRules: CachedRedirectRule[] | null = null; | ||
| let cachedRedirects: CachedRedirects | null = null; |
There was a problem hiding this comment.
Pre-existing pattern, not introduced here, but worth flagging for a follow-up. AGENTS.md ("Performance: caching and query patterns") explicitly calls out that module-scope singletons like let cachedRedirects: ... | null = null are vulnerable to Vite SSR chunk duplication: two chunks inlining this module each get their own cachedRedirects binding, so one chunk's writes (or invalidateRedirectCache() calls) won't be visible to readers in the other chunk. The result is stale-cache-after-write or duplicated cold-start queries.
The canonical fix is the globalThis + Symbol.for pattern from settings/index.ts (SITE_SETTINGS_CACHE_KEY/holder) and request-context.ts. This PR rolled with the existing pattern, which is fine -- but since the cache now holds more state and needs to be coherent across writes (especially the auto-redirect on slug change, which is a hot write path), it's a reasonable next perf/correctness pass.
No change requested in this PR; just noting for the maintainer.
| // Map + array, so the next request returns immediately without probing. | ||
| let cached = getCachedRedirects(); | ||
| if (!cached) { | ||
| const all = await repo.findAllEnabled(); |
There was a problem hiding this comment.
Cold-isolate cost note (no change requested, just confirming the trade-off is intentional):
Before this PR, the cold path was one indexed point lookup on source (findExactMatch, with the existing source index) plus -- on miss -- one scan of pattern rules. After: every cold isolate runs findAllEnabled(), a full scan of all enabled rows, even for paths that would have missed both. Sites with large WordPress-migration redirect sets (10k+ rows) will pay materially more on cold start than they did before, in exchange for zero-query warm requests.
The maintainer requested this shape explicitly in review (one query, both caches), and the warm-path zero-query case dominates in steady state -- so this is the right call. Worth keeping in mind if anyone files a cold-start regression on a large-scale migration site; mitigation would be a paginated load or a count-then-decide-strategy fallback for sites above some threshold.
| }); | ||
|
|
||
| it("skips silently when no database is available at all", async () => { | ||
| getDbMock.mockRejectedValueOnce(new Error("EmDash database not configured")); |
There was a problem hiding this comment.
Worth adding a test for findAllEnabled() itself rejecting (transient DB error) -- right now the suite only covers getDb() rejecting at acquisition time. The current outer-catch behavior would swallow that and call next(), but there's no test pinning that. Same shape as this test, just throw from inside findAllEnabled via vi.spyOn(RedirectRepository.prototype, "findAllEnabled").mockRejectedValueOnce(...). Optional.
| @@ -0,0 +1,179 @@ | |||
| /** | |||
There was a problem hiding this comment.
Nit: file lives at tests/unit/astro/middleware-redirect.test.ts but the source is at src/astro/middleware/redirect.ts. AGENTS.md says "Test files mirror source structure," which would suggest tests/unit/astro/middleware/redirect.test.ts. Existing files in the repo are inconsistent on this so not strictly required, but worth aligning if you're already touching it. No action needed if you'd rather not.
|
Review posted successfully at #817 (review). Summary: Posted a single COMMENT review with 4 line-anchored comments and 1 inline note in the summary body for a comment that couldn't anchor (line 113 falls outside the diff hunks). Verdict is the fix is correct and ready, with low-severity follow-up observations on the module-scope singleton pattern, cold-isolate query cost trade-off, silent catch blast radius, and a missing test for repo-level rejections. |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Thanks! I have a feeling we'll end up needing to optimise this at some point, but it's good for now
2 participants
What does this PR do?
Fixes the redirect middleware so 301/302 rules from
_emdash_redirectsactually fire for unauthenticated visitors.The runtime middleware sets up two different shapes of
locals.emdashdepending on the caller — the authenticated/edit-mode/preview branch exposes the full handlers +db, while the public-visitor fast path intentionally exposes onlycollectPageMetadata/collectPageFragments(defense-in-depth, so page renders can't reach into admin-capable handlers). The redirect middleware was readingdbfromlocals.emdashonly, so on the public branch it bailed out atif (!emdash?.db) return next()— meaning WordPress migration 301s, manual prefix rewrites and theAuto: slug changerows EmDash inserts on slug edits did nothing for real site traffic.hitsand_emdash_404_logalso stayed at zero, so the bug was invisible from the admin UI.The fix keeps the public-render boundary intact and routes the lookup through
getDb()from the loader, which already does the right thing: it returns the per-request scoped Kysely from ALS (theanonScoped.dbthe runtime middleware installs for D1 read-replica sessions) when present, and falls back to the singleton instance otherwise. Same accessor the template helpers use.Closes #808
Type of change
Checklist
pnpm typecheckpasses (core package — pre-existing cloudflare typecheck failures are unrelated to this change and reproduce onmain)pnpm lintpasses (no new diagnostics introduced — diagnostic count unchanged at 25 vs. baseline)pnpm testpasses for redirect/middleware tests (5 new + 45 redirect-repo + 7 redirect-handler + middleware-prerender all green; 3 pre-existing auth test failures onmainare unrelated)pnpm formathas been runpnpm locale:extracthas been run (n/a — server middleware, no UI strings)AI-generated code disclosure
Screenshots / test output
New regression test covers the public-visitor branch and verifies the fallback chain:
Reproducer from the issue (curl
/oldwith no cookies) now returns301withLocation: /newand_emdash_redirects.hitsincrements — same result as the edit-mode case.