Skip to content

Conversation

@moreorover
Copy link
Contributor

@moreorover moreorover commented Dec 27, 2025

closes #771

Summary by CodeRabbit

  • Refactor
    • Modified package manager version detection to execute in an isolated temporary directory context, improving execution reliability.

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

@vercel
Copy link

vercel bot commented Dec 27, 2025

@moreorover is attempting to deploy a commit to the Better T Stack Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working labels Dec 27, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 27, 2025

Walkthrough

Modified package manager version detection in the project configuration helper to execute commands within a temporary directory instead of the current working directory. Added node:os import to support this change. Preserves existing behavior for successful and failed version detection scenarios.

Changes

Cohort / File(s) Summary
Package Manager Version Detection
apps/cli/src/helpers/core/project-config.ts
Added node:os import and modified package manager version detection command to execute within temporary directory using os.tmpdir() as working directory.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change—fixing the missing packageManager field when running tests by modifying the working directory context for package manager version detection.
Linked Issues check ✅ Passed The code changes address the core requirement: packageManager detection now executes in a temporary directory, which resolves the test execution context issue preventing packageManager field generation in package.json.
Out of Scope Changes check ✅ Passed The changes are narrowly scoped to the package manager version detection mechanism in project-config.ts, directly addressing the linked issue #771 without introducing unrelated modifications.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f16a69 and 16760e3.

📒 Files selected for processing (1)
  • apps/cli/src/helpers/core/project-config.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/better-t-stack-repo.mdc)

Define functions using the standard function declaration syntax, not arrow functions

Files:

  • apps/cli/src/helpers/core/project-config.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/better-t-stack-repo.mdc)

**/*.{ts,tsx}: Use TypeScript type aliases instead of interface declarations
Do not use explicit return types

Files:

  • apps/cli/src/helpers/core/project-config.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)

**/*.{ts,tsx,js,jsx}: Use bun <file> instead of node <file> or ts-node <file> for running TypeScript/JavaScript files
Bun automatically loads .env files, so don't use the dotenv package
Use Bun.serve() which supports WebSockets, HTTPS, and routes instead of express
Use bun:sqlite module for SQLite instead of better-sqlite3
Use Bun.redis for Redis instead of ioredis
Use Bun.sql for Postgres instead of pg or postgres.js
Use built-in WebSocket instead of the ws package
Prefer Bun.file over node:fs readFile/writeFile methods
Use Bun.$ template literal syntax instead of execa for shell command execution
Import .css files directly in TypeScript/JavaScript files; Bun's CSS bundler will handle bundling
Run server with bun --hot <file> to enable hot reloading during development

Files:

  • apps/cli/src/helpers/core/project-config.ts
**/*.{ts,tsx,js,jsx,css}

📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)

Use bun build <file> instead of webpack or esbuild for bundling TypeScript, JavaScript, and CSS files

Files:

  • apps/cli/src/helpers/core/project-config.ts
**/*.{html,tsx,ts,jsx,js}

📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)

Use HTML imports with Bun.serve() for frontend instead of Vite

Files:

  • apps/cli/src/helpers/core/project-config.ts
🧠 Learnings (2)
📚 Learning: 2025-12-03T07:48:14.714Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-03T07:48:14.714Z
Learning: Applies to convex/**/*.ts : Always add 'use node'; to the top of files containing actions that use Node.js built-in modules

Applied to files:

  • apps/cli/src/helpers/core/project-config.ts
📚 Learning: 2025-12-03T07:48:26.419Z
Learnt from: CR
Repo: AmanVarshney01/create-better-t-stack PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-12-03T07:48:26.419Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.$` template literal syntax instead of `execa` for shell command execution

Applied to files:

  • apps/cli/src/helpers/core/project-config.ts
🔇 Additional comments (2)
apps/cli/src/helpers/core/project-config.ts (2)

4-4: LGTM! Import is necessary for the bug fix.

The addition of the node:os import is required to access os.tmpdir() used in the package manager version detection fix.


98-103: Good fix for the test failure. Running the version detection command in os.tmpdir() resolves the issue where the packageManager field was missing from the generated package.json.

However, the suggested refactor to Bun.$ contains a critical error: Bun's $ API uses $.cwd(path) as a method call, not $({ cwd: path }) object syntax. The code shown in the suggested refactor would fail if implemented. A proper migration to Bun would require:

$.cwd(os.tmpdir());
const { stdout } = await $`${packageManager} -v`.text();

For now, the execa-based fix is correct and functional. Any future refactor to Bun should account for the different API signature.

Likely an incorrect or invalid review comment.


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.

❤️ Share

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

@moreorover
Copy link
Contributor Author

this now runs pnpm -v command inside os temp folder and no longer collides with project default package manager which is bun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: packageManager is not declared when running tests

1 participant