feat: Better SEO info + refactor #137
Conversation
WalkthroughReplaces many exported SEO constants with a typed Web model and slug constants; introduces Web, WebPage, and WebPageUrls; wires a Web instance into Generator, Manifest, and JsonLD APIs; updates templates and tests to resolve page names/URLs via Web getters; NewManifest and NewJsonID now accept a Web. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as CLI/Caller
participant Gen as Generator
participant Web as Web
participant JLD as JsonLD
participant Man as Manifest
Note over Gen,Web: initialization
Dev->>Gen: NewGenerator(...)
Gen->>Web: NewWeb()
Web-->>Gen: *Web instance*
Note over Gen: page build flow
Dev->>Gen: BuildForPage("home")
Gen->>Web: GetHomePage()
Web-->>Gen: WebPage{Name, Url, Title, Excerpt}
Gen->>JLD: NewJsonID(Page, Web)
JLD-->>Gen: Json-LD
Gen->>Man: NewManifest(Page, TemplateData, Web)
Man-->>Gen: Manifest
Gen-->>Dev: Rendered assets (meta, manifest, json-ld)
alt Post detail
Dev->>Gen: BuildForPost(slug)
Gen->>Web: GetPostsDetailPage()
Web-->>Gen: WebPage
Gen->>JLD: NewJsonID(Page, Web)
Gen-->>Dev: Post SEO outputs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gocanto
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
metal/cli/seo/jsonld.go (2)
27-41: Defensive nil check forwebin NewJsonIDPassing a nil
webwill panic when accessingweb.FoundedYear. Add a small guard with a safe fallback.-func NewJsonID(tmpl Page, web *Web) *JsonID { - return &JsonID{ +func NewJsonID(tmpl Page, web *Web) *JsonID { + fy := "" + if web != nil { + fy = fmt.Sprintf("%d", web.FoundedYear) + } + return &JsonID{ Lang: tmpl.Lang, SiteURL: tmpl.SiteURL, LogoURL: tmpl.LogoURL, OrgName: tmpl.SiteName, WebName: tmpl.SiteName, APIName: tmpl.SiteName, SameAs: tmpl.SameAsURL, APIRepoURL: tmpl.APIRepoURL, WebRepoURL: tmpl.WebRepoURL, - FoundedYear: fmt.Sprintf("%d", web.FoundedYear), + FoundedYear: fy, Now: func() time.Time { return time.Now().UTC() }, } }
49-60: Use schema.org-standardfoundingDate
foundedYearisn’t a schema.org property. PreferfoundingDate(a year string is acceptable).- "foundedYear": j.FoundedYear, + "foundingDate": j.FoundedYear,metal/cli/seo/manifest_test.go (1)
78-80: Strengthen assertion: validate shortcut contentsYou already check presence. Consider asserting first shortcut URL/name to ensure
Webis actually used.if got["shortcuts"] == nil { t.Fatalf("expected shortcuts in manifest") } + sc := got["shortcuts"].([]any) + first := sc[0].(map[string]any) + if first["url"] != "/" || first["name"] != "Home" { + t.Fatalf("unexpected first shortcut: %#v", first) + }metal/cli/seo/manifest.go (1)
41-98: Guard against nilweband consider scope consistency
- If
webis nil, this will panic. Add a small guard to default toNewWeb()or document and enforce non-nil.- Optional: set
Scopeto an absolute URL (e.g.,tmpl.SiteURL + "/") for consistency withStartURL.- Optional: store
web.Get*Page()into locals to avoid repetitive lookups.Example minimal guard:
-func NewManifest(tmpl Page, data TemplateData, web *Web) *Manifest { +func NewManifest(tmpl Page, data TemplateData, web *Web) *Manifest { + if web == nil { + web = NewWeb() + }metal/cli/seo/web.go (1)
20-26: Acronym casing consistency (URLvsUrl)Go style prefers consistent acronym casing (e.g.,
LogoURL,RepoAPIURL). Consider aligning new fields to that style in a future cleanup to reduce cognitive load. Non-blocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
metal/cli/seo/defaults.go(1 hunks)metal/cli/seo/generator.go(12 hunks)metal/cli/seo/generator_test.go(4 hunks)metal/cli/seo/jsonld.go(2 hunks)metal/cli/seo/manifest.go(3 hunks)metal/cli/seo/manifest_test.go(2 hunks)metal/cli/seo/web.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
metal/cli/seo/jsonld.go (2)
metal/cli/seo/data.go (1)
Page(5-18)metal/cli/seo/web.go (1)
Web(3-11)
metal/cli/seo/manifest_test.go (2)
metal/cli/seo/manifest.go (1)
NewManifest(41-99)metal/cli/seo/web.go (1)
NewWeb(28-104)
metal/cli/seo/manifest.go (2)
metal/cli/seo/data.go (2)
Page(5-18)TemplateData(20-38)metal/cli/seo/web.go (1)
Web(3-11)
metal/cli/seo/web.go (1)
metal/cli/seo/defaults.go (7)
AuthorName(4-4)HomeSlug(5-5)AboutSlug(6-6)ProjectsSlug(7-7)ResumeSlug(8-8)PostsSlug(9-9)PostDetailsSlug(10-10)
metal/cli/seo/generator_test.go (1)
metal/cli/seo/web.go (2)
Web(3-11)NewWeb(28-104)
metal/cli/seo/generator.go (4)
metal/cli/seo/web.go (2)
Web(3-11)NewWeb(28-104)metal/cli/seo/data.go (1)
Page(5-18)metal/cli/seo/jsonld.go (1)
NewJsonID(27-41)metal/cli/seo/manifest.go (2)
Manifest(10-24)NewManifest(41-99)
🔇 Additional comments (12)
metal/cli/seo/manifest_test.go (2)
58-60: LGTM: passingNewWeb()to NewManifestMatches the new API and keeps test determinism via
manifest.Now.
126-128: LGTM: updated NewManifest callRefactor aligns with new constructor signature.
metal/cli/seo/generator_test.go (1)
56-62: LGTM: tests updated to useWebcontextInjecting
NewWeb()and sourcing home page data viaGetHomePage()matches the refactor and keeps tests readable.Also applies to: 119-124, 265-266, 347-348
metal/cli/seo/defaults.go (1)
4-10: LGTM: centralized author and slug constantsThis reduces scattered URL/name constants and aligns with the new Web model.
metal/cli/seo/generator.go (8)
30-38: LGTM! Clean refactoring to centralize SEO configuration.Adding the
Webfield centralizes SEO configuration data, which improves maintainability and reduces scattered constants throughout the codebase.
52-88: LGTM! Web initialization properly wired.The Web instance is created and properly integrated into the Generator. Field population from
web.Urlsis consistent, and validation occurs at the right point (line 73).
377-383: LGTM! Web fields properly integrated.The refactoring correctly replaces scattered constants with centralized
g.Webfields for robots, theme color, color scheme, background color, and description. The updatedNewJsonIDsignature with theg.Webparameter is also correctly applied.Also applies to: 385-385
463-463: LGTM! Dynamic home page name comparison.The comparison now uses
g.Web.GetHomePage().Nameinstead of a static constant, which is consistent with the refactoring approach.
489-489: LGTM! Centralized description fallback.Using
g.Web.Descriptionas the fallback ensures consistency across the application. Based on theNewWeb()implementation in the relevant code snippets, the Description field is always populated, so this is safe.
522-528: LGTM! Dynamic post detail URL generation.The refactoring correctly replaces the static
WebPostDetailUrlconstant withg.Web.GetPostsDetailPage().Url, ensuring URLs are sourced from the centralized Web model.
405-405: Verified NewManifest signature update All call sites now pass the*Webparameter.
152-153: Ignore missing Web getters All referenced getter methods (GetHomePage, GetAboutPage, GetProjectsPage, GetResumePage) are implemented in metal/cli/seo/web.go; no action required.Likely an incorrect or invalid review comment.
gocanto
force-pushed
the
feat/enrich-seo
branch
from
5cdecc5 to
7e987a5
Compare
Summary by CodeRabbit
New Features
Refactor