Repository navigation
feat(marketing): improve marketing starter template - #3058
khoinguyenpham04 wants to merge 22 commits into
Conversation
|
Scope checkThis PR changes 6,104 lines across 45 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@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: |
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. |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR ports the public emdashcms.com visual language into the Node and Cloudflare marketing templates. The overall approach is sound: a componentized set of Portable Text blocks (hero/features/pricing/faq/testimonials/CTA), a small editorial blog, a contact page, matching Node/Cloudflare trees, an updated seed file, agent docs, and a regression smoke test. The rendering code is safe, uses EmDash’s locale-aware helpers, and the tests cover the new archive/cursor paths.
I reviewed the changed template files, seed, docs, and smoke test. I did not run the test suite or build; everything below is based on static inspection.
There are no logic bugs or security vulnerabilities in the rendering path, but there are several issues that should be addressed before merge:
-
Logged-out query-count regression.
Base.astronow pays for five menu/settings round-trips on every logged-out render (getSiteSettings+ fourgetMenucalls). AGENTS.md is explicit: “Never add queries to the logged-out hot path” and treats increases as presumed wrong. The footer menus should be collapsed into a single hierarchicalfootermenu instead of three separate menus. -
EmDash branding remains in the public footer. The PR says EmDash branding was intentionally excluded, but
SiteFooter.astrostill links toemdashcms.com. That should be removed or made configurable before the template is used for public sites. -
Inaccurate color documentation.
AGENTS.mdandAGENTS-template.mdlist specific hex values for--color-bg,--color-surface, and--color-primarythat do not match the actualtokens.cssfallback palette, so agents following those docs will pick the wrong colors. -
Permanent redirect for
/pricing. Returning a301from/pricingto/#pricingwill be cached by browsers and search engines, which makes it hard for users to later create a real/pricingpage. A302/307is safer for a starter template. -
Contact page caches POST responses.
contact.astrocallsAstro.cache.set(cacheHint)unconditionally and swallows errors, which means a POST submission may also try to write a cache entry. It should only cache the GET response. -
Comment discipline.
src/plugins/marketing-blocks/index.ts,MarketingBlocks.astro, andcontact.astrocontain reviewer-facing notes, TODOs, and narrative that violate AGENTS.md’s comment rules. Move the demo-form caveats into the template docs and keep the code comments evergreen.
The same issues exist in the Cloudflare mirror templates (the relevant files are byte-for-byte copies in this PR), so any fixes should be applied to both templates/marketing/* and templates/marketing-cloudflare/*.
Findings
-
[needs fixing]
templates/marketing/src/layouts/Base.astro:27Base.astronow issues five menu/settings round-trips on every logged-out page (getSiteSettings+getMenu("primary")+ three footer menus). AGENTS.md says “Never add queries to the logged-out hot path” and that an increase is presumed wrong. The footer menus are a good candidate to collapse: create one hierarchicalfootermenu withProduct,Company, andSupportas top-level items and children underneath, then load onlygetMenu("primary")andgetMenu("footer")here.const [menu, footerMenu] = await Promise.all([ getMenu("primary"), getMenu("footer"), ]); -
[needs fixing]
templates/marketing/src/components/SiteFooter.astro:93The PR description says EmDash branding was intentionally excluded from the public templates, but the footer still hard-codes a “Powered by EmDash” link to
emdashcms.com. Generated sites based on this template will ship with EmDash branding, which contradicts the stated goal. Remove this block (and the matching.footer-poweredstyle) or make it a configurable site setting.<div class="footer-legal"> <p>© {currentYear} {siteTitle}.</p> </div> -
[suggestion]
templates/marketing/src/pages/pricing.astro:2A
301redirect is cached permanently by browsers and search engines. Because this is a starter template, a consumer may later want to turn/pricinginto a real page; a cached301to/#pricingwould make that painful. Use a non-permanent redirect such as302or307.return Astro.redirect("/#pricing", 302); -
[suggestion]
templates/marketing/src/pages/contact.astro:11-12Astro.cache.set(cacheHint)is called unconditionally and errors are swallowed, which means the POST submission path also attempts to write a cache entry. Cache hints should only apply to GET responses. Replace the defensivetry/catchwith an explicit method check.if (Astro.request.method === "GET" && Astro.cache?.enabled) { Astro.cache.set(cacheHint); } -
[needs fixing]
templates/marketing/AGENTS.md:99-101These documented hex values are wrong for the template’s actual fallback palette in
src/styles/tokens.css.--color-bgdark is#0a0a0aff(--background-base),--color-surfacedark is#171717ff(--surface), and--color-primaryis#0075deff(--heat-100). Publishing incorrect color values in agent instructions will lead users to the wrong palette. Either list the real token values or remove the hard-coded hexes and point readers totokens.css.- `--color-bg: var(--background-base)` (`#f7f9fc` / `#0a0a0a`) -- the light and dark canvas - `--color-surface: var(--surface)` (`#ffffff` / `#171717`) -- bordered cards and panels - `--color-primary: var(--heat-100)` (`#0075de` / `#0075de`) -- links, focus, and selected states -
[needs fixing]
templates/marketing/AGENTS-template.md:55-57Same issue as
AGENTS.md: the documented color hexes do not match the fallback values insrc/styles/tokens.css.- `--color-bg: var(--background-base)` (`#f7f9fc` / `#0a0a0a`) -- the light and dark canvas - `--color-surface: var(--surface)` (`#ffffff` / `#171717`) -- bordered cards and panels - `--color-primary: var(--heat-100)` (`#0075de` / `#0075de`) -- links, focus, and selected states -
[suggestion]
templates/marketing/src/plugins/marketing-blocks/index.ts:1-22This block comment is mostly PR-level narrative and rejected-alternative commentary (“Block Kit has no …”, “componentsEntry auto-wiring is a separate cleanup”), which AGENTS.md says comments should not be. Keep any genuinely non-obvious “why” (e.g., why the CTA fields are flattened) and move the rest to the template docs or the PR description.
-
[suggestion]
templates/marketing/src/components/MarketingBlocks.astro:1-8This JSDoc block restates what the component already says in code (
components={{ type: marketingTypes }}). Per AGENTS.md, comments should not repeat the code; remove it or keep only the exported type/interface docs.
github-actions
Bot
added
review/awaiting-author
khoinguyenpham04
changed the title
github-actions
Bot
added
review/needs-rereview
khoinguyenpham04
added
the
bot:review
There was a problem hiding this comment.
This is a sensible refresh of the marketing templates — the component structure is clean, Node and Cloudflare stay in sync, and the author has addressed most of the last review. The logged-out rendering path is still paying for five round-trips before any page-specific content, and the new contact form logs PII to stdout, so two real issues remain.
What I checked:
- All changed template files (layouts, pages, components, plugin, seed, styles) and their Cloudflare mirrors.
packages/core/tests/integration/smoke/site-matrix-smoke.test.ts— the new marketing regression smoke is a useful addition.templates/marketing/AGENTS.mdandAGENTS-template.mdagainstsrc/styles/tokens.css.- Root
AGENTS.mdconventions, especially the rules on logged-out query counts and privacy/PII handling.
What’s fixed from the prior review:
- EmDash branding removed from
SiteFooter.astro(no "Powered by EmDash" /emdashcms.comlinks remain in rendered output). /pricingredirect changed from301to302.contact.astronow only sets the cache hint onGET.- Documented color hex values are now accurate for the fallback palette.
- The over-long, reviewer-facing comments in
marketing-blocks/index.tsandMarketingBlocks.astroare gone.
What’s still open:
- The base layout still issues
getSiteSettings()plus four menu lookups on every logged-out render. PerAGENTS.mdthis is a clear violation of the "never add queries to the logged-out hot path" rule. - The contact form prints the submitter’s name, email, company, and message to
console.log, which writes PII into server logs in production.
No new changeset is needed — the only published-package touch is test code.
Findings
-
[needs fixing]
templates/marketing/src/layouts/Base.astro:27-30On every logged-out render this layout still pays for
getSiteSettings()plus four separate menu lookups. That is five round-trips before the page’s own content query, which directly conflicts withAGENTS.md: “Never add queries to the logged-out hot path.” The footer columns are the obvious consolidation target: define one hierarchicalfootermenu whose top-level items areProduct,Company, andSupportwith children underneath, load onlygetMenu("primary")andgetMenu("footer"), then map children to the existingfooterColumnsshape.const [menu, footerMenu] = await Promise.all([ getMenu("primary"), getMenu("footer"), ]); const footerColumns = (footerMenu?.items ?? []) .filter((item) => item.children && item.children.length > 0) .map((item) => ({ heading: item.label, menu: { items: item.children }, }));With that change,
seed/seed.jsonshould be updated to ship a singlefootermenu instead offooter_product,footer_company, andfooter_support, andAGENTS.mdshould describe the new menu shape. -
[needs fixing]
templates/marketing-cloudflare/src/layouts/Base.astro:27-30Same unresolved logged-out query-count issue as the Node template:
getSiteSettings()plus four menu lookups on every render is too many for the logged-out hot path and violatesAGENTS.md. Apply the same single hierarchicalfootermenu fix here. -
[needs fixing]
templates/marketing/src/pages/contact.astro:31-36The POST handler prints the submitter’s name, email, company, and message with
console.log. In production that writes PII into server stdout/logs, which is unnecessary exposure and will likely break privacy/data-minimization expectations for any site based on this template. Replace it with a wiring comment and do not ship a default sink that logs personal data.// Wire this to an email service or webhook; don't log personal data. formStatus = "success"; -
[needs fixing]
templates/marketing-cloudflare/src/pages/contact.astro:31-36Same
console.logPII issue as the Node template: the Cloudflare contact form also writes name, email, company, and message to stdout. Remove the logging here as well.
github-actions
Bot
added
review/awaiting-author
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | ebe6a55 | Sep 25 2026, 08:39 PM |
github-actions
Bot
added
review/needs-rereview
khoinguyenpham04
added
the
bot:review
There was a problem hiding this comment.
This is a sensible refresh of the marketing templates — the component architecture is cleaner, Node and Cloudflare stay in sync, the smoke test is genuinely useful, and the visual changes align with the PR description. The only remaining issue is the logged-out query-count problem from the prior review, which is still unaddressed.
What I checked:
- All changed template files in both
templates/marketingandtemplates/marketing-cloudflare(layouts, pages, components, plugin, seed, styles). packages/core/tests/integration/smoke/site-matrix-smoke.test.ts— the new marketing regression smoke is a good addition and covers pricing redirect, CTA targets, anonymous header, paginated archive, invalid cursor/slug handling, and editor-set SEO metadata.templates/marketing/AGENTS.mdandAGENTS-template.mdagainst the implementation.- Root
AGENTS.mdconventions, especially the logged-out hot-path guidance.
What’s fixed from the prior review:
- EmDash branding removed from the footer (no "Powered by EmDash" /
emdashcms.comlinks remain). /pricingredirect is a302.- The contact form and its PII-logging POST handler are gone;
contact.astronow only renders direct email cards. - Documented token hex values match the fallback palette.
- Reviewer-facing comments in
marketing-blocks/index.tsandMarketingBlocks.astroare gone.
What’s still open:
Base.astroin both templates still issuesgetSiteSettingsWithCacheHint()plus four separate menu lookups on every logged-out render — five round-trips before the page’s own content query. That violatesAGENTS.md’s "Protect the logged-out hot path" guidance and remains the one real blocker from the previous review.
No new changeset is needed — the only touched package code is the smoke test, and the templates themselves are not published packages.
Findings
-
[needs fixing]
templates/marketing/src/layouts/Base.astro:38-45On every logged-out render this layout still pays for
getSiteSettingsWithCacheHint()plus four separate menu lookups (primary,footer_product,footer_company,footer_support). That is five round-trips before the page’s own content query, which violatesAGENTS.md’s "Protect the logged-out hot path" convention for logged-out routes. The footer columns are the obvious consolidation target: ship one hierarchicalfootermenu whose top-level items areProduct,Company, andSupportwith children underneath, load onlygetMenuWithCacheHint("primary")andgetMenuWithCacheHint("footer"), then map children to the existingfooterColumnsshape.const [settingsResult, menuResult, footerMenuResult] = await Promise.all([ getSiteSettingsWithCacheHint(), getMenuWithCacheHint("primary"), getMenuWithCacheHint("footer"), ]);With that change, update the cache-hint loop to include
footerMenuResult, buildfooterColumnsfromfooterMenuResult.data.items, updateseed/seed.jsonto ship a singlefootermenu, and updateAGENTS.mdto describe the new menu shape. -
[needs fixing]
templates/marketing-cloudflare/src/layouts/Base.astro:38-45This is the same unresolved logged-out query-count issue as the Node template:
getSiteSettingsWithCacheHint()plus four menu lookups on every render is too many for the logged-out hot path. Apply the same single hierarchicalfootermenu fix here: load onlyprimaryandfootermenus, build the footer columns from the children of thefootermenu’s top-level items, and mirror theseed/seed.jsonandAGENTS.mdupdates in the Cloudflare variant.const [settingsResult, menuResult, footerMenuResult] = await Promise.all([ getSiteSettingsWithCacheHint(), getMenuWithCacheHint("primary"), getMenuWithCacheHint("footer"), ]);
github-actions
Bot
added
review/awaiting-author
What does this PR do?
Brings the public emdashcms.com visual language to the reusable Node and Cloudflare marketing templates with a neutral blue palette. The templates remain generic rather than copying EmDash-specific branding or dashboard content.
/pricingredirects to/#pricing. The contact page offers direct email links rather than a simulated form submission.Closes: Not applicable.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testwas not run)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Homepage — light desktop
Blog — light and dark desktop
Contact — dark mobile
Verified locally:
pnpm build, marketing Node and Cloudflare production builds,pnpm typecheck,pnpm typecheck:templates,pnpm lint, andpnpm format