-
Notifications
You must be signed in to change notification settings - Fork 45
[WEB-4839] Add comprehensive redirect file validation and instrumentation #3010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes implement comprehensive redirect validation across the Gatsby build pipeline. CircleCI config enhances validation with detailed checks on redirect file integrity. The createPages module adds reporter-based redirect monitoring and post-build validation. The writeRedirectToConfigFile module introduces initialization guards and redirect counting. The onPostBuild module adds runtime validation and early failure mechanisms. Changes
Sequence DiagramsequenceDiagram
participant GatsbyBuild as Gatsby Build
participant CreatePages as createPages
participant WriteRedirect as writeRedirectToConfigFile
participant FileSystem as File System
participant Validation as Validation
participant Reporter as Reporter
GatsbyBuild->>CreatePages: invoke with reporter
CreatePages->>WriteRedirect: initialize with reporter
WriteRedirect->>WriteRedirect: check init flag & log InitID
WriteRedirect->>FileSystem: clear nginx-redirects.conf
CreatePages->>CreatePages: iterate pages & create redirects
CreatePages->>WriteRedirect: writeRedirect(...) per page
WriteRedirect->>FileSystem: append redirect to file
WriteRedirect->>WriteRedirect: increment redirectCount
WriteRedirect->>Reporter: log redirect (if reporter provided)
CreatePages->>Validation: post-write validation
Validation->>FileSystem: read nginx-redirects.conf
Validation->>Validation: count lines & compare with getRedirectCount()
alt Mismatch or File Issues
Validation->>Reporter: emit error/warning
else Success
Validation->>Reporter: log validation success
end
Reporter-->>GatsbyBuild: return result
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Comment |
f68b910 to
c739d3f
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
data/onPostBuild/index.ts (1)
46-58: Redirect format regex may produce false positives for external redirects.The regex
^\/[^\s]+ \/[^\s]+;$requires both the source and destination paths to start with/. If there are any external URL redirects (e.g.,https://...) or paths with query parameters, they would be flagged as invalid even if they're intentional.Since this is a warning (not a panic), the impact is limited to log noise, but worth noting.
Consider expanding the regex to also allow external URLs if they're expected:
- .filter((line) => line.length > 0 && !line.match(/^\/[^\s]+ \/[^\s]+;$/)); + .filter((line) => line.length > 0 && !line.match(/^\/[^\s]+ (\/[^\s]+|https?:\/\/[^\s]+);$/));data/createPages/writeRedirectToConfigFile.ts (2)
6-7:initTimestampcaptured at module load, not initialization time.
initTimestampis set when the module is first loaded, not whenwriteRedirectToConfigFileis actually called. If there's a delay between module import and initialization, the logged timestamp won't reflect the actual initialization time.Move timestamp generation inside the function:
-const initTimestamp = new Date().toISOString(); +let initTimestamp: string; export const writeRedirectToConfigFile = (filePath: string, reporter?: Reporter) => { // Detect re-initialization if (isInitialized) { // ... } + initTimestamp = new Date().toISOString(); reporter?.info(`[REDIRECTS] Initializing redirect file at ${filePath}`);
44-49: Consider documentingresetRedirectCountusage scope.This function resets module state and is likely intended for testing. Consider adding a JSDoc comment to clarify its purpose and prevent accidental usage in production code.
+/** + * Resets redirect count and initialization state. + * Intended for testing purposes only. + */ export const resetRedirectCount = (): void => { redirectCount = 0; isInitialized = false; };data/createPages/index.ts (1)
222-239: Clarify that count represents pages, not individual redirects.The logging shows "pages with redirect_from" which is accurate. However, a page can have multiple
redirect_fromentries, so the total pages count won't equal the final redirect count. This is fine but worth noting in the logs for clarity.- reporter.info(`[REDIRECTS] - Total pages with redirects: ${documentPagesWithRedirects + apiPagesWithRedirects + mdxPagesWithRedirects}`); + reporter.info(`[REDIRECTS] - Total pages with redirects: ${documentPagesWithRedirects + apiPagesWithRedirects + mdxPagesWithRedirects} (note: pages may have multiple redirect_from entries)`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.circleci/config.yml(1 hunks)data/createPages/index.ts(8 hunks)data/createPages/writeRedirectToConfigFile.ts(1 hunks)data/onPostBuild/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
data/onPostBuild/index.ts (2)
data/onPostBuild/llmstxt.ts (1)
onPostBuild(255-507)data/onPostBuild/compressAssets.ts (1)
onPostBuild(21-45)
data/createPages/index.ts (1)
data/createPages/writeRedirectToConfigFile.ts (2)
writeRedirectToConfigFile(9-42)getRedirectCount(44-44)
🔇 Additional comments (6)
.circleci/config.yml (1)
126-161: LGTM! Comprehensive CI validation for redirect file integrity.The validation logic is well-structured with clear error messages and progressive checks. The
|| echo "0"fallback forgrep -ccorrectly handles the case where no matches are found (which would otherwise return exit code 1).data/onPostBuild/index.ts (1)
63-69: LGTM! Proper fail-fast pattern with validateRedirects.The flow correctly validates redirects first before proceeding with other post-build tasks. The pattern of destructuring
reporterand spreading to downstream functions matches the existing patterns inllmstxtandcompressAssets.data/createPages/writeRedirectToConfigFile.ts (1)
11-19: Re-initialization guard is a good defensive measure.This correctly prevents the file from being cleared multiple times during a build, which was likely a root cause of the empty redirect file issue. The stack trace logging will help diagnose any unexpected re-initialization scenarios.
data/createPages/index.ts (3)
118-119: LGTM! Proper initialization with reporter passed to redirect writer.This ensures all redirect-related logging goes through Gatsby's reporter system for consistent build output.
393-439: Solid post-write validation with clear error context.The validation provides immediate feedback during
createPagesrather than waiting untilonPostBuild, enabling faster failure detection. The detailed error messages at lines 420-423 provide actionable debugging guidance.Note: This validation is intentionally redundant with
onPostBuildvalidation for defense in depth.
402-408:returnstatement afterpanicOnBuildis correct and necessary.
reporter.panicOnBuild()marks the build as failed but does not throw an exception; it only stops the Gatsby build process. Thereturnstatement is required to prevent further code execution, as seen in the validation checks at lines 402–408 and 416–430 where additional file operations would otherwise continue. This pattern is consistent with the codebase approach inllmstxt.ts, where bothpanicOnBuild()and explicit control flow interruption (throw) are used together for the same reason.
jamiehenson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this disclaimer was presented in advance and this is meant to be a quick thing but this is very botty.
Given this is not product facing I'm not standing with both feet on this hill but I would invite you to glance back through and remove anything that isn't relevant, and determine whether any logic could be shared between the different SSR files as there's a lot added to them and a lot of additions tracing similar paths.
If you disagree with my stance I can re-review this with a different perspective but I'm treating this as I see it.
.circleci/config.yml
Outdated
| # Get file size | ||
| FILE_SIZE=$(wc -c < config/nginx-redirects.conf) | ||
| echo "✓ File size: ${FILE_SIZE} bytes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care what the file size is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it to be non-zero. Currently on the failing tests it is actually zero, and the original tests pass because they simply tested for the existence of the file with the -f flag. That said, I'll rework the test for clarity
.circleci/config.yml
Outdated
| MIN_REDIRECTS=50 | ||
| if [ "$REDIRECT_COUNT" -lt "$MIN_REDIRECTS" ]; then | ||
| echo "ERROR: Expected at least ${MIN_REDIRECTS} redirects, found ${REDIRECT_COUNT}" | ||
| echo "First 10 lines of file:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this relevant? If we have 20 redirects and expected 50, what does this tell us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing, so to the bin it went
data/createPages/index.ts
Outdated
| ]); | ||
|
|
||
| // Post-write validation: Verify redirects were actually written to file | ||
| reporter.info(`[REDIRECTS] Promise.all completed, validating redirect file...`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'Promise.all completed' could probably be reworded here
data/createPages/index.ts
Outdated
| `); | ||
|
|
||
| // Log redirect data availability for investigation | ||
| const documentPagesWithRedirects = documentResult.data?.allFileHtml.edges.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.length doesn't return undefined or null after a filter, so I'm not sure what this nullish coalescing achieves. If you're concerned about data availability the optional chaining would be better served before the .filter instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took me a bit to parse this out but I've made the tweak, thanks for the suggestion!
data/createPages/index.ts
Outdated
| const redirectLines = redirectFileContent.trim().split('\n').filter((line) => line.length > 0); | ||
|
|
||
| reporter.info(`[REDIRECTS] Redirect lines in file: ${redirectLines.length}`); | ||
| reporter.info(`[REDIRECTS] File size: ${fs.statSync(redirectFilePath).size} bytes`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the obsession with file size came from my initial prompt when I stated "the file is empty", removed this along with other size checks
c739d3f to
7fb4eeb
Compare
@jamiehenson I took your concerns to heart, thank you for pulling me up. After manually cleaning up several of the small issues I added another separate fixup that a) consolidates shared responsibilities, and b) trims back the fat on excessive feedback. With this in place I can hopefully figure out what on earth is blocking #2992 (and other PRs) |
This commit addresses WEB-4839 by implementing a multi-layered approach to detect and prevent empty redirect files in CI builds. Key changes: 1. Added comprehensive logging using Gatsby's reporter infrastructure - Track redirect initialization, write attempts, and counts - Log GraphQL query results showing pages with redirect_from - Report hash fragment filtering - Re-initialization detection with warnings 2. Post-write validation in createPages - Validates redirect file after Promise.all completes - Compares expected vs actual redirect counts - Uses reporter.panicOnBuild() to fail builds immediately if empty 3. Build validation hook in onPostBuild - Safety net before build artifacts are published - Validates file existence, size, content, and format - Checks minimum redirect threshold (50+) 4. Enhanced CI validation - Comprehensive bash checks for file content - Counts redirects and validates minimum threshold - Shows file details if validation fails 5. Error handling improvements - Try-catch around all file operations - Proper error reporting with context - Prevents silent failures All logging now uses Gatsby's reporter for structured, colored output consistent with the rest of the codebase. This makes it impossible for empty redirect files to reach production silently. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
7fb4eeb to
287357c
Compare
Problem
We've been experiencing intermittent CI failures where
config/nginx-redirects.confis sometimes empty after successful Gatsby builds. This causes smoke test failures and is a production risk - redirects may silently fail without warning.Key finding: Builds complete successfully with no errors in logs, but the redirects file ends up empty.
Solution
This PR implements a multi-layered approach to detect and prevent empty redirect files:
1. Comprehensive Instrumentation
Using Gatsby's Reporter Infrastructure (consistent with existing codebase patterns):
data/createPages/writeRedirectToConfigFile.ts
reporterparameterdata/createPages/index.ts
redirect_from2. Post-Write Validation (createPages)
After
Promise.all()completes:reporter.panicOnBuild()to fail builds immediately if empty3. Build Validation Hook (onPostBuild)
Safety net before artifacts are published:
reporter.panicOnBuild()for critical failures4. Enhanced CI Validation
.circleci/config.yml improvements:
grep -c ';$'5. Benefits of Using Gatsby Reporter
All logging now uses Gatsby's
reporterinstead ofconsole.log:reporter.panicOnBuild()properly fails buildscompressAssets.tsandllmstxt.tsExpected Outcomes
After merging:
Testing
[REDIRECTS]log messages visibleDeployment Notes
After deployment, monitor the first few builds to:
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.