Fix taxonomy links missing from admin sidebar#93
Conversation
🦋 Changeset detectedLatest commit: 90fc110 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
@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: |
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes missing runtime-created taxonomy links in the admin sidebar by sourcing taxonomy definitions dynamically and exposing them via the manifest.
Changes:
- Load taxonomy definitions from the DB at runtime and include them in the manifest (and hash).
- Extend manifest types in core and admin to include taxonomy definitions.
- Render admin sidebar taxonomy links from the manifest instead of hardcoding Categories/Tags.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/emdash-runtime.ts | Loads taxonomy definitions from DB, includes them in manifest + hash. |
| packages/core/src/astro/types.ts | Adds taxonomies to the core EmDashManifest type. |
| packages/core/src/astro/routes/api/manifest.ts | Ensures the “default” manifest shape includes taxonomies: []. |
| packages/admin/src/lib/api/client.ts | Extends admin-side manifest typing to include taxonomies. |
| packages/admin/src/components/Sidebar.tsx | Builds taxonomy sidebar links from manifest-provided taxonomies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const rows = await this.db.selectFrom("_emdash_taxonomy_defs").selectAll().execute(); | ||
| manifestTaxonomies = rows.map((row) => ({ | ||
| name: row.name, | ||
| label: row.label, | ||
| labelSingular: row.label_singular ?? undefined, | ||
| hierarchical: row.hierarchical === 1, | ||
| collections: row.collections ? JSON.parse(row.collections) : [], |
There was a problem hiding this comment.
The manifest hash can become non-deterministic because selectAll().execute() has no explicit ordering, so DB row order may vary between runs (and JSON.stringify(manifestTaxonomies) will then vary). This can cause unnecessary cache invalidations. Make the taxonomy query ordering stable (e.g., orderBy('name')) and/or sort manifestTaxonomies before hashing (and consider sorting nested collections too if ordering isn’t guaranteed).
| const rows = await this.db.selectFrom("_emdash_taxonomy_defs").selectAll().execute(); | |
| manifestTaxonomies = rows.map((row) => ({ | |
| name: row.name, | |
| label: row.label, | |
| labelSingular: row.label_singular ?? undefined, | |
| hierarchical: row.hierarchical === 1, | |
| collections: row.collections ? JSON.parse(row.collections) : [], | |
| const rows = await this.db | |
| .selectFrom("_emdash_taxonomy_defs") | |
| .selectAll() | |
| .orderBy("name") | |
| .execute(); | |
| manifestTaxonomies = rows.map((row) => ({ | |
| name: row.name, | |
| label: row.label, | |
| labelSingular: row.label_singular ?? undefined, | |
| hierarchical: row.hierarchical === 1, | |
| collections: row.collections | |
| ? (JSON.parse(row.collections) as string[]).sort((a, b) => a.localeCompare(b)) | |
| : [], |
| JSON.stringify(manifestCollections) + | ||
| JSON.stringify(manifestPlugins) + | ||
| JSON.stringify(manifestTaxonomies), |
There was a problem hiding this comment.
The manifest hash can become non-deterministic because selectAll().execute() has no explicit ordering, so DB row order may vary between runs (and JSON.stringify(manifestTaxonomies) will then vary). This can cause unnecessary cache invalidations. Make the taxonomy query ordering stable (e.g., orderBy('name')) and/or sort manifestTaxonomies before hashing (and consider sorting nested collections too if ordering isn’t guaranteed).
| manifestTaxonomies = rows.map((row) => ({ | ||
| name: row.name, | ||
| label: row.label, | ||
| labelSingular: row.label_singular ?? undefined, | ||
| hierarchical: row.hierarchical === 1, | ||
| collections: row.collections ? JSON.parse(row.collections) : [], | ||
| })); |
There was a problem hiding this comment.
JSON.parse(row.collections) can throw for a single malformed row, which will currently drop all taxonomies due to the outer try/catch. Consider parsing per-row (catching/handling errors locally and defaulting that row’s collections to []), so one bad entry doesn’t remove the entire taxonomy list from the manifest.
| manifestTaxonomies = rows.map((row) => ({ | |
| name: row.name, | |
| label: row.label, | |
| labelSingular: row.label_singular ?? undefined, | |
| hierarchical: row.hierarchical === 1, | |
| collections: row.collections ? JSON.parse(row.collections) : [], | |
| })); | |
| manifestTaxonomies = rows.map((row) => { | |
| let collections: string[] = []; | |
| if (row.collections) { | |
| try { | |
| collections = JSON.parse(row.collections); | |
| } catch (error) { | |
| console.debug( | |
| `EmDash: Could not parse collections for taxonomy definition "${row.name}":`, | |
| error, | |
| ); | |
| } | |
| } | |
| return { | |
| name: row.name, | |
| label: row.label, | |
| labelSingular: row.label_singular ?? undefined, | |
| hierarchical: row.hierarchical === 1, | |
| collections, | |
| }; | |
| }); |
| taxonomies?: Array<{ | ||
| name: string; | ||
| label: string; | ||
| labelSingular?: string; | ||
| hierarchical: boolean; | ||
| collections: string[]; | ||
| }>; |
There was a problem hiding this comment.
In core, EmDashManifest.taxonomies is required, and the API default manifest also always returns taxonomies: []. On the admin side it’s typed as optional (taxonomies?), which makes consumers handle an unnecessary undefined case and can hide contract mismatches. Consider making taxonomies required in AdminManifest (and in SidebarNavProps, where it’s also optional) and relying on the backend to provide an empty array when none exist.
|
@eyupcanakman this would be great to get in. Could you address the revew comments? |
- Order taxonomy query by name for deterministic manifest hash - Make taxonomies required in AdminManifest and SidebarNavProps - Add changeset for emdash and @emdash-cms/admin
- Add tests for seeded and custom taxonomy loading from DB - Add taxonomies field to AdminManifest test fixtures
e34420c to
bb3d72d
Compare
|
Addressed the review comments. Added orderBy, sorted collections for stable hashing, and made taxonomies required. Skipped the per-row try/catch since our own code writes that column. |
* Fix taxonomy links missing from admin sidebar * Address review feedback and add changeset - Order taxonomy query by name for deterministic manifest hash - Make taxonomies required in AdminManifest and SidebarNavProps - Add changeset for emdash and @emdash-cms/admin * Add taxonomy manifest tests and fix test fixtures - Add tests for seeded and custom taxonomy loading from DB - Add taxonomies field to AdminManifest test fixtures * Sort taxonomy collections for deterministic manifest hash * style: format --------- Co-authored-by: emdashbot[bot] <emdashbot[bot]@users.noreply.github.com>
What does this PR do?
Closes #68
The admin sidebar hardcoded only "Categories" and "Tags" links. Any taxonomy created at runtime never showed up, so there was no way to manage its terms from the UI.
The fix loads taxonomy definitions from the database into the manifest and renders sidebar links from that list instead of a static array.
Type of change
Checklist
pnpm typecheckpassespnpm --silent lint:json | jq '.diagnostics | length'returns 0pnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
N/A