Migrate workspace to Vite+ and pnpm#2899
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Published CLI overrides include internal vite-plus aliases
- Added a filter after resolving catalog overrides that removes entries whose resolved values use the npm: protocol prefix, which are internal workspace package aliases (vite→vite-plus-core, vitest→vite-plus-test) that should never appear in the published CLI package.json.
Or push these changes by commenting:
@cursor push 7cd038fb6a
Preview (7cd038fb6a)
diff --git a/apps/server/scripts/cli.ts b/apps/server/scripts/cli.ts
--- a/apps/server/scripts/cli.ts
+++ b/apps/server/scripts/cli.ts
@@ -231,6 +231,16 @@
const workspaceConfig = yield* readWorkspaceConfig();
const workspaceCatalog = workspaceConfig.catalog ?? {};
const workspaceOverrides = workspaceConfig.overrides ?? {};
+ const resolvedOverrides = resolveCatalogDependencies(
+ workspaceOverrides,
+ workspaceCatalog,
+ "apps/server",
+ );
+ // Filter out internal workspace aliases (npm: protocol redirections
+ // like vite→vite-plus) that are irrelevant to published consumers.
+ const publishOverrides = Object.fromEntries(
+ Object.entries(resolvedOverrides).filter(([, v]) => !v.startsWith("npm:")),
+ );
const pkg: PackageJson = {
name: serverPackageJson.name,
repository: serverPackageJson.repository,
@@ -244,11 +254,7 @@
workspaceCatalog,
"apps/server",
),
- overrides: resolveCatalogDependencies(
- workspaceOverrides,
- workspaceCatalog,
- "apps/server",
- ),
+ overrides: publishOverrides,
};
const original = yield* fs.readFileString(packageJsonPath);You can send follow-ups to the cloud agent here.
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| handlers?.onRequestStart?.({ | ||
| id: request.id, | ||
| tag: request.tag, | ||
| stream: false, |
There was a problem hiding this comment.
🟠 High src/wsRpcProtocol.ts:299
In the onRequestStart handler, stream is hardcoded to false regardless of the actual request type. The caller in wsTransport.ts checks info.stream to notify streamRequestStartListeners, so stream requests will never trigger those listeners. Consider passing the actual request.stream value instead.
- stream: false,
+ stream: request.stream,🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/client-runtime/src/wsRpcProtocol.ts around line 299:
In the `onRequestStart` handler, `stream` is hardcoded to `false` regardless of the actual request type. The caller in `wsTransport.ts` checks `info.stream` to notify `streamRequestStartListeners`, so stream requests will never trigger those listeners. Consider passing the actual `request.stream` value instead.
Evidence trail:
- packages/client-runtime/src/wsRpcProtocol.ts line 299: `stream: false` hardcoded
- packages/client-runtime/src/wsRpcProtocol.ts lines 292-304: `send` interceptor wrapping protocol
- packages/client-runtime/src/wsTransport.ts lines 290-298: `onRequestStart` handler checks `info.stream`, returns early if false, skipping `streamRequestStartListeners`
- packages/client-runtime/src/wsTransport.ts lines 74, 156, 295: `streamRequestStartListeners` definition and usage
- patches/effect@4.0.0-beta.73.patch lines 118-122: patched `onStreamRequest` uses `stream: true` in `RequestHooks`
- patches/effect@4.0.0-beta.73.patch lines 77-81: patched `onEffectRequest` uses `stream: false` in `RequestHooks`
- git_grep for `RequestHooks` in packages/**: no results, confirming `RequestHooks` is never provided in client-runtime
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Dev script hangs on child spawn failure
- Added
children.delete(child)and aprocess.exit()guard in theerrorhandler so the script terminates when a child fails to spawn.
- Added
- ✅ Fixed: Dev script won't exit on clean child termination
- Changed the
exithandler to callstopChildren()unconditionally (when not already shutting down), so a clean Electron exit also terminates the bundler watcher.
- Changed the
Or push these changes by commenting:
@cursor push b6534bdc2c
Preview (b6534bdc2c)
diff --git a/apps/desktop/scripts/dev.mjs b/apps/desktop/scripts/dev.mjs
--- a/apps/desktop/scripts/dev.mjs
+++ b/apps/desktop/scripts/dev.mjs
@@ -2,7 +2,11 @@
const commands = [
{ name: "desktop-bundle", command: "vp", args: ["pack", "--watch"] },
- { name: "desktop-electron", command: "node", args: ["scripts/dev-electron.mjs"] },
+ {
+ name: "desktop-electron",
+ command: "node",
+ args: ["scripts/dev-electron.mjs"],
+ },
];
let shuttingDown = false;
@@ -29,16 +33,23 @@
child.once("error", (error) => {
console.error(`[${name}] failed to start:`, error);
+ children.delete(child);
stopChildren();
process.exitCode = 1;
+
+ if (children.size === 0) {
+ process.exit(process.exitCode);
+ }
});
child.once("exit", (code, signal) => {
children.delete(child);
- if (!shuttingDown && (signal !== null || code !== 0)) {
+ if (!shuttingDown) {
stopChildren();
- process.exitCode = code ?? 1;
+ if (signal !== null || code !== 0) {
+ process.exitCode = code ?? 1;
+ }
}
if (children.size === 0) {You can send follow-ups to the cloud agent here.
| child.once("error", (error) => { | ||
| console.error(`[${name}] failed to start:`, error); | ||
| stopChildren(); | ||
| process.exitCode = 1; |
There was a problem hiding this comment.
Dev script hangs on child spawn failure
Low Severity
The error handler on a spawned child calls stopChildren() and sets process.exitCode but never removes the errored child from the children set. In Node.js, when a process fails to spawn (e.g., vp not found — ENOENT), the error event fires but exit does not. Since the errored child is never deleted from children, children.size never reaches zero, and the final process.exit() guard in the exit handler never triggers. The dev script hangs indefinitely after printing the error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 837e24a. Configure here.
|
|
||
| if (!shuttingDown && (signal !== null || code !== 0)) { | ||
| stopChildren(); | ||
| process.exitCode = code ?? 1; |
There was a problem hiding this comment.
Dev script won't exit on clean child termination
Medium Severity
The exit handler only calls stopChildren() when a child exits abnormally (signal !== null || code !== 0). When the Electron process exits cleanly (code 0, no signal) — e.g., the user closes the app window — the bundler watcher continues running and the dev script hangs indefinitely. The previous bun run --parallel behavior terminated all children when any child exited. This makes the dev script unusable without a manual Ctrl+C after every Electron close.
Reviewed by Cursor Bugbot for commit 837e24a. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Playwright missing
--with-depsbreaks CI browser installs- Restored the
--with-depsflag to thetest:browser:installscript so that system-level shared libraries required by Chromium are installed on CI runners.
- Restored the
Or push these changes by commenting:
@cursor push 7f2f71b4c1
Preview (7f2f71b4c1)
diff --git a/apps/web/package.json b/apps/web/package.json
--- a/apps/web/package.json
+++ b/apps/web/package.json
@@ -10,7 +10,7 @@
"typecheck": "tsgo --noEmit",
"test": "vp test run --passWithNoTests",
"test:browser": "node scripts/run-browser-tests.mjs",
- "test:browser:install": "playwright install chromium"
+ "test:browser:install": "playwright install --with-deps chromium"
},
"dependencies": {
"@base-ui/react": "^1.4.1",You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Release preflight uses different test command than CI
- Changed
vp testtovp run testin release.yml line 204 to match CI and invoke the recursive package test scripts with their custom flags.
- Changed
Or push these changes by commenting:
@cursor push 4ba05886c9
Preview (4ba05886c9)
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -201,7 +201,7 @@
run: vp run typecheck
- name: Test
- run: vp test
+ run: vp run test
- id: previous_tag
name: Resolve previous release tagYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 65b29b3. Configure here.
juliusmarminge
force-pushed
the
codex/vite-plus-pnpm-migration
branch
from
c2780fc to
368a54c
Compare
| const readWorkspaceConfig = Effect.fn("readWorkspaceConfig")(function* () { | ||
| const path = yield* Path.Path; | ||
| const fs = yield* FileSystem.FileSystem; | ||
| const repoRoot = yield* RepoRoot; | ||
| const workspaceYaml = yield* fs.readFileString(path.join(repoRoot, "pnpm-workspace.yaml")); | ||
| return parseYaml(workspaceYaml) as WorkspaceConfig; |
There was a problem hiding this comment.
🟢 Low scripts/cli.ts:56
parseYaml returns null when the YAML is empty or invalid, but the result is cast to WorkspaceConfig without a null check. When the file is empty, accessing workspaceConfig.catalog throws TypeError: Cannot read properties of null. Consider adding a null check or providing a default empty object before the cast.
const workspaceYaml = yield* fs.readFileString(path.join(repoRoot, "pnpm-workspace.yaml"));
- return parseYaml(workspaceYaml) as WorkspaceConfig;
+ const parsed = parseYaml(workspaceYaml);
+ return (parsed ?? {}) as WorkspaceConfig;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/scripts/cli.ts around lines 56-61:
`parseYaml` returns `null` when the YAML is empty or invalid, but the result is cast to `WorkspaceConfig` without a null check. When the file is empty, accessing `workspaceConfig.catalog` throws `TypeError: Cannot read properties of null`. Consider adding a null check or providing a default empty object before the cast.
Evidence trail:
apps/server/scripts/cli.ts lines 21 (import parseYaml), 42-45 (WorkspaceConfig interface), 56-62 (readWorkspaceConfig function with `as` cast), 231-233 (usage accessing .catalog and .overrides on potentially null value). The `yaml` npm package's `parse()` function returns `null` for empty strings and YAML documents with only comments.
| if (hasReceivedValue) { | ||
| try { | ||
| options?.onResubscribe?.(); | ||
| } catch { | ||
| // Ignore reconnect hook failures so the stream can recover. | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium src/wsTransport.ts:166
During each resubscription, options?.onResubscribe?.() is invoked twice: once before starting the stream (lines 166-172) and again via the onStreamRequestStart listener (lines 141-155). Both fire when hasReceivedValue is true, causing duplicate callback execution on every reconnect. Consider removing the newly added block (lines 166-172) since the listener already handles this case.
- if (hasReceivedValue) {
- try {
- options?.onResubscribe?.();
- } catch {
- // Ignore reconnect hook failures so the stream can recover.
- }
- }
- const runningStream = this.runStreamOnSession(🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/client-runtime/src/wsTransport.ts around lines 166-172:
During each resubscription, `options?.onResubscribe?.()` is invoked twice: once before starting the stream (lines 166-172) and again via the `onStreamRequestStart` listener (lines 141-155). Both fire when `hasReceivedValue` is true, causing duplicate callback execution on every reconnect. Consider removing the newly added block (lines 166-172) since the listener already handles this case.
Evidence trail:
packages/client-runtime/src/wsTransport.ts lines 141-155 (listener calls onResubscribe when hasReceivedValue), lines 166-172 (new direct call to onResubscribe when hasReceivedValue), lines 173-184 (runStreamOnSession starts stream on session), lines 290-297 (onRequestStart iterates streamRequestStartListeners for stream requests), line 156 (current subscription's listener is added to set). git_diff MERGE_BASE..REVIEWED_COMMIT confirms lines 166-172 are newly added in this PR.
| ); | ||
|
|
||
| const { data: gitStatus = null, error: gitStatusError } = useVcsStatus({ | ||
| const { data: gitStatus, error: gitStatusError } = useVcsStatus({ |
There was a problem hiding this comment.
🟠 High components/GitActionsControl.tsx:1060
Removing the = null default from gitStatus causes gitStatusForActions to be undefined during loading, but line 1599 checks gitStatusForActions !== null. Since undefined !== null is true, canPublishRepository incorrectly evaluates to true while git status is still loading, causing the "Publish repository" option to appear prematurely.
- const { data: gitStatus, error: gitStatusError } = useVcsStatus({
+ const { data: gitStatus = null, error: gitStatusError } = useVcsStatus({🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/GitActionsControl.tsx around line 1060:
Removing the `= null` default from `gitStatus` causes `gitStatusForActions` to be `undefined` during loading, but line 1599 checks `gitStatusForActions !== null`. Since `undefined !== null` is `true`, `canPublishRepository` incorrectly evaluates to `true` while git status is still loading, causing the "Publish repository" option to appear prematurely.
Evidence trail:
apps/web/src/components/GitActionsControl.tsx line 1060 (REVIEWED_COMMIT): `const { data: gitStatus, error: gitStatusError } = useVcsStatus(...)` — no default, so `gitStatus` is `undefined` during loading.
PR diff (MERGE_BASE → REVIEWED_COMMIT): confirms change from `data: gitStatus = null` to `data: gitStatus`.
apps/web/src/components/GitActionsControl.tsx line 1073 (REVIEWED_COMMIT): `const gitStatusForActions = gitStatus;` — alias.
apps/web/src/components/GitActionsControl.tsx line 1071-1072 (REVIEWED_COMMIT): `isRepo` defaults to `true`, `hasPrimaryRemote` defaults to `false` during loading.
apps/web/src/components/GitActionsControl.tsx line 1599 (REVIEWED_COMMIT): `const canPublishRepository = isRepo && gitStatusForActions !== null && !hasPrimaryRemote;` — `undefined !== null` is `true`, so `canPublishRepository` evaluates to `true` during loading.
1 participant
Summary
tsgoviavp run typecheckvite-plus/vite-plus/testand preserve pnpm Vite+ aliases forviteandvitestsetup-vp,node-version-file: package.json,vp install,vp check,vp run typecheck,vp test, andvp buildValidation
vp installvp checkvp run typecheckvp testvp buildNotes
vp checkintentionally keeps Oxlint typed checks disabled until the Oxlinttsgolintpath can integrate with@effect/tsgodiagnostics. Effect-aware typechecking runs through the patchedtsgoworkspace script instead.vp installcompletes, but pnpm still prints its warning about ignoredmswbuild scripts.Note
High Risk
Large cross-cutting change to package manager, bundler, test runner, and CI/release paths; any missed script or environment still on Bun will break installs and pipelines.
Overview
This PR migrates the monorepo from Bun/Turbo to pnpm and Vite+ (
vp), touching install scripts, local dev, packaging, tests, and all CI/release automation.Tooling and workspace:
packageManagerbecomes pnpm; catalog/overrides move topnpm-workspace.yaml/pnpm-lock.yaml(replacingbun.lock). Root.oxfmtrc.json/.oxlintrc.jsonand per-appturbo.jsoncare removed in favor of a shared rootvite.config.ts(fmt, lint, test defaults,vp staged). Bun is dropped from.mise.toml; docs/AGENTS now requirevp check,vp run typecheck, andvp test/vp run test.Apps/packages: Desktop and server builds move from tsdown to
vp pack(vite.config.tswithpackentries). Web usesvp dev/vp previewand collapses unit + Playwright browser tests into Vite+ test projects (deletes separatevitest.config.tsfiles). Test imports switch fromvitesttovite-plus/testacross the repo. ACP mock agents and similar spawns usenodeinstead ofbun.@pierre/diffsimports narrow toutils/parsePatchFilesandtypes.CI/CD: Workflows use
voidzero-dev/setup-vp@v1,vp install --frozen-lockfile,vp check, andvp runfor typecheck/test/build. CI splits browser tests into targetedvp test run --mode browsersteps and adds a manual Electron zip download/extract on Linux. Release/finalize steps usepnpm-lock.yamlin git commits,vp dlx vercel, and marketing/web Vercel install/build commands viavp install/vp run.Reviewed by Cursor Bugbot for commit 368a54c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Migrate monorepo toolchain from Bun/Turbo/Vitest to pnpm and Vite+
vp) for all CI, release, dev, build, test, lint, and format commands; removes turbo.json.apps/andpackages/fromvitestimports tovite-plus/test; adds vite.config.ts as the root Vite+ config covering tests, staged checks, formatting, and linting rules.vitest.config.ts/tsdown.config.ts, with namedunitandbrowsertest projects and Playwright support for browser tests in the web app.voidzero-dev/setup-vp@v1; Electron binaries are now downloaded via curl and extracted with Python instead of Bun.bun run <script>tonode <script>for mock agents and peer processes.turbo.jsonis deleted, so any remaining Turbo-specific pipeline caching or task ordering is lost.Macroscope summarized 368a54c.