fix(core): RecentPosts widget links by slug and supports urlTemplate (#1332) - #1899
fix(core): RecentPosts widget links by slug and supports urlTemplate (#1332)#1899swissky wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: f7c2224 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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/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/registry-moderation
@emdash-cms/registry-verification
@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: |
There was a problem hiding this comment.
This PR takes the right approach to #1332: it fixes the core:recent-posts widget's hardcoded /posts/${id} link by routing it through the already-tested buildLiveSearchResultUrl helper, and it reuses the same :slug / :id / :path / :collection token vocabulary that LiveSearch's routeMap already uses. That's a clean, minimal change that matches the sibling component.
I read the widget code, the helper implementation and its unit tests, the widget registry (packages/core/src/widgets/components.ts), the admin widget form (packages/admin/src/components/Widgets.tsx), and the updated docs/changeset.
Headline finding: the new optional urlTemplate prop is declared on the Astro component but not added to the coreWidgetComponents registry in packages/core/src/widgets/components.ts. The admin UI renders prop fields from that registry (ComponentPropField iterates selectedComponent.props), so the prop is currently invisible to admin users. You also need to update the should return core widget components test in packages/core/tests/unit/widgets/widgets.test.ts to assert the prop exists.
Otherwise the change is solid: the helper coverage is already in place, the default /posts/{slug} behavior matches LiveSearch, and the changeset is appropriate.
| showThumbnails?: boolean; | ||
| showDate?: boolean; |
There was a problem hiding this comment.
[needs fixing] The new urlTemplate prop is declared on the component, but it is not registered in packages/core/src/widgets/components.ts under the core:recent-posts coreWidgetComponents entry. The admin UI builds the prop form from that registry (packages/admin/src/components/Widgets.tsx iterates selectedComponent.props), so this prop is currently invisible to admin users.
Add the prop definition there, e.g.:
urlTemplate: {
type: "string",
label: "URL template",
},Also update the should return core widget components test in packages/core/tests/unit/widgets/widgets.test.ts to assert that recentPosts?.props has the new urlTemplate property.
There was a problem hiding this comment.
Fixed in 4a35b0d — urlTemplate is now registered in coreWidgetComponents (type string, label "URL template (e.g. /blog/:slug)") so the admin prop form renders it, and the should return core widget components test asserts the new prop.
There was a problem hiding this comment.
This re-review confirms the prior finding is resolved: urlTemplate is now registered in coreWidgetComponents and the registry test asserts it. The implementation otherwise routes the widget through buildLiveSearchResultUrl, matching the LiveSearch pattern.
However, the change rests on a mistaken premise. The PR description and changeset claim the old /posts/${post.id} link used the ULID and therefore 404ed. That is not what the loader returns: in packages/core/src/loader.ts, entry.id is the slug (or locale/slug when i18n prefixing is active), and entry.data.id is the ULID. So the old default link already used the slug; the real new capability here is the optional urlTemplate prop.
More importantly, the implementation does not preserve the shared template vocabulary it advertises. LiveSearch passes the content ULID as the helper's id token (the search API returns row.id), but RecentPosts.astro passes post.id — which is the slug — as id. That makes a urlTemplate like /posts/:id resolve to the slug in RecentPosts and the ULID in LiveSearch, breaking the promise of one token vocabulary.
The change also silently alters default behavior for i18n sites: when prefixing is active, post.id was locale/slug, so the old link was /posts/locale/slug; the helper now emits /posts/slug by default. Sites relying on the embedded locale prefix will need to set a urlTemplate, but there is no :locale token to help them do so generically.
The changeset should be rewritten to describe the actual, observable change (adds urlTemplate) rather than a bug that does not exist in the claimed form. I recommend fixing the id argument to use the ULID so the token semantics align with LiveSearch, and noting the i18n default-shift in the changeset/docs.
| const featuredImage = getString(post.data, "featured_image"); | ||
| const title = getString(post.data, "title"); | ||
| const href = buildLiveSearchResultUrl( | ||
| { collection: "posts", id: post.id, slug: getString(post.data, "slug") }, |
There was a problem hiding this comment.
[needs fixing] This passes post.id as the helper's id token, but post.id is the slug (the loader sets entry.id to slug, or locale/slug with i18n prefixing). LiveSearch passes the content ULID as id (its search API returns row.id), so a urlTemplate like /posts/:id would resolve to the slug here and the ULID there. That contradicts the PR's goal of a shared token vocabulary.
Use the ULID from post.data.id (which the loader maps into data.id) so the :id token means the same thing in both components:
| { collection: "posts", id: post.id, slug: getString(post.data, "slug") }, | |
| const href = buildLiveSearchResultUrl( | |
| { collection: "posts", id: getString(post.data, "id") ?? post.id, slug: getString(post.data, "slug") }, | |
| urlTemplate ? { posts: urlTemplate } : {}, | |
| ); |
There was a problem hiding this comment.
Fixed in 85bb8e6 — when a urlTemplate is set, :id now resolves to the content ULID (post.data.id, which the loader maps from the row's id column) and :slug to the bare slug, matching LiveSearch's token semantics exactly.
| const publishedAt = getString(post.data, "publishedAt"); | ||
| const featuredImage = getString(post.data, "featured_image"); | ||
| const title = getString(post.data, "title"); | ||
| const href = buildLiveSearchResultUrl( |
There was a problem hiding this comment.
[suggestion] Switching the default path from post.id to post.data.slug drops the locale prefix that post.id carries when i18n prefixing is enabled (loader.ts builds id as locale/slug in that case). For sites relying on the old /posts/locale/slug shape, the new default will change link behavior unless they set urlTemplate. There is also no :locale token available to reconstruct the prefix in a template.
Consider documenting this behavior change in the widget guide, or preserving the locale-aware default when no template is provided.
There was a problem hiding this comment.
Fixed in 85bb8e6 by not changing the default at all: without a template the widget keeps its long-standing /posts/${post.id} link, so the locale/slug prefix from i18n prefixing is preserved. buildLiveSearchResultUrl is only used when a urlTemplate is provided, making the PR purely additive.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "emdash": patch | |||
| --- | |||
There was a problem hiding this comment.
[needs fixing] The changeset claims the old link was /posts/{id} where {id} is the ULID, but the loader exposes the slug as entry.id. The old default already linked by slug for non-i18n sites (and by locale/slug when i18n prefixing is active). The actual user-facing change is adding the optional urlTemplate prop and routing the link through buildLiveSearchResultUrl.
Please reword the changeset to describe what actually changed, e.g.:
| --- | |
| --- | |
| "emdash": patch | |
| --- | |
| Adds an optional `urlTemplate` prop to the `core:recent-posts` widget (e.g. `"/blog/:slug"`) using the same `:collection`, `:id`, `:slug`, and `:path` tokens as LiveSearch's `routeMap`. The default post link is now built through `buildLiveSearchResultUrl` and uses the post slug. |
There was a problem hiding this comment.
You're right — the loader exposes the slug (or locale/slug) as entry.id, so the old default was already slug-based and there was no 404. Reworded the changeset in 85bb8e6 to describe the actual change: an optional urlTemplate prop with LiveSearch's token vocabulary, default link unchanged. PR body updated too.
| urlTemplate: { | ||
| type: "string", |
There was a problem hiding this comment.
[suggestion] The urlTemplate prop label is a user-facing admin UI string ("URL template (e.g. /blog/:slug)"), but it is not routed through Lingui. This follows the existing pattern for other widget prop labels, but that pattern violates AGENTS.md's requirement that all admin UI strings be localizable. If you localize the registry later, this label will need to be included.
There was a problem hiding this comment.
Acknowledged — this follows the existing pattern for widget prop labels in coreWidgetComponents (none are routed through Lingui today, since the registry lives server-side in core, not in the admin bundle). Localizing the registry is a systemic change across all core widgets, so I'd rather keep it out of this PR; happy to pick it up separately if the maintainers want it.
There was a problem hiding this comment.
Right — this is pre-existing debt across the whole coreWidgetComponents registry (every widget label is a plain string, not just the new one), and your own note flags it as a follow-up migration to MessageDescriptor. Migrating the registry's i18n is out of scope for this bugfix PR; the new label follows the same pattern as the existing ones so the surface doesn't grow inconsistently. Happy to pick up the registry-wide migration as a separate PR once there's an agreed approach.
There was a problem hiding this comment.
This is a re-review of #1899. The prior round asked for the new urlTemplate prop to be registered, for the :id token to match LiveSearch's ULID semantics, and for the changeset to stop claiming the old default was ULID-based. All of those are now resolved.
Approach judgment: Adding an optional urlTemplate prop to the core:recent-posts widget, routing through the existing buildLiveSearchResultUrl helper, is the right approach. It stays additive and preserves the long-standing default (/posts/${post.id}), so existing sites are unaffected.
What I checked:
- Token semantics now align with LiveSearch:
getString(post.data, "id")returns the content ULID becauseloader.tsmaps the rowidcolumn intodata.id, whilepost.idremains the slug-shaped (possiblylocale/slug) entry identifier. This matches the search API'srow.id. - The default branch bypasses the helper entirely, so the locale prefix is preserved when no template is set.
- The
urlTemplateprop is registered incoreWidgetComponentsand the unit test asserts its presence. - The changeset now describes the observable addition rather than a non-existent bug.
- No SQL, authorization, locale-filter, or content-table issues are introduced by the change; the component only reads its own collection data.
Headline conclusion: The implementation is clean and the stated goals are met. I have only non-blocking suggestions.
Residual notes:
- The new admin-visible prop label is hard-coded English. Per AGENTS.md all admin UI strings should go through Lingui; the whole widget registry currently shares this debt, so this is a note rather than a merge-blocker.
- There is no
:localetoken, so i18n sites that want locale-prefixed catch-all URLs (e.g./:locale/:slug) cannot express it generically. This mirrors LiveSearch'srouteMapand is a feature-gap, not a regression.
| }, | ||
| urlTemplate: { | ||
| type: "string", | ||
| label: "URL template (e.g. /blog/:slug)", |
There was a problem hiding this comment.
[suggestion] The new admin-facing prop label is hard-coded English. AGENTS.md requires every user-facing admin string to be wrapped for Lingui. The whole coreWidgetComponents registry currently uses plain strings, so this is pre-existing debt rather than a new violation, but any new label added now increases the migration surface. Consider migrating WidgetComponentDef.label (and prop labels) to MessageDescriptor in a follow-up and extracting the new string when that infrastructure is in place.
There was a problem hiding this comment.
Right — this is pre-existing debt across the whole coreWidgetComponents registry (every widget label is a plain string, not just the new one), and your own note flags it as a follow-up migration to MessageDescriptor. Migrating the registry's i18n is out of scope for this bugfix PR; the new label follows the same pattern as the existing ones so the surface doesn't grow inconsistently. Happy to pick up the registry-wide migration as a separate PR once there's an agreed approach.
| expect(recentPosts?.props).toHaveProperty("count"); | ||
| expect(recentPosts?.props).toHaveProperty("showThumbnails"); | ||
| expect(recentPosts?.props).toHaveProperty("showDate"); | ||
| expect(recentPosts?.props).toHaveProperty("urlTemplate"); |
There was a problem hiding this comment.
[suggestion] This assertion verifies the prop is registered, but it does not exercise the actual link generation in RecentPosts.astro. The existing live-search-routing.test.ts covers the helper in isolation, so a regression in the arguments passed from the widget (for example :id accidentally receiving the slug) would not be caught. Consider adding a render or e2e test that creates a core:recent-posts widget with urlTemplate and asserts the rendered anchor href.
There was a problem hiding this comment.
Fair point on coverage. The :id token semantics (ULID vs slug) are exercised in live-search-routing.test.ts for the helper itself; the widget call site is a thin pass-through. A full render test here needs a seeded DB (getEmDashCollection) — I'll fold that into a follow-up rather than grow this bugfix. Tracked.
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
There was a problem hiding this comment.
Approach judgment: Adding an optional urlTemplate prop to the core:recent-posts widget and routing it through the existing buildLiveSearchResultUrl helper is the right fix for #1332. It is additive, keeps the long-standing default of /posts/${post.id} (so existing sites and locale prefixes are unaffected), and aligns token semantics with LiveSearch.
What is fixed since the prior round: the urlTemplate prop is now registered in coreWidgetComponents; the :id token correctly receives the content ULID from post.data.id rather than the slug-shaped post.id; and the changeset now describes the observable addition instead of a non-existent ULID default bug.
What I checked: the loader maps row.id into data.id while entry.id stays slug-shaped (or locale/slug), the default branch bypasses the helper to preserve locale prefixing, the import path uses the repo's .js ESM convention, the docs/changeset claims match the implementation, and the existing live-search-routing.test.ts still covers the generic helper.
Headline conclusion: the implementation is correct and does what it says. My only blocking-ish concern is test coverage: the only new test asserts the prop exists in the registry, which does not reproduce or verify the actual URL-generation behavior. Per AGENTS.md's "TDD for bugs" rule, please add a behavioral test. I also carry forward the prior non-blocking note that the new admin-visible prop label is hard-coded English, continuing the registry's shared Lingui debt.
Verdict: comment.
| expect(recentPosts?.props).toHaveProperty("count"); | ||
| expect(recentPosts?.props).toHaveProperty("showThumbnails"); | ||
| expect(recentPosts?.props).toHaveProperty("showDate"); | ||
| expect(recentPosts?.props).toHaveProperty("urlTemplate"); |
There was a problem hiding this comment.
[needs fixing] The added assertion only checks that urlTemplate exists on the registry definition. AGENTS.md requires "TDD for bugs: failing test -> fix -> verify. A bug without a reproducing test is not fixed." This PR is framed as a fix for #1332, but there is no test that exercises the actual link generation — that :id maps to post.data.id (the content ULID), :slug maps to post.data.slug, and the default branch still emits /posts/${post.id} (preserving any locale prefix).
The existing live-search-routing.test.ts covers the generic helper, but it does not guard the RecentPosts mapping; a future regression that swapped post.data.id for post.id would not be caught. Add a behavioral test. One option is a render test in tests/repro/recent-posts-url.render.test.ts that mocks getEmDashCollection, or extract the URL builder to a small pure helper and unit-test it.
// Example: tests/repro/recent-posts-url.render.test.ts
import { experimental_AstroContainer as AstroContainer } from "astro/container";
import { describe, expect, it, vi } from "vitest";
import RecentPosts from "../../src/components/widgets/RecentPosts.astro";
vi.mock("../../src/query.js", () => ({
getEmDashCollection: async () => ({
entries: [
{
id: "hello-world",
data: {
id: "01JABCULID",
slug: "hello-world",
title: "Hello World",
publishedAt: "2026-01-01",
},
edit: {},
},
],
}),
}));
describe("RecentPosts urlTemplate", () => {
it("uses :slug from data.slug", async () => {
const container = await AstroContainer.create();
const html = await container.renderToString(RecentPosts, {
props: { urlTemplate: "/blog/:slug", count: 1 },
locals: {},
});
expect(html).toContain('href="/blog/hello-world"');
});
it("uses :id from data.id", async () => {
const container = await AstroContainer.create();
const html = await container.renderToString(RecentPosts, {
props: { urlTemplate: "/post/:id", count: 1 },
locals: {},
});
expect(html).toContain('href="/post/01JABCULID"');
});
it("preserves the default link shape when no template is set", async () => {
const container = await AstroContainer.create();
const html = await container.renderToString(RecentPosts, {
props: { count: 1 },
locals: {},
});
expect(html).toContain('href="/posts/hello-world"');
});
});|
Added render-level tests (Astro container) that verify the actual link URLs: the locale-prefixed default without a template, |
There was a problem hiding this comment.
The approach is still right: adding an optional urlTemplate prop to core:recent-posts and routing it through the existing buildLiveSearchResultUrl helper is a minimal, additive fix that leaves the locale-prefixed default unchanged and keeps token semantics consistent with LiveSearch.
Since the previous round, the author added a behavioral render test in packages/core/tests/repro/recent-posts-url.render.test.ts that reproduces the default, :slug, and :id cases, exactly addressing the prior blocking-ish concern about test coverage. The implementation matches the loader shape: entry.id is the slug or locale/slug, data.slug is the bare slug, and data.id is the content ULID. The default branch bypasses the helper to preserve locale prefixing, the import path uses the repo's .js ESM convention, and the changeset accurately describes the observable addition.
The one remaining real issue is admin localization: the new urlTemplate prop is exposed in coreWidgetComponents with a hard-coded English label, but the admin UI localizes core widget prop labels through CORE_WIDGET_META in packages/admin/src/components/Widgets.tsx. The new prop is missing from that map, so admin users will see untranslated English while the sibling props are localized. That's an AGENTS.md localization violation for a user-facing admin string.
Findings
-
[needs fixing]
packages/admin/src/components/Widgets.tsx:131The new
urlTemplateprop is user-facing in the admin widget form, but it is missing from theCORE_WIDGET_METAmap that localizes core widget prop labels. The server registry falls back to the hard-coded English label"URL template (e.g. /blog/:slug)", which violates AGENTS.md's rule to localize every admin UI string.Add the prop to
CORE_WIDGET_META["core:recent-posts"].propsso the admin can render a translated label:showDate: { label: msg`Show date` }, urlTemplate: { label: msg`URL template (e.g. /blog/:slug)` },
Adds an optional urlTemplate prop with the same token semantics as LiveSearch's routeMap; without a template the widget keeps its locale-prefixed default links. The admin widget form shows a localized label for the new prop.
bbe883d to
cc0dfa0
Compare
|
Rebased onto current main (the widgets guide was restructured upstream — the |
There was a problem hiding this comment.
The approach is still right: adding an optional urlTemplate prop to core:recent-posts and routing it through the existing buildLiveSearchResultUrl helper is a minimal, additive fix that leaves the locale-prefixed default unchanged and keeps token semantics consistent with LiveSearch. The prior localization finding is resolved: urlTemplate is now in CORE_WIDGET_META with a msg-wrapped label.
New review pass: the implementation looks correct against the loader shape (entry.id is slug/locale/slug, data.slug is bare slug, data.id is the ULID), and the render test reproduces the default, :slug, and :id cases. I did spot one real security gap and one docs gap:
urlTemplateis not URL-scheme-sanitized before rendering. Because the prop comes from the admin widget form (stored incomponentProps), a value likejavascript:alert(1)/:slugproduces an executablehrefon public pages.sanitizeHrefalready exists and is used for menu links, so the recent-posts link should go through the same sanitizer. The existing render tests pass unchanged with this fix, and a new test should assert the sanitization.- The widgets guide doesn't explain the tokens. The docs table mentions the prop but doesn't list
:collection/:id/:slug/:pathor note the locale-prefix default behavior. The canonical guide should carry that detail rather than leaving it only in the changeset.
Once the XSS vector is closed and the docs are slightly expanded, this looks ready.
Findings
-
[needs fixing]
packages/core/src/components/widgets/RecentPosts.astro:38-47urlTemplateis configured through the admin widget form and stored incomponentProps, then interpolated into anhrefwithout URL-scheme validation. A template such asjavascript:alert(1)/:slugproducesjavascript:alert(1)/hello-world, which the browser will execute when clicked (stored XSS via the widget form). The codebase already hassanitizeHrefinutils/url.tsand uses it for menu links inWidgetRenderer.astro; recent-posts links should be sanitized the same way.Add
import { sanitizeHref } from "../../utils/url.js";and wrap the templated URL before rendering. The existing render tests still pass because their results all start with/.const href = urlTemplate ? sanitizeHref( buildLiveSearchResultUrl( { collection: "posts", id: getString(post.data, "id") ?? post.id, slug: getString(post.data, "slug") ?? post.id, }, { posts: urlTemplate }, ), ) : `/posts/${post.id}`;Also add a repro test case (e.g.
urlTemplate: "javascript:alert(1)/:slug"→#) so the sanitization is protected against future regressions. -
[suggestion]
docs/src/content/docs/guides/widgets.mdx:43The table cell mentions the new URL template but doesn't tell readers which tokens are available or that omitting a template preserves the existing i18n locale prefix. Add a short explanatory paragraph after the component table so the canonical docs—not just the changeset—carry the useful detail.
The `core:recent-posts` link template accepts the same tokens as LiveSearch's `routeMap`: `:collection`, `:id` (the content ULID), `:slug` (the bare slug), and `:path` (slug or ID). Without a template, links keep their existing shape, including any Astro i18n locale prefix.
A widget-form template could carry an unsafe URL scheme into public page links; route templated URLs through sanitizeHref like menu links, and document the template tokens in the widgets guide.
|
Both findings addressed: templated hrefs now go through |
There was a problem hiding this comment.
Re-review: this is a clean, additive fix for the second half of #1332. The approach — giving core:recent-posts an optional urlTemplate prop and routing it through the existing buildLiveSearchResultUrl helper — keeps token semantics consistent with LiveSearch, preserves the existing locale-prefixed default, and introduces no new queries on the logged-out path.
All of the prior blockers have been addressed:
- XSS vector closed.
RecentPosts.astronow importssanitizeHreffrom../../utils/url.jsand wraps the interpolated template URL before rendering. Ajavascript:alert(1)/:slugtemplate is neutralized to#, matching the sanitizer used for menu links. - Docs gap fixed. The widgets guide now explains the available tokens (
:collection,:id,:slug,:path) and notes that omitting the template preserves the existing link shape including the Astro i18n locale prefix. - Test added. The new render test covers the default locale-prefixed link,
:slug,:id, and the unsafe-scheme case.
Other details look right: the prop is registered in coreWidgetComponents with a non-localized server label (the admin form maps core prop labels via CORE_WIDGET_META and Lingui), the admin meta entry is msg-wrapped, and the changeset is technically accurate. I did a static pass over the changed component, tests, docs, admin component, core registry, changeset, and existing URL-sanitizer/live-search-routing helpers. Nothing else jumps out.
What does this PR do?
Addresses the remaining half of #1332. The LiveSearch half was already fixed by #1387 (
routeMaptemplates); thecore:recent-postswidget still hardcoded its link shape as/posts/${post.id}with no way to adapt it to custom or catch-all routes.Changes:
urlTemplateprop (passed via the widget'scomponentProps, e.g."/blog/:slug"or"/:slug"for catch-all sites) using the same:collection/:id/:slug/:pathtokens as LiveSearch'srouteMap— one template vocabulary for both components, per the issue's suggestion. Token semantics match LiveSearch exactly::idis the content ULID,:slugthe bare slug.coreWidgetComponents, so it shows up in the admin widget prop form.post.idfrom the loader is already the slug (orlocale/slugwhen i18n prefixing is enabled), so existing links keep working exactly as before, including the locale prefix. (An earlier revision of this PR mischaracterized the old default as a ULID-based 404 — thanks to the review for catching that.)Templating goes through the existing
buildLiveSearchResultUrlhelper; token interpolation and the slug/id fallback are covered bytests/unit/components/live-search-routing.test.ts, and the registry test asserts the new prop.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runlive-search-routingtestsurlTemplateprop follows the maintainer-mergedrouteMappattern from [codex] Add LiveSearch route templates #1387 for the sibling componentAI-generated code disclosure
Screenshots / test output
n/a — default link shape unchanged; with
urlTemplate: "/blog/:slug"the widget now renders/blog/my-post-slug.