-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(core): RecentPosts widget links by slug and supports urlTemplate (#1332) #1899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "emdash": patch | ||
| "@emdash-cms/admin": patch | ||
| --- | ||
|
|
||
| Adds an optional `urlTemplate` prop to the `core:recent-posts` widget (e.g. `"/blog/:slug"` or `"/:slug"` for catch-all routes), using the same `:collection`, `:id`, `:slug`, and `:path` tokens as LiveSearch's `routeMap`, with a localized label in the admin widget form. Without a template the widget links exactly as before. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,17 @@ | ||
| --- | ||
| import { getEmDashCollection } from "../../query.js"; | ||
| import { sanitizeHref } from "../../utils/url.js"; | ||
| import { buildLiveSearchResultUrl } from "../live-search-routing.js"; | ||
|
|
||
| interface Props { | ||
| count?: number; | ||
| showThumbnails?: boolean; | ||
| showDate?: boolean; | ||
|
Comment on lines
8
to
9
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [needs fixing] The new Add the prop definition there, e.g.: urlTemplate: {
type: "string",
label: "URL template",
},Also update the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 4a35b0d — |
||
| /** URL template for post links, e.g. "/blog/:slug" (tokens: :collection, :id, :slug, :path) */ | ||
| urlTemplate?: string; | ||
| } | ||
|
|
||
| const { count = 5, showThumbnails = false, showDate = true } = Astro.props; | ||
| const { count = 5, showThumbnails = false, showDate = true, urlTemplate } = Astro.props; | ||
|
|
||
| const { entries: posts } = await getEmDashCollection("posts", { | ||
| limit: count, | ||
|
|
@@ -27,6 +31,23 @@ function getString(data: Record<string, unknown>, key: string): string | undefin | |
| const publishedAt = getString(post.data, "publishedAt"); | ||
| const featuredImage = getString(post.data, "featured_image"); | ||
| const title = getString(post.data, "title"); | ||
| // Without a template, keep the widget's long-standing default: | ||
| // `post.id` is the loader's slug (or `locale/slug` with i18n | ||
| // prefixing), so the locale prefix is preserved. With a template, | ||
| // use the same token semantics as LiveSearch's routeMap — `:id` | ||
| // is the content ULID (`data.id`), `:slug` the bare slug. | ||
| 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}`; | ||
| return ( | ||
| <li> | ||
| {showThumbnails && featuredImage && ( | ||
|
|
@@ -36,7 +57,7 @@ function getString(data: Record<string, unknown>, key: string): string | undefin | |
| class="widget-recent-posts__thumbnail" | ||
| /> | ||
| )} | ||
| <a href={`/posts/${post.id}`} class="widget-recent-posts__link"> | ||
| <a href={href} class="widget-recent-posts__link"> | ||
| {title} | ||
| </a> | ||
| {showDate && publishedAt && ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,10 @@ export const coreWidgetComponents: WidgetComponentDef[] = [ | |
| label: "Show date", | ||
| default: true, | ||
| }, | ||
| urlTemplate: { | ||
| type: "string", | ||
|
Comment on lines
+28
to
+29
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] The
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged — this follows the existing pattern for widget prop labels in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right — this is pre-existing debt across the whole |
||
| label: "URL template (e.g. /blog/:slug)", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right — this is pre-existing debt across the whole |
||
| }, | ||
| }, | ||
| }, | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| 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: vi.fn(async () => ({ | ||
| entries: [ | ||
| { | ||
| // The loader's entry id is the slug, or `locale/slug` with | ||
| // i18n prefixing — distinct from the content ULID in data.id. | ||
| id: "en/hello-world", | ||
| data: { | ||
| id: "01ARZ3NDEKTSV4RRFFQ69G5FAV", | ||
| slug: "hello-world", | ||
| title: "Hello World", | ||
| publishedAt: "2026-01-01T00:00:00.000Z", | ||
| }, | ||
| }, | ||
| ], | ||
| })), | ||
| })); | ||
|
|
||
| async function renderHref(props: Record<string, unknown>): Promise<string> { | ||
| const container = await AstroContainer.create(); | ||
| const html = await container.renderToString(RecentPosts, { props, locals: {} }); | ||
| const match = html.match(/<a href="([^"]*)"/); | ||
| if (!match) throw new Error(`no link in rendered widget: ${html}`); | ||
| return match[1]!; | ||
| } | ||
|
|
||
| describe("RecentPosts link URLs", () => { | ||
| it("keeps the locale-prefixed default without a template", async () => { | ||
| expect(await renderHref({})).toBe("/posts/en/hello-world"); | ||
| }); | ||
|
|
||
| it("substitutes the bare slug into :slug", async () => { | ||
| expect(await renderHref({ urlTemplate: "/blog/:slug" })).toBe("/blog/hello-world"); | ||
| }); | ||
|
|
||
| it("substitutes the content ULID into :id, not the slug-shaped entry id", async () => { | ||
| expect(await renderHref({ urlTemplate: "/posts/:id" })).toBe( | ||
| "/posts/01ARZ3NDEKTSV4RRFFQ69G5FAV", | ||
| ); | ||
| }); | ||
|
|
||
| it("neutralizes a template with an unsafe URL scheme", async () => { | ||
| expect(await renderHref({ urlTemplate: "javascript:alert(1)/:slug" })).toBe("#"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -508,6 +508,7 @@ describe("Widget System", () => { | |
| expect(recentPosts?.props).toHaveProperty("count"); | ||
| expect(recentPosts?.props).toHaveProperty("showThumbnails"); | ||
| expect(recentPosts?.props).toHaveProperty("showDate"); | ||
| expect(recentPosts?.props).toHaveProperty("urlTemplate"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] This assertion verifies the prop is registered, but it does not exercise the actual link generation in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point on coverage. The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [needs fixing] The added assertion only checks that The existing // 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"');
});
}); |
||
| }); | ||
|
|
||
| it("should include categories component", () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[needs fixing] The changeset claims the old link was
/posts/{id}where{id}is the ULID, but the loader exposes the slug asentry.id. The old default already linked by slug for non-i18n sites (and bylocale/slugwhen i18n prefixing is active). The actual user-facing change is adding the optionalurlTemplateprop and routing the link throughbuildLiveSearchResultUrl.Please reword the changeset to describe what actually changed, e.g.:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right — the loader exposes the slug (or
locale/slug) asentry.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 optionalurlTemplateprop with LiveSearch's token vocabulary, default link unchanged. PR body updated too.