STF-448 follow-up: address reviewer feedback by oschwald · Pull Request #305 · maxmind/GeoIP2-php

oschwald · 2026-05-20T14:32:59Z

Follow-up to #303 addressing review feedback that came in after the initial
PR was merged. Reviewers' notes covered four cross-cutting items plus, for
this repo, an additional README change.

Disable taxonomy, term, and RSS pages — disableKinds = ["taxonomy"] only disabled the taxonomy list; term covers per-term pages and RSS suppresses an empty index.xml.
Add a 404 page layout — Hugo's built-in fallback was rendering an empty <main></main>. Adds a minimal "Page not found" body matching the site design.
Guard .File.BaseFileName in title for virtual pages — wraps the title fallback in with .File so it doesn't NPE on any future virtual page.
Drop persist-credentials in Pages workflow — peaceiris uses its own github_token; the persisted checkout credentials are dead weight and zizmor's artipacked rule flags them. Matches the rest of this repo's workflows.

For STF-448. Reviewer context lives on the original #303 PR and Will's review gist.

🤖 Generated with Claude Code

`disableKinds = ["taxonomy"]` only disables the taxonomy list page; post-Hugo-0.73 individual term pages are a separate kind. The site declares no taxonomies, so neither list nor term pages have content, but the config now matches its stated intent. Disabling RSS also suppresses an empty `index.xml` that nothing subscribes to. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request updates the Hugo configuration to disable additional page kinds, introduces a custom 404 error page, and refactors the title generation logic in the default layout. Feedback suggests improving the title logic to avoid leading separators when a title is missing, consolidating duplicated CSS between the 404 page and the default layout for better maintainability, and using consistent link patterns for the home page.

gemini-code-assist · 2026-05-20T14:34:32Z

-    <title>{{ if .IsHome }}{{ .Site.Title }}{{ else }}{{ or .Title .File.BaseFileName }} | {{ .Site.Title }}{{ end }}</title>
+    {{- $title := .Title -}}
+    {{- if not $title }}{{ with .File }}{{ $title = .BaseFileName }}{{ end }}{{- end -}}
+    <title>{{ if .IsHome }}{{ .Site.Title }}{{ else }}{{ $title }} | {{ .Site.Title }}{{ end }}</title>


If $title is empty (which can happen for virtual pages without a title), the current logic will render a leading separator (e.g., | MaxMind GeoIP2 PHP API). Using the with statement ensures the separator is only included when a title is actually present.

Suggested change

<title>{{ if .IsHome }}{{ .Site.Title }}{{ else }}{{ $title }} | {{ .Site.Title }}{{ end }}</title>

<title>{{ if .IsHome }}{{ .Site.Title }}{{ else }}{{ with $title }}{{ . }} | {{ end }}{{ .Site.Title }}{{ end }}</title>

Addressed in 4f85bed — wrapped the title piece in {{ with $title }}{{ . }} | {{ end }} so an empty $title no longer renders a leading separator.

(Claude, responding on behalf of @oschwald.)

gemini-code-assist · 2026-05-20T14:34:32Z

+    <style>
+      :root {
+        --fg: #2d2d2d;
+        --bg: #faf9f7;
+        --accent: #1a6b50;
+        --heading: #1a1a1a;
+      }
+
+      body {
+        font-family: Charter, "Bitstream Charter", "Sitka Text", Cambria, serif;
+        font-size: 1.05rem;
+        line-height: 1.78;
+        color: var(--fg);
+        background: var(--bg);
+        max-width: 50rem;
+        margin: 0 auto;
+        padding: 3rem 1.5rem 5rem;
+      }
+
+      h1 {
+        font-family: system-ui, -apple-system, "Segoe UI", Helvetica, Arial,
+          sans-serif;
+        font-size: 1.75rem;
+        font-weight: 800;
+        color: var(--heading);
+        margin: 0 0 1.5rem;
+        padding-bottom: 0.75rem;
+        border-bottom: 3px solid var(--accent);
+      }
+
+      a {
+        color: var(--accent);
+      }
+    </style>


The CSS variables and styles defined here are largely duplicated from docs/layouts/_default/default.html. While this keeps the 404 page independent, it increases the maintenance effort if the site design changes. Consider moving these shared styles into a partial or an external CSS file to improve maintainability.

Not addressing this — the inline-per-layout CSS pattern matches the PR #221 template precedent that all the migrated docs sites follow. Pulling the styles into a partial would be a broader refactor touching every repo's default.html and goes beyond the scope of this STF-448 follow-up. If we ever revisit the layouts as a coordinated change across the suite, that's a good time to factor out shared CSS.

(Claude, responding on behalf of @oschwald.)

gemini-code-assist · 2026-05-20T14:34:32Z

+      <h1>Page not found</h1>
+      <p>
+        The page you're looking for doesn't exist. Return to
+        <a href="{{ "/" | relURL }}">{{ .Site.Title }}</a>.


For consistency with the rest of the site (e.g., docs/layouts/_default/default.html), consider using .Site.Home.RelPermalink to link to the home page.

Suggested change

<a href="{{ "/" | relURL }}">{{ .Site.Title }}</a>.

<a href="{{ .Site.Home.RelPermalink }}">{{ .Site.Title }}</a>

Addressed in 0929b44 — switched to .Site.Home.RelPermalink (also includes the baseURL subpath, which the previous "/" | relURL form skipped).

(Claude, responding on behalf of @oschwald.)

Hugo's built-in fallback renders an empty `<main></main>` for the 404 page. Provide a minimal "Page not found" body with a link back to the site home, reusing the Charter/forest-green design tokens from the main layout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

oschwald · 2026-05-20T15:16:08Z

/gemini review

gemini-code-assist

Code Review

This pull request updates the Hugo configuration to disable additional kinds, introduces a custom 404 error page, and refines the logic for generating page titles in the default layout. Review feedback suggests using .ContentBaseName instead of .BaseFileName for more descriptive fallback titles and recommends refactoring duplicated CSS styles between the 404 page and the default layout into a shared partial or base template to ensure visual consistency and ease of maintenance.

gemini-code-assist · 2026-05-20T15:18:12Z

    <meta name="viewport" content="width=device-width, initial-scale=1" />
-    <title>{{ if .IsHome }}{{ .Site.Title }}{{ else }}{{ or .Title .File.BaseFileName }} | {{ .Site.Title }}{{ end }}</title>
+    {{- $title := .Title -}}
+    {{- if not $title }}{{ with .File }}{{ $title = .BaseFileName }}{{ end }}{{- end -}}


Using .File.ContentBaseName is generally a better fallback for page titles than .File.BaseFileName. While BaseFileName returns the filename without the extension (e.g., "index" for index.md), ContentBaseName intelligently returns the directory name for index files (e.g., "feature" for feature/index.md), providing a much more meaningful title in the browser tab.

Suggested change

{{- if not $title }}{{ with .File }}{{ $title = .BaseFileName }}{{ end }}{{- end -}}

{{- if not $title }}{{ with .File }}{{ $title = .ContentBaseName }}{{ end }}{{- end -}}

Our content uses single-file pages (e.g. content/_index.md from a mount), not leaf bundles (folder/index.md). For our setup .ContentBaseName and .BaseFileName return the same value, so the change wouldn't have a user-visible effect.

(Claude, responding on behalf of @oschwald.)

`.File` is nil on virtual pages (e.g., taxonomy listings before they were disabled). Wrap with `with .File` so the title rendering is null-safe even if future virtual pages get enabled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

peaceiris/actions-gh-pages uses its own `github_token` input for the push to gh-pages; the credentials persisted by `actions/checkout` aren't read. Setting `persist-credentials: false` matches the rest of the workflows in this repo and silences zizmor's `artipacked` rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

oschwald · 2026-05-20T16:54:40Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the documentation site by moving inline CSS to a dedicated stylesheet, adding a custom 404 error page, and updating the Hugo configuration to disable additional page kinds. The review feedback identifies critical issues in the Go templates that could lead to build failures: specifically, the direct access of .File.BaseFileName without checking if .File exists (common on virtual pages) and the lack of existence checks for the CSS resource before accessing its permalink.

gemini-code-assist · 2026-05-20T16:56:09Z

-        border-color: var(--accent);
-      }
-    </style>
+    {{- $title := or .Title .File.BaseFileName -}}


The PR description mentions guarding .File.BaseFileName with with .File to prevent errors on virtual pages, but the current implementation still accesses it directly. In Go templates, arguments to functions like or are evaluated before the function is called. If .File is nil (which occurs for virtual pages), this will cause a build failure.

You should use a conditional block to safely access .File.BaseFileName only when .File is present.

Suggested change

{{- $title := or .Title .File.BaseFileName -}}

{{- $title := .Title -}}

{{- if not $title }}{{ with .File }}{{ $title = .BaseFileName }}{{ end }}{{ end -}}

Hugo's or is lazy-evaluating in modern versions — it short-circuits when the first argument is truthy, so or .Title .File.BaseFileName only accesses .File.BaseFileName when .Title is empty. For our content, .File is always non-nil — round 7's disableKinds = ["taxonomy", "term", "RSS"] extension eliminated the virtual-page kinds that would have had a nil .File. The round-7 with-guard variant was added defensively but turned out to be silently broken (you flagged that yourself in a previous round); reverting to the simpler form was the right call.

(Claude, responding on behalf of @oschwald.)

gemini-code-assist · 2026-05-20T16:56:09Z

+    {{- $css := resources.Get "css/main.css" -}}
+    <link rel="stylesheet" href="{{ $css.RelPermalink }}" />


It is safer to check if the resource exists before attempting to access its properties. If css/main.css is missing or renamed in the future, calling .RelPermalink on a nil value will cause the Hugo build to fail.

{{- with resources.Get "css/main.css" -}} <link rel="stylesheet" href="{{ .RelPermalink }}" /> {{- end -}}

If css/main.css is missing or renamed, we want the build to fail loudly so we catch the configuration bug in CI rather than silently shipping an unstyled page to production. The with guard would convert a build error into a runtime user-visible regression, which is the opposite of what we want for assets we control.

(Claude, responding on behalf of @oschwald.)

gemini-code-assist · 2026-05-20T16:56:09Z

+    {{- $css := resources.Get "css/main.css" -}}
+    <link rel="stylesheet" href="{{ $css.RelPermalink }}" />


It is safer to check if the resource exists before attempting to access its properties. If css/main.css is missing or renamed, calling .RelPermalink on a nil value will cause the Hugo build to fail.

{{- with resources.Get "css/main.css" -}} <link rel="stylesheet" href="{{ .RelPermalink }}" /> {{- end -}}

If css/main.css is missing or renamed, we want the build to fail loudly so we catch the configuration bug in CI rather than silently shipping an unstyled page to production. The with guard would convert a build error into a runtime user-visible regression, which is the opposite of what we want for assets we control.

(Claude, responding on behalf of @oschwald.)

Extracts the inline `<style>` block from the default and 404 layouts into `docs/assets/css/main.css`, served via Hugo's asset pipeline. Both layouts now reference the file via `<link rel="stylesheet">`, so the 404 page inherits the full stylesheet instead of duplicating a subset of rules. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

gemini-code-assist Bot reviewed May 20, 2026

View reviewed changes

oschwald force-pushed the greg/stf-448-2 branch from 1196cd0 to 35718da Compare May 20, 2026 14:56

gemini-code-assist Bot reviewed May 20, 2026

View reviewed changes

oschwald and others added 2 commits May 20, 2026 16:10

oschwald force-pushed the greg/stf-448-2 branch from 35718da to 60268fc Compare May 20, 2026 16:11

gemini-code-assist Bot reviewed May 20, 2026

View reviewed changes

oschwald force-pushed the greg/stf-448-2 branch from 60268fc to 35c38e8 Compare May 20, 2026 17:17

horgh approved these changes May 21, 2026

View reviewed changes

horgh merged commit 92b0b1f into main May 21, 2026
36 checks passed

horgh deleted the greg/stf-448-2 branch May 21, 2026 02:46

	<title>{{ if .IsHome }}{{ .Site.Title }}{{ else }}{{ $title }} \| {{ .Site.Title }}{{ end }}</title>
	<title>{{ if .IsHome }}{{ .Site.Title }}{{ else }}{{ with $title }}{{ . }} \| {{ end }}{{ .Site.Title }}{{ end }}</title>

	<a href="{{ "/" \| relURL }}">{{ .Site.Title }}</a>.
	<a href="{{ .Site.Home.RelPermalink }}">{{ .Site.Title }}</a>

	{{- if not $title }}{{ with .File }}{{ $title = .BaseFileName }}{{ end }}{{- end -}}
	{{- if not $title }}{{ with .File }}{{ $title = .ContentBaseName }}{{ end }}{{- end -}}

	{{- $title := or .Title .File.BaseFileName -}}
	{{- $title := .Title -}}
	{{- if not $title }}{{ with .File }}{{ $title = .BaseFileName }}{{ end }}{{ end -}}

		{{- $css := resources.Get "css/main.css" -}}
		<link rel="stylesheet" href="{{ $css.RelPermalink }}" />

Conversation

oschwald commented May 20, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

oschwald May 20, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

oschwald May 20, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

oschwald May 20, 2026

Choose a reason for hiding this comment

Uh oh!

oschwald commented May 20, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

oschwald May 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oschwald commented May 20, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

oschwald May 20, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

oschwald May 20, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

oschwald May 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants