Repository navigation
Conversation
🦋 Changeset detectedLatest commit: f3b18f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Moving descendant cleanup into MenuRepository.deleteItem is the right fix for the lost parent_id cascade on SQLite and D1, and it makes every dialect behave the same. Both reviewers found the traversal cycle-safe and the deletes child-first and chunked to SQL_BATCH_SIZE. They also found the tests meaningful, and no other delete path needs changes. The reviewers disagreed on the verdict. One approved with no findings. The other raised two issues: cache invalidation after a partial D1 delete, and missing locale guidance in the changeset cleanup steps. Both are kept below. The approving reviewer did not raise them, and the first one contradicts their reading of D1 failure behavior. They noted that a mid-delete failure on D1 leaves the remaining subtree attached, but did not consider the stale cache. The review was static and nothing was run.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
Findings
-
[needs fixing]
packages/core/src/database/repositories/menu.ts:652Cache invalidation is reached only when every delete batch succeeds. On D1,
withTransactionruns without a transaction, so for a subtree larger thanSQL_BATCH_SIZE, an earlier batch can commit before a later batch throws. The handler then returns an error without invalidating the menu object cache, and the route also skips route-cache invalidation. Public navigation can keep showing descendants that were actually deleted until the cache expires.Handle the partial-write error path as well as success, including the route-cache invalidation needed when committed writes precede an error. Add a regression test for a later-batch failure with an already populated menu cache.
The other reviewer did not raise this. They noted only that a partial failure on D1 leaves the remaining subtree still attached to its parent, and that this is the best the approach can do without a transaction.
-
[needs fixing]
.changeset/menu-item-delete-children.md:9The cleanup procedure omits locale selection. When a menu name has multiple translations,
GET /_emdash/api/menus/{name}selects one locale, whilehandleMenuItemDeleterejects the unqualified DELETE withAMBIGUOUS_LOCALE. Readers following these instructions on a multilingual site cannot remove the orphan, and may not find it in the GET response.Document
?locale={locale}on both requests for translated menus, using the affected menu's locale. Also link to the REST authentication instructions so the procedure is usable outside the PR context.The other reviewer did not raise this. They praised the changeset for naming the audience and giving the API calls.
@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-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
upgrade-emdash
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-ai-search
@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: |
There was a problem hiding this comment.
The repository-level descendant deletion remains the right approach, and the follow-up correctly moves object-cache invalidation into finally. The changeset now addresses the earlier locale-selection problem.
One part of the previous cache finding remains open: a partial D1 deletion still skips route-cache invalidation, so cached public pages can continue displaying deleted descendants. I checked the follow-up diff, transaction behavior, handler error conversion, and both cache invalidation paths. No additional issues found in the changed code. Review was static; no checks were run.
Follow-up review by openai/gpt-6.1-sol, scoped to the changes since the last review.
emdashbot
Bot
added
review/needs-review
There was a problem hiding this comment.
The single-statement recursive deletion is the right approach: it removes the partial-commit state behind the earlier cache finding rather than trying to repair caches afterward.
I reviewed the follow-up SQL, menu scoping, cycle termination, cache invalidation paths, and the new D1 failure regression test. The author’s response addresses the previous finding: a failed statement leaves the subtree unchanged, while successful deletion follows the existing cache invalidation paths. No new issues found in the changed code.
Review was static; no tests, builds, or lint checks were run.
Follow-up review by openai/gpt-6.1-sol, scoped to the changes since the last review.
…Lite and D1 Migration 036 rebuilt `_emdash_menu_items` without the `parent_id` ON DELETE CASCADE from migration 005, and `deleteItem` removed only the one row. Deleting a parent item on SQLite and D1 therefore left its children in the database: the admin menu editor no longer reaches them, while the public menu shows them at the top level. PostgreSQL kept the cascade and was not affected. `deleteItem` now collects the item and everything nested under it and deletes them, deepest first, in a transaction where the database supports one (D1 does not). Child rows left behind by earlier deletes are not touched.
On D1 the delete batches run without a transaction, so batches before a failed one stay committed. The menu object cache is now cleared once a delete has started, whether it finishes or throws. The changeset's cleanup steps now add ?locale= for translated menus: without it GET returns one translation and DELETE fails with AMBIGUOUS_LOCALE.
On D1 the delete batches ran without a transaction, so a failure partway left some children deleted and the caches not refreshed. One DELETE with a recursive query for the subtree removes the whole branch or nothing on every dialect, and needs no read first.
7ad8d06 to
f3b18f3
Compare
There was a problem hiding this comment.
The single-statement recursive deletion remains the right approach: it fixes descendant deletion on SQLite/D1 and avoids partial commits rather than repairing caches afterward.
I checked the PR-relevant follow-up changes, menu scoping, cycle termination, handler and cache-invalidation paths, regression tests, and the changeset’s cleanup instructions. The earlier cache finding is addressed by atomic deletion; no findings remain, and I found no new defects in the reviewed changes.
Review was static; no tests, builds, or lint checks were run.
Follow-up review by openai/gpt-6.1-sol, scoped to the changes since the last review.
What does this PR do?
Deleting a menu item on SQLite and D1 now deletes the items nested under it too. Before, the children stayed in the database: the admin menu editor no longer showed them, but the public menu still listed them at the top level.
Migration 036 rebuilt the menu items table without the cascade on
parent_idthat migration 005 had created, andMenuRepository.deleteItemdeleted only the one row. PostgreSQL kept the cascade, so it was not affected.deleteItemnow deletes the item and everything below it in one statement, with a recursive query that finds the branch. Because it is one statement, it removes either the whole branch or nothing, on D1 too, which has no transactions. Items that earlier deletes already left behind are not touched.I found this while working on #4285. Before opening it, I confirmed the regression tests fail without the fix and pass with it.
Type of change
Your connection to this 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. (n/a: no admin UI change)AI assistance disclosure
Screenshots / test output
Not applicable.
pnpm build,pnpm typecheckandpnpm lintpass. The new tests inmenu-item-parent-validation.test.tsfail on SQLite without the change and pass with it, and also pass on PostgreSQL.tests/workerd/menu-item-delete-d1.test.tsfails on D1 without the change and passes with it. Its second test makes the delete fail partway and checks that nothing was removed; it fails on the earlier batched version of this PR, which had already deleted 50 of 60 children.