[feature]: Add SEO engine #110
Conversation
|
Warning Rate limit exceeded@gocanto has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughAdds an SEO generation subsystem, fixture-driven static routing and WebsiteRoutes, introduces SeoEnvironment and App.URL, renames Ping→PingEnvironment, makes middleware environment-aware with local bypasses, updates router/kernel wiring and CLI, adjusts Makefile/docker/.env examples, and updates tests and repositories to match new types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User (CLI)
participant CLI as metal/cli
participant G as seo.Generator
participant C as seo.Client
participant R as router.WebsiteRoutes
participant H as StaticRouteResource
participant FS as Filesystem
U->>CLI: Select "Generate SEO"
CLI->>G: NewGenerator(db, env, validator)
G->>R: NewWebsiteRoutes(env)
G->>C: NewClient(R)
G->>G: Page.Load() (embed stub.html)
CLI->>G: GenerateHome()
G->>C: GetProfile/GetTalks/GetProjects
C->>H: invoke fixture handler (httptest)
H-->>C: JSON payload
C-->>G: unmarshaled data
G->>G: Build TemplateData (JSON-LD, Manifest)
G->>FS: Write index.html to OutputDir
FS-->>CLI: success
CLI-->>U: Success
sequenceDiagram
autonumber
participant Client as C
participant Server as S
participant Router as R
participant PM as PublicMiddleware
participant TM as TokenMiddleware
participant Handler as H
C->>S: HTTP Request
S->>R: route match
R->>PM: Handle(w,r)
alt non-production
PM-->>R: next(w,r) (short-circuit)
else production
PM->>PM: checks (deps, rate, timestamp)
PM-->>R: next(w,r)
end
R->>TM: Handle(w,r) with env
alt env.App.IsLocal()
TM-->>R: next(w,r) (bypass)
else
TM->>TM: validate token/signature
TM-->>R: next or error
end
R->>Handler: Serve
Handler-->>Client: Response
note right of R: Validator is now public for signature route
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
force-pushed
the
feature/add-seo
branch
from
116c52d to
cd61e50
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/middleware/public_middleware.go (1)
54-61: Enforce IP allowlist in PublicMiddleware.Handle (prod-only).MakePublicMiddleware sets allowedIP but Handle doesn't validate the client IP; TestPublicMiddleware_IPWhitelist (pkg/middleware/public_middleware_test.go) expects 401 for non-allowed IPs. Insert a production-only check after reading headers (after reqID := ... in pkg/middleware/public_middleware.go) using the existing helper:
if p.isProduction && p.allowedIP != "" {
clientIP := portal.ParseClientIP(r)
if clientIP != p.allowedIP {
return mwguards.UnauthenticatedError("Unauthorized IP", fmt.Sprintf("client ip %s not allowed", clientIP))
}
}metal/cli/accounts/handler.go (2)
35-53: Avoid double decoding and stale error usage in ShowAccount.Decode once, check error, then print. print shouldn’t re-decode.
func (h Handler) ShowAccount(accountName string) error { item := h.Tokens.FindBy(accountName) @@ - token, err := h.TokenHandler.DecodeTokensFor( + token, err := h.TokenHandler.DecodeTokensFor( item.AccountName, item.SecretKey, item.PublicKey, ) - - if h.print(token, item) != nil { - return fmt.Errorf("could not decode the given account [%s] keys: %v", item.AccountName, err) - } + if err != nil { + return fmt.Errorf("could not decode the given account [%s] keys: %v", item.AccountName, err) + } + if perr := h.print(token, item); perr != nil { + return perr + }
55-64: print re-decodes and shadows token; use provided token and only decode if nil.This avoids duplicate work and shadowing.
-func (h Handler) print(token *auth.Token, item *database.APIKey) error { - token, err := h.TokenHandler.DecodeTokensFor( - item.AccountName, - item.SecretKey, - item.PublicKey, - ) - - if err != nil { - return fmt.Errorf("could not decode the given account [%s] keys: %v", item.AccountName, err) - } +func (h Handler) print(token *auth.Token, item *database.APIKey) error { + var err error + if token == nil { + token, err = h.TokenHandler.DecodeTokensFor( + item.AccountName, + item.SecretKey, + item.PublicKey, + ) + if err != nil { + return fmt.Errorf("could not decode the given account [%s] keys: %v", item.AccountName, err) + } + }pkg/middleware/token_middleware.go (1)
37-51: Constructor signature changed — update all MakeTokenMiddleware call sites to include env as first argSignature is now: MakeTokenMiddleware(e *env.Environment, tokenHandler *auth.TokenHandler, apiKeys *repository.ApiKeys).
Fix callers still using the 2-arg form:
- pkg/middleware/token_middleware_additional_test.go: lines 104, 119, 144, 172, 214 (currently
MakeTokenMiddleware(th, repo)).- pkg/middleware/token_middleware_test.go: lines 28, 41, 55, 74, 223, 304, 340, 372 (mix of
MakeTokenMiddleware(nil, nil)andMakeTokenMiddleware(th, repo)).metal/router/router.go already passes
r.Env(no change).Change calls to MakeTokenMiddleware(, tokenHandler, apiKeys) — use the real env (e.g., r.Env) where available, or
nilin unit tests if appropriate — then run tests and pipeline.
🧹 Nitpick comments (50)
pkg/middleware/mwguards/valid_timestamp_guard.go (2)
51-53: Good switch to centralized errors; include structured context for better logs.Pass request context to the helper so logs are actionable; also align the log message with the condition.
- return TimestampTooOldError("Request timestamp is too old or invalid", "Request timestamp invalid") + return TimestampTooOldError( + "Request timestamp is too old or invalid", + "Request timestamp expired", + map[string]any{ + "epoch": epoch, + "now": now, + "skew_secs": skewSecs, + "min": minValue, + "max": maxValue, + "disallowFuture": disallowFuture, + }, + )Please confirm 401 is the intended status per your auth spec for this case.
55-57: Mirror the above: add context and a precise log message for “future” failures.- return TimestampTooNewError("Request timestamp is too recent or invalid", "Request timestamp invalid") + return TimestampTooNewError( + "Request timestamp is too recent or invalid", + "Request timestamp too far in future", + map[string]any{ + "epoch": epoch, + "now": now, + "skew_secs": skewSecs, + "min": minValue, + "max": maxValue, + "disallowFuture": disallowFuture, + }, + )Also sanity‑check that the added slog.Error emissions from these helpers won’t create noisy logs on client mistakes in high‑traffic paths.
pkg/middleware/public_middleware.go (1)
46-48: Don’t short‑circuit the middleware in non‑prod; remove this bypass.This early return disables dependency checks, replay/timestamp validation, and rate limiting in all non‑prod envs. Given the PR checklist (“Remove middleware changes”), this should not ship.
Apply this diff to drop the bypass:
- if !p.isProduction { - return next(w, r) //@todo remove! - } + // Middleware remains active in all environments. Any env-specific relaxations are handled explicitly below..gitignore (1)
8-11: Broaden ignore to directory (catch files without extensions and nested paths).Use a directory-wide ignore so generated assets like files without dots (e.g., "index") or nested folders are ignored too.
-# -- [SEO]: static files -storage/seo/pages/*.* +# -- [SEO]: static files +# Ignore everything under pages/, but keep .gitkeep +storage/seo/pages/** !storage/seo/.gitkeep !storage/seo/pages/.gitkeepmetal/env/ping.go (2)
18-21: Trim both input and stored creds before comparison.Prevents false negatives when callers pass whitespace-padded values.
func (p PingEnvironment) HasInvalidCreds(username, password string) bool { - return username != strings.TrimSpace(p.Username) || - password != strings.TrimSpace(p.Password) + u := strings.TrimSpace(username) + pw := strings.TrimSpace(password) + return u != strings.TrimSpace(p.Username) || + pw != strings.TrimSpace(p.Password) }
5-8: Add a brief doc comment for the exported type.Improves public API clarity and passes linters that require comments on exported identifiers.
-type PingEnvironment struct { +// PingEnvironment holds basic auth credentials for ping endpoints. +type PingEnvironment struct { Username string `validate:"required,min=16"` Password string `validate:"required,min=16"` }metal/makefile/build.mk (1)
9-13: Prefer $(MAKE) for recursive invocations.Propagates flags (e.g., -j, --warn-undefined-variables) and is make‑portable.
build-fresh: - make fresh && \ - make db:fresh && \ - make db:migrate && \ - make db:seed + $(MAKE) fresh && \ + $(MAKE) db:fresh && \ + $(MAKE) db:migrate && \ + $(MAKE) db:seedmetal/makefile/app.mk (2)
56-58: Use $(MAKE) to chain targets.More robust than calling the binary directly.
run-cli-docker: - make run-cli DB_SECRET_USERNAME=./database/infra/secrets/pg_username DB_SECRET_PASSWORD=./database/infra/secrets/pg_password DB_SECRET_DBNAME=./database/infra/secrets/pg_dbname + $(MAKE) run-cli DB_SECRET_USERNAME=./database/infra/secrets/pg_username DB_SECRET_PASSWORD=./database/infra/secrets/pg_password DB_SECRET_DBNAME=./database/infra/secrets/pg_dbname
62-63: Consider mirroring env guards from run-cli.run-metal runs outside Docker; if it also requires DB secrets, reuse the guard or load a local env file.
Makefile (1)
100-100: Fix minor typo in help text.Double “the”.
- @printf " $(BOLD)$(GREEN)supv:api:logs-err$(NC): Show the the API service supervisor error logs.\n\n" + @printf " $(BOLD)$(GREEN)supv:api:logs-err$(NC): Show the API service supervisor error logs.\n\n"Note: There’s a similar “the the” in supv:api:logs above; consider fixing in a follow‑up.
metal/cli/seo/support.go (1)
19-27: Harden Fetch: set Accept header, check status code, and wrap handler error.Improves correctness and debuggability.
func Fetch[T any](response *T, handler func() router.StaticRouteResource) error { - req := httptest.NewRequest("GET", "http://localhost:8080/proxy", nil) + req := httptest.NewRequest("GET", "http://localhost:8080/proxy", nil) + req.Header.Set("Accept", "application/json") rr := httptest.NewRecorder() maker := handler() - if err := maker.Handle(rr, req); err != nil { - return err + if apiErr := maker.Handle(rr, req); apiErr != nil { + return fmt.Errorf("handler error: %v", apiErr) } + + if rr.Code < 200 || rr.Code >= 300 { + return fmt.Errorf("unexpected status code: %d", rr.Code) + }Please confirm router.ApiError implements the error interface; if not, we should wrap it as shown.
metal/cli/seo/data.go (3)
30-38: Use numeric types for OG image dimensions with validation.Avoids accidental non‑numeric values and enables range checks.
type TagOgData struct { Type string `validate:"required,oneof=website"` Image string `validate:"required,url"` ImageAlt string `validate:"required,min=10"` - ImageWidth string `validate:"required"` - ImageHeight string `validate:"required"` + ImageWidth int `validate:"required,gt=0"` + ImageHeight int `validate:"required,gt=0"` SiteName string `validate:"required,min=5"` Locale string `validate:"required,min=5"` }
19-25: Avoid html/template.JS unless the content is guaranteed safe.If these fields can ever include untrusted data, prefer passing structured data and JSON‑encoding in the template to prevent XSS.
Example direction:
- Make JsonLD and Manifest concrete structs.
- Add helper funcs in the template to JSON‑marshal them safely (or pre‑marshal to string without using htmltemplate.JS).
5-10: Confirm validator tag for directory paths.Is
dirpathsupported by your validator? If not, consider a custom rule or reuse an existing tag (e.g., path existence check).I can add a custom validator or switch to a supported tag once confirmed.
handler/keep_alive_db_test.go (2)
18-18: Use credentials that satisfy PingEnvironment validation tags.PingEnvironment enforces min=16 for Username/Password; "user"/"pass" may start failing if validation is invoked in constructor/wiring. Use longer test creds to future‑proof.
Apply:
- e := env.PingEnvironment{Username: "user", Password: "pass"} + e := env.PingEnvironment{Username: "user-aaaaaaaaaaaa", Password: "pass-aaaaaaaaaaaa"}
17-17: Don’t discard potential error from MakeTestDB.If MakeTestDB returns an error, failing fast improves signal.
- db, _ := handlertests.MakeTestDB(t) + db, err := handlertests.MakeTestDB(t) + if err != nil { + t.Fatalf("MakeTestDB: %v", err) + }metal/env/seo.go (1)
3-5: Tighten validation for SpaDir (directory existence).“required” ensures presence only. Consider verifying that SpaDir points to an existing directory or add a custom validator, since generators will read/write here.
Example options:
- Add a custom validator tag (e.g., “dir”) in portal.Validator and use:
validate:"required,dir".- Or, validate SpaDir in env.Environment.Validate() by checking
os.StatandMode().IsDir().metal/env/app.go (1)
28-30: Lang() default is fine; consider future override hook.Returning a constant works now. If i18n lands, plan for an env/config override (e.g., AppEnvironment.Language with sane default).
handler/keep_alive_test.go (1)
16-16: Align test creds with PingEnvironment min length.Same rationale as the DB test—use ≥16‑char strings to avoid future validation coupling issues.
- e := env.PingEnvironment{Username: "user", Password: "pass"} + e := env.PingEnvironment{Username: "user-aaaaaaaaaaaa", Password: "pass-aaaaaaaaaaaa"}pkg/middleware/pipeline.go (1)
13-13: Document deprecation and plan removal of TokenHandler.Inline “@todo Remove!” is easy to miss. Mark as Deprecated in a doc comment, reference an issue, and define removal timeline to avoid accidental API breakage.
-type Pipeline struct { +// Pipeline builds request middleware chains. +type Pipeline struct { Env *env.Environment ApiKeys *repository.ApiKeys - TokenHandler *auth.TokenHandler //@todo Remove! + // Deprecated: TokenHandler will be removed after vX.Y (see issue #NNN). + TokenHandler *auth.TokenHandler PublicMiddleware PublicMiddleware }.env.example (2)
7-7: Provide a sensible default/example for ENV_APP_URL.Helps first‑time setup.
-ENV_APP_URL= +ENV_APP_URL="http://localhost:8080"
37-38: Add guidance/example for ENV_SPA_DIR.Clarify expected path semantics (absolute vs relative) and typical location.
-# --- SEO: SPA application directory -ENV_SPA_DIR= +# --- SEO: SPA application directory (directory must exist; absolute or project‑relative) +ENV_SPA_DIR="./storage/spa"metal/kernel/helpers.go (1)
11-13: Accept a pointer to Router to avoid copying and escape‑addressing a value param.Current signature copies Router, then takes its address—this allocates and decouples later caller mutations. Accepting *router.Router is simpler and avoids unintended copies.
-func (a *App) SetRouter(router router.Router) { - a.router = &router -} +func (a *App) SetRouter(r *router.Router) { + a.router = r +}metal/router/router_keep_alive_test.go (1)
14-17: Test creds violate min-length validation; use ≥16 chars.
PingEnvironment enforces min=16; align test data accordingly.- Env: &env.Environment{Ping: env.PingEnvironment{Username: "user", Password: "pass"}}, + Env: &env.Environment{Ping: env.PingEnvironment{Username: "user-credential-1234", Password: "pass-credential-1234"}},metal/router/router_keep_alive_db_test.go (1)
16-17: Test creds violate min-length validation; use ≥16 chars.
Align with PingEnvironment tags to avoid drifting from production constraints.- Env: &env.Environment{Ping: env.PingEnvironment{Username: "user", Password: "pass"}}, + Env: &env.Environment{Ping: env.PingEnvironment{Username: "user-credential-1234", Password: "pass-credential-1234"}},metal/cli/seo/stub.html (2)
8-10: Fix meta usage: theme-color vs color-scheme.
color-scheme expects “light dark”; theme color should hold the hex. Replace and drop nonstandard theme-scheme.- <meta name="color-scheme" content="{{.ThemeColor}}"> - <meta name="theme-scheme" content="{{.ThemeColor}}"> + <meta name="theme-color" content="{{.ThemeColor}}"> + <meta name="color-scheme" content="light dark">
14-15: Trim stray space in canonical href.
Minor template cleanliness.- <link rel="canonical" href="{{.Canonical }}"> + <link rel="canonical" href="{{.Canonical}}">docker-compose.yml (1)
85-85: Pin builder image to a digest for supply‑chain safety.
Tag-only images drift; prefer digest pinning.- image: golang:1.25.1-alpine + image: golang:1.25.1-alpine@sha256:<PINNED_DIGEST>handler/keep_alive.go (1)
15-20: *Signature update to env.PingEnvironment — LGTM.
Optional: metal/env/ping.go:18–20 — HasInvalidCreds uses != for string comparison; replace with crypto/subtle.ConstantTimeCompare on trimmed []byte to avoid timing leaks.handler/keep_alive_db.go (1)
16-22: *Signature update to env.PingEnvironment — LGTM.Small ask: avoid hanging on DB ping — update Connection.Ping (database/connection.go) to use context.WithTimeout and call conn.PingContext(ctx) with a short timeout so the handler cannot block.
metal/kernel/kernel_test.go (1)
160-170: Avoid package shadowing — rename localroutervariable.Rename the local var to avoid shadowing the imported router package; update the SetRouter call.
File: metal/kernel/kernel_test.go (lines 160-170)
- router := router.Router{ + rt := router.Router{ Env: env, Mux: http.NewServeMux(), Pipeline: middleware.Pipeline{ Env: env, ApiKeys: &repository.ApiKeys{DB: &database.Connection{}}, TokenHandler: handler, PublicMiddleware: middleware.MakePublicMiddleware("", false), }, Db: &database.Connection{}, } - app := &App{} - app.SetRouter(router) + app := &App{} + app.SetRouter(rt)Confirmed: Pipeline.Env is still set in multiple files (metal/kernel/app.go, metal/kernel/kernel_test.go, metal/router/router_keep_alive_test.go, metal/router/router_keep_alive_db_test.go) — keep Pipeline.Env.
metal/cli/main.go (1)
165-194: LGTM: timestamp utility is fine.Small nit: if consistency with UTC is desired across outputs, use now.UTC(). Not blocking.
metal/makefile/db.mk (1)
59-61: compose run --env-file portability.docker compose --env-file with run isn’t uniformly available across environments and is redundant if the service already has env_file in docker-compose.yml. Prefer relying on the service’s env_file or pass -e flags only.
Apply this diff to simplify:
-db:seed: - docker compose --env-file ./.env run --rm $(DB_MIGRATE_DOCKER_ENV_FLAGS) $(DB_API_RUNNER_SERVICE) \ - go run ./database/seeder/main.go +db:seed: + docker compose run --rm $(DB_MIGRATE_DOCKER_ENV_FLAGS) $(DB_API_RUNNER_SERVICE) \ + go run ./database/seeder/main.goIf you require variable substitution from .env for compose itself, use:
- docker compose --env-file ./.env up (not run), or
- service-level env_file: ./.env in docker-compose.yml.
metal/kernel/factory.go (2)
116-118: Minor: error message wording.“invalid [logs Credentials] model” reads odd. Consider “invalid [LOGS] model”.
- panic(errorSuffix + "invalid [logs Credentials] model: " + validate.GetErrorsAsJson()) + panic(errorSuffix + "invalid [LOGS] model: " + validate.GetErrorsAsJson())
136-151: Minor: naming consistency.Variable blog holds the env; envModel or cfg would be clearer.
- blog := &env.Environment{ + model := &env.Environment{ ... - if _, err := validate.Rejects(blog); err != nil { + if _, err := validate.Rejects(model); err != nil { ... - return blog + return modelmetal/cli/seo/jsonld.go (1)
57-60: Schema property: use foundingDate instead of foundedYear.Schema.org expects foundingDate (ISO date string) for Organization; foundedYear isn’t a standard property.
- "foundedYear": j.FoundedYear, + "foundingDate": j.FoundedYear,metal/router/web.go (1)
10-13: Path constants lack leading slash.For consistency with router paths (e.g., "/profile"), prefer “/about”, “/resume”, “/projects”.
-const AboutPage = "about" -const ResumePage = "resume" -const ProjectsPage = "projects" +const AboutPage = "/about" +const ResumePage = "/resume" +const ProjectsPage = "/projects"metal/kernel/app.go (1)
86-99: Avoid copying the router by value in Boot.Copying the struct is unnecessary and can surprise if Router gains internal state. Use the pointer directly.
- modem := *a.router - - modem.KeepAlive() - modem.KeepAliveDB() - modem.Profile() + modem := a.router + + modem.KeepAlive() + modem.KeepAliveDB() + modem.Profile() modem.Experience() modem.Projects() modem.Social() modem.Talks() modem.Education() modem.Recommendations() modem.Posts() modem.Categories() modem.Signature()metal/cli/accounts/handler.go (1)
68-74: Secrets printed in plaintext.Consider gating plaintext secret output behind a confirmation prompt or environment check to reduce accidental exposure, especially for ShowAccount.
metal/cli/seo/web.go (1)
26-28: Content/semantics nits: description typo and invalid theme color.
- Duplicate word “of” in Description.
- ThemeColor must be a valid CSS color; “light dark” is not.
Apply:
-const Description = "Gustavo is a full-stack Software Engineer leader with over two decades of of experience..." +const Description = "Gustavo is a full-stack Software Engineering leader with over two decades of experience..." -const ThemeColor = "light dark" +const ThemeColor = "#ffffff"metal/cli/seo/manifest.go (2)
51-56: Use relative start_url for better PWA behavior.Absolute URLs work but relative “/” is recommended and matches scope.
- StartURL: tmpl.SiteURL, + StartURL: WebHomeUrl,
21-23: Unused field: Manifest.Now is never read.If not used elsewhere, remove to simplify.
metal/cli/seo/generator.go (4)
154-177: Provide required BgColor, valid ThemeColor, and richer icons.
- TemplateData.BgColor is required (tags) but not set.
- ThemeColor should be a valid color.
- Add at least 192x192 and 512x512 PNG icons for manifest quality.
Apply:
data := TemplateData{ OGTagOg: og, - Robots: Robots, + Robots: Robots, Twitter: twitter, - ThemeColor: ThemeColor, + ThemeColor: ThemeColor, // e.g., "#ffffff" Lang: g.Tmpl.Lang, Description: Description, Canonical: g.Tmpl.SiteURL, AppleTouchIcon: g.Tmpl.LogoURL, Title: g.Tmpl.SiteName, JsonLD: NewJsonID(g.Tmpl).Render(), - Categories: []string{"one", "two"}, //@todo Fetch this! + Categories: []string{"one", "two"}, //@todo Fetch this! + BgColor: "#ffffff", HrefLang: []HrefLangData{ {Lang: g.Tmpl.Lang, Href: g.Tmpl.SiteURL}, }, Favicons: []FaviconData{ { - Rel: "icon", - Sizes: "48x48", - Type: "image/ico", - Href: g.Tmpl.SiteURL + "/favicon.ico", + Rel: "icon", + Sizes: "192x192", + Type: "image/png", + Href: g.Tmpl.SiteURL + "/icons/icon-192.png", + }, + { + Rel: "icon", + Sizes: "512x512", + Type: "image/png", + Href: g.Tmpl.SiteURL + "/icons/icon-512.png", }, }, }
137-184: Generate() builds data but discards it.Consider returning the assembled TemplateData (and/or writing output) to make this method useful to callers.
45-59: Minor: naming consistency for URL fields.Constants use Url while Template uses LogoURL/WebRepoURL/APIRepoURL. Consider consistent “URL”.
66-73: Router field on Generator is unused here.If not used elsewhere, remove to keep Generator focused.
metal/router/fixture.go (2)
29-56: Mutable receiver state makes sequencing order-sensitive.resolveFor mutates the same Fixture instance; it works but is easy to misuse and not concurrency-safe. Prefer pure helpers that return paths without side effects.
Example:
-func (f *Fixture) resolveFor(slug string) *Fixture { - f.fullPath = f.getFileFor(slug) - f.file = slug - return f -} +func (f *Fixture) resolveFor(slug string) *Fixture { + f.fullPath = f.getFileFor(slug) + f.file = slug + return f +} + +// Consider adding: +func (f Fixture) FileFor(slug string) (route, fullPath string) { + return slug, f.getFileFor(slug) +}Then adapt callers to use FileFor.
38-40: Parity: consider GetSocialFile().Other fixtures expose a File() helper; Social does not.
+func (f *Fixture) GetSocialFile() string { + return f.resolveFor(FixtureSocial).fullPath +}metal/router/router.go (2)
51-60: HTTP verb for index should be GET.Listing posts on POST is unexpected and likely a mistake.
Apply:
- r.Mux.HandleFunc("POST /posts", index) + r.Mux.HandleFunc("GET /posts", index)
176-197: Nil safety: ensure Router.Mux is initialized before route registration.If r.Mux can be nil here, handlers will panic on use.
Consider constructing Router with a non-nil ServeMux or guarding with a panic-friendly check early in setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.env.example(2 hunks).env.gh.example(0 hunks).env.prod.example(0 hunks).gitignore(1 hunks)Makefile(2 hunks)docker-compose.yml(1 hunks)handler/keep_alive.go(1 hunks)handler/keep_alive_db.go(1 hunks)handler/keep_alive_db_test.go(1 hunks)handler/keep_alive_test.go(1 hunks)metal/cli/accounts/handler.go(4 hunks)metal/cli/main.go(3 hunks)metal/cli/panel/menu.go(1 hunks)metal/cli/seo/data.go(1 hunks)metal/cli/seo/generator.go(1 hunks)metal/cli/seo/jsonld.go(1 hunks)metal/cli/seo/manifest.go(1 hunks)metal/cli/seo/stub.html(1 hunks)metal/cli/seo/support.go(1 hunks)metal/cli/seo/web.go(1 hunks)metal/env/app.go(2 hunks)metal/env/env.go(1 hunks)metal/env/ping.go(1 hunks)metal/env/seo.go(1 hunks)metal/kernel/app.go(2 hunks)metal/kernel/factory.go(4 hunks)metal/kernel/helpers.go(1 hunks)metal/kernel/kernel_test.go(3 hunks)metal/kernel/router.go(0 hunks)metal/makefile/app.mk(2 hunks)metal/makefile/build.mk(1 hunks)metal/makefile/db.mk(2 hunks)metal/router/fixture.go(1 hunks)metal/router/router.go(1 hunks)metal/router/router_keep_alive_db_test.go(2 hunks)metal/router/router_keep_alive_test.go(2 hunks)metal/router/router_signature_test.go(2 hunks)metal/router/web.go(1 hunks)pkg/middleware/mwguards/valid_timestamp_guard.go(1 hunks)pkg/middleware/pipeline.go(1 hunks)pkg/middleware/public_middleware.go(1 hunks)pkg/middleware/token_middleware.go(3 hunks)
💤 Files with no reviewable changes (3)
- .env.gh.example
- metal/kernel/router.go
- .env.prod.example
🧰 Additional context used
🧬 Code graph analysis (28)
handler/keep_alive_db.go (3)
metal/env/ping.go (1)
PingEnvironment(5-8)database/connection.go (1)
Connection(12-17)metal/kernel/router.go (1)
r(93-101)
pkg/middleware/pipeline.go (1)
pkg/auth/handler.go (1)
TokenHandler(10-14)
metal/kernel/helpers.go (3)
metal/kernel/app.go (1)
App(17-24)metal/router/router.go (1)
Router(16-25)metal/kernel/router.go (4)
r(127-129)Env(25-31)r(111-113)r(33-40)
metal/env/seo.go (3)
metal/kernel/router.go (1)
Env(25-31)metal/env/logs.go (1)
Level(3-7)metal/env/network.go (1)
HttpHost(3-8)
metal/kernel/kernel_test.go (3)
metal/router/router.go (1)
Router(16-25)metal/kernel/router.go (4)
r(127-129)r(111-113)Env(25-31)r(119-121)metal/kernel/router_signature_test.go (1)
TestSignatureRoute_PublicMiddleware(15-44)
handler/keep_alive.go (2)
metal/env/ping.go (1)
PingEnvironment(5-8)metal/kernel/router.go (1)
r(93-101)
metal/router/router_keep_alive_db_test.go (4)
metal/env/env.go (1)
Environment(9-17)metal/env/ping.go (1)
PingEnvironment(5-8)metal/kernel/router_keep_alive_test.go (1)
TestKeepAliveRoute(12-39)metal/kernel/router.go (3)
Env(25-31)r(93-101)r(83-91)
pkg/middleware/public_middleware.go (1)
pkg/middleware/public_middleware_test.go (1)
TestPublicMiddleware_IPWhitelist(111-152)
handler/keep_alive_test.go (3)
metal/env/ping.go (1)
PingEnvironment(5-8)metal/kernel/router_keep_alive_test.go (1)
TestKeepAliveRoute(12-39)metal/kernel/router_keep_alive_db_test.go (1)
TestKeepAliveDBRoute(13-42)
metal/router/router_keep_alive_test.go (4)
metal/env/env.go (1)
Environment(9-17)metal/env/ping.go (1)
PingEnvironment(5-8)metal/kernel/router_keep_alive_db_test.go (1)
TestKeepAliveDBRoute(13-42)metal/kernel/router.go (2)
Env(25-31)r(93-101)
pkg/middleware/mwguards/valid_timestamp_guard.go (2)
pkg/middleware/mwguards/mw_response_messages.go (2)
TimestampTooOldError(101-112)TimestampTooNewError(114-125)pkg/middleware/mwguards/valid_timestamp_guard_test.go (2)
TestValidate_TooOldTimestamp(40-49)TestValidate_FutureWithinSkew_Behavior(51-68)
metal/router/router_signature_test.go (2)
pkg/portal/validator.go (2)
Validator(13-16)GetDefaultValidator(23-33)metal/kernel/router.go (4)
r(127-129)r(76-81)Env(25-31)r(33-40)
metal/cli/seo/manifest.go (3)
metal/cli/seo/web.go (12)
Description(26-26)ThemeColor(28-28)WebHomeUrl(12-12)WebHomeName(11-11)WebPostUrl(15-15)WebPostName(14-14)WebProjectsUrl(21-21)WebProjectsName(20-20)WebAboutUrl(18-18)WebAboutName(17-17)WebResumeUrl(24-24)WebResumeName(23-23)metal/cli/seo/generator.go (1)
Template(22-34)metal/cli/seo/data.go (1)
TemplateData(12-28)
metal/cli/seo/generator.go (17)
metal/cli/seo/web.go (8)
AboutPhotoUrl(9-9)LogoUrl(8-8)RepoWebUrl(6-6)RepoApiUrl(5-5)GocantoUrl(4-4)Robots(27-27)ThemeColor(28-28)Description(26-26)metal/env/env.go (1)
Environment(9-17)pkg/portal/validator.go (1)
Validator(13-16)database/connection.go (1)
Connection(12-17)metal/router/web.go (3)
WebsiteRoutes(31-38)NewWebsiteRoutes(40-49)StaticRouteResource(15-17)metal/router/router.go (1)
Router(16-25)metal/router/fixture.go (4)
FixtureProfile(9-9)Fixture(15-20)FixtureTalks(7-7)FixtureProjects(10-10)handler/profile.go (1)
MakeProfileHandler(16-20)handler/talks.go (1)
MakeTalksHandler(16-20)handler/projects.go (1)
MakeProjectsHandler(16-20)handler/payload/talks.go (1)
TalksResponse(3-6)handler/payload/profile.go (1)
ProfileResponse(3-6)handler/payload/projects.go (1)
ProjectsResponse(3-6)metal/cli/seo/support.go (1)
Fetch(19-34)metal/cli/seo/data.go (5)
TagOgData(30-38)TwitterData(40-44)TemplateData(12-28)HrefLangData(46-49)FaviconData(51-56)metal/cli/seo/jsonld.go (1)
NewJsonID(27-41)metal/cli/seo/manifest.go (2)
NewManifest(40-93)Manifest(10-23)
metal/env/app.go (2)
metal/kernel/router.go (1)
Env(25-31)metal/env/env_test.go (1)
TestAppEnvironmentChecks(43-67)
handler/keep_alive_db_test.go (1)
metal/env/ping.go (1)
PingEnvironment(5-8)
metal/cli/seo/data.go (2)
metal/cli/seo/web.go (3)
Description(26-26)Robots(27-27)ThemeColor(28-28)metal/cli/seo/manifest.go (1)
Manifest(10-23)
pkg/middleware/token_middleware.go (5)
metal/env/env.go (1)
Environment(9-17)metal/kernel/app.go (1)
App(17-24)pkg/middleware/token_middleware_additional_test.go (2)
TestTokenMiddlewareGuardDependencies(88-100)TestTokenMiddleware_PublicTokenMismatch(102-115)pkg/middleware/token_middleware_test.go (3)
TestTokenMiddleware_DB_Integration(199-277)TestTokenMiddleware_DB_Integration_HappyPath(280-335)TestTokenMiddlewareHandleInvalid(40-52)metal/kernel/router.go (1)
Env(25-31)
metal/cli/accounts/handler.go (7)
database/attrs.go (1)
APIKeyAttr(7-11)metal/cli/accounts/factory.go (1)
Handler(12-17)pkg/auth/schema.go (1)
Token(11-17)database/model.go (1)
APIKey(27-39)pkg/auth/encryption.go (1)
CreateSignatureFrom(80-85)metal/cli/accounts/handler_test.go (2)
TestCreateReadSignature(21-35)TestReadAccountNotFound(45-51)pkg/auth/handler.go (1)
SetupNewAccount(49-72)
metal/router/web.go (4)
pkg/http/schema.go (1)
ApiError(11-15)metal/router/fixture.go (2)
Fixture(15-20)NewFixture(22-27)metal/env/env.go (1)
Environment(9-17)metal/kernel/router.go (8)
addStaticRoute(19-23)r(111-113)r(103-105)r(119-121)r(115-117)r(107-109)r(56-65)r(123-125)
metal/cli/main.go (3)
pkg/cli/message.go (2)
Errorln(9-11)Error(5-7)metal/cli/seo/generator.go (1)
NewGenerator(45-74)pkg/portal/validator.go (1)
GetDefaultValidator(23-33)
metal/env/env.go (9)
metal/kernel/app.go (1)
App(17-24)metal/env/app.go (1)
AppEnvironment(9-14)metal/env/db.go (1)
DBEnvironment(5-14)metal/env/logs.go (1)
LogsEnvironment(3-7)metal/env/network.go (1)
NetEnvironment(3-8)pkg/portal/sentry.go (1)
Sentry(8-12)metal/env/sentry.go (1)
SentryEnvironment(3-6)metal/env/ping.go (1)
PingEnvironment(5-8)metal/env/seo.go (1)
SeoEnvironment(3-5)
metal/router/router.go (10)
metal/env/env.go (1)
Environment(9-17)pkg/middleware/pipeline.go (1)
Pipeline(10-15)database/connection.go (1)
Connection(12-17)pkg/portal/validator.go (1)
Validator(13-16)metal/router/web.go (2)
WebsiteRoutes(31-38)StaticRouteResource(15-17)pkg/http/schema.go (1)
ApiHandler(25-25)pkg/http/handler.go (1)
MakeApiHandler(9-28)pkg/middleware/token_middleware.go (1)
MakeTokenMiddleware(37-51)metal/router/fixture.go (1)
Fixture(15-20)metal/kernel/router.go (9)
r(56-65)Env(25-31)r(33-40)r(111-113)r(115-117)r(107-109)r(42-54)r(119-121)r(67-74)
metal/kernel/factory.go (6)
metal/env/env.go (2)
GetEnvVar(23-25)Environment(9-17)metal/env/logs.go (1)
LogsEnvironment(3-7)metal/env/network.go (1)
NetEnvironment(3-8)metal/env/sentry.go (1)
SentryEnvironment(3-6)metal/env/ping.go (1)
PingEnvironment(5-8)metal/env/seo.go (1)
SeoEnvironment(3-5)
metal/cli/seo/jsonld.go (2)
metal/cli/seo/web.go (1)
FoundedYear(3-3)metal/cli/seo/generator.go (1)
Template(22-34)
metal/cli/seo/support.go (1)
metal/router/web.go (1)
StaticRouteResource(15-17)
metal/router/fixture.go (4)
handler/talks_test.go (1)
TestTalksHandler(5-12)handler/file_handler_test.go (2)
make(18-24)runFileHandlerTest(26-87)metal/kernel/router.go (4)
r(119-121)r(111-113)r(123-125)addStaticRoute(19-23)handler/projects_test.go (1)
TestProjectsHandler(5-12)
metal/kernel/app.go (7)
metal/router/router.go (1)
Router(16-25)pkg/auth/handler.go (2)
MakeTokensHandler(16-26)TokenHandler(10-14)pkg/middleware/pipeline.go (1)
Pipeline(10-15)database/repository/api_keys.go (1)
ApiKeys(16-18)pkg/middleware/public_middleware.go (2)
PublicMiddleware(20-29)MakePublicMiddleware(31-42)metal/router/web.go (2)
WebsiteRoutes(31-38)NewWebsiteRoutes(40-49)metal/kernel/router.go (3)
r(127-129)Env(25-31)r(33-40)
🔇 Additional comments (17)
metal/cli/panel/menu.go (1)
92-94: LGTM; confirm wiring for options 4 and 5.Ensure "4) Generate SEO." triggers the SEO pipeline and "5) Print Timestamp." is handled in cli/main.go, plus update tests/help if any.
metal/env/app.go (1)
7-14: URL field addition looks good; add a quick validation test.The
validate:"required,url"tag is appropriate. Add a unit test asserting invalid/missing ENV_APP_URL fails validation to avoid regressions.metal/router/router_signature_test.go (2)
21-22: Validator field switch to exported looks correct.Using
Validator: portal.GetDefaultValidator()matches the new Router API.
34-38: Guard against time‑skew flakiness in public middleware.If the middleware enforces a tight timestamp window, tests may flicker. Consider injecting a fixed clock or widening the allowed skew for tests.
metal/kernel/kernel_test.go (2)
12-12: Import refactor to new router pkg — LGTM.
123-123: Type migration to router.Router — LGTM. Add ENV_APP_URL & ENV_SPA_DIR to test fixtures.metal/kernel/factory.go reads ENV_APP_URL (line 65) and ENV_SPA_DIR (line 105); .env.example defines them (.env.example:7, .env.example:38). No fixtures/fixtures entry setting these was found — add ENV_APP_URL and ENV_SPA_DIR to your test fixtures so App.URL and Seo.SpaDir validation is exercised.
docker-compose.yml (2)
83-86: env_file for api-runner — LGTM.
83-95: Verify .env contains ENV_APP_URL and ENV_SPA_DIRAutomated check produced no output; confirm ENV_APP_URL and ENV_SPA_DIR exist in .env to unblock SEO/env validation.
metal/cli/seo/stub.html (1)
40-41: JsonLD is correctly typed as htmltemplate.JS — no change required.
TemplateData.JsonLD is htmltemplate.JS in metal/cli/seo/data.go (line 19), so the JSON-LD will not be HTML-escaped.metal/env/env.go (1)
10-16: Environment schema tightening — LGTM; add test fixtures for ENV_APP_URL and ENV_SPA_DIRFactory reads ENV_APP_URL (metal/kernel/factory.go:65) and ENV_SPA_DIR (metal/kernel/factory.go:105); extend test fixtures/test setup (or CI test env) to set ENV_APP_URL and ENV_SPA_DIR so validations pass.
metal/cli/main.go (1)
147-164: LGTM: generateSEO flow is cohesive.Constructor wiring matches NewGenerator signature and GenerateHome is invoked with errors surfaced.
metal/kernel/factory.go (1)
104-107: New required var: ENV_SPA_DIR — add to env files and ensure path exists at runtime.
.env.example:38 currently hasENV_SPA_DIR=(empty). Add/populate ENV_SPA_DIR in .env and your production env files and verify the SPA directory is present/mounted at runtime.metal/makefile/db.mk (1)
18-18: Confirm removal of Ping creds from migration flags.db:seed uses DB_MIGRATE_DOCKER_ENV_FLAGS but those flags no longer pass ENV_PING_USERNAME/ENV_PING_PASSWORD; the code reads these vars (metal/kernel/factory.go) and docker-compose.yml/.env.example reference them — ensure ./.env or the host environment provides ENV_PING_USERNAME and ENV_PING_PASSWORD when running make db:seed, or re-add -e ENV_PING_USERNAME -e ENV_PING_PASSWORD to DB_MIGRATE_DOCKER_ENV_FLAGS in metal/makefile/db.mk.
metal/kernel/app.go (1)
59-67: Incorrect — no change required: Router already instantiates token middleware with env.Router.PipelineFor calls middleware.MakeTokenMiddleware(r.Env, r.Pipeline.TokenHandler, r.Pipeline.ApiKeys) and App.NewRouter already sets Pipeline.Env; leave as-is.
Likely an incorrect or invalid review comment.
metal/cli/seo/manifest.go (1)
58-89: Shortcuts rely on constants corrected in web.go.Once URLs there are fixed, these become valid manifest shortcuts. No code changes here beyond adopting those constants.
metal/router/router.go (2)
98-175: Static fixture composition looks correct and matches test expectations.Endpoints should resolve to /profile, /experience, /projects, /social, /talks, /education, /recommendations.
36-49: Token middleware receives r.Env — confirm no public auth bypass remainsToken middleware is wired with r.Env in PipelineFor; sandbox ripgrep skipped files so full verification failed. Run locally and paste results:
# search for bypass/allow-public flags rg -n -S -i -uu --hidden --glob '!vendor/**' 'bypass|skip.*auth|noauth|no-auth|ALLOW_.*PUBLIC|PUBLIC_BYPASS|non-?prod|AllowUnauthenticated|SkipAuth|allow_unauthenticated' # search for token middleware / pipeline usage rg -n -S -i -uu --hidden --glob '!vendor/**' -C3 'MakeTokenMiddleware|TokenMiddleware|MakeApiHandler|func .*PipelineFor|Pipeline\.TokenHandler|Pipeline\.ApiKeys|Pipeline\.Chain' # search for env vars referenced rg -n -S -i -uu --hidden --glob '!vendor/**' 'ENV_SPA_DIR|ENV_APP_URL|ENV_ALLOW|ALLOW_PUBLIC'
gocanto
force-pushed
the
feature/add-seo
branch
from
49284cf to
bbb7129
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
metal/cli/accounts/handler_test.go (1)
115-133: Inconsistency: main.go still calls ReadAccount instead of ShowAccount.The test has been updated to use
ShowAccount, but according to the code snippet frommetal/cli/main.go, theshowApiAccountfunction still callshandler.ReadAccount(account)on line 128.This mismatch will cause a compilation error. The main.go file needs to be updated to use the renamed method:
- if err = handler.ReadAccount(account); err != nil { + if err = handler.ShowAccount(account); err != nil {
🧹 Nitpick comments (18)
.env.example (3)
5-7: Satisfy dotenv-linter: reorder and unquote date format.Addresses UnorderedKey and QuoteCharacter warnings.
Apply this diff:
-ENV_APP_LOGS_DIR="./storage/logs/logs_%s.log" -ENV_APP_LOGS_DATE_FORMAT="2006-01-02" +ENV_APP_LOGS_DATE_FORMAT=2006-01-02 +ENV_APP_LOGS_DIR="./storage/logs/logs_%s.log"
7-7: Add example and guidance for ENV_APP_URL (avoid trailing slash).Improves DX and reduces ambiguity for SEO link generation.
Apply this diff:
-ENV_APP_URL= +# Base URL for absolute links (no trailing slash), e.g. http://localhost:8080 +ENV_APP_URL=http://localhost:8080Please verify the code trims/validates trailing slashes when reading this var to avoid double slashes in generated URLs.
37-38: Document expected path for ENV_SPA_DIR and provide example.Clarifies how SEO locates SPA assets.
Apply this diff:
-# --- SEO: SPA application directory -ENV_SPA_DIR= +# --- SEO: SPA application directory +# Absolute or repo-relative path to built SPA assets used for SEO (e.g., ./spa/dist) +ENV_SPA_DIR=./spa/distConfirm the runtime expects this directory to exist and handles missing/invalid paths with a clear error.
metal/cli/seo/jsonld.go (1)
38-39: Consider making the Now function configurable for testing.The
Nowfield is initialized with a function that always returnstime.Now().UTC(). This makes testing time-dependent behavior difficult.Consider accepting the time function as a parameter or providing a default that can be overridden:
-func NewJsonID(tmpl Page) *JsonID { +func NewJsonID(tmpl Page, opts ...func(*JsonID)) *JsonID { 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", FoundedYear), Now: func() time.Time { return time.Now().UTC() }, + } + + for _, opt := range opts { + opt(j) } + + return j }metal/cli/seo/generator.go (4)
65-95: Consider extracting resource factory functions.The resource initialization logic could be extracted into a separate method for better organization and testability.
Consider extracting the resource map creation:
+func (g *Generator) createResourceHandlers() map[string]func() router.StaticRouteResource { + web := g.WebsiteRoutes + resource := make(map[string]func() router.StaticRouteResource) + + resource[router.FixtureProfile] = func() router.StaticRouteResource { + return handler.MakeProfileHandler(web.Fixture.GetProfileFile()) + } + + resource[router.FixtureTalks] = func() router.StaticRouteResource { + return handler.MakeTalksHandler(web.Fixture.GetTalksFile()) + } + + resource[router.FixtureProjects] = func() router.StaticRouteResource { + return handler.MakeProjectsHandler(web.Fixture.GetProjectsFile()) + } + + return resource +} func (g *Generator) GenerateHome() error { var err error - web := g.WebsiteRoutes - resource := make(map[string]func() router.StaticRouteResource) - - resource[router.FixtureProfile] = func() router.StaticRouteResource { - return handler.MakeProfileHandler(web.Fixture.GetProfileFile()) - } - - resource[router.FixtureTalks] = func() router.StaticRouteResource { - return handler.MakeTalksHandler(web.Fixture.GetTalksFile()) - } - - resource[router.FixtureProjects] = func() router.StaticRouteResource { - return handler.MakeProjectsHandler(web.Fixture.GetProjectsFile()) - } + resource := g.createResourceHandlers()
127-127: Consider using structured logging instead of fmt.Println.Using
fmt.Printlnfor status messages is not ideal for production code. Consider using a structured logger that respects log levels.Consider using a logger from your environment or a logging package:
- fmt.Println("Home: Done.") + // Consider using a logger from g.Env or a logging package + // log.Info("SEO generation completed", "page", "home")
177-191: Consider consolidating validation error handling.The validation error messages are repetitive and could be consolidated for better maintainability.
Consider creating a helper method:
+func (g *Generator) validateWithContext(data interface{}, context string) error { + if _, err := g.Validator.Rejects(data); err != nil { + return fmt.Errorf("invalid %s: %s", context, g.Validator.GetErrorsAsJson()) + } + return nil +} // Then use it like: - if _, err := g.Validator.Rejects(og); err != nil { - return TemplateData{}, fmt.Errorf("invalid og data: %s", g.Validator.GetErrorsAsJson()) - } + if err := g.validateWithContext(og, "og data"); err != nil { + return TemplateData{}, err + }
161-161: Fetch categories from DB (replace hardcoded list)Replace the hardcoded
Categories: []string{"one","two"}in metal/cli/seo/generator.go:161 with a DB-backed fetch using repository.Categories{DB: g.DB}.GetAll(...) (see database/repository/categories.go) and map the returned database.Category items to a []string (e.g., use Slug or Name) to populate TemplateData.Categories. I can implement this change or open an issue to track it.metal/cli/accounts/handler.go (2)
28-30: Wrap returned error with %wUse %w to preserve the original error.
- if err = h.print(token, item); err != nil { - return fmt.Errorf("could not decode the given account [%s] keys: %v", item.AccountName, err) - } + if err = h.print(token, item); err != nil { + return fmt.Errorf("could not decode the given account [%s] keys: %w", item.AccountName, err) + }
55-77: print: simplify API and avoid double work; beware of printing secrets
- The function re-decodes tokens; the token param is redundant. Drop it and accept only item; adjust callers.
- You’re printing plaintext Secret Key. If this output can end up in logs/CI, consider gating behind an explicit flag.
Proposed signature change:
-func (h Handler) print(token *auth.Token, item *database.APIKey) error { +func (h Handler) print(item *database.APIKey) error { - token, err := h.TokenHandler.DecodeTokensFor( + token, err := h.TokenHandler.DecodeTokensFor( item.AccountName, item.SecretKey, item.PublicKey, )Then update call sites accordingly:
- if err = h.print(token, item); err != nil { ... } + if err = h.print(item); err != nil { ... }metal/cli/seo/defaults.go (2)
5-10: Prefer Go initialism style for URL/API namesUse URL/API capitalization: e.g., GocantoURL, RepoAPIURL, RepoWebURL, LogoURL, AboutPhotoURL.
This is a breaking rename; ensure all references are updated across seo package before applying.
25-31: Consider environment-driven Robots/ThemeColor for non‑prodRobots “index,follow” and theme color could vary by env (e.g., noindex in staging).
Are ENV_APP_URL/ENV_SPA_DIR (per PR checklist) intended to influence these defaults? If yes, wire them through SeoEnvironment and use here instead of hard-coded constants.
metal/cli/seo/data.go (4)
15-16: Use BCP 47 language tags (“en-GB”), not “en_GB”Validator will enforce the current (non-standard) form. Prefer “en-GB”.
- Lang string `validate:"required,oneof=en_GB"` + Lang string `validate:"required,oneof=en-GB"` - StubPath string `validate:"required,oneof=stub.html"` + StubPath string `validate:"required,oneof=stub.html"`
19-35: Align TemplateData.Lang with BCP 47 and Page.LangKeep TemplateData.Lang consistent with Page.Lang (“en-GB”).
- Lang string `validate:"required,oneof=en_GB"` + Lang string `validate:"required,oneof=en-GB"`
53-56: Expand hreflang validation to support “en-GB”Current oneof=en is too restrictive for regional variants.
- Lang string `validate:"required,oneof=en"` + Lang string `validate:"required,oneof=en en-GB"`
58-63: Rel type too restrictive for faviconsAllow common rel values like apple-touch-icon.
- Rel string `validate:"required,oneof=icon"` + Rel string `validate:"required,oneof=icon apple-touch-icon"`metal/cli/seo/manifest.go (1)
50-88: Confirm StartURL/Scope semanticsStartURL is absolute (tmpl.SiteURL) while Scope is “/”. Many UAs expect StartURL relative to scope. Consider StartURL = WebHomeUrl, and keep absolute URLs for Canonical elsewhere.
- StartURL: tmpl.SiteURL, + StartURL: WebHomeUrl,metal/router/router.go (1)
180-186: Use PublicPipelineFor for static resourcesServe static fixtures without token auth.
func addStaticRoute[H StaticRouteResource](r *Router, route, fixture string, maker func(string) H) { abstract := maker(fixture) - resolver := r.PipelineFor(abstract.Handle) + resolver := r.PublicPipelineFor(abstract.Handle) route = strings.TrimLeft(route, "/") r.Mux.HandleFunc("GET /"+route, resolver) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.env.example(2 hunks)metal/cli/accounts/handler.go(4 hunks)metal/cli/accounts/handler_test.go(2 hunks)metal/cli/seo/data.go(1 hunks)metal/cli/seo/defaults.go(1 hunks)metal/cli/seo/generator.go(1 hunks)metal/cli/seo/jsonld.go(1 hunks)metal/cli/seo/manifest.go(1 hunks)metal/cli/seo/stub.html(1 hunks)metal/cli/seo/support.go(1 hunks)metal/env/app.go(2 hunks)metal/router/router.go(5 hunks)metal/router/static.go(1 hunks)pkg/middleware/token_middleware.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/middleware/token_middleware.go
- metal/env/app.go
- metal/cli/seo/stub.html
🧰 Additional context used
🧬 Code graph analysis (9)
metal/cli/seo/jsonld.go (2)
metal/cli/seo/defaults.go (1)
FoundedYear(27-27)metal/cli/seo/data.go (1)
Page(5-17)
metal/cli/accounts/handler.go (7)
database/attrs.go (1)
APIKeyAttr(7-11)metal/cli/accounts/factory.go (1)
Handler(12-17)pkg/auth/schema.go (1)
Token(11-17)database/model.go (1)
APIKey(27-39)pkg/auth/encryption.go (1)
CreateSignatureFrom(80-85)metal/cli/main.go (3)
showApiAccount(116-134)generateApiAccountsHTTPSignature(136-154)createNewApiAccount(96-114)pkg/auth/handler.go (1)
SetupNewAccount(49-72)
metal/cli/seo/support.go (1)
metal/router/static.go (1)
StaticRouteResource(10-12)
metal/cli/accounts/handler_test.go (1)
metal/cli/main.go (1)
showApiAccount(116-134)
metal/cli/seo/manifest.go (2)
metal/cli/seo/defaults.go (10)
Description(31-31)ThemeColor(29-29)WebHomeUrl(13-13)WebHomeName(14-14)WebProjectsUrl(23-23)WebProjectsName(22-22)WebAboutUrl(17-17)WebAboutName(16-16)WebResumeUrl(20-20)WebResumeName(19-19)metal/cli/seo/data.go (2)
Page(5-17)TemplateData(19-35)
metal/router/static.go (4)
pkg/http/schema.go (1)
ApiError(11-15)metal/router/fixture.go (2)
Fixture(15-20)NewFixture(22-27)metal/env/env.go (1)
Environment(9-17)metal/kernel/router.go (9)
addStaticRoute(19-23)r(103-105)r(119-121)r(107-109)r(115-117)r(111-113)Handle(15-17)r(127-129)r(123-125)
metal/cli/seo/data.go (2)
metal/cli/seo/defaults.go (5)
AboutPhotoUrl(9-9)StubPath(28-28)Description(31-31)Robots(30-30)ThemeColor(29-29)metal/cli/seo/manifest.go (1)
Manifest(10-23)
metal/cli/seo/generator.go (5)
metal/cli/seo/data.go (6)
Page(5-17)TemplateData(19-35)TagOgData(37-45)TwitterData(47-51)HrefLangData(53-56)FaviconData(58-63)pkg/portal/validator.go (1)
Validator(13-16)metal/router/static.go (3)
WebsiteRoutes(14-20)NewWebsiteRoutes(22-30)StaticRouteResource(10-12)metal/cli/seo/defaults.go (9)
LogoUrl(8-8)StubPath(28-28)RepoWebUrl(7-7)RepoApiUrl(6-6)AboutPhotoUrl(9-9)GocantoUrl(5-5)Robots(30-30)ThemeColor(29-29)Description(31-31)metal/cli/seo/support.go (2)
Fetch(12-27)ManifestDataURL(29-34)
metal/router/router.go (13)
metal/router/static.go (2)
WebsiteRoutes(14-20)StaticRouteResource(10-12)metal/env/env.go (1)
Environment(9-17)pkg/portal/validator.go (1)
Validator(13-16)pkg/middleware/pipeline.go (1)
Pipeline(10-15)handler/signatures.go (1)
MakeSignaturesHandler(24-29)handler/profile.go (1)
MakeProfileHandler(16-20)metal/router/fixture.go (1)
Fixture(15-20)handler/experience.go (1)
MakeExperienceHandler(16-20)handler/projects.go (1)
MakeProjectsHandler(16-20)handler/social.go (1)
MakeSocialHandler(16-20)handler/talks.go (1)
MakeTalksHandler(16-20)handler/education.go (1)
MakeEducationHandler(16-20)handler/recommendations.go (1)
MakeRecommendationsHandler(16-20)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 6-6: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 6-6: [UnorderedKey] The ENV_APP_LOGS_DATE_FORMAT key should go before the ENV_APP_LOGS_DIR key
(UnorderedKey)
🔇 Additional comments (12)
metal/cli/seo/support.go (1)
22-24: Bug: json.Unmarshal uses &response (double pointer).metal/router/static.go (1)
1-31: LGTM! Clean interface and struct design.The
StaticRouteResourceinterface andWebsiteRoutesstruct are well-designed with clear responsibilities. The constructor properly initializes all fields from the environment.metal/cli/seo/jsonld.go (1)
137-141: Security: remove enc.SetEscapeHTML(false)metal/cli/seo/generator.go (2)
97-106: LGTM! Data struct is correctly populated after fetches.The data struct is properly constructed after the Fetch calls with the populated responses. This ensures the template receives the loaded data correctly.
118-125: LGTM! OutputDir is correctly treated as a directory.The code properly creates the
OutputDirdirectory and writesindex.htmlwithin it, addressing the previous concern about treating the directory path correctly.metal/cli/accounts/handler.go (3)
18-22: Persisting APIKey using repository looks goodStoring encrypted keys via APIKeyAttr is correct.
48-50: Capture and return h.print error; avoid stale err and redundant decodeYou’re returning a possibly stale err and decoding twice. Remove the pre-decode and return the print error directly.
- token, err := h.TokenHandler.DecodeTokensFor( - item.AccountName, - item.SecretKey, - item.PublicKey, - ) - - if h.print(token, item) != nil { - return fmt.Errorf("could not decode the given account [%s] keys: %v", item.AccountName, err) - } + if perr := h.print(nil, item); perr != nil { + return perr + }Also applies to: 42-47
35-35: Method renamed to ShowAccount — verify & update call sitesI ran a repo search for "ReadAccount(" and found no matches; confirm there are no remaining references (including receiver calls, method expressions, generated or vendor files).
metal/cli/seo/manifest.go (1)
41-48: Good guard against out-of-range favicon accessUsing the first favicon with a safe fallback fixes the panic risk. LGTM.
metal/router/router.go (3)
17-22: Ensure non-nil Router deps at constructionWebsiteRoutes, Env, Validator, Mux, Pipeline, Db must be set; otherwise handlers will panic at runtime.
Would you like a simple constructor that validates these and returns an error if any are nil?
35-39: Token middleware now env-aware: LGTMPassing r.Env into MakeTokenMiddleware aligns with the env-aware bypasses.
70-74: Signatures handler uses public Validator: LGTMThis matches the Router field change and handler.MakeSignaturesHandler signature.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
metal/cli/seo/data.go (3)
21-21: Relax overly strict min lengths to avoid false negatives.Typical site names (e.g., “Oullin”) will fail
min=10, causing unnecessary build failures for Title/ImageAlt.- Title string `validate:"required,min=10"` + Title string `validate:"required,min=3"` ... - ImageAlt string `validate:"required,min=10"` + ImageAlt string `validate:"required,min=5"` ... - ImageAlt string `validate:"required,min=10"` + ImageAlt string `validate:"required,min=5"`Also applies to: 41-41, 51-51
9-9: Validate eachSameAsURLelement, not just the slice.Ensure every entry is a valid URI.
- SameAsURL []string `validate:"required"` + SameAsURL []string `validate:"required,dive,uri"`
14-14: Name consistency: useURLacronym casing.Consider renaming
AboutPhotoUrl→AboutPhotoURLfor consistency with other fields (LogoURL,SiteURL).Note: This is API‑breaking; if you want, I can generate a follow‑up PR with the refactor and call‑site updates across the repo.
metal/cli/main.go (1)
64-69: Preflight-check required env vars before SEO generation.Fail fast if
ENV_APP_URLorENV_SPA_DIRis unset; current flow will error later via validation with a less actionable message.Apply this diff inside
generateSEO():func generateSEO() error { - gen, err := seo.NewGenerator( + // Preflight checks for required env values + if environment.App.URL == "" { + return fmt.Errorf("ENV_APP_URL is not set") + } + if environment.Seo.SpaDir == "" { + return fmt.Errorf("ENV_SPA_DIR is not set") + } + + gen, err := seo.NewGenerator( dbConn, environment, portal.GetDefaultValidator(), )Also ensure your
.env(.example)has sane defaults for both values.metal/cli/seo/client.go (4)
67-75: Avoid shadowing the importedhandlerpackage infetch.Parameter name
handlershadows the imported package name; rename for clarity.-func fetch[T any](response *T, handler func() router.StaticRouteResource) error { +func fetch[T any](response *T, factory func() router.StaticRouteResource) error { req := httptest.NewRequest("GET", "http://localhost:8080/proxy", nil) rr := httptest.NewRecorder() - maker := handler() + maker := factory()
33-34: Unify error message (remove stray "home:" prefix).Keep error prefixes consistent across methods.
- return nil, fmt.Errorf("home: error fetching talks: %w", err) + return nil, fmt.Errorf("error fetching talks: %w", err)
13-22: Drop unusedWebsiteRoutesfrom Client to reduce coupling.Field isn’t used; keep only what you need.
type Client struct { - WebsiteRoutes *router.WebsiteRoutes Fixture router.Fixture } func NewClient(routes *router.WebsiteRoutes) *Client { return &Client{ - WebsiteRoutes: routes, Fixture: routes.Fixture, } }
73-79: Check HTTP status code before unmarshalling.If a handler writes a non‑2xx status without returning an error,
Unmarshalmay mislead. Guard on status code.if err := maker.Handle(rr, req); err != nil { return err } + if rr.Code < 200 || rr.Code > 299 { + return fmt.Errorf("unexpected status code %d: %s", rr.Code, rr.Body.String()) + } + if err := json.Unmarshal(rr.Body.Bytes(), response); err != nil { return err }metal/cli/seo/generator.go (1)
195-203: Consider standard favicon type.Many UAs expect
image/x-iconfor.ico. Optional tweak.- Type: "image/ico", + Type: "image/x-icon",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
metal/cli/main.go(3 hunks)metal/cli/seo/client.go(1 hunks)metal/cli/seo/data.go(1 hunks)metal/cli/seo/generator.go(1 hunks)metal/cli/seo/stub.html(1 hunks)metal/cli/seo/support.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- metal/cli/seo/stub.html
- metal/cli/seo/support.go
🧰 Additional context used
🧬 Code graph analysis (4)
metal/cli/seo/client.go (8)
metal/router/static.go (2)
WebsiteRoutes(14-20)StaticRouteResource(10-12)metal/router/fixture.go (1)
Fixture(15-20)handler/payload/talks.go (1)
TalksResponse(3-6)handler/talks.go (1)
MakeTalksHandler(16-20)handler/payload/profile.go (1)
ProfileResponse(3-6)handler/profile.go (1)
MakeProfileHandler(16-20)handler/payload/projects.go (1)
ProjectsResponse(3-6)handler/projects.go (1)
MakeProjectsHandler(16-20)
metal/cli/main.go (3)
pkg/cli/message.go (2)
Errorln(9-11)Error(5-7)metal/cli/seo/generator.go (1)
NewGenerator(32-67)pkg/portal/validator.go (1)
GetDefaultValidator(23-33)
metal/cli/seo/data.go (2)
metal/cli/seo/defaults.go (5)
AboutPhotoUrl(9-9)StubPath(28-28)Description(31-31)Robots(30-30)ThemeColor(29-29)metal/cli/seo/manifest.go (1)
Manifest(10-23)
metal/cli/seo/generator.go (14)
metal/cli/seo/data.go (6)
Page(5-17)TemplateData(19-36)TagOgData(38-46)TwitterData(48-52)HrefLangData(54-57)FaviconData(59-64)metal/cli/seo/client.go (2)
Client(13-16)NewClient(18-23)metal/env/env.go (1)
Environment(9-17)pkg/portal/validator.go (1)
Validator(13-16)database/connection.go (1)
Connection(12-17)metal/router/static.go (2)
WebsiteRoutes(14-20)NewWebsiteRoutes(22-30)metal/cli/seo/defaults.go (9)
LogoUrl(8-8)StubPath(28-28)RepoWebUrl(7-7)RepoApiUrl(6-6)AboutPhotoUrl(9-9)GocantoUrl(5-5)Robots(30-30)ThemeColor(29-29)Description(31-31)handler/payload/talks.go (1)
TalksResponse(3-6)handler/payload/profile.go (1)
ProfileResponse(3-6)handler/payload/projects.go (1)
ProjectsResponse(3-6)pkg/cli/message.go (1)
Successln(17-19)metal/cli/seo/jsonld.go (1)
NewJsonID(27-41)metal/cli/seo/manifest.go (2)
Manifest(10-23)NewManifest(40-91)metal/cli/seo/support.go (1)
ManifestDataURL(8-13)
🔇 Additional comments (4)
metal/cli/main.go (2)
140-140: Rename ripple is correct.
ShowAccountcall site is updated and aligns with the handler refactor.
165-194: LGTM on the timestamp utility.Self-contained and harmless; good as a CLI helper.
metal/cli/seo/generator.go (2)
150-158: Correct handling ofOutputDir(write index.html inside).Good fix: you now create the directory and write to
<OutputDir>/index.html, addressing the earlier issue.
208-218: Validation sequence looks good (duplication removed).Only one validation for
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
metal/cli/seo/generator.go (1)
111-116: Consider logging the export target for easier troubleshooting.After
Exportsucceeds, log the absolute output path (or URL) so operators immediately know where the generated file was written. That small addition pays for itself when multiple pages are emitted.metal/cli/seo/data.go (1)
6-16: Adddiveto slice validations.All slice fields (
SameAsURL,HrefLang,Favicons,Categories,Body) currently only check that the slice itself isn’t empty. Adddiveso each element is validated too; otherwise malformed URLs or empty strings slip through unseen.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
metal/cli/seo/data.go(1 hunks)metal/cli/seo/generator.go(1 hunks)metal/cli/seo/sections.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
metal/cli/seo/data.go (4)
metal/cli/seo/defaults.go (5)
AboutPhotoUrl(9-9)StubPath(28-28)Description(31-31)Robots(30-30)ThemeColor(29-29)metal/cli/seo/manifest.go (1)
Manifest(10-23)database/repository/categories.go (1)
Categories(12-14)pkg/markdown/schema.go (2)
Title(11-19)FrontMatter(21-26)
metal/cli/seo/generator.go (14)
metal/cli/seo/data.go (6)
Page(5-17)TemplateData(19-36)TagOgData(38-46)TwitterData(48-52)HrefLangData(54-57)FaviconData(59-64)metal/cli/seo/client.go (2)
Client(13-16)NewClient(18-23)metal/env/env.go (1)
Environment(9-17)pkg/portal/validator.go (1)
Validator(13-16)metal/router/static.go (2)
WebsiteRoutes(14-20)NewWebsiteRoutes(22-30)metal/cli/seo/defaults.go (9)
LogoUrl(8-8)StubPath(28-28)RepoWebUrl(7-7)RepoApiUrl(6-6)AboutPhotoUrl(9-9)GocantoUrl(5-5)Robots(30-30)ThemeColor(29-29)Description(31-31)handler/payload/talks.go (1)
TalksResponse(3-6)handler/payload/profile.go (1)
ProfileResponse(3-6)handler/payload/projects.go (1)
ProjectsResponse(3-6)metal/cli/seo/sections.go (1)
NewSections(12-14)pkg/cli/message.go (1)
Successln(17-19)metal/cli/seo/jsonld.go (1)
NewJsonID(27-41)metal/cli/seo/manifest.go (2)
Manifest(10-23)NewManifest(40-91)metal/cli/seo/support.go (1)
ManifestDataURL(8-13)
metal/cli/seo/sections.go (3)
handler/payload/profile.go (1)
ProfileResponse(3-6)handler/payload/talks.go (1)
TalksResponse(3-6)handler/payload/projects.go (1)
ProjectsResponse(3-6)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
metal/makefile/build.mk (1)
10-13: Use$(MAKE)for recursive invocations.Hard-coding
makedrops jobserver flags andMAKEFLAGSpropagation; splitting the commands also removes the brittle&&chaining.-build-fresh: - make fresh && \ - make db:fresh && \ - make db:migrate && \ - make db:seed +build-fresh: + $(MAKE) fresh + $(MAKE) db:fresh + $(MAKE) db:migrate + $(MAKE) db:seedpkg/middleware/pipeline.go (1)
13-13: Use Go’s deprecation convention for TokenHandler fieldAdopt a “Deprecated:” doc comment to surface in docs/linters.
Apply:
- TokenHandler *auth.TokenHandler //@todo Remove! + // Deprecated: remove after migrating token flow off Pipeline. Use token middleware wiring via env. + TokenHandler *auth.TokenHandlermetal/makefile/db.mk (1)
59-61: Nice: compose now pulls from .env; consider making the path configurableOptional: promote ./.env to a variable (e.g., ENV_FILE ?= ./.env) used across make targets to avoid drift.
+ENV_FILE ?= ./.env ... - docker compose --env-file ./.env run --rm $(DB_MIGRATE_DOCKER_ENV_FLAGS) $(DB_API_RUNNER_SERVICE) \ + docker compose --env-file $(ENV_FILE) run --rm $(DB_MIGRATE_DOCKER_ENV_FLAGS) $(DB_API_RUNNER_SERVICE) \ go run ./database/seeder/main.gometal/kernel/helpers.go (1)
11-12: Consider accepting a pointer to avoid an extra copy.
SetRouterultimately stores a*router.Router, so taking a pointer parameter would skip the intermediate copy and signal the expected ownership a bit more clearly.metal/kernel/app.go (3)
35-39: Avoid copying Router; keep and use the pointerYou copy the router value into App and later copy it again in Boot. Use the pointer throughout to avoid unintended state divergence and reduce allocations.
Apply:
- if modem, err := app.NewRouter(); err != nil { + if modem, err := app.NewRouter(); err != nil { return nil, err - } else { - app.SetRouter(*modem) - } + } else { + app.router = modem + }
81-100: Call router methods on the pointer; don’t copy before useCopying the Router struct before invoking methods is unnecessary and can be risky if it contains mutable state.
Apply:
func (a *App) Boot() { if a == nil || a.router == nil { panic("kernel error > boot: Invalid setup") } - - modem := *a.router - - modem.KeepAlive() - modem.KeepAliveDB() - modem.Profile() - modem.Experience() - modem.Projects() - modem.Social() - modem.Talks() - modem.Education() - modem.Recommendations() - modem.Posts() - modem.Categories() - modem.Signature() + a.router.KeepAlive() + a.router.KeepAliveDB() + a.router.Profile() + a.router.Experience() + a.router.Projects() + a.router.Social() + a.router.Talks() + a.router.Education() + a.router.Recommendations() + a.router.Posts() + a.router.Categories() + a.router.Signature() }
44-76: *Change SetRouter signature to accept router.Router
SetRouter is still used in kernel_test.go (lines 125, 174) and in app.go (line 38); for API stability, update it tofunc (a *App) SetRouter(r *router.Router) { a.router = r }metal/env/env.go (1)
27-42: Remove redundant fallback branch in GetSecretOrEnvCurrent code returns the env var for any read error; the last return duplicates the NotExist branch. Simplify without changing behavior.
Apply:
func GetSecretOrEnv(secretName string, envVarName string) string { secretPath := filepath.Join(SecretsDir, secretName) // Try to read the secret file first. content, err := os.ReadFile(secretPath) if err == nil { return strings.TrimSpace(string(content)) } - // If the file does not exist, fall back to the environment variable. - if os.IsNotExist(err) { - return GetEnvVar(envVarName) // Use your existing function here - } - - return GetEnvVar(envVarName) + // Fall back to the environment variable for any error. + return GetEnvVar(envVarName) }metal/cli/seo/defaults.go (1)
5-10: Static asset defaults OK; consider future configurabilityConstants are fine for now. Later, allow overrides via env or config to avoid rebuilding for changes.
metal/cli/seo/generator.go (2)
141-200: Ensure SiteName meets min-length validation or adjust schema; externalize categories
- TemplateData.Title, TagOgData.ImageAlt and TwitterData.ImageAlt use g.Page.SiteName and require
min=10; short names (<10 chars) will fail validation. Either lower theminconstraint or generate a longer Title/ImageAlt.- data.Categories is hardcoded (
[]string{"one","two"}); replace the TODO by fetching categories from configuration or an API.
31-66: Approve: Page validation covers SiteURL (required,uri) and OutputDir (required)
val.Rejects(page)enforces non-empty & valid URI for ENV_APP_URL and non-empty ENV_SPA_DIR; directory writability must be ensured at deploy or via an explicit runtime check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
.env.example(2 hunks).env.gh.example(0 hunks).env.prod.example(0 hunks).gitignore(1 hunks)Makefile(2 hunks)docker-compose.yml(1 hunks)handler/keep_alive.go(1 hunks)handler/keep_alive_db.go(1 hunks)handler/keep_alive_db_test.go(1 hunks)handler/keep_alive_test.go(1 hunks)metal/cli/accounts/handler.go(4 hunks)metal/cli/accounts/handler_test.go(2 hunks)metal/cli/main.go(3 hunks)metal/cli/panel/menu.go(1 hunks)metal/cli/seo/client.go(1 hunks)metal/cli/seo/data.go(1 hunks)metal/cli/seo/defaults.go(1 hunks)metal/cli/seo/generator.go(1 hunks)metal/cli/seo/jsonld.go(1 hunks)metal/cli/seo/manifest.go(1 hunks)metal/cli/seo/sections.go(1 hunks)metal/cli/seo/stub.html(1 hunks)metal/cli/seo/support.go(1 hunks)metal/env/app.go(2 hunks)metal/env/env.go(1 hunks)metal/env/ping.go(1 hunks)metal/env/seo.go(1 hunks)metal/kernel/app.go(2 hunks)metal/kernel/factory.go(4 hunks)metal/kernel/helpers.go(1 hunks)metal/kernel/kernel_test.go(3 hunks)metal/makefile/app.mk(2 hunks)metal/makefile/build.mk(1 hunks)metal/makefile/db.mk(2 hunks)metal/router/fixture.go(1 hunks)metal/router/router.go(5 hunks)metal/router/router_keep_alive_db_test.go(2 hunks)metal/router/router_keep_alive_test.go(2 hunks)metal/router/router_signature_test.go(2 hunks)metal/router/static.go(1 hunks)pkg/middleware/mwguards/valid_timestamp_guard.go(1 hunks)pkg/middleware/pipeline.go(1 hunks)pkg/middleware/public_middleware.go(1 hunks)pkg/middleware/token_middleware.go(3 hunks)
💤 Files with no reviewable changes (2)
- .env.gh.example
- .env.prod.example
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 6-6: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 6-6: [UnorderedKey] The ENV_APP_LOGS_DATE_FORMAT key should go before the ENV_APP_LOGS_DIR key
(UnorderedKey)
🔇 Additional comments (39)
pkg/middleware/mwguards/valid_timestamp_guard.go (1)
51-57: Double-check new error constructors return 401.The prior implementation explicitly set
Status: http.StatusUnauthorizedwhen the timestamp was out of range. Please confirmTimestampTooOldError/TimestampTooNewErrorpreserve that status; if they default to another code, callers will regress. If they need the unauthorized status, update the constructors or override it here.metal/cli/accounts/handler_test.go (1)
28-30: Update the failure message to matchShowAccount.The method was renamed, but the failure string still says “read”.
- if err := h.ShowAccount("tester"); err != nil { - t.Fatalf("read: %v", err) + if err := h.ShowAccount("tester"); err != nil { + t.Fatalf("show: %v", err)metal/cli/seo/data.go (2)
21-21: Loosen the title/image-alt min-length to accept the default config.The generator still feeds
TemplateData.TitleandTagOgData.ImageAltwithPage.SiteName(“Oullin”), somin=10keeps rejecting the stock configuration. Please relax or drop the 10-character floor so validation passes out of the box.- Title string `validate:"required,min=10"` + Title string `validate:"required,min=5"` @@ - ImageAlt string `validate:"required,min=10"` + ImageAlt string `validate:"required,min=5"`Also applies to: 41-41
55-55: Allow both base and regional hreflang codes.
Generator.Build()still emits bothenanden_GBlinks, so restricting the validator toen_GBmakes the build fail whenever the base language entry is present. Please accept the base code (and common hyphen variant) in the tag.- Lang string `validate:"required,oneof=en_GB"` + Lang string `validate:"required,oneof=en en_GB en-GB"`metal/cli/seo/jsonld.go (1)
137-138: Restore json.Encoder’s HTML escaping on JSON-LD output.Disabling
SetEscapeHTMLwhile returningtemplate.JSreopens the</script>injection vector if any value comes from configuration. Letencoding/jsonescape HTML characters.- enc := json.NewEncoder(&buf) - enc.SetEscapeHTML(false) + enc := json.NewEncoder(&buf)metal/kernel/kernel_test.go (2)
12-12: Import switch to router package: OK
123-124: Ensure tests set public middleware headers when bypass is disabled
PublicMiddleware in production (isProduction=true) rejects requests missingportal.RequestIDHeaderandportal.TimestampHeader; tests exercising these routes must callreq.Header.Set(portal.RequestIDHeader, "<id>") req.Header.Set(portal.TimestampHeader, "<timestamp>")metal/cli/panel/menu.go (1)
92-94: Option handling for choices 4 and 5 is correctly mapped
case 4 calls generateSEO() and case 5 calls printTimestamp(), matching the menu text.metal/router/router_keep_alive_db_test.go (1)
16-16: No env.Ping references found; please manually verify that all old Ping usages are removed.metal/router/router_keep_alive_test.go (1)
14-14: PingEnvironment rename fully migrated
No remaining env.Ping references found.metal/makefile/db.mk (1)
18-18: No seeder/migrations reference ENV_PING_USERNAME or ENV_PING_PASSWORD – verified by a repo-wide search; safe to remove these vars from db.mk.handler/keep_alive_test.go (1)
16-18: Fixture reflects renamed environment type.Using
env.PingEnvironmenthere keeps the test aligned with the handler’s dependency after the rename.metal/cli/seo/support.go (1)
8-13: Helper cleanly packages manifest data.The base64 wrapping plus
template.URLcasting gives the template a safe, self-contained manifest URL..env.example (2)
6-8: App log format and URL placeholders look good.Thanks for documenting the updated date format and the new
ENV_APP_URLrequirement directly in the example file.
37-38: SPA directory placeholder captured.Adding
ENV_SPA_DIRhere makes the new SEO configuration discoverable for ops.metal/env/ping.go (1)
5-21: Rename propagates cleanly.The
PingEnvironmenttype and its helpers mirror the old behavior while making the intent explicit.Makefile (1)
57-60: CLI targets nicely expanded.The new
run-cli-dockerandrun-metaloptions make local workflows clearer in the help output.handler/keep_alive.go (1)
15-20: Handler signature matches environment rename.The struct field and constructor now consistently depend on
*env.PingEnvironment, which keeps the handler wiring straightforward.metal/router/router_signature_test.go (1)
1-22: Test update tracks router API changes.Switching to the router package and the exported
Validatorfield keeps this test up-to-date with the public surface.metal/makefile/app.mk (1)
56-63: Local CLI shortcut looks goodLine 56 wraps the secret paths so we don’t have to export them manually; thanks for the ergonomic target.
metal/cli/seo/stub.html (1)
1-53: Meta coverage is solidLines 1-53 cover the essential meta tags (canonical, hreflang, OG/Twitter) the generator needs; LGTM.
pkg/middleware/token_middleware.go (1)
55-57: Thanks for guarding local modeLine 55’s
t.env != nilcheck removes the panic risk while preserving the local bypass—good catch.handler/keep_alive_db.go (1)
16-22: PingEnvironment wiring matches the renamesLines 16-22 align the DB keep-alive handler with the new
env.PingEnvironmenttype; looks consistent.metal/router/static.go (1)
22-29: WebsiteRoutes initializer reads clearlyLines 22-29 cleanly plumb the environment through to the fixture bundle; LGTM.
metal/cli/seo/client.go (1)
67-82: Generic fetch helper is tidyLines 67-82 keep the fixture fetch logic DRY across payload types—looks good.
metal/kernel/factory.go (1)
63-144: Environment wiring covers the new SEO fields nicelyLines 63-144 consistently populate and validate the new SEO block; all good.
metal/cli/main.go (1)
147-191: Still missing ENV_SPA_DIR/ENV_APP_URL validation before SEO generationLines 147-163 still call into the generator without verifying that
ENV_SPA_DIRexists/is writable or thatENV_APP_URLis populated. In empty.envsetups (including.env.example),environment.Seo.SpaDiris"", sogen.Generate()fails later with a cryptic filesystem error instead of a clear message. The previous review already requested these checks; we still need to guard and prepare the output directory up front.Please validate the env vars and create the directory (or fail fast) before instantiating the generator. Suggested fix:
diff --git a/metal/cli/main.go b/metal/cli/main.go @@ -import ( - "fmt" - "time" +import ( + "fmt" + "os" + "strings" + "time" @@ func generateSEO() error { + if environment == nil { + return fmt.Errorf("environment not initialized") + } + + spaDir := strings.TrimSpace(environment.Seo.SpaDir) + if spaDir == "" { + return fmt.Errorf("ENV_SPA_DIR is required") + } + if err := os.MkdirAll(spaDir, 0o755); err != nil { + return fmt.Errorf("unable to prepare SPA dir %q: %w", spaDir, err) + } + + if strings.TrimSpace(environment.App.URL) == "" { + return fmt.Errorf("ENV_APP_URL is required") + } + gen, err := seo.NewGenerator( dbConn, environment, portal.GetDefaultValidator(), )metal/cli/seo/sections.go (1)
16-23: Fix the string ↔︎template.HTMLconcatenation bug.
Sections.Profilecurrently tries to add a raw string totemplate.HTML, which doesn’t compile. Build the markup as a plain string (with the escaped fields) and only then cast totemplate.HTML.-func (s *Sections) Profile(profile *payload.ProfileResponse) template.HTML { - return "<h1>Profile</h1>" + - template.HTML("<p"+ - template.HTMLEscapeString(profile.Data.Name)+", "+ - template.HTMLEscapeString(profile.Data.Profession)+ - "</p>", - ) -} +func (s *Sections) Profile(profile *payload.ProfileResponse) template.HTML { + var builder strings.Builder + builder.WriteString("<h1>Profile</h1>") + builder.WriteString("<p>") + builder.WriteString(template.HTMLEscapeString(profile.Data.Name)) + builder.WriteString(", ") + builder.WriteString(template.HTMLEscapeString(profile.Data.Profession)) + builder.WriteString("</p>") + + return template.HTML(builder.String()) +}metal/cli/accounts/handler.go (1)
48-50: Propagate the actual print error inShowAccount.
h.printmay fail for reasons unrelated to the earlier decode, but the code drops that error and reuses the staleerr(which can be nil). Captureh.print’s return value so we surface the real failure.- if h.print(token, item) != nil { - return fmt.Errorf("could not decode the given account [%s] keys: %v", item.AccountName, err) - } + if perr := h.print(token, item); perr != nil { + return fmt.Errorf("could not decode the given account [%s] keys: %v", item.AccountName, perr) + }metal/router/router.go (1)
181-185: Serve fixture endpoints via the public pipeline
These profile/experience/projects/etc. fixtures are meant to be unauthenticated, but wiring them throughPipelineForstill attaches the token middleware and keeps 401’ing public requests—the same issue flagged earlier. Switch this to the public pipeline so the SPA (and crawlers/SEO) can reach the JSON without auth.- resolver := r.PipelineFor(abstract.Handle) + resolver := r.PublicPipelineFor(abstract.Handle)metal/env/seo.go (1)
3-5: LGTM: minimal env type with required fieldSeoEnvironment is simple and correct; required tag is appropriate.
handler/keep_alive_db_test.go (1)
18-19: LGTM: updated to PingEnvironmentThe test correctly uses env.PingEnvironment and constructs the handler with a pointer.
metal/env/env.go (1)
9-17: Stronger validation posture looks goodMarking all sub-environments as required (including Seo and PingEnvironment) will surface misconfiguration early.
metal/cli/seo/generator.go (3)
96-103: Section assembly is safe and readableSwitching to sections.* returning template.HTML keeps type consistency and avoids the previous []template.HTML append issue.
120-139: Correctly treat OutputDir as a directory and write a file inside itMkdirAll on OutputDir and writing origin.seo.html resolves the earlier path misuse.
202-221: Template loading via embed.FS with func map is solidGood error wrapping; ManifestDataURL export is available to templates.
metal/cli/seo/defaults.go (1)
27-32: Meta defaults look reasonableTheme color, robots, and description are sensible defaults.
metal/cli/seo/manifest.go (2)
40-49: Good: safe icons fallback and no out-of-range indexingSelecting index 0 with a fallback to LogoURL avoids panics when only one favicon exists.
93-129: Manifest JSON rendering is robustHTML escaping disabled (correct for JSON), with compaction and sensible fallback paths. Returning template.JS is appropriate for embedding.
gocanto
force-pushed
the
feature/add-seo
branch
from
a77b259 to
c424c96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
database/repository/categories.go (1)
20-24: Add explicit ordering for deterministic SEO exportsThe results currently depend on the database’s default ordering, so the list can shuffle unexpectedly between runs. Given this powers SEO/static generation, we should return a deterministic sequence.
err := c.DB.Sql(). Model(&database.Category{}). + Order("categories.name ASC"). Where("categories.deleted_at is null"). Find(&categories).Error
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
database/repository/categories.go(1 hunks)metal/cli/seo/categories.go(1 hunks)metal/cli/seo/data.go(1 hunks)metal/cli/seo/generator.go(1 hunks)metal/cli/seo/sections.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- metal/cli/seo/sections.go
- metal/cli/seo/data.go
🧰 Additional context used
🧬 Code graph analysis (3)
metal/cli/seo/categories.go (3)
database/connection.go (1)
Connection(12-17)database/repository/categories.go (1)
Categories(13-15)database/model.go (1)
Category(106-118)
metal/cli/seo/generator.go (12)
metal/cli/seo/data.go (6)
Page(5-18)TemplateData(20-37)TagOgData(39-47)TwitterData(49-53)HrefLangData(55-58)FaviconData(60-65)metal/cli/seo/client.go (2)
Client(13-16)NewClient(18-23)metal/env/env.go (1)
Environment(9-17)pkg/portal/validator.go (1)
Validator(13-16)database/connection.go (1)
Connection(12-17)metal/router/static.go (2)
WebsiteRoutes(14-20)NewWebsiteRoutes(22-30)metal/cli/seo/categories.go (1)
NewCategories(16-21)metal/cli/seo/defaults.go (9)
LogoUrl(8-8)StubPath(28-28)RepoWebUrl(7-7)RepoApiUrl(6-6)AboutPhotoUrl(9-9)GocantoUrl(5-5)Robots(30-30)ThemeColor(29-29)Description(31-31)metal/cli/seo/sections.go (1)
NewSections(12-14)metal/cli/seo/jsonld.go (1)
NewJsonID(27-41)metal/cli/seo/manifest.go (2)
Manifest(10-23)NewManifest(40-91)metal/cli/seo/support.go (1)
ManifestDataURL(8-13)
database/repository/categories.go (2)
database/connection.go (1)
Connection(12-17)database/model.go (1)
Category(106-118)
🔇 Additional comments (1)
metal/cli/seo/generator.go (1)
168-206: Generator fails validation when App.Name is shorter than 10 chars
Title,og.ImageAlt, andtwitter.ImageAltare all populated withg.Page.SiteName, butTemplateData,TagOgData, andTwitterDataeach imposemin=10. In our default config (config/environments/base.yaml)App.Nameis"Oullin"(6 chars), soBuildalways returnsinvalid template dataandGenerateHomenever writes a file. Please either relax the validator constraints or ensure we populate these fields with strings that satisfy the minimum length.og := TagOgData{ ImageWidth: "600", ImageHeight: "400", Type: "website", Locale: g.Page.Lang, - ImageAlt: g.Page.SiteName, + ImageAlt: fmt.Sprintf("%s portfolio", g.Page.SiteName), SiteName: g.Page.SiteName, Image: g.Page.AboutPhotoUrl, } @@ twitter := TwitterData{ Card: "summary_large_image", Image: g.Page.AboutPhotoUrl, - ImageAlt: g.Page.SiteName, + ImageAlt: fmt.Sprintf("%s portfolio", g.Page.SiteName), } @@ AppleTouchIcon: g.Page.LogoURL, - Title: g.Page.SiteName, + Title: fmt.Sprintf("%s | Software Engineering Leadership", g.Page.SiteName),
Go live
envvar:ENV_SPA_DIR.envvar:ENV_APP_URL.Summary by CodeRabbit
New Features
Configuration
Build/Chores