Skip to content

Conversation

@svelle
Copy link
Member

@svelle svelle commented Oct 22, 2025

this pr should blacklist all redirects from sitemap creation

@svelle
Copy link
Member Author

svelle commented Oct 22, 2025

@claude review this pr

@svelle svelle self-assigned this Oct 22, 2025
@claude
Copy link
Contributor

claude bot commented Oct 22, 2025

Claude finished @svelle's task —— View job


Code Review: Exclude redirect-only pages from sitemap

Todo List:

  • Read repository guidelines (CLAUDE.md) - No CLAUDE.md found
  • Analyze the modified sitemap extension file
  • Review code changes for correctness and best practices
  • Provide comprehensive review feedback

Review Summary

This PR effectively implements the goal of excluding redirect-only pages from sitemap generation. The implementation is well-structured and follows good practices.

🟢 Positive Aspects

  1. Clean Architecture: The solution properly separates concerns by adding dedicated functions for redirect handling
  2. Caching Strategy: Smart use of environment caching in _get_redirect_docnames() to avoid recomputing redirect sets
  3. Robust Normalization: _normalize_redirect_source() handles various edge cases (empty strings, HTML extensions, trailing slashes, fragments)
  4. Logical Filtering: The main filtering logic correctly distinguishes between redirect-only pages and real docs with redirects

🟡 Areas for Improvement

Minor Issues

  1. Type Annotation Missing (extensions/sitemap/__init__.py:115):

    def _get_redirect_docnames(app: Sphinx, env) -> Set[str]:

    The env parameter should be type-annotated (likely BuildEnvironment).

  2. Potential Edge Case (extensions/sitemap/__init__.py:134-135):

    if normalized.endswith("/index"):
        docnames.add(normalized[: -len("/index")])

    This adds both path/index and path variants. Consider if this could create false positives where legitimate content pages get excluded.

Documentation

  1. Function Documentation: Consider adding examples to the docstrings:
    def _normalize_redirect_source(source: str) -> Optional[str]:
        """
        Normalizes a redirect source entry from the configuration.
        
        Examples:
            "path/file.html#anchor" -> "path/file"
            "path/" -> "path" 
            "" -> None
        """

🟢 Code Quality Assessment

  • Logic: ✅ Sound implementation that correctly identifies redirect-only pages
  • Performance: ✅ Efficient with proper caching
  • Error Handling: ✅ Gracefully handles edge cases and malformed input
  • Maintainability: ✅ Well-structured and readable code

🟢 Security & Best Practices

  • No security concerns identified
  • Follows Python typing conventions
  • Proper use of Sphinx extension patterns
  • Good separation of concerns

Final Recommendation

✅ APPROVE - This is a solid implementation that meets the PR objectives. The minor improvements suggested above are optional and don't prevent the code from working correctly.

The solution effectively prevents redirect-only pages from cluttering the sitemap while preserving legitimate content pages that happen to have redirects pointing to them.


@github-actions
Copy link

Newest code from mattermost has been published to preview environment for Git SHA 25edb63

1 similar comment
@github-actions
Copy link

Newest code from mattermost has been published to preview environment for Git SHA 25edb63

@svelle
Copy link
Member Author

svelle commented Oct 22, 2025

@cwarnermm would appreciate a review on this.

right now we're indexing all the sitemap urls that are redirects and I'm getting a ton of errors in zendesk for this.
this pr removes all the redirects from the sitemap since we don't need it in there anyway.

@svelle svelle requested a review from cwarnermm October 22, 2025 09:42
@github-actions
Copy link

Newest code from mattermost has been published to preview environment for Git SHA 19eb06f

@cwarnermm cwarnermm merged commit 694b23a into master Oct 27, 2025
5 checks passed
@cwarnermm cwarnermm deleted the fix-sitemap-redirect-excludes branch October 27, 2025 15:33
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.

3 participants