fix: dashboard 500 on D1 with many title-bearing collections (#895)#896
fix: dashboard 500 on D1 with many title-bearing collections (#895)#896cristianuibar wants to merge 1 commit intoemdash-cms:mainfrom
Conversation
Replace the chained UNION ALL composition in fetchRecentItems with N per-collection queries fired in parallel and merged in JS. The previous single-statement composition trips D1's SQLITE_LIMIT_COMPOUND_SELECT cap once enough title-bearing collections are present, returning a 500 on every dashboard load. better-sqlite3's higher cap masked the bug locally. Adds a regression test covering the merged top-10 contract across 12 title-bearing collections. Closes emdash-cms#895
🦋 Changeset detectedLatest commit: 05717b8 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 |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
@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: |
|
/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, minimal, and well-scoped. Replacing the chained UNION ALL with N parallel per-collection queries side-steps D1's SQLITE_LIMIT_COMPOUND_SELECT cap without changing semantics: each arm preserves the same titleExpr, the same sql.lit interpolation for collection/collection_label, and the final merge/sort/slice yields equivalent output. The round-trip count is now O(N), but the dashboard already does an O(N) parallel fan-out for getStats immediately above, so this isn't a new shape.
Non-blocking nits
Stale doc comments still reference UNION ALL — both should be updated since the implementation no longer uses it:
packages/core/src/api/handlers/dashboard.ts:105— theRecentItemRowinterface comment says "Raw row shape from the UNION ALL query". Suggest:/** Raw row shape from per-collection queries — all snake_case. */packages/core/src/api/handlers/dashboard.ts:117-126— thefetchRecentItemsJSDoc says "Uses UNION ALL over each ec_* table". Suggest something like:/** * Fetch the 10 most recently updated items across all collections. * * Issues one query per `ec_*` table in parallel and merges in JS. * A single `UNION ALL` across many collections trips D1's * `SQLITE_LIMIT_COMPOUND_SELECT` cap (#895). The query is safe because * collection slugs come from the system table and are validated. * * `title` is not a standard column — it's a user-defined field. We query * `_emdash_fields` to discover which collections have one and fall back * to `slug` (which is always present) otherwise. */
Test caveat is accurate — the new regression test admits it can't reproduce the D1 compound-SELECT cap under better-sqlite3. It exercises the merge/sort/slice contract, which is fine as a behavioral guard against a future revert; calling that out explicitly in the test comment is the right move.
Test timing is tight but should be fine — the 5ms setTimeout between creates relies on updated_at ms-resolution from new Date().toISOString(). Matches the pattern at line 76–77 in the same file.
Other things checked
validateIdentifier(col.slug)still gates theec_${col.slug}table name interpolation. ✓sql.lit(col.label)is preserved as-is from main, so any pre-existing concern there is not a regression introduced by this PR. ✓toSortedis supported (ES2023 target, used elsewhere in the codebase). ✓- No schema changes, no migrations, no API shape changes. ✓
- Changeset present (
emdash: patch). ✓
LGTM modulo the stale comments.
|
Review posted successfully. Summary of findings:
|
2 participants
What does this PR do?
Fixes the
GET /_emdash/api/dashboard500 error on Cloudflare D1 (and the miniflare D1 emulator) when a project has enough title-bearing collections. The admin dashboard fails to render for every authenticated user once the threshold is crossed (reproduced at 9 arms in the issue).Root cause:
fetchRecentItemsinpackages/core/src/api/handlers/dashboard.tscomposes a single chainedUNION ALLacross every title-bearing collection and wraps it in an outerSELECT * FROM (combined) ORDER BY updated_at DESC LIMIT 10. D1 enforces aSQLITE_LIMIT_COMPOUND_SELECTcap that is tighter than upstream SQLite's default of 500, so the query fails on D1 even though the same SQL runs fine againstbetter-sqlite3. This is a 0.7.0 → 0.8.0 regression and is unchanged in 0.9.0.Fix: Replace the chained
UNION ALLwith N per-collection queries fired in parallel viaPromise.all, then merge / sort / slice in JS. Equivalent semantics, no schema changes, no D1-specific branching, and at mostN * 10rows materialized before the finalslice(0, 10). Per-armsql.lit(label)interpolation is preserved (kept on the outer side of each per-collection query, identical to the previous arm shape).This patch was developed and validated in production at https://github.com/cristianuibar/wishday-web (12 collections, 9 title-bearing) by running
pnpm patch emdash@0.8.0against the affected file. The dashboard has rendered cleanly under that patch through staging deploys; this PR upstreams the same change.Closes #895
Type of change
Checklist
pnpm typecheckpassespnpm lintpasses (no new diagnostics introduced; pre-existing main-branch warnings unchanged)pnpm testpasses (or targeted tests for my change) —tests/unit/api/dashboard-handlers.test.ts11/11 pass; the 8 pre-existing failures invite-config.test.ts,wordpress-slug-sanitization.test.ts, andwp-prepare-invalidate.test.tsreproduce onorigin/mainand are unrelated to this changepnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. N/A — no admin UI strings touched..changeset/fix-dashboard-d1-compound-select.md,emdash: patchTest notes
The added regression test (
merges recent items across many title-bearing collections (#895)) creates 12 collections each with atitlefield and one entry, then asserts the merged top-10 contract: count = 10, ordered byupdated_atdesc, drawn across all collections.Caveat — the original D1
SQLITE_LIMIT_COMPOUND_SELECTcap cannot be reproduced underbetter-sqlite3(its compound-SELECT limit is upstream SQLite's default of 500), so this test will not fail against the previousUNION ALLcode path. It exists as a behavioral regression guard for the per-collection merge contract introduced by this fix and would catch a future revert. The actual 500 was reproduced and fixed against live Cloudflare D1 in https://github.com/cristianuibar/wishday-web viapnpm patch; the patch shipped here is equivalent.AI-generated code disclosure
Screenshots / test output