Remove package-lock.json #1804
Remove package-lock.json #1804
Conversation
WalkthroughDocumentation updates clarify formatting responsibilities (Prettier for non‑Ruby, RuboCop for Ruby), mandate Yarn Classic (no npm), and add a CI compliance rule disallowing non‑Yarn lock files (except Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (8)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
| - **Build**: `yarn run build` (compiles TypeScript to JavaScript in node_package/lib) | ||
| - **Type checking**: `yarn run type-check` | ||
| - **⚠️ MANDATORY BEFORE GIT PUSH**: `bundle exec rubocop` and fix ALL violations + ensure trailing newlines | ||
| - Never run `npm` commands, only equivalent Yarn Classic ones |
There was a problem hiding this comment.
Could tell it to look up the packageManager field in package.json, but this seems more reliable. Just need to update this if/when we switch to npm.
Pull Request ReviewThank you for this important cleanup PR! I've reviewed the changes and have the following feedback: ✅ Strengths
🎯 Suggestions for Improvement
🔍 Security & Performance
📋 Testing RecommendationsBefore merging:
✅ Overall AssessmentThis is a valuable housekeeping PR that improves project consistency. The changes are minimal, focused, and well-documented. Once the minor suggestions above are addressed (particularly adding Approved with minor suggestions 👍 |
| ## Important Notes | ||
|
|
||
| - Use `yalc` for local development when testing with external apps | ||
| - The project supports both Webpacker and Shakapacker |
CODING_AGENTS.md
Outdated
| ```ruby | ||
| # Bad | ||
| let(:value) { "test" } | ||
| let(:value) { "test" } |
There was a problem hiding this comment.
Wasn't a bad example before.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CLAUDE.md (1)
42-42: Yarn‑only policy LGTM; add npm usage guard in CIPolicy is clear. Add a CI step to fail if npm is used or if npm‑generated artifacts appear:
- name: Block npm usage run: | if rg -nP '\bnpm\s+(install|ci|run|publish)\b' -g '!**/node_modules/**' -S .; then echo "Detected npm command usage. Use yarn."; exit 1; fiAlso applies to: 127-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
CLAUDE.md(3 hunks)CODING_AGENTS.md(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx,json,md,yml,yaml,css,scss,html,graphql,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all formatting; never manually format code
Files:
CLAUDE.mdCODING_AGENTS.md
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-21T00:37:42.807Z
Learning: Applies to {**/*.rb,**/*.rake,**/*.gemspec,Gemfile,Rakefile} : Before every commit/push, run `bundle exec rubocop` and fix all violations; Ruby code must pass RuboCop with zero offenses
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-21T00:37:42.807Z
Learning: Applies to **/*.{js,jsx,ts,tsx,json,md,yml,yaml,css,scss,html,graphql,mdx} : Let Prettier handle all formatting; never manually format code
📚 Learning: 2025-09-21T00:37:42.807Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-21T00:37:42.807Z
Learning: Applies to {**/*.rb,**/*.rake,**/*.gemspec,Gemfile,Rakefile} : Before every commit/push, run `bundle exec rubocop` and fix all violations; Ruby code must pass RuboCop with zero offenses
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-21T00:37:42.807Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-21T00:37:42.807Z
Learning: Applies to **/*.{js,jsx,ts,tsx,json,md,yml,yaml,css,scss,html,graphql,mdx} : Let Prettier handle all formatting; never manually format code
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-21T00:37:42.807Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-21T00:37:42.807Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : JavaScript/TypeScript must pass ESLint with zero offenses before committing
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
CLAUDE.md
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
CODING_AGENTS.md
🪛 GitHub Actions: Lint JS and Ruby
CODING_AGENTS.md
[warning] 1-1: Code style issues found in CODING_AGENTS.md. Run Prettier with --write to fix.
⏰ 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). (7)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: claude-review
🔇 Additional comments (2)
CLAUDE.md (1)
12-12: Formatting ownership language LGTM; align other docsThe “Prettier+RuboCop handle ALL formatting” statements look correct. Ensure CODING_AGENTS.md (Line 20) is updated to remove “format manually” and point to automated formatting commands for consistency.
Also applies to: 46-46
CODING_AGENTS.md (1)
33-33: Add CI guard for non‑Yarn lockfiles and fix Prettier warningEnforce the checklist in CI (fail on disallowed lockfiles) and fix formatting in CODING_AGENTS.md.
File: CODING_AGENTS.md — checklist at line 33; change the "format manually" note at line 20 to the repo formatter command.
CI guard (add to an existing workflow step):
- name: Disallow non-Yarn lockfiles run: | set -euo pipefail disallowed=(package-lock.json npm-shrinkwrap.json pnpm-lock.yaml bun.lockb) found="$(git ls-files "${disallowed[@]}" || true)" if [[ -n "$found" ]]; then echo "Found disallowed lock files:"; echo "$found"; exit 1 fiFormatting: run
yarn prettier --write CODING_AGENTS.mdand replace the manual-format instruction (line 20) with the repo formatter command (e.g.,yarn start formatorrake autofix).
alexeyr-ci
force-pushed
the
alexeyr/remove-package-lock.json
branch
from
5f67d94 to
b798615
Compare
Code Review for PR #1804: Remove package-lock.json✅ Overall AssessmentThis PR correctly addresses an important issue in the repository's package management consistency. The changes are appropriate and necessary for maintaining a clean development environment. 🎯 Positive Aspects
📝 Suggestions for Improvement
✔️ Code Quality
🧪 Test CoverageNo tests needed for this change as it's purely a build configuration/documentation update. 🔒 Security ConsiderationsRemoving package-lock.json when yarn.lock exists is actually a security improvement as it ensures consistent dependency resolution. ⚡ Performance ImpactThis change will prevent potential performance issues caused by conflicting package managers and ensure faster, more reliable installs. ConclusionApproved ✅ - This is a necessary housekeeping change that improves the project's build consistency. The only minor suggestion would be to add preventive measures via .gitignore to ensure this doesn't happen again. Great work on catching this issue and documenting the package manager requirements clearly! |
Code Review for PR #1804: Remove package-lock.json✅ SummaryThis PR correctly removes the inadvertently committed package-lock.json file and updates documentation to prevent its future introduction. The changes align well with the project's use of Yarn Classic as the standard package manager. 📋 Review FindingsCode Quality & Best Practices ✅
Potential Issues
|
Summary
package-lock.json.Pull Request checklist
[ ] Add/update test to cover these changes[ ] Update documentation[ ] Update CHANGELOG fileThis change is
Summary by CodeRabbit