refactor ( website-builder ): Refactor website-builder to typescript #5
Conversation
The index.js file is now refactored to index.ts and a tsconfig.json has been added along with required type packages as dev dependencies closes #1054
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for contributing!
Please make sure the actions pass. Once your pull request has been merged, you will automatically be added to the contributor's list on the website.
If you want to be shouted out on Linkedin, have a look for the shoutout issue of the specific version.
You can also find us on slack at idrinth-api-bench.
WalkthroughThe overall changes involve converting import statements to use Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 5
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (3)
- index.ts (2 hunks)
- package.json (2 hunks)
- tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (2)
- package.json
- tsconfig.json
Additional Context Used
Path-based Instructions (1)
index.ts (1)
Pattern
**/*.ts: Review the code for conformity with the .eslintrc files in / and /*/.
Additional comments not posted (3)
index.ts (3)
1-4: CodeRabbit observes that the import syntax has been changed from ES modules to CommonJS. This is consistent with the removal of"type": "module"inpackage.json. Ensure that all dependent modules and build configurations are compatible with CommonJS to avoid runtime issues.
11-17: CodeRabbit notes the addition of explicit type annotations for boolean and number types. This enhances type safety and is in line with TypeScript best practices.
79-106: CodeRabbit notes the logic inbuildProjectfunction handles various build conditions. Ensure that the logic correctly reflects the intended behaviour, especially the conditions under which each build is triggered.
| }; | ||
|
|
||
| const initiateBuild = async (command, projectPath, destPath) => { | ||
| const initiateBuild = async (command: string, projectPath: string, destPath: string) => { |
There was a problem hiding this comment.
CodeRabbit suggests refactoring the initiateBuild function to reduce redundancy and potential errors in command execution paths.
- await executeCmd(`cd ${projectPath}/ && git pull`);
- await executeCmd(`cd ${projectPath}/ && npm ci`);
- await executeCmd(`cd ${projectPath}/ && ${command}`);
- await executeCmd(`cp -r ${projectPath}/dist/ ${destPath}/`);
+ const commands = ['git pull', 'npm ci', `${command}`, `cp -r dist/ ${destPath}/`];
+ for (const cmd of commands) {
+ await executeCmd(`cd ${projectPath}/ && ${cmd}`);
+ }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.
| const initiateBuild = async (command: string, projectPath: string, destPath: string) => { | |
| const initiateBuild = async (command: string, projectPath: string, destPath: string) => { | |
| const commands = ['git pull', 'npm ci', `${command}`, `cp -r dist/ ${destPath}/`]; | |
| for (const cmd of commands) { | |
| await executeCmd(`cd ${projectPath}/ && ${cmd}`); | |
| } |
| const getBranchStatus = async (req: any) => { | ||
| console.log("Webhook received successfully"); | ||
|
|
||
| const branchName = req.body?.ref?.split("/").pop(); | ||
| if (!branchName) { | ||
| return 400, "Branch name not found in the request."; | ||
| return {result: 400, respMessage: "Branch name not found in the request."}; | ||
| } | ||
| return branchName === process.env.BRANCH_NAME ? await buildProject() : 202, "Build not required."; | ||
| return branchName === process.env.BRANCH_NAME ? await buildProject() : {result: 202, respMessage: "Build not required."}; |
There was a problem hiding this comment.
CodeRabbit suggests improving the type safety of the req parameter in getBranchStatus. Currently, it is typed as any, which defeats the purpose of using TypeScript for type safety.
- const getBranchStatus = async (req: any) => {
+ const getBranchStatus = async (req: express.Request) => {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.
| const getBranchStatus = async (req: any) => { | |
| console.log("Webhook received successfully"); | |
| const branchName = req.body?.ref?.split("/").pop(); | |
| if (!branchName) { | |
| return 400, "Branch name not found in the request."; | |
| return {result: 400, respMessage: "Branch name not found in the request."}; | |
| } | |
| return branchName === process.env.BRANCH_NAME ? await buildProject() : 202, "Build not required."; | |
| return branchName === process.env.BRANCH_NAME ? await buildProject() : {result: 202, respMessage: "Build not required."}; | |
| const getBranchStatus = async (req: express.Request) => { | |
| console.log("Webhook received successfully"); | |
| const branchName = req.body?.ref?.split("/").pop(); | |
| if (!branchName) { | |
| return {result: 400, respMessage: "Branch name not found in the request."}; | |
| } | |
| return branchName === process.env.BRANCH_NAME ? await buildProject() : {result: 202, respMessage: "Build not required."}; |
| app.post("/webhook", async (req: express.Request, res: express.Response) => { | ||
| console.log("req receieved"); | ||
| const signature = req.headers["x-hub-signature"]; | ||
| const payload = JSON.stringify(req.body); | ||
|
|
||
| const hmac = crypto.createHmac("sha1", process.env.GITHUB_SECRET); | ||
| const hmac = crypto.createHmac("sha1", process.env.GITHUB_SECRET as string); | ||
| const calculatedSignature = `sha1=${hmac.update(payload).digest("hex")}`; | ||
|
|
||
| if (crypto.timingSafeEqual(Buffer.from(signature), Buffer.from(calculatedSignature))) { | ||
| const { result, respMessage } = await getBranchStatus(); | ||
| if (crypto.timingSafeEqual(Buffer.from(signature as string), Buffer.from(calculatedSignature))) { | ||
| const { result, respMessage } = await getBranchStatus(null); |
There was a problem hiding this comment.
CodeRabbit identifies a potential security risk. The signature from req.headers["x-hub-signature"] should be validated for existence before being used. This prevents potential runtime errors when the header is missing.
- if (crypto.timingSafeEqual(Buffer.from(signature as string), Buffer.from(calculatedSignature))) {
+ if (signature && crypto.timingSafeEqual(Buffer.from(signature as string), Buffer.from(calculatedSignature))) {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.
| app.post("/webhook", async (req: express.Request, res: express.Response) => { | |
| console.log("req receieved"); | |
| const signature = req.headers["x-hub-signature"]; | |
| const payload = JSON.stringify(req.body); | |
| const hmac = crypto.createHmac("sha1", process.env.GITHUB_SECRET); | |
| const hmac = crypto.createHmac("sha1", process.env.GITHUB_SECRET as string); | |
| const calculatedSignature = `sha1=${hmac.update(payload).digest("hex")}`; | |
| if (crypto.timingSafeEqual(Buffer.from(signature), Buffer.from(calculatedSignature))) { | |
| const { result, respMessage } = await getBranchStatus(); | |
| if (crypto.timingSafeEqual(Buffer.from(signature as string), Buffer.from(calculatedSignature))) { | |
| const { result, respMessage } = await getBranchStatus(null); | |
| app.post("/webhook", async (req: express.Request, res: express.Response) => { | |
| console.log("req receieved"); | |
| const signature = req.headers["x-hub-signature"]; | |
| const payload = JSON.stringify(req.body); | |
| const hmac = crypto.createHmac("sha1", process.env.GITHUB_SECRET as string); | |
| const calculatedSignature = `sha1=${hmac.update(payload).digest("hex")}`; | |
| if (signature && crypto.timingSafeEqual(Buffer.from(signature as string), Buffer.from(calculatedSignature))) { | |
| const { result, respMessage } = await getBranchStatus(null); |
| const executeCmd = async (cmd: string) => { | ||
| let stdout; | ||
| let stderr; | ||
| try { | ||
| const { stdout, stderr } = await exec(cmd); | ||
| const cmdResp = await exec(cmd); | ||
| stderr = cmdResp.stderr; | ||
| stdout = cmdResp.stdout |
There was a problem hiding this comment.
CodeRabbit detects an issue with error handling. The error thrown in the catch block should include more contextual information to aid in debugging. Additionally, consider logging the command that failed.
- throw new Error(stderr + "\n" + stdout);
+ throw new Error(`Command failed: ${cmd}\nError: ${stderr}\nOutput: ${stdout}`);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.
| const executeCmd = async (cmd: string) => { | |
| let stdout; | |
| let stderr; | |
| try { | |
| const { stdout, stderr } = await exec(cmd); | |
| const cmdResp = await exec(cmd); | |
| stderr = cmdResp.stderr; | |
| stdout = cmdResp.stdout | |
| const executeCmd = async (cmd: string) => { | |
| let stdout; | |
| let stderr; | |
| try { | |
| const cmdResp = await exec(cmd); | |
| stderr = cmdResp.stderr; | |
| stdout = cmdResp.stdout; | |
| } catch (error) { | |
| throw new Error(`Command failed: ${cmd}\nError: ${stderr}\nOutput: ${stdout}`); | |
| } | |
| }; |
| const currentTime: number = Date.now(); | ||
| let mindmapUpdateTimeInterval = parseInt(process.env.MINDMAP_UPDATE_TIME_INTERVAL as string) | ||
| let docWebsiteUpdateTimeInterval = parseInt(process.env.DOCUMENTATION_WEBSITE_UPDATE_TIME_INTERVAL as string) | ||
|
|
||
| isMindmapUpdated = (currentTime - mindmapBuildTime) / 1000 / 60 > mindmapUpdateTimeInterval ? true : false; | ||
| isDocumentationWebsiteUpdated = (currentTime - documentationWebsiteBuildTime) / 1000 / 60 > docWebsiteUpdateTimeInterval ? true : false; |
There was a problem hiding this comment.
Tip
Codebase Verification
CodeRabbit observes the use of environment variables without documentation or validation. Ensure these variables are documented and validated to prevent runtime errors due to missing or incorrect configurations.
MINDMAP_UPDATE_TIME_INTERVALDOCUMENTATION_WEBSITE_UPDATE_TIME_INTERVALCONTRIBUTORS_UPDATE_TIME_INTERVAL
Analysis chain
CodeRabbit observes the use of environment variables for configuration. Ensure these variables are documented and validated to prevent runtime errors due to missing or incorrect configurations.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all required environment variables are documented.
# Test: Search for documentation of environment variables. Expect: Documentation entries for each variable used.
rg --type markdown 'MINDMAP_UPDATE_TIME_INTERVAL|DOCUMENTATION_WEBSITE_UPDATE_TIME_INTERVAL|CONTRIBUTORS_UPDATE_TIME_INTERVAL'
Length of output: 128
Script:
#!/bin/bash
# Description: Verify the usage and validation of environment variables in the codebase.
# Search for the usage of the environment variables in the codebase.
rg 'process\.env\.MINDMAP_UPDATE_TIME_INTERVAL|process\.env\.DOCUMENTATION_WEBSITE_UPDATE_TIME_INTERVAL|process\.env\.CONTRIBUTORS_UPDATE_TIME_INTERVAL'
# Search for validation logic related to these environment variables.
rg 'if\s*\(.*process\.env\.MINDMAP_UPDATE_TIME_INTERVAL.*\)|if\s*\(.*process\.env\.DOCUMENTATION_WEBSITE_UPDATE_TIME_INTERVAL.*\)|if\s*\(.*process\.env\.CONTRIBUTORS_UPDATE_TIME_INTERVAL.*\)'
Length of output: 681
| import express = require("express"); | ||
| import {exec} from "child_process"; | ||
| import crypto from "crypto"; | ||
| import dotenv from "dotenv"; | ||
| import crypto = require("crypto"); | ||
| import dotenv = require("dotenv"); |
There was a problem hiding this comment.
proper imports please, no requires.
| const cmdResp = await exec(cmd); | ||
| stderr = cmdResp.stderr; | ||
| stdout = cmdResp.stdout |
There was a problem hiding this comment.
please revert the structural change, the extra lines for splitting are unnecessary :)
| return stderr + "\n" + stdout; | ||
| } catch (error) { | ||
| console.error(`exec error: ${error}`); | ||
| throw new Error(stderr + "\n" + stdout); | ||
| } | ||
| }; | ||
|
|
||
| const getBranchStatus = async (req) => { | ||
| const getBranchStatus = async (req: any) => { |
There was a problem hiding this comment.
no use of any please. Create an interface for the github token please
| let mindmapUpdateTimeInterval = parseInt(process.env.MINDMAP_UPDATE_TIME_INTERVAL as string) | ||
| let docWebsiteUpdateTimeInterval = parseInt(process.env.DOCUMENTATION_WEBSITE_UPDATE_TIME_INTERVAL as string) | ||
|
|
||
| isMindmapUpdated = (currentTime - mindmapBuildTime) / 1000 / 60 > mindmapUpdateTimeInterval ? true : false; |
There was a problem hiding this comment.
remove the ? true : false, it doesn't serve a benefit
| let docWebsiteUpdateTimeInterval = parseInt(process.env.DOCUMENTATION_WEBSITE_UPDATE_TIME_INTERVAL as string) | ||
|
|
||
| isMindmapUpdated = (currentTime - mindmapBuildTime) / 1000 / 60 > mindmapUpdateTimeInterval ? true : false; | ||
| isDocumentationWebsiteUpdated = (currentTime - documentationWebsiteBuildTime) / 1000 / 60 > docWebsiteUpdateTimeInterval ? true : false; |
There was a problem hiding this comment.
remove the ? true : false, it doesn't serve a benefit
| @@ -3,7 +3,6 @@ | |||
| "version": "1.0.0", | |||
| "description": "This repository is our website deploy and update tool to minimize github api queries.", | |||
| "main": "index.js", | |||
| "type" : "module", | |||
| @@ -0,0 +1,111 @@ | |||
| { | |||
There was a problem hiding this comment.
this does not follow our usual tsconfigs, please take the one from the framework as baseline.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return isMindmapUpdated || isDocumentationWebsiteUpdated; | ||
| }; | ||
|
|
||
| const buildProject = async () => { | ||
| const currentTime = Date.now(); | ||
| if (!isUpdateRequired()) { | ||
| if (contributorsBuildRequired || (currentTime - contributorsBuildTime) / 1000 / 60 > process.env.CONTRIBUTORS_UPDATE_TIME_INTERVAL) { | ||
| let contUpdateTimeInterval = parseInt(process.env.CONTRIBUTORS_UPDATE_TIME_INTERVAL as string) |
Check notice
Code scanning / CodeQL
Semicolon insertion Note
| try { | ||
| const { stdout, stderr } = await exec(cmd); | ||
| const cmdResp = await exec(cmd); |
Check warning
Code scanning / CodeQL
Indirect uncontrolled command line Medium
The index.js file is now refactored to index.ts and a tsconfig.json has been added along with required type packages as dev dependencies
closes #1054
The Pull Request is ready
Overview
index.js is now refactored to index.ts, tsconfig.json has been added for the typescript compiler
Website Builder