Added the expire-old-content.js script #20
Conversation
I FUCKING DISCARDED MY LAST WORKING SCRIPT RIGHT BEFORE COMMITTING IT I WILL ACTUALLY KILL MYSELF
|
Warning Rate limit exceeded@RichardKanshen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve significant updates to multiple files, including the addition of a new script Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (5)
webpify.js (2)
19-19: Consider enhancing error handling for specific scenarios.While the retry mechanism is a good addition, consider improving error handling by checking for specific error types (e.g., EBUSY, EPERM) to provide more informative error messages.
-await fs.rm(conversion.original, { force: true, maxRetries: 5, retryDelay: 1000 }); +try { + await fs.rm(conversion.original, { force: true, maxRetries: 5, retryDelay: 1000 }); +} catch (error) { + if (error.code === 'EBUSY') { + console.error(`File ${conversion.original} is being used by another process`); + } else if (error.code === 'EPERM') { + console.error(`Permission denied when deleting ${conversion.original}`); + } + throw error; +}
Line range hint
1-180: Consider adding detailed documentation.Given the complexity of the path handling and the issues mentioned in the PR description, consider adding:
- A README section explaining the path handling strategy
- Troubleshooting guidelines for common issues
- Examples of expected behavior for different path scenarios
This would help prevent future frustrations and make the tool more maintainable.
expire-old-content.js (3)
38-55: Improve robustness of import statement removalThe current regex may not match all variations of import statements, such as those using single quotes, different spacing, or without semicolons. Consider using a parser like Babel to reliably find and remove import statements.
42-46: Handle file deletion asynchronously to prevent blockingUsing synchronous file operations like
fs.unlinkSynccan block the event loop, impacting performance. Opt for asynchronous versions likefs.unlinkand handle the callbacks or use promises for better performance.Example refactor:
- if (fs.existsSync(fullImagePath)) { - fs.unlinkSync(fullImagePath); + fs.unlink(fullImagePath, (err) => { + if (err) { + console.error(`Error deleting image file ${fullImagePath}:`, err); + } else { + console.log(`Deleted image file: ${fullImagePath}`); + } + });
91-92: Ensure robust replacement of cron schedules in the workflow fileThe regex used to replace cron schedules might not cover all YAML formatting cases. Consider parsing the YAML file using a YAML parser like
js-yamlto accurately modify the cron schedules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/workflows/buildndeploy.yml(1 hunks)expire-old-content.js(1 hunks)package.json(1 hunks)webpify.js(3 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/buildndeploy.yml
[error] 8-8: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
package.json (1)
21-21: Verify the new script execution order.
The prebuild script now runs expire-old-content.js before webpify.js. Let's verify that both scripts exist and understand their dependencies.
✅ Verification successful
Let me gather more information about the scripts' content to understand their functionality and verify if the order matters.
The script execution order is correct and necessary
After analyzing both scripts, the order is important because:
expire-old-content.jsremoves expired images and their imports firstwebpify.jsthen processes the remaining valid images to convert them to WebP format
This order prevents unnecessary WebP conversions of images that would be removed anyway. The scripts are independent of each other (no direct dependencies) but the sequence is logically sound for optimization purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of both scripts
# Test 1: Check if both scripts exist
echo "Checking for script files..."
ls expire-old-content.js webpify.js
# Test 2: Look for any dependencies between the scripts
echo -e "\nChecking for dependencies between scripts..."
rg -A 5 "webpify" expire-old-content.js
rg -A 5 "expire-old-content" webpify.js
# Test 3: Check if scripts handle errors appropriately
echo -e "\nChecking error handling in scripts..."
rg "try|catch|throw|error" expire-old-content.js webpify.js
Length of output: 2039
Script:
#!/bin/bash
# Get the content of both scripts to understand their functionality
echo "Content of expire-old-content.js:"
cat expire-old-content.js
echo -e "\nContent of webpify.js:"
cat webpify.js
Length of output: 13045
webpify.js (3)
10-12: LGTM! Well-structured utility function.
The normalizePath function correctly standardizes path separators using POSIX style (forward slashes), which is essential for consistent path handling across different operating systems.
127-127: Consider making image exclusions configurable.
The list of excluded files is currently hardcoded. Consider moving these to a configuration file for better maintainability and flexibility.
#!/bin/bash
# Check if any image files might be unexpectedly excluded
fd -e jpg -e jpeg -e png | grep -v 'node_modules\|build\|dist\|favicon.png\|banner.png'
# Verify if there are any additional common image paths that should be excluded
fd -t d -d 2 'assets\|images\|img'89-101: 🛠️ Refactor suggestion
Verify path handling on Windows systems.
While the path normalization is improved, using path.resolve on Windows might still produce backslashes internally. Consider normalizing paths consistently throughout the resolution process.
-const absoluteMatchedPath = path.resolve(path.dirname(filePath), matchedPath);
+const absoluteMatchedPath = normalizePath(path.resolve(path.dirname(filePath), matchedPath));
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
expire-old-content.js (2)
129-131: Rename parameter for clarity incommitAndPushChangesfunctionThe parameter
hasComponentChangesincommitAndPushChanges()can be confusing since it now represents changes in both the component and the workflow files. Renaming it tohasChangesenhances readability.Apply this diff to rename the parameter and update its usage:
-function commitAndPushChanges(hasComponentChanges) { +function commitAndPushChanges(hasChanges) { try { - if (hasComponentChanges) { + if (hasChanges) { // ... } } catch (error) { // ... } } // Run steps const hasComponentChanges = updateComponentFile(); const hasWorkflowChanges = updateCronSchedule(); +const hasChanges = hasComponentChanges || hasWorkflowChanges; -commitAndPushChanges(hasComponentChanges || hasWorkflowChanges); +commitAndPushChanges(hasChanges);Also applies to: 147-149
88-93: Use the correct file path when reading the componentIn
updateCronSchedule(), the error message mentions the component file but could be clearer.Optionally, adjust the error message for clarity:
content = fs.readFileSync(componentPath, 'utf8'); } catch (error) { - console.error(`Error reading component file ${componentPath}:`, error); + console.error(`Error reading component file for cron update ${componentPath}:`, error); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
expire-old-content.js(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Learnings (1)
expire-old-content.js (1)
Learnt from: RichardKanshen
PR: CAsocialu/CAsocialu.github.io#20
File: expire-old-content.js:105-106
Timestamp: 2024-11-09T11:41:05.527Z
Learning: In `expire-old-content.js`, the `execSync` commands use constants (`componentPath` and `workflowPath`), not user-provided variables, minimizing security risks.
🪛 Biome
expire-old-content.js
[error] 27-27: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 38-38: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
expire-old-content.js (1)
6-7: Consider parameterizing file pathsHardcoding file paths reduces flexibility and maintainability. Consider accepting these as command-line arguments or environment variables.
-const componentPath = './src/pages/Domov/Domov.js'; // React component with images -const workflowPath = './.github/workflows/buildndeploy.yml'; // GitHub Actions workflow +const componentPath = process.env.COMPONENT_PATH || './src/pages/Domov/Domov.js'; +const workflowPath = process.env.WORKFLOW_PATH || './.github/workflows/buildndeploy.yml';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/buildndeploy.yml(2 hunks).github/workflows/prcheck.yml(1 hunks)expire-old-content.js(1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/buildndeploy.yml
[error] 8-8: trailing spaces
(trailing-spaces)
🪛 Biome
expire-old-content.js
[error] 27-27: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 38-38: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (2)
expire-old-content.js (2)
150-153: LGTM!
The main execution flow is clear and correctly handles changes from both component and workflow updates.
125-125:
Fix incorrect return value in updateCronSchedule
The function should return true when changes are made, but it's currently returning true when no changes are made.
-return workflowContent == oldWorkflowContent
+return workflowContent !== oldWorkflowContentLikely invalid or redundant comment.
expire-old-content.js
Outdated
| const cronEntries = Array.from(cronDates).map(dateStr => { | ||
| const date = new Date(dateStr); | ||
| return `0 0 ${date.getDate()} ${date.getMonth() + 1} *`; | ||
| }); |
There was a problem hiding this comment.
Add date validation for cron entries
Invalid dates could lead to incorrect cron entries. Add validation before generating the cron expression.
const cronEntries = Array.from(cronDates).map(dateStr => {
const date = new Date(dateStr);
+ if (Number.isNaN(date.getTime())) {
+ console.log(`WARNING! Invalid date format - ${dateStr} - Skipping cron entry.`);
+ return null;
+ }
return `0 0 ${date.getDate()} ${date.getMonth() + 1} *`;
-});
+}).filter(Boolean);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cronEntries = Array.from(cronDates).map(dateStr => { | |
| const date = new Date(dateStr); | |
| return `0 0 ${date.getDate()} ${date.getMonth() + 1} *`; | |
| }); | |
| const cronEntries = Array.from(cronDates).map(dateStr => { | |
| const date = new Date(dateStr); | |
| if (Number.isNaN(date.getTime())) { | |
| console.log(`WARNING! Invalid date format - ${dateStr} - Skipping cron entry.`); | |
| return null; | |
| } | |
| return `0 0 ${date.getDate()} ${date.getMonth() + 1} *`; | |
| }).filter(Boolean); |
expire-old-content.js
Outdated
| // First pass: collect image names that should be removed | ||
| content.replace(/<img [^>]*src=\{([^}]+)\}[^>]*valid-until="([^"]+)"[^>]*>/g, (match, imgVar, dateStr) => { | ||
| const expirationDate = new Date(dateStr); | ||
| if (isNaN(expirationDate.getTime())) { | ||
| console.log(`WARNING! Invalid date format - ${dateStr} - This will be ignored, but should be fixed ASAP.`); | ||
| return match; // Keep the image or handle accordingly | ||
| } | ||
| if (expirationDate < today) { | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix empty conditional block in first pass
The conditional block is empty, so removedImages is never populated. This affects the removal of imports in the third pass.
if (expirationDate < today) {
+ removedImages.add(imgVar.trim());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // First pass: collect image names that should be removed | |
| content.replace(/<img [^>]*src=\{([^}]+)\}[^>]*valid-until="([^"]+)"[^>]*>/g, (match, imgVar, dateStr) => { | |
| const expirationDate = new Date(dateStr); | |
| if (isNaN(expirationDate.getTime())) { | |
| console.log(`WARNING! Invalid date format - ${dateStr} - This will be ignored, but should be fixed ASAP.`); | |
| return match; // Keep the image or handle accordingly | |
| } | |
| if (expirationDate < today) { | |
| } | |
| }); | |
| // First pass: collect image names that should be removed | |
| content.replace(/<img [^>]*src=\{([^}]+)\}[^>]*valid-until="([^"]+)"[^>]*>/g, (match, imgVar, dateStr) => { | |
| const expirationDate = new Date(dateStr); | |
| if (isNaN(expirationDate.getTime())) { | |
| console.log(`WARNING! Invalid date format - ${dateStr} - This will be ignored, but should be fixed ASAP.`); | |
| return match; // Keep the image or handle accordingly | |
| } | |
| if (expirationDate < today) { | |
| removedImages.add(imgVar.trim()); | |
| } | |
| }); |
🧰 Tools
🪛 Biome
[error] 27-27: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
RichardKanshen
force-pushed
the
old-content-expiration
branch
4 times, most recently
from
ebebdb8
to
fb20ee1
Compare
RichardKanshen
force-pushed
the
old-content-expiration
branch
5 times, most recently
from
f75a233
to
573827c
Compare
PLEASE WORK PLEASE WORK PLEASE WORK PLEASE WORK PLEASE WORK
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
src/pages/Domov/Domov.js (1)
17-18: Consider renaming image files to avoid special characters.Using diacritical marks in filenames (
vánoce.png,rozdělení.png) might cause issues in different environments or build systems. Consider using ASCII-only characters (e.g.,vanoce.png,rozdeleni.png).expire-old-content.js (3)
6-7: Make file paths configurableHardcoding file paths reduces flexibility and reusability. Consider making these paths configurable through environment variables or command-line arguments.
-const componentPath = './src/pages/Domov/Domov.js'; // React component with images -const workflowPath = './.github/workflows/buildndeploy.yml'; // GitHub Actions workflow +const componentPath = process.env.COMPONENT_PATH || './src/pages/Domov/Domov.js'; +const workflowPath = process.env.WORKFLOW_PATH || './.github/workflows/buildndeploy.yml';
60-68: Ensure proper cleanup of temporary filesFile deletion operations should be handled more gracefully with proper cleanup, especially for temporary files. Consider implementing a cleanup mechanism that tracks and removes all temporary files, even if an error occurs.
+const tempFiles = new Set(); + try { if (fs.existsSync(fullImagePath)) { fs.unlinkSync(fullImagePath); + tempFiles.add(fullImagePath); console.log(`Deleted expired image file: ${fullImagePath}`); } } catch (error) { console.error(`Error deleting image file ${fullImagePath}:`, error); + // Ensure cleanup on error + for (const file of tempFiles) { + try { + if (fs.existsSync(file)) { + fs.unlinkSync(file); + } + } catch (cleanupError) { + console.error(`Cleanup failed for ${file}:`, cleanupError); + } + } }
202-206: Handle updateUpcomingComponents result consistentlyThe result of
updateUpcomingComponentsshould be handled consistently with other functions, even though its changes won't be committed.const hasOldComponentChanges = updateOldComponents(); const hasWorkflowChanges = updateCronSchedule(); -commitAndPushChanges(hasOldComponentChanges || hasWorkflowChanges); -updateUpcomingComponents(); // Run after commit, changes won't be committed +const success = commitAndPushChanges(hasOldComponentChanges || hasWorkflowChanges); +if (success) { + const hasUpcomingChanges = updateUpcomingComponents(); + if (hasUpcomingChanges) { + console.log('Upcoming component changes applied but not committed.'); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/images/main-page-carousel/rozdělení.pngis excluded by!**/*.pngsrc/images/main-page-carousel/vánoce.pngis excluded by!**/*.png
📒 Files selected for processing (4)
.github/workflows/buildndeploy.yml(2 hunks).github/workflows/prcheck.yml(2 hunks)expire-old-content.js(1 hunks)src/pages/Domov/Domov.js(2 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/buildndeploy.yml
[error] 9-9: trailing spaces
(trailing-spaces)
🪛 actionlint
.github/workflows/buildndeploy.yml
29-29: shellcheck reported issue in this script: SC2086:info:1:86: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
.github/workflows/prcheck.yml
22-22: shellcheck reported issue in this script: SC2086:info:1:86: Double quote to prevent globbing and word splitting
(shellcheck)
22-22: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
🪛 Biome
expire-old-content.js
[error] 27-27: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 39-39: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 96-96: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 104-104: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 150-150: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (5)
.github/workflows/prcheck.yml (1)
21-25:
Fix security vulnerabilities in branch handling
The branch handling steps have security issues:
- Using
github.event.pull_request.head.refdirectly in shell commands is unsafe - Lack of proper shell quoting could lead to command injection
However, these steps are entirely unnecessary as noted in the previous review - actions/checkout@v4 already handles PR branches automatically.
Let's verify the PR branch handling in actions/checkout:
#!/bin/bash
# Description: Verify that actions/checkout handles PR branches correctly
# Expected: Documentation confirming automatic PR branch checkout
rg -A 5 "pull_request|PR branch" .github/workflows/README.md || \
echo "For reference, see: https://github.com/actions/checkout#checkout-pull-request-head-commit"Apply this diff to remove the unnecessary and unsafe steps:
- - name: Determine Workflow Branch
- run: echo "TARGET_BRANCH=${{ github.event.pull_request.head.ref || github.ref_name }}" >> $GITHUB_ENV
-
- - name: Switch to Workflow Branch
- run: git checkout ${{ env.TARGET_BRANCH }}🧰 Tools
🪛 actionlint
22-22: shellcheck reported issue in this script: SC2086:info:1:86: Double quote to prevent globbing and word splitting
(shellcheck)
22-22: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
.github/workflows/buildndeploy.yml (2)
8-9: Empty schedule section issue persists.
This issue was previously identified in the past review comments.
🧰 Tools
🪛 yamllint
[error] 9-9: trailing spaces
(trailing-spaces)
11-11:
Verify the need for write permissions.
The change from read to write permissions for contents increases the security scope. Please confirm if write access is necessary for the deployment process.
✅ Verification successful
Write permissions are required for this workflow
The contents: write permission is justified as this workflow performs several operations that require write access:
- Deploys to GitHub Pages using
actions/deploy-pages@v4 - Uploads artifacts using
actions/upload-artifact@v4andactions/upload-pages-artifact@v3
These actions are part of the standard GitHub Pages deployment process and require write permissions to function correctly. The permission settings are appropriate for this deployment workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any steps in the workflow actually require write permissions
echo "Checking for write operations in the workflow..."
rg "uses: actions/(create|update|delete|push)" .github/workflows/buildndeploy.yml
Length of output: 190
Script:
#!/bin/bash
# Let's check the entire workflow file content to understand the deployment process
echo "Checking the complete workflow file..."
cat .github/workflows/buildndeploy.yml
# Also check for other potential write operations using broader patterns
echo -e "\nChecking for other potential write operations..."
rg -i "(deploy|publish|upload|push|gh-pages)" .github/workflows/buildndeploy.yml
Length of output: 2159
src/pages/Domov/Domov.js (1)
40-41: Verify the date-based content expiration implementation.
The images use custom attributes valid-from and valid-until for date-based visibility control. Let's verify how these attributes are handled by the expire-old-content.js script.
✅ Verification successful
Let me gather more information about the expire-old-content.js implementation to verify its functionality.
Date-based content expiration is properly implemented and integrated
The implementation in expire-old-content.js correctly handles both valid-from and valid-until attributes through:
- Pre-build processing that removes expired content (
valid-until< current date) - Runtime filtering of future content (
valid-from> current date) - Automatic cleanup of associated image files and imports
- GitHub Actions workflow updates with cron schedules for content transitions
The script is properly integrated into the build process via the prebuild npm script.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of date-based content expiration
# Test 1: Look for the expire-old-content.js script
echo "Looking for expire-old-content.js script..."
fd -e js -x cat {} \; | rg -A 10 "expire-old-content"
# Test 2: Check if these custom attributes are processed
echo "Checking for valid-from/valid-until attribute handling..."
rg -A 5 "valid-(from|until)"
# Test 3: Verify the script is integrated into the build process
echo "Checking build process integration..."
cat package.json
Length of output: 4384
Script:
#!/bin/bash
# Get the full implementation of expire-old-content.js
echo "Content of expire-old-content.js:"
cat expire-old-content.js
# Check if there are any other files using these date attributes
echo -e "\nChecking for other files using valid-from/valid-until:"
rg -l "valid-(from|until)" --type js
Length of output: 8315
expire-old-content.js (1)
156-159:
Validate cron expressions and handle workflow file structure
The current implementation lacks validation of cron expressions and assumes a specific workflow file structure. This could lead to invalid cron schedules or broken workflow files.
const cronEntries = Array.from(cronDates).map(dateStr => {
const date = new Date(dateStr);
+ if (Number.isNaN(date.getTime())) {
+ console.log(`WARNING! Invalid date format - ${dateStr} - Skipping cron entry.`);
+ return null;
+ }
+ // Validate that date components are within valid ranges
+ const day = date.getDate();
+ const month = date.getMonth() + 1;
+ if (day < 1 || day > 31 || month < 1 || month > 12) {
+ console.log(`WARNING! Invalid date components - ${dateStr} - Skipping cron entry.`);
+ return null;
+ }
return `0 0 ${date.getDate()} ${date.getMonth() + 1} *`;
-});
+}).filter(Boolean);Likely invalid or redundant comment.
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary complete history fetch
Setting fetch-depth: 0 fetches the complete repository history, which is unnecessary for PR checks and increases workflow execution time. The default shallow clone is sufficient here.
Apply this diff to remove the parameter:
uses: actions/checkout@v4
- with:
- fetch-depth: 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with: | |
| fetch-depth: 0 | |
| uses: actions/checkout@v4 |
| <img src={Vánoce} alt="" draggable="false" valid-from="2024-12-24" valid-until="2024-12-27" /> | ||
| <img src={rozdělení} alt="" draggable="false" valid-from="2025-01-01" valid-until="2025-01-02" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add descriptive alt text for accessibility.
Empty alt attributes (alt="") should be replaced with meaningful descriptions for screen readers. Consider adding descriptive alt text for both images.
- <img src={Vánoce} alt="" draggable="false" valid-from="2024-12-24" valid-until="2024-12-27" />
- <img src={rozdělení} alt="" draggable="false" valid-from="2025-01-01" valid-until="2025-01-02" />
+ <img src={Vánoce} alt="Christmas celebration image" draggable="false" valid-from="2024-12-24" valid-until="2024-12-27" />
+ <img src={rozdělení} alt="New Year celebration image" draggable="false" valid-from="2025-01-01" valid-until="2025-01-02" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <img src={Vánoce} alt="" draggable="false" valid-from="2024-12-24" valid-until="2024-12-27" /> | |
| <img src={rozdělení} alt="" draggable="false" valid-from="2025-01-01" valid-until="2025-01-02" /> | |
| <img src={Vánoce} alt="Christmas celebration image" draggable="false" valid-from="2024-12-24" valid-until="2024-12-27" /> | |
| <img src={rozdělení} alt="New Year celebration image" draggable="false" valid-from="2025-01-01" valid-until="2025-01-02" /> |
| console.log('Changes pushed to repository. Stopping further build process execution in favour of the new commit.'); | ||
| process.exit(1); | ||
| } else { |
There was a problem hiding this comment.
Avoid abrupt process exit and verify Git commands
Exiting the process immediately after pushing changes might interrupt cleanup tasks. Additionally, Git commands should be verified for success.
- console.log('Changes pushed to repository. Stopping further build process execution in favour of the new commit.');
- process.exit(1);
+ console.log('Changes pushed to repository.');
+ // Allow natural process completion for proper cleanup
+ return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('Changes pushed to repository. Stopping further build process execution in favour of the new commit.'); | |
| process.exit(1); | |
| } else { | |
| console.log('Changes pushed to repository.'); | |
| // Allow natural process completion for proper cleanup | |
| return true; | |
| } else { |
This is hopefully everything this update needs
RichardKanshen
force-pushed
the
old-content-expiration
branch
from
0c9abf5
to
c8ae1be
Compare
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (2)
.github/workflows/buildndeploy.yml (1)
Line range hint
53-56: Configure artifact retention period.The
webpify-resultsartifact upload lacks a retention configuration. Consider adding a retention period to manage storage and automatically clean up old artifacts:- name: Upload extra artifacts uses: actions/upload-artifact@v4 with: name: webpify-results path: ./webp-conversion-results.json + retention-days: 5🧰 Tools
🪛 yamllint
[error] 56-56: trailing spaces
(trailing-spaces)
expire-old-content.js (1)
5-7: Consider making file paths configurableThe hardcoded file paths reduce script reusability. Consider accepting these as command-line arguments or environment variables.
-const componentPath = './src/pages/Domov/Domov.js'; // React component with images -const workflowPath = './.github/workflows/buildndeploy.yml'; // GitHub Actions workflow +const componentPath = process.env.COMPONENT_PATH || './src/pages/Domov/Domov.js'; +const workflowPath = process.env.WORKFLOW_PATH || './.github/workflows/buildndeploy.yml';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/buildndeploy.yml(3 hunks).github/workflows/prcheck.yml(3 hunks)expire-old-content.js(1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/buildndeploy.yml
[error] 13-13: trailing spaces
(trailing-spaces)
🪛 actionlint
.github/workflows/buildndeploy.yml
35-35: shellcheck reported issue in this script: SC2086:info:1:86: Double quote to prevent globbing and word splitting
(shellcheck)
35-35: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
.github/workflows/prcheck.yml
24-24: shellcheck reported issue in this script: SC2086:info:1:86: Double quote to prevent globbing and word splitting
(shellcheck)
24-24: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
🪛 Biome
expire-old-content.js
[error] 27-27: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 39-39: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 96-96: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 104-104: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 150-150: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (4)
.github/workflows/buildndeploy.yml (2)
32-32:
Review the necessity of ACTIONS_TOKEN.
Using a custom ACTIONS_TOKEN instead of the default GITHUB_TOKEN:
- May grant broader permissions than necessary
- Requires manual secret management
- Could pose security risks if compromised
Consider using the default GITHUB_TOKEN unless there's a specific need for custom token permissions.
#!/bin/bash
# Check if any operations require elevated token permissions
echo "Checking for operations that might require elevated permissions..."
rg "uses: actions/" .github/workflows/buildndeploy.yml15-18:
Verify the necessity of elevated permissions.
The workflow has been granted broad write permissions:
contents: writeallows modifying repository contentactions: writeallows managing GitHub Actions
These elevated permissions could pose security risks if compromised. Please verify if all these write permissions are necessary for the workflow's functionality.
✅ Verification successful
The elevated permissions are justified for this workflow
After analyzing the complete workflow file, the write permissions are necessary for the following operations:
contents: write- Required for checkout with token and branch switchingpages: write- Required for GitHub Pages deploymentid-token: write- Required for GitHub Pages deployment authenticationactions: write- Required for artifact uploads and Pages deployment
The workflow performs legitimate operations that require these permissions:
- Uses
ACTIONS_TOKENfor repository checkout - Uploads artifacts using
actions/upload-artifact - Deploys to GitHub Pages using
actions/upload-pages-artifactandactions/deploy-pages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the workflow actually needs write permissions
echo "Checking for write operations in the workflow..."
rg "git (push|commit|config)|actions/create|actions/update" .github/workflows/buildndeploy.yml
Length of output: 508
Script:
#!/bin/bash
# Let's examine the entire workflow file to understand the actions and their requirements
echo "Examining the complete workflow file..."
cat .github/workflows/buildndeploy.yml
# Also check for any documentation about the workflow
echo -e "\nChecking for workflow documentation..."
if [ -f .github/workflows/README.md ]; then
cat .github/workflows/README.md
fi
Length of output: 1895
expire-old-content.js (2)
229-233: LGTM! Well-structured execution flow
The execution order is logical, and change tracking is properly implemented.
214-215:
Use local Git configuration instead of global
Using --global affects the entire system. Use local configuration instead.
- execSync(`git config --global user.email "41898282+github-actions[bot]@users.noreply.github.com"`);
- execSync(`git config --global user.name "Anti-Social Bot"`);
+ execSync(`git config user.email "41898282+github-actions[bot]@users.noreply.github.com"`);
+ execSync(`git config user.name "Anti-Social Bot"`);Likely invalid or redundant comment.
| @@ -12,6 +16,15 @@ jobs: | |||
| steps: | |||
| - name: Checkout Repository | |||
| uses: actions/checkout@v4 | |||
| with: | |||
| fetch-depth: 0 | |||
| token: ${{ secrets.ACTIONS_TOKEN }} | |||
There was a problem hiding this comment.
💡 Codebase verification
Use GITHUB_TOKEN instead of ACTIONS_TOKEN
The workflow only performs standard repository operations (checkout, build, and artifact uploads) which are fully supported by the default GITHUB_TOKEN. The defined permissions (contents: write, actions: write) are already sufficient for these operations when using GITHUB_TOKEN. Using a custom ACTIONS_TOKEN introduces unnecessary security risks and token management overhead.
🔗 Analysis chain
Review token usage
Using ACTIONS_TOKEN instead of the default GITHUB_TOKEN requires justification. The default token provides sufficient permissions for most operations and automatically expires.
Consider using the default GITHUB_TOKEN unless there's a specific need for extended permissions.
Let's examine the workflow files to understand the token usage context and see if there are other token patterns that might justify or contradict the use of ACTIONS_TOKEN.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ACTIONS_TOKEN is used elsewhere in the repository
rg -l "ACTIONS_TOKEN"
Length of output: 21
Script:
#!/bin/bash
# Check for any secret or token references in GitHub workflow files
fd -e yml -e yaml -p '.github/workflows/' --exec grep -l -i "token\|secret" {}
# Check the specific workflow file content
cat .github/workflows/prcheck.yml
Length of output: 1379
There was a problem hiding this comment.
ACTIONS_TOKEN also has workflows: write. Is that also included in the GITHUB_TOKEN?
There was a problem hiding this comment.
@RichardKanshen, yes, the GITHUB_TOKEN can include workflows: write permissions when specified in the permissions section of your workflow file. You can modify your permissions like this:
permissions:
contents: write
actions: write
workflows: writeThis will grant the GITHUB_TOKEN the necessary workflows: write permission, allowing it to perform actions on workflows. Therefore, you can use the GITHUB_TOKEN instead of the ACTIONS_TOKEN, which simplifies token management and enhances security by avoiding the use of a custom token.
As you mentioned earlier, your build process edits and commits changes to workflow files, requiring contents: write permissions. By adding workflows: write, the GITHUB_TOKEN will have all the permissions needed for your workflow to function correctly.
Let me know if you have any questions or need further assistance.
🧠 Learnings used
Learnt from: RichardKanshen
PR: CAsocialu/CAsocialu.github.io#20
File: .github/workflows/prcheck.yml:3-5
Timestamp: 2024-11-10T15:59:00.411Z
Learning: In this project, the build process in `.github/workflows/prcheck.yml` edits and commits changes to workflow files, requiring `contents: write` permissions.
| console.log('Changes pushed to repository. Stopping further build process execution in favour of the new commit.'); | ||
| process.exit(1); |
There was a problem hiding this comment.
Avoid abrupt process termination
Using process.exit(1) can interrupt cleanup tasks and indicates an error condition.
- console.log('Changes pushed to repository. Stopping further build process execution in favour of the new commit.');
- process.exit(1);
+ console.log('Changes pushed to repository.');
+ return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('Changes pushed to repository. Stopping further build process execution in favour of the new commit.'); | |
| process.exit(1); | |
| console.log('Changes pushed to repository.'); | |
| return true; |
There was a problem hiding this comment.
process.exit(1) is there to stop the build process, as some changes were committed to the repo, which causes another build to start.
There was a problem hiding this comment.
@RichardKanshen, understood. Thank you for explaining the purpose of using process.exit(1) to halt the build process when changes are committed. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: RichardKanshen
PR: CAsocialu/CAsocialu.github.io#20
File: expire-old-content.js:219-220
Timestamp: 2024-11-10T16:08:18.249Z
Learning: In the `commitAndPushChanges` function within `expire-old-content.js`, `process.exit(1)` is intentionally used to stop the build process when changes are committed to the repository, triggering another build.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (fs.existsSync(fullImagePath)) { | ||
| fs.unlinkSync(fullImagePath); | ||
| console.log(`Deleted expired image file: ${fullImagePath}`); | ||
| } |
There was a problem hiding this comment.
Add backup mechanism before deleting files
Directly deleting files without backup could lead to data loss if there are issues during execution.
try {
if (fs.existsSync(fullImagePath)) {
+ const backupPath = `${fullImagePath}.backup`;
+ fs.copyFileSync(fullImagePath, backupPath);
fs.unlinkSync(fullImagePath);
console.log(`Deleted expired image file: ${fullImagePath}`);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I don't think this is necessary. These images will still be in the git history, and when they are removed in this process, they are not needed anymore.
There was a problem hiding this comment.
@RichardKanshen, understood. If the images are no longer needed and can be retrieved from the Git history if necessary, then adding a backup mechanism may not be necessary. Thank you for the clarification.
✏️ Learnings added
Learnt from: RichardKanshen
PR: CAsocialu/CAsocialu.github.io#20
File: expire-old-content.js:61-64
Timestamp: 2024-11-10T16:07:44.834Z
Learning: In `expire-old-content.js`, when deleting expired image files that are no longer needed and can be retrieved from Git history, a backup mechanism is not necessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
RichardKanshen
force-pushed
the
old-content-expiration
branch
from
cb9e9f7
to
c1d0d22
Compare
Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
RichardKanshen
force-pushed
the
old-content-expiration
branch
2 times, most recently
from
12f25c8
to
e654178
Compare
I FUCKING DISCARDED MY LAST WORKING SCRIPT RIGHT BEFORE COMMITTING IT I WILL ACTUALLY KILL MYSELF
anyways... :'3
I hope this works.
We will see.
My computer is acting up, and some parts of webpify do not seem to work all of a sudden? so that's fun. tried fixing that, hopefully it works.
Summary by CodeRabbit
New Features
Domovcomponent, enhancing visual content.Bug Fixes
Chores