Skip to content

Namespace release changelog heading ids#227

Open
PrzemyslawKlys wants to merge 1 commit intomainfrom
codex/releasehub-id-namespacing
Open

Namespace release changelog heading ids#227
PrzemyslawKlys wants to merge 1 commit intomainfrom
codex/releasehub-id-namespacing

Conversation

@PrzemyslawKlys
Copy link
Member

Summary

  • namespace release changelog heading ids by release tag in the release-hub renderer
  • rewrite local fragment links inside release bodies to match the namespaced ids
  • add a regression test for repeated "What's Changed" headings across multiple releases

Validation

  • dotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Release --filter FullyQualifiedName~WebReleaseHubRenderingTests
  • .& .\build.ps1 -CI -PowerForgeRoot C:\Support\GitHub\PSPublishModule-codex-releasehub-ids from C:\Support\GitHub\OfficeIMO-codex-website\Website

This removes the last remaining audit warning on the OfficeIMO website changelog timeline when built against this engine branch.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2453d7ff06

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +566 to +567
var namespacedTarget = NamespaceFragment(prefix, target);
return $"{match.Groups["prefix"].Value}{match.Groups["quote"].Value}#{namespacedTarget}{match.Groups["quote"].Value}{match.Groups["suffix"].Value}";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid rewriting non-heading fragment links

The link rewrite currently namespaces every href="#..." in release bodies, but only heading IDs are renamed. This breaks valid in-body links that target non-heading anchors (for example custom <a id="...">, footnotes, or intentionally cross-release fragments), because those target IDs are never updated. In those cases navigation silently regresses after this commit. Consider restricting rewrites to fragment IDs that were actually remapped from headings.

Useful? React with 👍 / 👎.

@claude
Copy link

claude bot commented Mar 25, 2026

test comment

@claude
Copy link

claude bot commented Mar 25, 2026

PR Review: Namespace release changelog heading IDs

Overall this is a clean, well-scoped fix for a real problem. The approach (prefix heading IDs and rewrite fragment links per-release) is correct, and the regression test covers the core scenario well. A few items worth addressing:


Performance / Consistency

The three Regex.Replace calls inside Slugify (ReleaseHubRenderer.cs:590-592) compile a new pattern on every invocation. The codebase already establishes the right pattern in WebApiDocsGenerator.cs:239-242: compiled static fields with a regex timeout. The new Slugify should follow that same convention (or reuse those patterns via a shared utility).

The inline Regex inside the match evaluator at ReleaseHubRenderer.cs:549 creates a new object on every heading match and should be a compiled static field (e.g., StripTagsRegex).


Missing timeout on new compiled regexes

HeadingWithIdRegex, IdAttributeRegex, and FragmentLinkRegex are compiled static fields (good) but lack a matchTimeout. Since these run against externally-sourced HTML content (release bodies from GitHub), a TimeSpan.FromSeconds(1) timeout provides a safety net against pathological inputs. The rest of the codebase uses RegexTimeout for exactly this reason.


Minor: redundant double-slugification in NamespaceFragment

prefix always arrives pre-slugified from NamespaceReleaseBodyHtml, so Slugify(prefix) inside NamespaceFragment is a no-op in practice. Harmless, but if intentional for defensive safety a brief comment would clarify.


Fragment link rewriting scope

FragmentLinkRegex rewrites all href fragment links in the release body, not only those targeting headings that were actually namespaced. For standard GitHub-generated release notes this is fine, but if a body ever contains a fragment link to an anchor defined outside that release block, the link would silently break. A short comment documenting this assumption would be useful.


Test feedback

The test Build_ReleaseChangelog_NamespacesRepeatedHeadingIdsPerRelease is solid. Consider adding:

  • A case where a heading has no existing id attribute (exercises the Slugify(text) fallback path at line 549)
  • A case with an empty body (should be a no-op)

Summary

Area Status
Correctness Looks correct
Test coverage Core regression covered; edge cases could be added
Performance Uncompiled regexes in Slugify and inline tag-strip
Consistency Missing RegexTimeout on new static patterns (diverges from WebApiDocsGenerator convention)
Design Slugify now has three separate private implementations; worth consolidating eventually, not a blocker

The timeout and uncompiled-regex concerns are the most actionable items before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant