chore: add basic test to ci #80
Conversation
basic test: run packaged app without errors for 10 secs
WalkthroughThe CI workflow was updated to consolidate dependency installation and package building steps, and to introduce a new macOS-specific job that builds a DMG application, installs and runs it, checks its output logs for errors, and uploads the logs as artifacts. Changes
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Claude finished @maceip's task —— View job PR Review: Basic Test Addition to CITodo List:
Review FeedbackThis PR adds a basic smoke test to verify the packaged macOS app can run without errors for 10 seconds. Here's my analysis: ✅ Positive Aspects
|
![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
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
56-60: Remember to detach the DMG and clean up mountsAfter copying the app, detach the volume to free the mount point and avoid side-effects when jobs run locally.
- sudo cp -R "/Volumes/COBROWSER/vibe.app" /Applications/ + sudo cp -R "/Volumes/COBROWSER/vibe.app" /Applications/ + hdiutil detach "/Volumes/COBROWSER"
61-71: Log scan is prone to false positives & masking exit codes
grep -i "error"will also match harmless lines like “No error found”.
Considergrep -iE '\berror\b'.kill $APP_PIDmay leave children running; preferkill -TERM -- -$APP_PIDorpkill -P $APP_PID.set +edisables pipe-fail; wrap only the grep in a subshell to keep other failures visible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
74-74: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
74-76: Updateactions/upload-artifactto v4
actionlintflags v3 as deprecated for new runners. A one-line bump keeps the workflow future-proof.- uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Claude finished @maceip's task —— View job PR Review: Basic Test Addition to CITodo List:
Review FeedbackThis PR adds a basic smoke test to verify the packaged macOS app can run without errors for 10 seconds. Here's my analysis: 🚨 Critical IssuesYAML Structure Problem: Dependency Installation Order:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
28-42:pnpm installstill executed too late – CI will run without dependenciesThe install step is after the quality gates; on a clean runner
node_modulesis missing andlint / typecheck / format / testwill fail first.
Move the install step up and remove the duplicate call:- name: Setup Node.js uses: actions/setup-node@v4 with: node-version: 20 cache: 'pnpm' + - name: Install dependencies + run: pnpm install --frozen-lockfile - name: Run linting run: pnpm lint … - - name: Run tests - run: pnpm test - - - name: Install dependencies - run: pnpm install --frozen-lockfile + - name: Run tests + run: pnpm test
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
65-79: Hard-coded DMG volume name – copy will break if it changes
/Volumes/COBROWSER/vibe.appassumes the volume is always “COBROWSER”. Use the mount-point returned byhdiutil:MOUNT_POINT=$(hdiutil attach apps/electron-app/dist/vibe-*.dmg | awk '/\/Volumes\//{print $3;exit}') sudo cp -R "$MOUNT_POINT/vibe.app" /Applications/
70-80: Unprotectedkillmay mask real errorsIf the app exits early
kill $APP_PIDreturns non-zero underset +e, potentially hiding problems. Guard it:if ps -p "$APP_PID" >/dev/null; then kill "$APP_PID"; fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
46-46: could not parse as YAML: yaml: line 46: did not find expected key
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 45-45: duplication of key "jobs" in mapping
(key-duplicates)
[error] 47-47: syntax error: expected , but found '-'
(syntax)
[warning] 89-89: too many blank lines (1 > 0)
(empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Claude finished @maceip's task —— View job PR Review: Basic Test Addition to CITodo List:
Review FeedbackThis PR adds a basic smoke test to verify the packaged macOS app can run without errors for 10 seconds. Here's my analysis: 🚨 Critical IssuesYAML Structure Problem: Dependency Installation Order: Missing Node.js/pnpm Setup:
|
|
Claude finished @maceip's task —— View job PR Review: Basic Test Addition to CITodo List:
Review FeedbackThis PR adds a basic smoke test to verify the packaged macOS app can run without errors for 10 seconds. Here's my analysis: 🚨 Critical IssuesDependency Installation Order:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
.github/workflows/ci.yml (1)
28-42: Dependency installation happens after commands that require them – CI will fail
pnpm lint,pnpm typecheck,pnpm format:checkandpnpm testrun beforepnpm install --frozen-lockfile, sonode_moduleswill be missing on a fresh runner.- name: Setup Node.js uses: actions/setup-node@v4 with: node-version: 20 cache: 'pnpm' + - name: Install dependencies + run: pnpm install --frozen-lockfile - - name: Run linting - run: pnpm lint + - name: Run linting + run: pnpm lint … - - name: Run tests - run: pnpm test - - - name: Install dependencies - run: pnpm install --frozen-lockfile + - name: Run tests + run: pnpm testMove the install step up (and drop the later duplicate) so every command has its dependencies available.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
70-76: DMG mount/CP step is brittle and leaves the image attached
"/Volumes/COBROWSER/vibe.app"hard-codes the volume name; codesign/distribution often changes it.- The workflow never detaches the DMG, leaving the runner in a dirty state (subsequent CI steps may fail).
Consider:
hdiutil attach apps/electron-app/dist/*.dmg -nobrowse -mountpoint /Volumes/vibe sudo cp -R /Volumes/vibe/vibe.app /Applications/ hdiutil detach /Volumes/vibe(You can compute the mount-point returned by
hdiutil attach -plistfor 100 % robustness.)
82-85:grep -i "error"picks up false positivesAny line containing “error” (even inside “No errors found”) fails the build. Narrow the pattern, e.g.:
if grep -Ei "\berror\b" app_output.log; thenor whitelist benign messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
48-48: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
89-89: the runner of "actions/upload-artifact@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Claude finished @maceip's task —— View job PR Review: Basic Test Addition to CITodo List:
Review FeedbackThis PR adds a basic smoke test to verify the packaged macOS app can run without errors for 10 seconds. Based on the context and previous reviews, here's my comprehensive analysis: 🚨 Critical IssuesYAML Structure Problem: Dependency Installation Order: Missing Node.js/pnpm Setup:
|
|
Claude finished @maceip's task —— View job PR Review: Basic Test Addition to CITodo List:
Review FeedbackThis PR adds a basic smoke test to verify the packaged macOS app can run without errors for 10 seconds. Based on the context and previous reviews, here's my comprehensive analysis: 🚨 Critical IssuesDependency Installation Order:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
37-41: Dependencies are installed after they are needed – tests will fail on a clean runner
pnpm test(and the earlier lint / type-check / format steps) requirenode_modules, yet thepnpm install --frozen-lockfilestep is placed after them.
This is the same issue raised in the previous review – it remains unresolved.- - name: Run linting - run: pnpm lint + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Run linting + run: pnpm lint … - - name: Run tests - run: pnpm test + - name: Run tests + run: pnpm testMove the install step before any command that touches the workspace and drop the duplicate later.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
75-86: Minor: simplify log scan & avoid false positivesUsing plain
grep -i "error"can flag benign lines (e.g., “Electron Security Warning”). Consider stricter matching or exit-code only:-if grep -i "error" app_output.log; then +if grep -iE "^\s*Error:" app_output.log; thenAlternatively, use
grep -iqand rely on its exit code:if grep -iqE '^\s*Error:' app_output.log; then … fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
43-56: Nice – runtime prerequisites for the macOS job are now set up correctlyThe new
test-macosjob properly installs pnpm and Node.js before the build, resolving the failure highlighted in the earlier review.
| - name: Mount DMG | ||
| run: hdiutil attach apps/electron-app/dist/vibe-*.dmg | ||
| - name: Install App | ||
| run: | | ||
| sudo cp -R "/Volumes/COBROWSER/vibe.app" /Applications/ | ||
| - name: Run App and Check for Errors |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded DMG path is brittle & mount is never detached
hdiutil attach apps/electron-app/dist/vibe-*.dmgassumes a fixed output path. A version bump or directory change will break the workflow.
Recommend locating the DMG dynamically:
-run: hdiutil attach apps/electron-app/dist/vibe-*.dmg
+run: |
+ DMG_PATH=$(fd --full-path -e dmg '^vibe-.*\.dmg$' apps | head -n1)
+ hdiutil attach "$DMG_PATH"- The DMG volume remains mounted; subsequent jobs on the same runner can collide. Detach after copying:
- - name: Install App
- run: |
- sudo cp -R "/Volumes/COBROWSER/vibe.app" /Applications/
+ - name: Install App
+ run: |
+ VOLUME="/Volumes/COBROWSER"
+ sudo cp -R "$VOLUME/vibe.app" /Applications/
+ hdiutil detach "$VOLUME"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 70 to 75, the DMG path is hard-coded
which can break the workflow if the version or directory changes, and the
mounted DMG is never detached causing potential conflicts. Fix this by
dynamically locating the DMG file using a pattern or variable to find the latest
or correct DMG path, then after copying the app, run a command to detach the
mounted volume to clean up before the next steps or jobs.
|
Claude finished @maceip's task —— View job PR Review: Basic Test Addition to CITodo List:
Review FeedbackThis PR adds a basic smoke test to verify the packaged macOS app can run without errors for 10 seconds. Here's my comprehensive analysis: 🚨 Critical IssuesDependency Installation Order:
|
basic test: run packaged app without errors for 10 secs
Description
Brief description of the changes in this PR.
Type of Change
Summary by CodeRabbit