fix(start): sync npm deps on lockfile change and bump builder-sdk to 0.5.0#416
Conversation
…0.5.0 ./bin/start.sh only ran npm install on a missing node_modules directory, so pulling new dependencies left packages like @pymthouse/builder-sdk unresolved. Detect stale lockfiles and missing declared deps, then re-run npm install. Pin @pymthouse/builder-sdk at 0.5.0 across web-next and developer-api.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughUpdates ChangesBuilder SDK Version Bump
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/start.sh (1)
158-158: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
A && B || Cis not a true if-then-else (shellcheck SC2015).If
log_successwere to fail (e.g., write error),log_warnwould also execute, producing a misleading "Could not install git hooks" message after a successful install. Sincelog_successis justecho, this is very low risk, but a properif/then/elsewould be more robust.🛡️ Proposed fix: use if/else
- bash "$SCRIPT_DIR/install-git-hooks.sh" 2>/dev/null && \ - log_success "Git hooks installed" || log_warn "Could not install git hooks" + if bash "$SCRIPT_DIR/install-git-hooks.sh" 2>/dev/null; then + log_success "Git hooks installed" + else + log_warn "Could not install git hooks" + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/start.sh` at line 158, The git hooks install flow in start.sh uses an A && B || C pattern that is not a reliable if/then/else and can incorrectly trigger the warning path after a successful install if log_success fails. Refactor the install-git-hooks.sh invocation block to use an explicit if/then/else structure, keeping the success path in the then branch and log_warn only in the else branch, so the behavior is tied to the actual install result rather than the logging command.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/start.sh`:
- Around line 155-161: The git hooks install block in start.sh is out of sync
with its comment: the current condition around the install-git-hooks.sh call
only checks for missing .git/hooks/pre-push, so it can run on any start, not
just a fresh node_modules creation. Either update the comment near the
hook-install logic to describe the actual behavior, or, if fresh-clone-only
behavior is intended, guard the block with the existing
was_fresh_clone/needs_npm_install state before calling install-git-hooks.sh.
---
Nitpick comments:
In `@bin/start.sh`:
- Line 158: The git hooks install flow in start.sh uses an A && B || C pattern
that is not a reliable if/then/else and can incorrectly trigger the warning path
after a successful install if log_success fails. Refactor the
install-git-hooks.sh invocation block to use an explicit if/then/else structure,
keeping the success path in the then branch and log_warn only in the else
branch, so the behavior is tied to the actual install result rather than the
logging command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e99f5574-adb3-4eb5-b5b3-0b36a3f3ee19
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!package-lock.json
📒 Files selected for processing (5)
apps/web-next/package.jsonbin/start.shdocs/pymthouse-integration.mdplugins/developer-api/backend/package.jsonplugins/developer-api/frontend/package.json
Align the hook-install comment with actual behavior and replace the A && B || C pattern with an explicit if/then/else for shellcheck SC2015.
Summary
./bin/start.shnow re-runsnpm installwhenpackage-lock.jsonis newer than the last install,node_modulesis incomplete, or@pymthouse/builder-sdkis missing — fixing the case where an existingnode_modulestree leaves newly declared registry deps unresolved aftergit pull.@pymthouse/builder-sdkat 0.5.0 (release notes) acrossapps/web-nextandplugins/developer-api/*.docs/pymthouse-integration.mdto reflect the new pin and install behavior.Root cause
start.shonly rannpm installwhennode_modules/was entirely absent. Developers who already had a partial install (or pulled after@pymthouse/builder-sdkwas added) hitModule not found: Can't resolve '@pymthouse/builder-sdk'on Next.js build even when running./bin/start.sh --all.Test plan
npm installresolves@pymthouse/builder-sdk@0.5.0at repo rootapps/web-nextpymthouse unit tests pass (pymthouse-client.test.ts,pymthouse-adapter.test.ts)node_modules: run./bin/start.sh --alland confirm dependency sync runs when lockfile changesSummary by CodeRabbit
Bug Fixes
Chores
0.5.0across the app and related plugins.Documentation
0.5.0dependency version.