Enhance build-release-history-page with validations and logging#288
Enhance build-release-history-page with validations and logging#288krataratha wants to merge 5 commits intorefactoringhq:mainfrom
Conversation
Added validation for file existence and payload structure, included timestamp in HTML output, and improved logging.
There was a problem hiding this comment.
Pull request overview
Updates the build-release-history-page script to add validations and extra logging around release-history HTML generation.
Changes:
- Adds pre-flight checks for releases JSON file existence and emptiness.
- Adds payload shape validation (expects an array) and optional
--debuglogging. - Adds a generated-at timestamp comment and logs output file size.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add timestamp comment to HTML | ||
| const timestamp = new Date().toISOString() | ||
| const htmlWithMeta = `<!-- Generated at ${timestamp} -->\n${html}` | ||
|
|
||
| mkdirSync(dirname(outputFilePath), { recursive: true }) | ||
| writeFileSync(outputFilePath, html) | ||
| // Write using modified HTML | ||
| writeFileSync(outputFilePath, htmlWithMeta) |
There was a problem hiding this comment.
html, outputFilePath, and writeFileSync are not defined/imported anywhere in this file, so this script will not compile/run. Restore the missing imports (writeFileSync and path utilities) and the logic that builds html (e.g., read JSON + buildReleaseHistoryPage(...)) and sets outputFilePath.
| // Write using modified HTML | ||
| writeFileSync(outputFilePath, htmlWithMeta) |
There was a problem hiding this comment.
The previous implementation created the output directory before writing (see scripts/build-release-download-page.ts:36-37 with mkdirSync(dirname(outputFilePath), { recursive: true })). This script writes directly to outputFilePath without ensuring the parent directory exists, which can fail in CI/release builds when the directory is missing.
|
|
||
| return value | ||
| // Ensure file is not empty | ||
| if (existsSync(releasesJsonPath) && statSync(releasesJsonPath).size === 0) { |
There was a problem hiding this comment.
After the !existsSync(releasesJsonPath) guard above, the extra existsSync(releasesJsonPath) in this condition is redundant. Once releasesJsonPath is properly defined, this can be simplified to just the statSync(...).size === 0 check.
| if (existsSync(releasesJsonPath) && statSync(releasesJsonPath).size === 0) { | |
| if (statSync(releasesJsonPath).size === 0) { |
| // Validate file existence before reading | ||
| if (!existsSync(releasesJsonPath)) { | ||
| throw new Error(`Releases JSON file not found: ${releasesJsonPath}`) | ||
| } |
There was a problem hiding this comment.
releasesJsonPath is referenced before it is defined (and the argument parsing/resolve(getArg('--releases-json')) logic appears to have been removed). As written, this script will throw a ReferenceError before any validation runs. Reintroduce/define releasesJsonPath (and related arg parsing) before using it here.
| } | ||
| // Validate payload structure | ||
| if (!Array.isArray(releasesPayload)) { | ||
| console.warn('Invalid payload format: expected an array. Falling back to empty list.') |
There was a problem hiding this comment.
The warning says it's "falling back to empty list", but releasesPayload is never reassigned and the code continues as-is. Either coerce to [] (e.g., const safePayload = Array.isArray(releasesPayload) ? releasesPayload : []) or throw to avoid generating a page from an invalid payload.
| console.warn('Invalid payload format: expected an array. Falling back to empty list.') | |
| throw new Error('Invalid payload format: expected an array.') |
Added validation for file existence and payload structure, included timestamp in HTML output, and improved logging.