STF-448 follow-up: address reviewer feedback by oschwald · Pull Request #259 · maxmind/minfraud-api-php

oschwald · 2026-05-20T14:33:02Z

Follow-up to #257 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 #257 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 'term' and 'RSS' kinds, introduces a custom 404 error page, and refactors the title generation logic in the default layout. Feedback includes improving internal linking on the 404 page by using .Site.Home.RelPermalink for better portability and refining the title template with a with statement to prevent rendering empty separators when a page title is unavailable.

gemini-code-assist · 2026-05-20T14:35:02Z

+      <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 navigation in default.html (line 207) and to ensure the link works correctly regardless of the site's base URL or subpath, use .Site.Home.RelPermalink instead of relURL. This follows Hugo best practices for internal linking.

Suggested change

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

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

References

Prefer using built-in page methods like '.RelPermalink' over hardcoded paths and the 'relURL' filter for internal links to ensure better portability and consistency.

Addressed in 855b028 — switched to .Site.Home.RelPermalink (matches the main layout and preserves the baseURL subpath, which the previous "/" | relURL form skipped).

(Claude, responding on behalf of @oschwald.)

gemini-code-assist · 2026-05-20T14:35:02Z

-    <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 occur for virtual pages without a title), the current logic will render a leading separator (e.g., | minFraud PHP API). Using the with function allows you to conditionally include the title and separator only when a value is present, providing a cleaner fallback and adhering to Hugo template best practices.

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>

References

Use the 'with' statement in Hugo templates to safely handle optional variables and avoid rendering empty separators or broken HTML when data is missing.

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

(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:09Z

/gemini review

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 to better handle missing titles. Feedback focuses on improving the 404 page's styling—specifically adding custom selection colors, paragraph margins, and enhanced link decorations—to ensure visual consistency with the main site layout.

`.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:41Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the documentation site by externalizing inline CSS into a new stylesheet and adding a custom 404 page. It also updates the Hugo configuration to disable 'term' and 'RSS' page types. Review feedback highlights potential build failures in Go templates: specifically, the need to safely access .File.BaseFileName to avoid errors on virtual pages and the importance of verifying that CSS resources exist before retrieving their permalinks.

gemini-code-assist · 2026-05-20T17:00:14Z

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


The pull request description mentions guarding .File.BaseFileName with with .File to prevent errors on virtual pages. However, the current implementation still uses or .Title .File.BaseFileName. In Go templates, all arguments to a function (like or) are evaluated before the function is called. If .File is nil, accessing .File.BaseFileName will still cause a build error. Using a conditional block ensures the field is only accessed when the object exists.

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-20T17:00:14Z

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


Accessing .RelPermalink on the result of resources.Get without a null check can lead to build failures if the asset is missing or renamed. It is safer to wrap the resource usage in a with block to ensure the resource was successfully retrieved.

{{- 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-20T17:00:14Z

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


Accessing .RelPermalink on the result of resources.Get without a null check can lead to build failures if the asset is missing or renamed. It is safer to wrap the resource usage in a with block to ensure the resource was successfully retrieved.

{{- 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 b9b430d to 24749ca Compare May 20, 2026 14:56

gemini-code-assist Bot reviewed May 20, 2026

View reviewed changes

Comment thread docs/layouts/404.html Outdated

Comment thread docs/layouts/404.html Outdated

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

oschwald force-pushed the greg/stf-448-2 branch from 24749ca to ef0d211 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 ef0d211 to 3bd1f19 Compare May 20, 2026 17:17

horgh approved these changes May 21, 2026

View reviewed changes

horgh merged commit 1be1fa8 into main May 21, 2026
36 checks passed

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

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

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

	{{- $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!

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!

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