ci: Add GH workflow to build app & deploy to GH Pages; Update demo link to point at GH Pages deployment. #114
Conversation
WalkthroughThe pull request includes updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
junhaoliao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
.github/workflows/deploy-gh-pages.yml (1)
29-29: Remove trailing space in npm install command.Apply this diff:
- run: "npm install " + run: "npm install"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/deploy-gh-pages.yml(1 hunks)README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
🪛 actionlint
.github/workflows/deploy-gh-pages.yml
20-20: shellcheck reported issue in this script: SC2016:info:1:6: Expressions don't expand in single quotes, use double quotes for that
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:1:39: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/deploy-gh-pages.yml (3)
1-6: LGTM! Workflow trigger configuration is appropriate.
The workflow is correctly configured to run only on pushes to the main branch, which is the standard practice for GitHub Pages deployments.
37-51: LGTM! Deploy job configuration follows best practices.
The deployment configuration is well-structured with:
- Appropriate permission scoping for Pages and ID token
- Correct job dependencies
- Proper environment configuration
32-35: Verify build output and deployment artifacts.
Let's ensure the build process generates the expected files in the dist directory.
✅ Verification successful
Let me gather more information about the webpack configuration and build process.
Let me check the webpack configuration to verify the build output settings.
Build configuration and deployment path are properly configured
The verification confirms that:
- Webpack is correctly configured to output build files to
./distdirectory - The build process includes proper bundling of assets, styles, and scripts
- Production build configuration is set up with appropriate optimizations
- HTML template and static assets are properly handled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify build configuration and output directory
# Test 1: Check if build script is defined in package.json
echo "Checking build script configuration:"
jq '.scripts.build' package.json
# Test 2: Check if dist directory is specified in .gitignore
echo -e "\nVerifying .gitignore configuration:"
rg "^dist/?$" .gitignore
# Test 3: Look for any existing build configuration files
echo -e "\nChecking for build configuration files:"
fd -e config.js -e config.ts -e vite.config.js -e webpack.config.js
Length of output: 429
Script:
#!/bin/bash
# Check webpack configuration files and dist directory structure
echo "Checking for webpack config files:"
fd webpack
echo -e "\nChecking package.json dependencies for webpack:"
jq '.dependencies + .devDependencies | with_entries(select(.key | contains("webpack")))' package.json
echo -e "\nChecking for any existing dist directory structure:"
fd -t d dist
echo -e "\nChecking for HTML entry points:"
fd -e html
Length of output: 925
Script:
#!/bin/bash
# Check webpack production configuration
echo "Checking webpack.prod.js configuration:"
cat webpack.prod.js
echo -e "\nChecking webpack.common.js configuration:"
cat webpack.common.js
echo -e "\nChecking index.html template:"
cat public/index.html
Length of output: 6365
| - uses: "actions/setup-node@v4" | ||
| with: | ||
| node-version: "22" |
There was a problem hiding this comment.
Consider using a stable Node.js version.
Node.js 22 is currently in pre-release status. For production deployments, it's recommended to use the current LTS version (20.x).
Apply this diff:
- node-version: "22"
+ node-version: "20"📝 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.
| - uses: "actions/setup-node@v4" | |
| with: | |
| node-version: "22" | |
| - uses: "actions/setup-node@v4" | |
| with: | |
| node-version: "20" |
| - name: "Get npm cache directory" | ||
| id: "get-npm-cache-dir" | ||
| run: "echo 'dir=$(npm config get cache)' >> ${GITHUB_OUTPUT}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix shell script quoting issues.
The echo command should use double quotes to allow variable expansion and prevent word splitting.
Apply this diff:
- run: "echo 'dir=$(npm config get cache)' >> ${GITHUB_OUTPUT}"
+ run: "echo \"dir=$(npm config get cache)\" >> \"${GITHUB_OUTPUT}\""📝 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.
| - name: "Get npm cache directory" | |
| id: "get-npm-cache-dir" | |
| run: "echo 'dir=$(npm config get cache)' >> ${GITHUB_OUTPUT}" | |
| - name: "Get npm cache directory" | |
| id: "get-npm-cache-dir" | |
| run: "echo \"dir=$(npm config get cache)\" >> \"${GITHUB_OUTPUT}\"" |
🧰 Tools
🪛 actionlint
20-20: shellcheck reported issue in this script: SC2016:info:1:6: Expressions don't expand in single quotes, use double quotes for that
(shellcheck)
20-20: shellcheck reported issue in this script: SC2086:info:1:39: Double quote to prevent globbing and word splitting
(shellcheck)
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
| build: | ||
| runs-on: "ubuntu-latest" | ||
| steps: | ||
| - uses: "actions/checkout@v4" |
There was a problem hiding this comment.
Shall we add
with:
submodules: "recursive"
here or remove it in ?
yscope-log-viewer/.github/workflows/lint.yaml
Lines 26 to 27 in bc8109f
There was a problem hiding this comment.
I would either add it here or leave both as is. Although we don't need it in lint.yaml currently, hopefully we'll add yscope-dev-utils to this repo soon in service of adding more automated linting. So in that case, changing lint.yaml would be unnecessary; I'm not sure whether we would need to change deploy-gh-pages.yml at that point.
Speaking of, I just noticed that we should rename this workflow file with .yaml.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
.github/workflows/deploy-gh-pages.yaml (2)
21-23: Enhance build step reliability and performance.Add error handling and caching for build outputs:
- - run: "npm install" + - name: "Install dependencies" + run: "npm ci" + if: steps.npm-cache.outputs.cache-hit != 'true' - - run: "npm run build" + - name: "Build application" + run: "npm run build" + id: build + continue-on-error: false
30-45: Enhance deployment reliability with timeouts and verification.Add timeout and post-deployment verification:
deploy: needs: "build" + timeout-minutes: 10 permissions: pages: "write" id-token: "write" runs-on: "ubuntu-latest" environment: name: "github-pages" url: "${{steps.deployment.outputs.page_url}}" steps: - name: "Deploy to GitHub Pages" id: "deployment" uses: "actions/deploy-pages@v4" + - name: "Verify deployment" + run: | + # Wait for DNS propagation + sleep 30 + # Verify deployment is accessible + curl --retry 3 --retry-delay 5 -sSf "${{steps.deployment.outputs.page_url}}" > /dev/null
| - uses: "actions/setup-node@v4" | ||
| with: | ||
| node-version: "22" | ||
| cache: "npm" |
There was a problem hiding this comment.
Use LTS version of Node.js for production deployments.
Node.js v22 is currently in beta. For production deployments, use an LTS version:
- uses: "actions/setup-node@v4"
with:
- node-version: "22"
+ node-version: "20"
cache: "npm"📝 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.
| - uses: "actions/setup-node@v4" | |
| with: | |
| node-version: "22" | |
| cache: "npm" | |
| - uses: "actions/setup-node@v4" | |
| with: | |
| node-version: "20" | |
| cache: "npm" |
There was a problem hiding this comment.
Usage of "22" is intended.
There was a problem hiding this comment.
@junhaoliao, understood. I've noted that using Node.js version 22 is intended for this project.
(꒦_꒦)
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#114
File: .github/workflows/deploy-gh-pages.yaml:24-27
Timestamp: 2024-11-09T21:02:27.209Z
Learning: In the yscope-log-viewer project, specifically in the `.github/workflows/deploy-gh-pages.yaml` file, Node.js version 22 is intentionally used in GitHub Actions workflows.
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.
| name: "deploy-github-pages" | ||
|
|
||
| on: | ||
| push: | ||
| branches: ["main"] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add workflow concurrency control to prevent deployment conflicts.
To prevent race conditions when multiple workflow runs are triggered, add concurrency control:
name: "deploy-github-pages"
on:
push:
branches: ["main"]
+concurrency:
+ group: "pages"
+ cancel-in-progress: 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.
| name: "deploy-github-pages" | |
| on: | |
| push: | |
| branches: ["main"] | |
| name: "deploy-github-pages" | |
| on: | |
| push: | |
| branches: ["main"] | |
| concurrency: | |
| group: "pages" | |
| cancel-in-progress: true |
There was a problem hiding this comment.
I pushed a fix in commit ba82b3a, please review it.
There was a problem hiding this comment.
@junhaoliao, thank you for adding the concurrency control to the workflow. This should effectively prevent deployment conflicts. The fix looks good.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
junhaoliao
changed the title
junhaoliao
changed the title
| permissions: | ||
| # So the workflow can cancel in-progress jobs | ||
| actions: "write" |
There was a problem hiding this comment.
This is my mistake in previous workflow PRs, but I found that this permission is not necessary for cancelling in-progress jobs. I think we should set these permissions to {} instead.
References
Resolves #102 (comment)
Description
Validation performed
On @junhaoliao 's fork:
Summary by CodeRabbit
New Features
Documentation