Skip to content

fix: 404 redirects#2820

Closed
NicholasKissel wants to merge 1 commit intomainfrom
07-28-fix_404_redirects
Closed

fix: 404 redirects#2820
NicholasKissel wants to merge 1 commit intomainfrom
07-28-fix_404_redirects

Conversation

@NicholasKissel
Copy link
Copy Markdown
Member

Changes

@vercel
Copy link
Copy Markdown

vercel Bot commented Jul 29, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rivet-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 29, 2025 4:52am

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jul 29, 2025

Claude finished @NicholasKissel's task —— View job


PR Review: 404 Redirects Fix

Todo List

  • Read repository guidelines (CLAUDE.md)
  • Analyze next.config.ts changes (main redirect logic)
  • Review deleted HTML files and understand impact
  • Examine header component changes
  • Check for potential bugs and security issues
  • Assess performance considerations
  • Evaluate test coverage needs
  • Provide comprehensive review feedback

Review Feedback

Positive Changes

  • Clean Architecture: Moving from HTML-based redirects to Next.js configuration is a good architectural decision that centralizes redirect logic
  • Comprehensive Coverage: The redirect rules cover a wide range of documentation reorganization scenarios
  • Consistent Discord Links: Fixing Discord links to use direct invite URLs improves user experience

⚠️ Issues & Recommendations

Code Quality
  1. Duplicate Redirects (site/next.config.ts:229-267): Several redirect rules are duplicated:

    • /docs/troubleshooting/docs/cloud/troubleshooting (lines 99-102 and 228-232)
    • /docs/container-runtime/docs/cloud/containers (lines 114-117 and 234-237)
    • /docs/networking/docs/cloud/networking (lines 64-67 and 254-257)
    • /docs/pricing/docs/cloud/pricing (lines 69-72 and 258-262)
    • /docs/tokens/docs/cloud/tokens (lines 94-97 and 263-267)
  2. Missing Redirect Consolidation: The Discord redirect that was handled by /site/public/discord/index.html should be added to the Next.js redirects for consistency

Performance Considerations
  • Redirect Performance: With 68+ redirect rules, consider the performance impact. Next.js processes redirects sequentially, but this number should be acceptable
  • Permanent vs Temporary: All redirects use permanent: false (302). Consider using permanent: true (301) for stable URL changes to improve SEO and caching
Security
  • External Discord Link: Direct Discord invite links are appropriate and safe
  • No Security Concerns: All redirect destinations are internal or trusted external URLs
Missing Test Coverage
  • No Tests: This PR lacks tests for the redirect rules. Consider adding tests to verify critical redirects work correctly
  • Manual Verification: The redirect rules should be manually tested, especially the more complex ones with path parameters (:slug*)
Potential Bugs
  1. Path Parameter Handling: Redirects using :slug* (lines 84-87, 89-92) should be tested to ensure proper parameter forwarding
  2. Edge Case: The encoded space redirect (line 173-176) for /talk-to-an-engineer%20/ is specific - ensure this is the intended behavior

Recommendations

  1. Remove duplicate redirect rules to clean up the configuration
  2. Add Discord redirect to Next.js config: /discordhttps://discord.gg/aXYfyNxYVn
  3. Consider permanent redirects for stable documentation moves
  4. Add basic redirect tests for critical paths
  5. Group related redirects with comments for better maintainability

Overall Assessment

This is a solid cleanup PR that improves the redirect architecture. The main issues are duplicate rules and missing test coverage, but the core functionality is sound and well-implemented.


Copy link
Copy Markdown
Member Author


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Jul 29, 2025

Deploying rivet with  Cloudflare Pages  Cloudflare Pages

Latest commit: e9dd875
Status:🚫  Build failed.

View logs

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jul 29, 2025

Claude finished @NicholasKissel's task —— View job


PR Review: 404 Redirects Fix

Todo List

  • Read repository guidelines (CLAUDE.md)
  • Analyze next.config.ts changes (main redirect logic)
  • Review deleted HTML files and understand impact
  • Examine header component changes
  • Check for potential bugs and security issues
  • Assess performance considerations
  • Evaluate test coverage needs
  • Provide comprehensive review feedback

Review Feedback

Positive Changes

  • Clean Architecture: Moving from HTML-based redirects to Next.js configuration is a good architectural decision that centralizes redirect logic
  • Comprehensive Coverage: The redirect rules cover a wide range of documentation reorganization scenarios
  • Consistent Discord Links: Fixing Discord links to use direct invite URLs improves user experience

⚠️ Issues & Recommendations

Code Quality
  1. Duplicate Redirects (site/next.config.ts:252-256): The /clients/javascript redirect appears twice:

    • Lines 161-163 and 252-256 both redirect to /docs/clients/javascript
    • This creates redundancy and potential confusion
  2. Missing Redirect Consolidation: The Discord redirect that was handled by /site/public/discord/index.html should be added to the Next.js redirects for consistency

Performance Considerations
  • Redirect Performance: With 60+ redirect rules, consider the performance impact. Next.js processes redirects sequentially, but this number should be acceptable
  • Permanent vs Temporary: All redirects use permanent: false (302). Consider using permanent: true (301) for stable URL changes to improve SEO and caching
Security
  • External Discord Link: Direct Discord invite links are appropriate and safe
  • No Security Concerns: All redirect destinations are internal or trusted external URLs
Missing Test Coverage
  • No Tests: This PR lacks tests for the redirect rules. Consider adding tests to verify critical redirects work correctly
  • Manual Verification: The redirect rules should be manually tested, especially the more complex ones with path parameters (:slug*)
Potential Bugs
  1. Path Parameter Handling: Redirects using :slug* (lines 84-87, 89-92) should be tested to ensure proper parameter forwarding
  2. Edge Case: The encoded space redirect (line 173-176) for /talk-to-an-engineer%20/ is specific - ensure this is the intended behavior

Recommendations

  1. Remove duplicate redirect rules to clean up the configuration
  2. Add Discord redirect to Next.js config: /discordhttps://discord.gg/aXYfyNxYVn
  3. Consider permanent redirects for stable documentation moves
  4. Add basic redirect tests for critical paths
  5. Group related redirects with comments for better maintainability

Overall Assessment

This is a solid cleanup PR that improves the redirect architecture. The main issues are duplicate rules and missing test coverage, but the core functionality is sound and well-implemented.


@NicholasKissel NicholasKissel force-pushed the 07-28-fix_404_redirects branch from f07108b to e9dd875 Compare July 29, 2025 04:49
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jul 29, 2025

Claude finished @NicholasKissel's task —— View job


PR Review: 404 Redirects Fix

Todo List

  • Read repository guidelines (CLAUDE.md)
  • Analyze next.config.ts changes (main redirect logic)
  • Review deleted HTML files and understand impact
  • Examine header component changes
  • Check for potential bugs and security issues
  • Assess performance considerations
  • Evaluate test coverage needs
  • Provide comprehensive review feedback

Review Feedback

Positive Changes

  • Clean Architecture: Moving from HTML-based redirects to Next.js configuration is excellent - centralizes redirect logic and improves maintainability
  • Comprehensive Coverage: The 60+ redirect rules thoroughly handle documentation reorganization from the site restructure
  • Proper Discord Integration: Correctly replaces /site/public/discord/index.html with Next.js redirect (lines 239-243) and updates header components to use direct URLs
  • Trailing Slash Handling: Good attention to detail with separate rules for URLs with/without trailing slashes

⚠️ Issues & Recommendations

Code Quality

  1. Redirect Organization: Consider grouping related redirects with comments for better maintainability:

    // Cloud documentation moves
    // API endpoint redirects  
    // Legacy path redirects
  2. Consistent Redirect Strategy: All redirects use permanent: false (302 status). Consider using permanent: true (301) for stable documentation moves to improve SEO and browser caching.

Performance Considerations

  • Acceptable Scale: 60+ redirect rules are within reasonable limits for Next.js
  • Sequential Processing: Next.js processes redirects in order, but performance impact should be minimal
  • Caching: 302 redirects won't be cached as aggressively as 301s

Security

  • External Links Safe: Discord invite URL https://discord.gg/aXYfyNxYVn is appropriate and secure
  • Internal Redirects: All other destinations are internal paths - no security concerns

Missing Test Coverage

  • Critical Gap: No tests verify redirect functionality
  • Risk: Complex redirects with :slug* parameters (lines 84-87, 89-92) should be tested
  • Recommendation: Add basic redirect tests for critical paths:
    // Test key redirects work correctly
    expect(redirect('/docs/pricing')).toBe('/docs/cloud/pricing')
    expect(redirect('/discord')).toBe('https://discord.gg/aXYfyNxYVn')

Potential Issues

  1. Path Parameter Forwarding: Redirects using :slug* should be manually verified to ensure parameters are correctly forwarded
  2. Header Component Changes: Discord link changes in Header.jsx:212 and v2/Header.tsx:124,139,236 are correct but bypass the centralized redirect

File Analysis

  • site/next.config.ts:25-245: Well-structured redirect configuration
  • Deleted HTML files: Properly replaced by Next.js redirects
  • Header components: Correctly updated to use direct Discord URLs

Recommendations

  1. Add redirect tests for critical documentation paths
  2. Consider permanent redirects for stable URL moves
  3. Group redirects with comments for better organization
  4. Manual testing of :slug* parameter redirects

Overall Assessment

This is a solid architectural improvement that properly consolidates redirect logic. The main concerns are missing test coverage and the use of temporary redirects, but the core implementation is sound and well-executed.


@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app Bot commented Jul 29, 2025

Merge activity

  • Jul 29, 4:58 AM UTC: NicholasKissel added this pull request to the Graphite merge queue.
  • Jul 29, 4:58 AM UTC: CI is running for this pull request on a draft pull request (#2821) due to your merge queue CI optimization settings.
  • Jul 29, 4:59 AM UTC: Merged by the Graphite merge queue via draft PR: #2821.

graphite-app Bot pushed a commit that referenced this pull request Jul 29, 2025
<!-- Please make sure there is an issue that this PR is correlated to. -->

## Changes

<!-- If there are frontend changes, please include screenshots. -->
@graphite-app graphite-app Bot closed this Jul 29, 2025
@graphite-app graphite-app Bot deleted the 07-28-fix_404_redirects branch July 29, 2025 04:59
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