Skip to content

Conversation

@grahamc
Copy link
Member

@grahamc grahamc commented Dec 10, 2025

Summary by CodeRabbit

  • Chores
    • Migrated project workflows and scripts from pnpm to npm.
    • Replaced legacy ESLint setup with a modern flat-format ESLint configuration.
    • Updated development tooling and linting dependencies to newer versions.
    • Cleaned up ancillary config: removed pnpm-related entries and adjusted formatting ignore list.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

Migrates the project from pnpm to npm, removes the legacy .eslintrc.json, and adds a new flat-format eslint.config.mjs. Updates tooling/devDependencies and adjusts CI, Prettier ignore, and Nix devShell entries accordingly.

Changes

Cohort / File(s) Summary
Package manager & CI
\.github/workflows/ci.yaml, package.json
Replace pnpm invocations with npm equivalents in CI and npm-script orchestration; remove top-level pnpm config from package.json.
ESLint config migration
\.eslintrc.json (deleted), eslint.config.mjs (added)
Delete legacy JSON ESLint config and add new flat-config module exporting a TypeScript-focused ESLint configuration (plugins, parser, parserOptions, rules, typescript resolver, GitHub recommended flat configs).
Tooling & deps
package.json
Add devDependencies: @eslint/eslintrc, @eslint/js, globals, @typescript-eslint/parser; update eslint, @typescript-eslint/eslint-plugin, eslint-plugin-github, prettier, tsup, and @types/node.
Prettier/Nix housekeeping
.prettierignore, flake.nix
Remove pnpm-lock.yaml from .prettierignore; remove nodePackages_latest.pnpm entry from Nix devShell overlay.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Check eslint.config.mjs for parity with removed .eslintrc.json (rule semantics, parserOptions.project path).
  • Validate CI changes (.github/workflows/ci.yaml) produce equivalent behavior with npm ci and npm run sequences.
  • Verify updated devDependencies and ESLint major-version changes for breaking rule/option differences.
  • Review Nix flake.nix change if dev shell previously relied on pnpm-specific packages.

Poem

🐰
From lockfile hops to npm's bright lawn,
I swapped old JSON for a flat dawn.
Rules rebloom, scripts take flight,
Lint and build in morning light.
A tiny rabbit cheers—code tidy and right.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main objective of the pull request: upgrading ESLint dependencies and switching from pnpm to npm as the package manager.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch grahamc/upgrade-deps-eslint-use-npm

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69f84fb and 53da526.

⛔ Files ignored due to path filters (2)
  • dist/index.js is excluded by !**/dist/**
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • package.json (2 hunks)
⏰ 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). (3)
  • GitHub Check: DeterminateCI / build (x86_64-linux, UbuntuLatest32Cores128G)
  • GitHub Check: DeterminateCI / build (aarch64-linux, UbuntuLatest32Cores128GArm)
  • GitHub Check: Check the dist/ folder is up to date
🔇 Additional comments (2)
package.json (2)

14-14: Script invocation updated to use npm.

The "all" script correctly invokes npm instead of pnpm, aligning with the project migration from pnpm to npm.


38-50: No action required — ESLint 9.x and eslint-plugin-github 6.0.0 compatibility confirmed.

The version upgrades are compatible. eslint-plugin-github 6.0.0 exports a flat-config-compatible getFlatConfigs() helper documented in its README, and @typescript-eslint/parser and @typescript-eslint/eslint-plugin (^8.49.0) support ESLint 9.x (supported range ^8.57.0 || ^9.0.0). The new dependencies align correctly with ESLint 9.x's flat config format.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
package.json (1)

14-52: Move @typescript-eslint/parser to devDependencies

The "all" script chaining npm run format && npm run lint && npm run build && npm run package is consistent with the CI workflow—no concerns there.

@typescript-eslint/parser is imported only in eslint.config.mjs (line 4) for linting configuration and never appears in the bundled action code (dist/index.js). Since linting happens only during development and the bundled action does not require this parser at runtime, it should be moved to devDependencies alongside the other ESLint tooling. The package-lock.json marks it with `"peer": true, indicating it is expected to be peer-installed with the ESLint ecosystem in devDependencies.

🧹 Nitpick comments (1)
eslint.config.mjs (1)

29-41: Flat config wiring looks good; consider targeting a newer ecmaVersion

The flat config structure (GitHub recommended base + TS‑specific override with @typescript-eslint parser/plugin and project: "./tsconfig.json") looks coherent and should work well with ESLint 9.

ecmaVersion: 9 is unnecessarily restrictive. Since your tsconfig.json targets ES2020, set ecmaVersion: 2020 to match the TypeScript target. Alternatively, use ecmaVersion: "latest" for forward compatibility. This avoids potential surprises if you use modern JS syntax in .ts files.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e00624d and 69f84fb.

⛔ Files ignored due to path filters (3)
  • dist/index.js is excluded by !**/dist/**
  • package-lock.json is excluded by !**/package-lock.json
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (6)
  • .eslintrc.json (0 hunks)
  • .github/workflows/ci.yaml (1 hunks)
  • .prettierignore (0 hunks)
  • eslint.config.mjs (1 hunks)
  • flake.nix (0 hunks)
  • package.json (2 hunks)
💤 Files with no reviewable changes (3)
  • flake.nix
  • .prettierignore
  • .eslintrc.json
⏰ 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). (4)
  • GitHub Check: DeterminateCI / build (aarch64-darwin, namespace-profile-mac-m2-12c28g)
  • GitHub Check: DeterminateCI / build (x86_64-linux, UbuntuLatest32Cores128G)
  • GitHub Check: DeterminateCI / build (aarch64-linux, UbuntuLatest32Cores128GArm)
  • GitHub Check: Check the dist/ folder is up to date
🔇 Additional comments (1)
.github/workflows/ci.yaml (1)

34-52: npm migration in CI is properly configured; lockfile is committed

The switch from pnpm to npm (npm ci, npm run check-fmt|lint|build|package) is correct. All referenced npm scripts exist in package.json, package-lock.json is committed to the repository, and the nix environment (flake.nix) properly supports the workflow.

@grahamc grahamc enabled auto-merge December 10, 2025 17:32
@grahamc grahamc added this pull request to the merge queue Dec 10, 2025
Merged via the queue into main with commit d56bf56 Dec 10, 2025
9 checks passed
@grahamc grahamc deleted the grahamc/upgrade-deps-eslint-use-npm branch December 10, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants