Conversation
|
Warning Rate limit exceeded@PavelVanecek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis PR adds comprehensive pnpm package manager support to the test infrastructure. Changes include a new PnpmController class, API extensions to expose pnpm version information, type system updates across backend and frontend, UI display for pnpm version, test fixtures, and configuration updates to recognize pnpm as a supported package manager. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend UI
participant API as Backend API
participant Factory as Controller Factory
participant Pnpm as PnpmController
participant CLI as pnpm CLI
UI->>API: GET /api/versions
API->>Factory: getControllerFactory("pnpm")
Factory->>Pnpm: new PnpmController(projectPath)
Pnpm-->>Factory: PnpmController instance
Factory-->>API: PnpmController ready
Pnpm->>CLI: run pnpm --version
CLI-->>Pnpm: version output
Pnpm-->>API: pnpm version
API-->>UI: { node, npm, yarn, pnpm }
UI->>UI: Render version info
Note over UI,CLI: User runs test/build
UI->>API: POST /run test (packageManager: pnpm)
API->>Factory: getControllerFactory("pnpm")
Factory-->>API: PnpmController instance
API->>Pnpm: test()
Pnpm->>CLI: pnpm test
CLI-->>Pnpm: test output
Pnpm-->>API: TestOutcome result
API-->>UI: { success, output }
UI->>UI: Render result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 4
🧹 Nitpick comments (2)
README.md (2)
4-29: Prerequisites section provides clear multi-package-manager setup guidance.The installation instructions for npm, yarn, and pnpm are well-structured with multiple methods per tool and helpful reference links. One minor suggestion: consider documenting the minimum Node.js version requirement (v22.18.x per your CI configuration) prominently in this section, since it affects tool compatibility and test execution. Per learnings, this is critical for the test-ui TypeScript/ESM implementation.
Consider adding a subsection like:
### Node.js This project requires **Node.js v22.18.x or higher** for full compatibility with the test infrastructure, especially the test-ui TypeScript/ESM features.
59-81: Command-line examples show only npm; consider adding pnpm/yarn examples for consistency.The examples demonstrate
npm:integrations/ts-react18syntax but don't show equivalent pnpm or yarn invocations. Since the PR adds pnpm support and users will want to test with different package managers, adding pnpm and yarn examples would improve clarity and usability.Add pnpm and yarn examples after the npm examples:
### Run with different package managers ```bash # npm ./run-test.sh npm:integrations/ts-react18 # pnpm ./run-test.sh pnpm:integrations/ts-react18 # yarn ./run-test.sh yarn:integrations/ts-react18</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between fb4948cf344a1feb4680158c3adc66fae7403444 and 7cc36bfec13b865dad2f9d2b0edbfc377b8ef001. </details> <details> <summary>📒 Files selected for processing (15)</summary> * `.gitignore` (1 hunks) * `README.md` (1 hunks) * `scripts/run.mts` (2 hunks) * `test-ui/package.json` (1 hunks) * `test-ui/server/scripts/Controller.ts` (1 hunks) * `test-ui/server/scripts/PnpmController.ts` (1 hunks) * `test-ui/server/scripts/test-registry.ts` (2 hunks) * `test-ui/server/server.ts` (3 hunks) * `test-ui/src/App.tsx` (1 hunks) * `test-ui/src/hooks/useLoadAllInfo.ts` (1 hunks) * `test-ui/src/hooks/useLoadVersions.ts` (1 hunks) * `test-ui/src/types.ts` (1 hunks) * `test-ui/test/server/scripts/PnpmController.spec.ts` (1 hunks) * `test-ui/test/server/scripts/YarnController.spec.ts` (1 hunks) * `test-ui/test/server/scripts/pnpm-list-sample-output.json` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (7)</summary> <details> <summary>test-ui/src/**/*.{ts,tsx}</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > Frontend state must persist results using sessionStorage, but not persist queue state, to ensure results survive page reloads Files: - `test-ui/src/types.ts` - `test-ui/src/App.tsx` - `test-ui/src/hooks/useLoadAllInfo.ts` - `test-ui/src/hooks/useLoadVersions.ts` </details> <details> <summary>test-ui/src/App.tsx</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > Main app component (`test-ui/src/App.tsx`) must render the test UI with all filter and execution controls Files: - `test-ui/src/App.tsx` </details> <details> <summary>test-ui/server/scripts/*Controller.ts</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > Controllers must use async `exec` (not `execSync`) to prevent blocking during package installation, testing, building, and dependency verification Files: - `test-ui/server/scripts/Controller.ts` - `test-ui/server/scripts/PnpmController.ts` </details> <details> <summary>test-ui/server/**/*.ts</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > `test-ui/server/**/*.ts`: Error handling in failed test phases must capture and display full stdout/stderr output, not just 'command failed' messages, with error objects containing `stdout` property > Server must expand `~` in file paths (path expansion) when receiving from frontend, but frontend should not attempt path expansion Files: - `test-ui/server/scripts/Controller.ts` - `test-ui/server/scripts/PnpmController.ts` - `test-ui/server/scripts/test-registry.ts` - `test-ui/server/server.ts` </details> <details> <summary>test-ui/test/**/*.{ts,tsx}</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > Test framework must use Vitest for unit testing, located in `test-ui/test/`, with tests for utility functions, Redux selectors, and components using React Testing Library Files: - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/test/server/scripts/PnpmController.spec.ts` </details> <details> <summary>test-ui/server/scripts/test-registry.ts</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > `test-ui/server/scripts/test-registry.ts`: Test Registry (`test-ui/server/scripts/test-registry.ts`) must be the single source of truth for all test definitions, containing metadata: test name, stability (stable/experimental), type, package manager, and dependencies > Stable tests (marked with `isCi: true`) must run reliably in CI on every commit; experimental tests (`isCi: false`) should be marked as research/debugging only Files: - `test-ui/server/scripts/test-registry.ts` </details> <details> <summary>test-ui/server/server.ts</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > WebSocket server in `test-ui/server/server.ts` must handle test execution and dispatch Redux actions for real-time updates to connected clients Files: - `test-ui/server/server.ts` </details> </details><details> <summary>🧠 Learnings (19)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to integrations/**/package.json : npmoverridesfield must be used in npm-based tests to resolve dependency conflicts; behavior differs between Node.js versions (works in v22, may fail in v24+)Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/server/scripts/test-registry.ts : Test Registry (test-ui/server/scripts/test-registry.ts) must be the single source of truth for all test definitions, containing metadata: test name, stability (stable/experimental), type, package manager, and dependencies</details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/tsconfig.json : TypeScript configuration in test UI must enable strict type checking and runnpm run check-typesto validate types**Applied to files:** - `test-ui/src/types.ts` - `test-ui/test/server/scripts/PnpmController.spec.ts` - `test-ui/server/scripts/test-registry.ts` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/server/scripts/test-registry.ts : Test Registry (test-ui/server/scripts/test-registry.ts) must be the single source of truth for all test definitions, containing metadata: test name, stability (stable/experimental), type, package manager, and dependencies**Applied to files:** - `test-ui/src/types.ts` - `test-ui/server/scripts/Controller.ts` - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/package.json` - `test-ui/test/server/scripts/PnpmController.spec.ts` - `test-ui/server/scripts/PnpmController.ts` - `test-ui/server/scripts/test-registry.ts` - `scripts/run.mts` - `README.md` - `test-ui/server/server.ts` - `test-ui/test/server/scripts/pnpm-list-sample-output.json` - `.gitignore` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to integrations/**/package.json : yarnresolutionsfield must be used in yarn-based tests to resolve dependency conflicts**Applied to files:** - `test-ui/src/types.ts` - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/package.json` - `test-ui/test/server/scripts/PnpmController.spec.ts` - `test-ui/server/scripts/test-registry.ts` - `README.md` - `.gitignore` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to integrations/**/package.json : npmoverridesfield must be used in npm-based tests to resolve dependency conflicts; behavior differs between Node.js versions (works in v22, may fail in v24+)**Applied to files:** - `test-ui/src/types.ts` - `test-ui/package.json` - `test-ui/test/server/scripts/PnpmController.spec.ts` - `test-ui/server/scripts/PnpmController.ts` - `test-ui/server/scripts/test-registry.ts` - `README.md` - `test-ui/test/server/scripts/pnpm-list-sample-output.json` - `.gitignore` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/test/**/*.{ts,tsx} : Test framework must use Vitest for unit testing, located intest-ui/test/, with tests for utility functions, Redux selectors, and components using React Testing Library**Applied to files:** - `test-ui/src/types.ts` - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/test/server/scripts/PnpmController.spec.ts` - `test-ui/server/scripts/test-registry.ts` - `scripts/run.mts` - `README.md` - `test-ui/test/server/scripts/pnpm-list-sample-output.json` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/nodemon.json : nodemon configuration intest-ui/nodemon.jsonmust enable auto-restart of server on TypeScript file changes**Applied to files:** - `test-ui/src/types.ts` - `test-ui/server/server.ts` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/src/App.tsx : Main app component (test-ui/src/App.tsx) must render the test UI with all filter and execution controls**Applied to files:** - `test-ui/src/App.tsx` - `README.md` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/server/scripts/*Controller.ts : Controllers must use asyncexec(notexecSync) to prevent blocking during package installation, testing, building, and dependency verification**Applied to files:** - `test-ui/server/scripts/Controller.ts` - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/test/server/scripts/PnpmController.spec.ts` - `test-ui/server/scripts/PnpmController.ts` - `scripts/run.mts` - `test-ui/server/server.ts` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/server/**/*.ts : Server must expand~in file paths (path expansion) when receiving from frontend, but frontend should not attempt path expansion**Applied to files:** - `test-ui/test/server/scripts/YarnController.spec.ts` - `scripts/run.mts` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: ReUse the same test registry (test-ui/server/scripts/test-registry.ts) for both CLI and UI tools to ensure consistency**Applied to files:** - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/test/server/scripts/PnpmController.spec.ts` - `test-ui/server/scripts/test-registry.ts` - `scripts/run.mts` - `README.md` - `test-ui/server/server.ts` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/server/scripts/test-registry.ts : Stable tests (marked withisCi: true) must run reliably in CI on every commit; experimental tests (isCi: false) should be marked as research/debugging only**Applied to files:** - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/test/server/scripts/PnpmController.spec.ts` - `test-ui/server/scripts/test-registry.ts` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Verify Node.js version compatibility before running tests; in case standard node is unavailable or too old, try/opt/homebrew/bin/node; if that fails, require Node.js v22.18 or higher**Applied to files:** - `test-ui/test/server/scripts/YarnController.spec.ts` - `README.md` - `test-ui/server/server.ts` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/vite.config.ts : Vite configuration must be properly set up intest-ui/vite.config.tsfor frontend build and dev server**Applied to files:** - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/package.json` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/server/**/*.ts : Error handling in failed test phases must capture and display full stdout/stderr output, not just 'command failed' messages, with error objects containingstdoutproperty**Applied to files:** - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/test/server/scripts/PnpmController.spec.ts` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to {list.js,run-test.sh,pack-and-run.sh} : CLI tools (list.js,run-test.sh,pack-and-run.sh) must remain stable and reliable as they are called from the main recharts repository on every pull request**Applied to files:** - `test-ui/test/server/scripts/YarnController.spec.ts` - `test-ui/server/scripts/test-registry.ts` - `README.md` - `test-ui/test/server/scripts/pnpm-list-sample-output.json` - `.gitignore` </details> <details> <summary>📚 Learning: 2025-11-30T04:39:58.861Z</summary>Learnt from: PavelVanecek
Repo: recharts/recharts-integ PR: 80
File: .github/workflows/integ.yml:57-57
Timestamp: 2025-11-30T04:39:58.861Z
Learning: In .github/workflows/integ.yml, Node.js version 22.18.x is the minimum required version because it enables built-in TypeScript type stripping by default, which is essential for the test-ui TypeScript/ESM implementation.**Applied to files:** - `test-ui/package.json` - `test-ui/server/scripts/test-registry.ts` - `README.md` </details> <details> <summary>📚 Learning: 2025-12-01T14:21:11.742Z</summary>Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Dependency resolution must handle complex peer dependencies: support React 16-19, Redux Toolkit v1 and v2 (noting that Redux v1 supports React 16-18, v2 requires React 18+)**Applied to files:** - `test-ui/server/scripts/test-registry.ts` - `README.md` - `test-ui/test/server/scripts/pnpm-list-sample-output.json` </details> <details> <summary>📚 Learning: 2025-12-01T13:43:29.725Z</summary>Learnt from: PavelVanecek
Repo: recharts/recharts-integ PR: 84
File: test-ui/server/server.ts:615-620
Timestamp: 2025-12-01T13:43:29.725Z
Learning: In test-ui/server/server.ts, the packing workflow intentionally triggers only once per queue (whentestQueue.length === 0 && !isRunningTest) to save time. After packing completes, all queued tests are updated with the packed version viatestQueue.forEach(). This design assumes all tests in a queue should use the same packed version for efficiency.**Applied to files:** - `test-ui/server/server.ts` </details> </details><details> <summary>🧬 Code graph analysis (5)</summary> <details> <summary>test-ui/server/scripts/Controller.ts (1)</summary><blockquote> <details> <summary>scripts/run-everything.js (1)</summary> * `fs` (3-3) </details> </blockquote></details> <details> <summary>test-ui/test/server/scripts/PnpmController.spec.ts (1)</summary><blockquote> <details> <summary>test-ui/server/scripts/PnpmController.ts (1)</summary> * `PnpmController` (4-106) </details> </blockquote></details> <details> <summary>test-ui/server/scripts/PnpmController.ts (2)</summary><blockquote> <details> <summary>test-ui/server/scripts/TestOutcome.ts (1)</summary> * `TestOutcome` (18-52) </details> <details> <summary>scripts/run-everything.js (1)</summary> * `output` (14-31) </details> </blockquote></details> <details> <summary>scripts/run.mts (3)</summary><blockquote> <details> <summary>test-ui/server/scripts/NpmController.ts (1)</summary> * `NpmController` (4-98) </details> <details> <summary>test-ui/server/scripts/YarnController.ts (1)</summary> * `YarnController` (27-208) </details> <details> <summary>test-ui/server/scripts/PnpmController.ts (1)</summary> * `PnpmController` (4-106) </details> </blockquote></details> <details> <summary>test-ui/server/server.ts (1)</summary><blockquote> <details> <summary>test-ui/server/scripts/PnpmController.ts (1)</summary> * `PnpmController` (4-106) </details> </blockquote></details> </details> </details> <details> <summary>⏰ 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). (19)</summary> * GitHub Check: yarn:integrations/ts-react18 on node 25.x * GitHub Check: yarn:my-charts-react17:app-react16 on node 25.x * GitHub Check: yarn:integrations/ts-react19 on node 25.x * GitHub Check: yarn:my-charts-react18:app-react17 on node 25.x * GitHub Check: npm:my-charts-react19:app-react19 on node 25.x * GitHub Check: yarn:my-charts-react19:app-react18 on node 25.x * GitHub Check: npm:my-charts-react18:app-react17 on node 25.x * GitHub Check: npm:my-charts-react19:app-react18 on node 25.x * GitHub Check: npm:my-charts-react18:app-react18 on node 25.x * GitHub Check: npm:integrations/ts4-react17 on node 25.x * GitHub Check: yarn:my-charts-react17:app-react16 on node 24.x * GitHub Check: yarn:my-charts-react17:app-react17 on node 24.x * GitHub Check: yarn:integrations/ts-react16-skip-lib-check-false on node 24.x * GitHub Check: yarn:integrations/ts-react16-overrides on node 24.x * GitHub Check: yarn:my-charts-react18:app-react17 on node 22.18.x * GitHub Check: yarn:integrations/ts-react16-resolutions on node 22.18.x * GitHub Check: yarn:my-charts-react17:app-react16 on node 22.18.x * GitHub Check: npm:integrations/ts4-react17 on node 22.18.x * GitHub Check: npm:my-charts-react16:app-react16 on node 22.18.x </details> <details> <summary>🔇 Additional comments (21)</summary><blockquote> <details> <summary>test-ui/server/scripts/Controller.ts (1)</summary><blockquote> `27-27`: **LGTM! Consistent lockfile cleanup.** The addition of `pnpm-lock.yaml` removal follows the same pattern as `package-lock.json` and `yarn.lock`, ensuring clean test environments across all supported package managers. </blockquote></details> <details> <summary>test-ui/test/server/scripts/YarnController.spec.ts (1)</summary><blockquote> `2-2`: **LGTM! Import path corrected.** The import path now correctly references the YarnController implementation in the server directory rather than incorrectly looking in the test directory. </blockquote></details> <details> <summary>.gitignore (1)</summary><blockquote> `9-10`: **LGTM! Appropriate ignore patterns.** Adding `pnpm-lock.yaml` aligns with the existing pattern of ignoring package manager lock files for integration tests, as explained by the comment on line 6. </blockquote></details> <details> <summary>test-ui/src/App.tsx (1)</summary><blockquote> `209-209`: **LGTM! Consistent version display.** The pnpm version display follows the same pattern as Node, npm, and Yarn, ensuring a consistent UI for all supported package managers. </blockquote></details> <details> <summary>test-ui/src/types.ts (1)</summary><blockquote> `5-5`: **LGTM! Type system extended for pnpm.** Extending the `packageManager` union type to include `'pnpm'` enables type-safe pnpm support throughout the application. </blockquote></details> <details> <summary>scripts/run.mts (2)</summary><blockquote> `5-5`: **LGTM! PnpmController imported.** The import statement correctly adds PnpmController to support pnpm package manager tests. --- `67-77`: **LGTM! Controller factory extended for pnpm.** The factory function now correctly returns PnpmController for pnpm-based tests, following the same pattern as npm and yarn. The error handling remains intact for unknown package managers. </blockquote></details> <details> <summary>test-ui/src/hooks/useLoadAllInfo.ts (1)</summary><blockquote> `15-15`: **LGTM! Versions state extended for pnpm.** Adding the `pnpm` field to the versions state type maintains consistency with the API response and enables pnpm version display in the UI. </blockquote></details> <details> <summary>test-ui/src/hooks/useLoadVersions.ts (1)</summary><blockquote> `4-4`: **LGTM! Function signature updated for pnpm.** The `setVersions` parameter type now includes `pnpm`, maintaining type consistency with the API response and state management throughout the application. </blockquote></details> <details> <summary>test-ui/test/server/scripts/PnpmController.spec.ts (1)</summary><blockquote> `1-4`: **LGTM!** Imports are correctly structured and follow the project guidelines for using Vitest. </blockquote></details> <details> <summary>test-ui/server/scripts/test-registry.ts (1)</summary><blockquote> `8-8`: **LGTM!** The PackageManager type expansion correctly adds pnpm support, maintaining the registry as the single source of truth for test definitions. Based on learnings, this aligns with the requirement that the Test Registry must be the single source of truth for all test definitions. </blockquote></details> <details> <summary>test-ui/server/server.ts (4)</summary><blockquote> `12-12`: **LGTM!** PnpmController import follows the same pattern as existing npm and yarn controller imports. --- `145-150`: **LGTM!** pnpm version detection correctly mirrors the yarn version detection pattern, with appropriate error handling for when pnpm is not installed. --- `156-156`: **LGTM!** pnpm version is correctly added to the API response, maintaining consistency with npm and yarn fields. --- `271-272`: **LGTM!** PnpmController is correctly integrated into the controller factory, following the same pattern as npm and yarn controllers. </blockquote></details> <details> <summary>test-ui/package.json (1)</summary><blockquote> `48-49`: **LGTM!** The `packageManager` field correctly specifies pnpm version 10.8.1 with a Corepack integrity hash, ensuring consistent package manager versions across development environments. </blockquote></details> <details> <summary>test-ui/server/scripts/PnpmController.ts (5)</summary><blockquote> `5-12`: **LGTM!** The `install()` method correctly uses async `exec` and proper error handling with TestOutcome. As per coding guidelines, controllers must use async exec (not execSync) to prevent blocking. --- `14-21`: **LGTM!** The `test()` method correctly uses `--if-present` flag to gracefully handle missing test scripts and follows async exec guidelines. --- `23-31`: **LGTM!** The `build()` method correctly uses async exec with appropriate error logging and handling. --- `33-36`: **LGTM!** The `pack()` method correctly executes pnpm pack and converts the output to a package reference. --- `58-105`: **LGTM with caveat!** The `verifySingleDependencyVersion()` method has comprehensive error handling and correct logic for verifying single dependency versions. However, it inherits the limitation from `parsePnpmLsOutput()` that doesn't check devDependencies (flagged separately). The error handling correctly captures stdout/stderr per coding guidelines for failed test phases. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| { | ||
| name: "pnpm:react19", | ||
| stability: "experimental", | ||
| type: "direct", | ||
| packageManager: "pnpm", | ||
| integrationPath: "integrations/ts-react19", | ||
| dependencies: { react: "19" }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all test entries in test-registry.ts to examine naming patterns for "direct" type tests
rg -A 8 '"direct"' test-ui/server/scripts/test-registry.ts | head -100Repository: recharts/recharts-integ
Length of output: 3038
🏁 Script executed:
#!/bin/bash
# Check if integrations/ts-react19 exists and look for pnpm configuration
fd -t f . integrations/ts-react19 | head -20Repository: recharts/recharts-integ
Length of output: 564
🏁 Script executed:
#!/bin/bash
# Quick check if package.json mentions pnpm (optional verification)
cat integrations/ts-react19/package.json | head -20Repository: recharts/recharts-integ
Length of output: 531
Inconsistent test naming convention.
The test name "pnpm:react19" breaks the established naming pattern for direct dependency tests. All npm and yarn direct tests follow the format "packageManager:integrations/path" (e.g., "npm:integrations/ts-react19", "yarn:integrations/ts-react19"). Update to:
- name: "pnpm:react19",
+ name: "pnpm:integrations/ts-react19",📝 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: "pnpm:react19", | |
| stability: "experimental", | |
| type: "direct", | |
| packageManager: "pnpm", | |
| integrationPath: "integrations/ts-react19", | |
| dependencies: { react: "19" }, | |
| }, | |
| { | |
| name: "pnpm:integrations/ts-react19", | |
| stability: "experimental", | |
| type: "direct", | |
| packageManager: "pnpm", | |
| integrationPath: "integrations/ts-react19", | |
| dependencies: { react: "19" }, | |
| }, |
🤖 Prompt for AI Agents
In test-ui/server/scripts/test-registry.ts around lines 204 to 211, the entry
name "pnpm:react19" doesn't follow the established naming convention for direct
dependency tests; update the name to match the
"packageManager:integrations/path" pattern (e.g.,
"pnpm:integrations/ts-react19") so it aligns with npm/yarn entries and other
tests.
There was a problem hiding this comment.
This name used to be the source of truth of the package manager and absolute path. That's now refactored away and the names can be shortened and easier to read. There's no more need to stick with the old convention.
There was a problem hiding this comment.
@PavelVanecek, thank you for the clarification! You're absolutely right—with the refactored TestMetadata structure where packageManager and integrationPath are now explicit fields, the test name no longer needs to encode that information. The shorter, more readable naming approach makes sense given that the metadata fields are the actual source of truth now. I appreciate you explaining the architectural reasoning behind this change.
🧠 Learnings used
Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/server/scripts/test-registry.ts : Test Registry (`test-ui/server/scripts/test-registry.ts`) must be the single source of truth for all test definitions, containing metadata: test name, stability (stable/experimental), type, package manager, and dependencies
Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to integrations/**/package.json : npm `overrides` field must be used in npm-based tests to resolve dependency conflicts; behavior differs between Node.js versions (works in v22, may fail in v24+)
Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Dependency resolution must handle complex peer dependencies: support React 16-19, Redux Toolkit v1 and v2 (noting that Redux v1 supports React 16-18, v2 requires React 18+)
Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/server/scripts/test-registry.ts : Stable tests (marked with `isCi: true`) must run reliably in CI on every commit; experimental tests (`isCi: false`) should be marked as research/debugging only
Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/test/**/*.{ts,tsx} : Test framework must use Vitest for unit testing, located in `test-ui/test/`, with tests for utility functions, Redux selectors, and components using React Testing Library
Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to integrations/**/package.json : yarn `resolutions` field must be used in yarn-based tests to resolve dependency conflicts
Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: ReUse the same test registry (`test-ui/server/scripts/test-registry.ts`) for both CLI and UI tools to ensure consistency
Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to {list.js,run-test.sh,pack-and-run.sh} : CLI tools (`list.js`, `run-test.sh`, `pack-and-run.sh`) must remain stable and reliable as they are called from the main recharts repository on every pull request
Learnt from: CR
Repo: recharts/recharts-integ PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-01T14:21:11.742Z
Learning: Applies to test-ui/tsconfig.json : TypeScript configuration in test UI must enable strict type checking and run `npm run check-types` to validate types
| [ | ||
| { | ||
| "name": "recharts", | ||
| "version": "3.5.1", | ||
| "path": "/absolute/path/github/recharts", | ||
| "private": false, | ||
| "dependencies": { | ||
| "@reduxjs/toolkit": { | ||
| "from": "@reduxjs/toolkit", | ||
| "version": "2.11.0", | ||
| "resolved": "https://registry.npmjs.org/@reduxjs/toolkit/-/toolkit-2.11.0.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/@reduxjs+toolkit@2.11.0_react-redux@9.2.0_@types+react@18.3.27_react@18.3.1_redux@5.0.1__react@18.3.1/node_modules/@reduxjs/toolkit", | ||
| "dependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| } | ||
| } | ||
| }, | ||
| "react-redux": { | ||
| "from": "react-redux", | ||
| "version": "9.2.0", | ||
| "resolved": "https://registry.npmjs.org/react-redux/-/react-redux-9.2.0.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react-redux@9.2.0_@types+react@18.3.27_react@18.3.1_redux@5.0.1/node_modules/react-redux", | ||
| "dependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| } | ||
| } | ||
| }, | ||
| "use-sync-external-store": { | ||
| "from": "use-sync-external-store", | ||
| "version": "1.6.0", | ||
| "resolved": "https://registry.npmjs.org/use-sync-external-store/-/use-sync-external-store-1.6.0.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/use-sync-external-store@1.6.0_react@18.3.1/node_modules/use-sync-external-store", | ||
| "dependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "devDependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| }, | ||
| "@storybook/addon-docs": { | ||
| "from": "@storybook/addon-docs", | ||
| "version": "10.1.2", | ||
| "resolved": "https://registry.npmjs.org/@storybook/addon-docs/-/addon-docs-10.1.2.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/@storybook+addon-docs@10.1.2_@types+react@18.3.27_esbuild@0.27.0_rollup@4.53.3_storyboo_c4dfad4f5dc5f19d048c28052f829188/node_modules/@storybook/addon-docs", | ||
| "dependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| } | ||
| } | ||
| }, | ||
| "@storybook/addon-links": { | ||
| "from": "@storybook/addon-links", | ||
| "version": "10.1.2", | ||
| "resolved": "https://registry.npmjs.org/@storybook/addon-links/-/addon-links-10.1.2.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/@storybook+addon-links@10.1.2_react@18.3.1_storybook@10.1.2_@testing-library+dom@10.4.1_565930c96a4d2d2668cca92e07b3c91a/node_modules/@storybook/addon-links", | ||
| "dependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| } | ||
| } | ||
| }, | ||
| "@storybook/icons": { | ||
| "from": "@storybook/icons", | ||
| "version": "1.6.0", | ||
| "resolved": "https://registry.npmjs.org/@storybook/icons/-/icons-1.6.0.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/@storybook+icons@1.6.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@storybook/icons", | ||
| "dependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| } | ||
| } | ||
| }, | ||
| "@storybook/react-vite": { | ||
| "from": "@storybook/react-vite", | ||
| "version": "10.1.2", | ||
| "resolved": "https://registry.npmjs.org/@storybook/react-vite/-/react-vite-10.1.2.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/@storybook+react-vite@10.1.2_esbuild@0.27.0_react-dom@18.3.1_react@18.3.1__react@18.3.1_5dbe196b873a932f273991973bbfa1ed/node_modules/@storybook/react-vite", | ||
| "dependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| } | ||
| } | ||
| }, | ||
| "@testing-library/react": { | ||
| "from": "@testing-library/react", | ||
| "version": "16.3.0", | ||
| "resolved": "https://registry.npmjs.org/@testing-library/react/-/react-16.3.0.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/@testing-library+react@16.3.0_@testing-library+dom@10.4.1_@types+react-dom@18.3.7_@type_518e95b9fd5d6a2824e6f20b085f4963/node_modules/@testing-library/react", | ||
| "dependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| } | ||
| } | ||
| }, | ||
| "react-dom": { | ||
| "from": "react-dom", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom", | ||
| "dependencies": { | ||
| "react": { | ||
| "from": "react", | ||
| "version": "18.3.1", | ||
| "resolved": "https://registry.npmjs.org/react/-/react-18.3.1.tgz", | ||
| "path": "/absolute/path/github/recharts/node_modules/.pnpm/react@18.3.1/node_modules/react" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
Sample data incomplete - missing React 19.1.0.
The test in PnpmController.spec.ts expects this sample output to contain React versions ["18.3.1", "19.1.0"], but this file only contains React 18.3.1. To properly test the parser's ability to detect multiple versions of the same dependency, add an entry with React 19.1.0.
For example, you could add a devDependency that uses React 19.1.0 to demonstrate the multi-version detection scenario that verifySingleDependencyVersion is designed to catch.
🤖 Prompt for AI Agents
In test-ui/test/server/scripts/pnpm-list-sample-output.json around lines 1 to
144, the sample pnpm output only contains React version 18.3.1 but the test
expects both "18.3.1" and "19.1.0"; add an entry (e.g., under the existing
package's devDependencies or dependencies) that references React with version
"19.1.0" (including minimal fields: from, version "19.1.0", resolved URL and
path) so the sample file includes both versions and the parser/test can detect
multiple React versions.
| describe("PnpmController", () => { | ||
| it("should parse pnpm ls output to extract dependency versions", () => { | ||
| const controller = new PnpmController(""); | ||
| const actual = controller.parsePnpmLsOutput( | ||
| JSON.stringify(exampleOutput), | ||
| "react", | ||
| ); | ||
| const expected = new Set(["18.3.1", "19.1.0"]); | ||
| // Convert sets to arrays for better comparison | ||
| const actualArray = Array.from(actual).sort(); | ||
| const expectedArray = Array.from(expected).sort(); | ||
| expect(actualArray).toEqual(expectedArray); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test expectations don't match the sample data.
The test expects to find React versions ["18.3.1", "19.1.0"], but the sample output file pnpm-list-sample-output.json only contains React version 18.3.1. This test will fail.
Review the sample output and either:
- Update
pnpm-list-sample-output.jsonto include React 19.1.0, or - Update the test expectation to match the actual sample data:
new Set(["18.3.1"])
Additionally, consider adding a test comment explaining what scenario this sample represents (e.g., "Tests parsing output with multiple React versions" vs "Tests parsing output with single React version").
🤖 Prompt for AI Agents
test-ui/test/server/scripts/PnpmController.spec.ts around lines 5 to 18: the
test expects React versions ["18.3.1","19.1.0"] but the sample
pnpm-list-sample-output.json only contains "18.3.1", causing the test to fail;
fix by either updating pnpm-list-sample-output.json to include the 19.1.0 entry
(if you intend to test multiple React versions) or change the test expectation
to new Set(["18.3.1"]) to match the sample (and add a brief comment above the
test describing whether the sample covers a single-version or multi-version
scenario).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ckifer
left a comment
There was a problem hiding this comment.
It's about the upgrade path and transitive deps i think. If you're on a certain version and only upgrade recharts...
Maybe
Trying to reproduce recharts/recharts#6580 but couldn't.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.