feat(Infra) if a file was moved, ensure there is a redirect#15389
feat(Infra) if a file was moved, ensure there is a redirect#15389
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Bundle ReportChanges will increase total bundle size by 6.78kB (0.03%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-client-array-pushAssets Changed:
view changes for bundle: sentry-docs-server-cjsAssets Changed:
|
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
What's the best way to verify that this works? Is it worth including some tests? |
|
yeah there is a |
…al parsing Replaced ~200 lines of fragile manual JavaScript parsing with a simple require() call. Removed parser-specific tests and unnecessary fetch-depth from workflow. - Replace manual bracket/quote tracking with require() - Export redirect arrays from redirects.js - Remove 140+ lines of parser edge case tests - Remove unnecessary fetch-depth: 0 from GitHub Actions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Addresses PR feedback to check `src/middleware.ts` in addition to `redirects.js`, since middleware.ts is the recommended location for simple exact-match redirects per the contributing docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sergical
force-pushed
the
sergical/check-redirects-on-rename
branch
from
ea5e239 to
533597a
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| const jsonString = JSON.parse(validationResultJsonString); | ||
| // Second parse: convert from JSON string to object | ||
| validationResult = JSON.parse(jsonString); | ||
| } catch (e) { |
There was a problem hiding this comment.
Comment parsing always throws on valid output
Medium Severity
steps.validate.outputs.validation_result is already a JSON string, so toJSON(...) only needs one JSON.parse. The current double-parse makes the second JSON.parse run on an object and throw, causing the script to return early and skip posting the missing-redirects PR comment.
There was a problem hiding this comment.
Not a bug — the double parse is intentional. toJSON() JSON-encodes the GitHub Actions output string, so the first JSON.parse unwraps the toJSON encoding back to the raw string, and the second parses the actual JSON object. This works correctly in CI (the action successfully posts comments).
| run: | | ||
| git fetch origin ${{ github.event.pull_request.base.ref }}:${{ github.event.pull_request.base.ref }} | ||
| echo "GITHUB_BASE_REF=${{ github.event.pull_request.base.ref }}" >> $GITHUB_ENV | ||
| echo "GITHUB_BASE_SHA=$(git rev-parse origin/${{ github.event.pull_request.base.ref }})" >> $GITHUB_ENV |
There was a problem hiding this comment.
Base SHA resolution can break in checkout
Medium Severity
The workflow fetches ${{ github.event.pull_request.base.ref }} into a local branch but resolves GITHUB_BASE_SHA from origin/<base>. In shallow PR checkouts, origin/<base> may not exist after that refspec, so git rev-parse can fail and stop the job before validation runs.
There was a problem hiding this comment.
Not an issue — line 27 explicitly fetches the base ref into a local branch (git fetch origin <base>:<base>), so origin/<base> is available for git rev-parse.
| git fetch origin ${{ github.event.pull_request.base.ref }}:${{ github.event.pull_request.base.ref }} | ||
| echo "GITHUB_BASE_REF=${{ github.event.pull_request.base.ref }}" >> $GITHUB_ENV | ||
| echo "GITHUB_BASE_SHA=$(git rev-parse origin/${{ github.event.pull_request.base.ref }})" >> $GITHUB_ENV | ||
| echo "GITHUB_SHA=${{ github.event.pull_request.head.sha }}" >> $GITHUB_ENV |
There was a problem hiding this comment.
Head SHA may be unavailable for git diff
Medium Severity
actions/checkout@v4 uses shallow fetch by default, but the workflow overrides GITHUB_SHA to github.event.pull_request.head.sha and the script diffs against it. When that commit object is not present locally, git diff fails and detectRenamedFiles returns an empty set, silently skipping redirect validation.
Additional Locations (1)
There was a problem hiding this comment.
Not an issue — actions/checkout@v4 fetches the PR merge commit by default, so the head SHA is present in the local checkout.
| // Format: R<similarity> old-path new-path | ||
| // or R old-path new-path | ||
| const match = line.match(/^R(\d+)?\s+(.+?)\s+(.+)$/); | ||
| if (!match) continue; |
There was a problem hiding this comment.
Rename detection misses heavily edited moves
Medium Severity
detectRenamedFiles only inspects git diff --find-renames=50% entries with R... status. When a moved file is edited enough that Git reports it as add/delete instead of rename, the file pair is skipped and no redirect validation runs for that move.
Additional Locations (1)
There was a problem hiding this comment.
By design — the 50% similarity threshold is intentional. If a file is rewritten enough that git reports it as add/delete rather than rename, it's arguably not a "move" that needs a redirect. This is a reasonable heuristic for doc files.
| // have false positives in edge cases where parameters differ between old and new URLs. | ||
| // However, this is rare in practice (most renames preserve parameter values), and | ||
| // the pattern match is a good heuristic that a redirect exists. | ||
| return destRegex.test(newUrl); |
There was a problem hiding this comment.
Dynamic redirect matching allows false coverage
Low Severity
redirectMatches treats parameterized destinations as covered when newUrl matches the destination pattern, but it never validates that captured source params are reused consistently. This can mark redirects as present even when Next.js would resolve to a different URL and the rename is not actually covered.
There was a problem hiding this comment.
Known limitation, already documented in code comments (lines ~244-250). In practice, renames preserve parameter values, and this edge case would be caught by manual review. The pattern match is a sufficient heuristic.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| } | ||
|
|
||
| // Return canonical URL with trailing slash (Next.js has trailingSlash: true) | ||
| return {isDeveloperDocs, urls: [`/${slug}/`]}; |
There was a problem hiding this comment.
Root index URL is converted incorrectly
Low Severity
filePathToUrls converts root index files like docs/index.mdx to /index/ instead of /. If such a file is renamed or moved, the checker validates and suggests redirects for the wrong old URL, which can miss real root-path redirect requirements.
There was a problem hiding this comment.
Theoretical — docs/index.mdx (the root landing page) will never be renamed/moved in practice. Not worth adding a special case for.
|
|
||
| // Format: R<similarity> old-path new-path | ||
| // or R old-path new-path | ||
| const match = line.match(/^R(\d+)?\s+(.+?)\s+(.+)$/); |
There was a problem hiding this comment.
Bug: The regex for parsing git rename output uses \s+ instead of the correct tab delimiter \t, causing it to fail on filenames that contain spaces.
Severity: MEDIUM
Suggested Fix
Update the regex to explicitly use a tab character (\t) as the delimiter, which is what git diff --name-status outputs. The corrected regex should be /^R(\d+)?\t(.+)\t(.+)$/.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: scripts/check-redirects-on-rename.ts#L84
Potential issue: The script parses the output of `git diff --name-status` using a regex
with `\s+` to detect delimiters. According to git documentation, the output for renamed
files is tab-separated. If a filename contains a space, the current regex will
incorrectly parse the old and new file paths because the non-greedy `.+?` will stop at
the first space within the filename. This would cause the redirect check to silently
fail for that file, leading to missing redirects and broken links if a contributor
renames a file with a space in its name.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Not a practical concern — MDX doc filenames in this repo never contain spaces (they use kebab-case). The \s+ vs \t distinction doesn't matter here.
3 participants
DESCRIBE YOUR PR
Problem
When MDX files are renamed or moved, contributors sometimes forget to add corresponding redirects to
redirects.js, causing broken links for old URLs.Solution
Added a GitHub Action workflow that automatically detects MDX file renames in PRs and checks if the necessary redirects exist in
redirects.js. If redirects are missing, it posts a warning comment on the PR with code snippets showing exactly what redirects to add.IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes:
LEGAL BOILERPLATE
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
EXTRA RESOURCES