fix(e2e): replace nextjs deploy-in-webServer with pnpm injected deps - #452
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR updates Next.js E2E projects to build injected workspace dependencies, synchronize them after builds, and start development servers directly. The widget test verifies that the terminal extension generates a command containing ChangesNext.js E2E setup
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This PR replaces the slow deployment-based test-server setup with injected dependencies and adds coverage for terminal extension loading; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
88ebb97 to
a2122a7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pnpm-workspace.yaml`:
- Around line 11-14: Add an uncached synchronization task in the workspace
configuration for the e2e test flow, using the existing
syncInjectedDepsAfterScripts behavior to refresh injected `@conciv/`* packages
before test:e2e. Ensure it runs even when Turbo restores cached build outputs,
and avoid changing the existing build synchronization setting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c57eb26-0d1b-4e73-b2c5-d76ae6264fe9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
e2e/e2e-utils/src/config.tse2e/nextjs-component/package.jsone2e/nextjs-component/playwright.config.tse2e/nextjs/package.jsone2e/nextjs/playwright.config.tse2e/nextjs/tests/widget.spec.tspnpm-workspace.yaml
| # on disk. This keeps the injected copies synced to a package's latest dist after every build: | ||
| # fires under turbo (which shells to `pnpm run`) and under a direct `pnpm --filter run build`. | ||
| syncInjectedDepsAfterScripts: | ||
| - build |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
required='10.5.0'
actual="$(pnpm --version)"
if [[ "$(printf '%s\n' "$required" "$actual" | sort -V | head -n1)" != "$required" ]]; then
printf 'pnpm %s is older than required pnpm %s\n' "$actual" "$required" >&2
exit 1
fi
rg -n '"packageManager"|"pnpm"' package.json
# Also run a clean-install build with the normal CI cache settings and inspect the injected copies after a cache hit.Repository: conciv-dev/conciv
Length of output: 519
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- local guidance and skills ---'
find .. -name AGENTS.md -o -path '*/skills/*' -o -iname '*tanstack*intent*' 2>/dev/null | sort | head -200
printf '%s\n' '--- workspace and package metadata ---'
cat -n pnpm-workspace.yaml
cat -n package.json | sed -n '1,180p'
printf '%s\n' '--- turbo and CI references ---'
find . -maxdepth 4 -type f \( -iname '*turbo*' -o -path './.github/workflows/*' -o -iname '*ci*' \) -print | sort
rg -n -i 'turbo|cache|pnpm|inject|syncInjectedDepsAfterScripts|workspace:|dist' .github package.json pnpm-workspace.yaml turbo.json turbo.jsonc 2>/dev/null | head -300Repository: conciv-dev/conciv
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- repository guidance ---'
cat -n AGENTS.md 2>/dev/null || true
printf '%s\n' '--- review skill ---'
cat -n .claude/skills/conciv-review/SKILL.md 2>/dev/null || true
printf '%s\n' '--- intent configuration and local checker references ---'
rg -n -C 3 'intent|TanStack Intent|skill check|skills' package.json AGENTS.md .claude .github 2>/dev/null | head -240
printf '%s\n' '--- CI cache and build steps ---'
for f in .github/workflows/ci.yml .github/workflows/fallow.yml .github/workflows/cache-cleanup.yml; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
rg -n -C 5 'setup-node|pnpm/action-setup|cache:|actions/cache|turbo|pnpm (install|i|run|test|build)|frozen|restore|save' "$f"
fi
done
printf '%s\n' '--- Turbo configuration ---'
cat -n turbo.json
for f in e2e/nextjs/turbo.json e2e/nextjs-component/turbo.json apps/examples/nextjs-app/turbo.json; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f"
fi
done
printf '%s\n' '--- injected dependency declarations and relevant scripts ---'
rg -l -n 'injected|dependenciesMeta|`@conciv/`' --glob 'package.json' . | sort | while read -r f; do
printf '\n--- %s ---\n' "$f"
rg -n -C 5 'injected|dependenciesMeta|scripts|build|`@conciv/`' "$f"
doneRepository: conciv-dev/conciv
Length of output: 50376
🌐 Web query:
pnpm 11 syncInjectedDepsAfterScripts Turbo cache hit build script does not execute documentation
💡 Result:
The issue where pnpm's syncInjectedDepsAfterScripts does not trigger after a Turbo cache hit is a known limitation caused by the design of how Turbo and pnpm interact [1][2][3]. When Turbo restores a build output from its cache, it bypasses the execution of the build script [1][2]. Because pnpm's syncInjectedDepsAfterScripts hook is designed to run only after the successful completion of a script directly invoked by pnpm, the sync process never fires when Turbo simply replays cached files [1][2][4]. As a result, the injected copies of dependencies in the pnpm store remain stale [2][3]. To resolve this, you can implement the following strategies: 1. Disable caching for the synchronization task: Create a dedicated synchronization task in Turbo (e.g., _syncPnpm) that runs after your build tasks [2][3]. Ensure this task has "cache": false in your turbo.json configuration so it always executes, even if the upstream build scripts were cache hits [2][3]. 2. Use a custom sync command: Instead of relying solely on the automated pnpm hook, create a script that performs the synchronization and include it in your task pipeline [1][3]. For example, you can run pnpm install --frozen-lockfile or a dedicated sync tool as a post-build step [2][3]. 3. Orchestration: Ensure your main build tasks depend on the synchronization task, or use Turbo's pipeline to explicitly define the execution order so that synchronization is forced to run after dependencies are restored [1][2]. Note that pnpm documentation defines syncInjectedDepsAfterScripts as a list of script names that, when executed, trigger the synchronization of injected dependencies [5][6]. It is explicitly documented that this mechanism relies on the script being executed by pnpm, which is why external task runners like Turbo can cause the hook to be skipped [1][2].
Citations:
- 1: syncInjectedDepsAfterScript does not work with turborepo pnpm/pnpm#9837
- 2: Fix injected deps sync when turbo replays cached builds universal-ember/ember-primitives#702
- 3: Fix injected deps sync when turbo replays cached builds universal-ember/ember-primitives#703
- 4: feat(injected-deps-syncer): sync injected copies after the named scripts pnpm/pnpm#13834
- 5: https://pnpm.io/workspaces
- 6: https://pnpm.io/10.x/workspaces
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- Turbo root task graph ---'
cat -n turbo.json
printf '%s\n' '--- affected package manifests ---'
for f in e2e/nextjs/package.json e2e/nextjs-component/package.json apps/examples/nextjs-app/package.json; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f"
fi
done
printf '%s\n' '--- all injected metadata ---'
rg -n -C 8 '"injected"|dependenciesMeta' --glob 'package.json' e2e apps packages
printf '%s\n' '--- all relevant task declarations ---'
rg -n -C 8 'build|test:e2e|cache: false|dependsOn|outputs|\.turbo' \
turbo.json e2e/nextjs/turbo.json e2e/nextjs-component/turbo.json \
.github/workflows/ci.yml .github/workflows/fallow.yml
printf '%s\n' '--- install and synchronization commands ---'
rg -n -C 4 'pnpm (i|install|run|exec)|syncInjectedDepsAfterScripts|node_modules|dist' \
.github/workflows e2e/nextjs e2e/nextjs-component package.json pnpm-workspace.yamlRepository: conciv-dev/conciv
Length of output: 50373
🌐 Web query:
site:pnpm.io/10.x/workspaces syncInjectedDepsAfterScripts injected dependencies scripts executed
💡 Result:
The syncInjectedDepsAfterScripts setting is a configuration option available in pnpm (introduced in version 10.5.0) for managing workspaces [1]. In pnpm workspaces, injected dependencies are implemented as collections of hardlinks [1]. Because these do not automatically update when the source files change, this can cause issues in environments where packages need to be built, such as TypeScript projects [1]. By specifying a list of script names in the syncInjectedDepsAfterScripts configuration, you instruct pnpm to synchronize the injected dependencies within node_modules whenever any of the listed scripts are executed in a workspace package [1]. This ensures that the hardlinked files remain up-to-date with their source counterparts following the execution of those scripts [1].
Citations:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import json
import re
from pathlib import Path
root = Path(".")
workspace = (root / "pnpm-workspace.yaml").read_text()
turbo = json.loads((root / "turbo.json").read_text())
ci = (root / ".github/workflows/ci.yml").read_text()
affected = [
Path("e2e/nextjs/package.json"),
Path("e2e/nextjs-component/package.json"),
]
for path in affected:
manifest = json.loads(path.read_text())
injected = [
name for name, meta in manifest.get("dependenciesMeta", {}).items()
if meta.get("injected") is True
]
assert injected, f"{path}: no injected dependencies found"
print(f"{path}: injected={','.join(injected)}")
assert "syncInjectedDepsAfterScripts:\n - build" in workspace
assert turbo["tasks"]["build"]["outputs"] == ["dist/**"]
assert turbo["tasks"]["build"]["dependsOn"] == ["^build"]
assert turbo["tasks"]["test:e2e"]["cache"] is False
assert turbo["tasks"]["test:e2e"]["dependsOn"] == ["^build"]
restore = ci.index("path: .turbo", ci.index("name: e2e consumer apps"))
install = ci.index("sfw pnpm i --frozen-lockfile", restore)
run_e2e = ci.index("conciv-ci-shards run --task e2e", install)
assert restore < install < run_e2e
e2e_block = ci[restore:run_e2e]
assert "turbo-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }}-" in e2e_block
assert "turbo-e2e-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }}-" in e2e_block
after_e2e_install = ci[install:run_e2e]
assert "syncInjectedDepsAfterScripts" not in after_e2e_install
assert not re.search(r"pnpm\s+(run\s+)?build|turbo\s+run\s+build", after_e2e_install)
print("CI ordering: restore .turbo < pnpm install < uncached test:e2e")
print("No post-install build or explicit injected-dependency synchronization occurs in the e2e job")
print("Result: a build cache hit can bypass the pnpm build hook for injected dependencies")
PYRepository: conciv-dev/conciv
Length of output: 534
Add an uncached synchronization step after dependency builds.
When CI restores a .turbo build result, Turbo restores dist/** without running build, so pnpm does not invoke syncInjectedDepsAfterScripts. The e2e job can then use stale or empty injected @conciv/* packages. Add an uncached synchronization task before test:e2e.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pnpm-workspace.yaml` around lines 11 - 14, Add an uncached synchronization
task in the workspace configuration for the e2e test flow, using the existing
syncInjectedDepsAfterScripts behavior to refresh injected `@conciv/`* packages
before test:e2e. Ensure it runs even when Turbo restores cached build outputs,
and avoid changing the existing build synchronization setting.
Source: MCP tools
a2122a7 to
77b3c09
Compare
The nextjs e2e apps deployed the whole workspace graph into a mktemp dir on every playwright run just to give Next/Turbopack a real non-symlinked node_modules (serverExternalPackages requires resolution under node_modules/). That deploy was slow and flaked under a single 180s webServer timeout (#435). dependenciesMeta.injected gives pnpm's own hard-copy node_modules without a deploy step, and syncInjectedDepsAfterScripts: [build] keeps those copies synced — but only when a package's own "build" script actually RUNS. CI's `turbo run test:e2e` gets a cache HIT on every package build, which restores dist straight into the source package dir without executing the script, so the sync never fires: a cold `pnpm install` (before any dist exists) leaves the injected copies permanently distless, and next dev fails with MODULE_NOT_FOUND. Ruled out two lighter fixes with evidence before landing on this one: pnpm 11.7.0 has no manual resync CLI command, and a plain `pnpm install --frozen-lockfile` no-ops on an already-satisfied lockfile even with a drifted injected copy (directory deps carry no integrity hash for pnpm to notice the drift). So each nextjs e2e app's playwright.config.ts webServer command prepends a direct `pnpm --filter <its injected deps> run build` before next dev, bypassing turbo's cache so the script always actually runs and the sync always fires. Both playwright configs keep the existing 180s webServer default. The build-and-sync step lives in playwright.config.ts, not the app's own package.json test:e2e script: e2e/init clones these apps as consumer templates and strips @conciv/* wiring from package.json before running `conciv init`, and a test:e2e script naming @conciv/it would leak conciv wiring into that stripped clone. dependenciesMeta has to stay in package.json (that's the injection mechanism itself), so e2e/init's own stripper (withoutConcivDependencies) now also drops @conciv/* entries from dependenciesMeta, deleting the field if that empties it — the stripper doing its declared job of removing all conciv wiring from the template. The previous symlink-only setup silently dropped builtin server extensions (MODULE_NOT_FOUND from the terminal extension's dynamic require) while the suite stayed green. Added a positive assertion in e2e/nextjs's widget spec that exercises the terminal extension's real connect-command RPC round trip, so that failure mode goes red instead of silent; verified it fails under the pre-fix symlinked state and passes under the injected-deps fix. Closes #435 Refs #451 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
77b3c09 to
1e0a837
Compare
The mechanism story lives in the history of this branch and PR #452: injected deps exist because Turbopack's serverExternalPackages needs real dirs under node_modules; syncInjectedDepsAfterScripts only fires when a dep's build script runs, so each nextjs e2e app's playwright webServer command runs those builds directly (turbo cache hits restore dist without executing scripts and would leave injected copies stale). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
e2e/nextjsande2e/nextjs-componentused topnpm deploy --legacythe whole workspace graph into a mktemp dir inside playwright'swebServer.commandon every run, purely to give Next/Turbopack a real non-symlinkednode_modules(serverExternalPackagesrequires resolution undernode_modules/; workspace symlinks resolve outside it). That deploy chain was slow and flaked under one 180s webServer timeout (flaky: e2e consumer webServer boot timeout (180s) on nextjs shard #435).dependenciesMeta.<pkg>.injected: trueon each app's runtime@conciv/*deps (verified per app:nextjsonly imports@conciv/it;nextjs-componentimports@conciv/it,@conciv/react,@conciv/extension-terminal), plussyncInjectedDepsAfterScripts: [build]inpnpm-workspace.yamlso the injected hard-copies stay synced to each package's latestdistafter every build (confirmed by inode: rebuilding@conciv/it/@conciv/reactre-links the injected copy to the fresh dist file, both underturbo run buildand a directpnpm --filter run build).pnpm exec next dev --port <port>with an honest 60s webServer timeout (was 180s for the deploy chain); no deploy helpers, no root-walkers.webServerTimeoutas an explicit, optional per-app override one2eConfigin@conciv/e2e-utils/config(other e2e apps keep the existing 180s default).The silent-failure mode this closes
Under the old deploy, extension packages resolved fine. Under a naive plain-symlink alternative (no injection at all), the terminal builtin server extension's dynamic
requirethrowsMODULE_NOT_FOUNDand the code swallows it (packages/it/src/plugin/nextjs.ts), loggingconciv: failed to load builtin server extensions ...while the suite stays green — a real coverage gap. Added a positive assertion ine2e/nextjs/tests/widget.spec.tsthat opens the Terminal extension's connect menu and asserts the real--mcp-configconnect command comes back (a genuine RPC round trip through the server-registered terminal extension), so this failure mode goes red instead of silent.I verified the new test both ways: reverted just the
dependenciesMetaflag one2e/nextjs(back to a plain symlink), reinstalled, and the new test failed red on the real regression; restored the fix, reinstalled, and it passed green again.Closes #435
Closes #451
Test plan
pnpm turbo run typecheck --filter=conciv-e2e-nextjs --filter=conciv-e2e-nextjs-component --filter=@conciv/e2e-utils— passpnpm lint— pass (0 errors; pre-existing unrelated warnings inapps/site)pnpm format:check— pass on all touched files (one pre-existing unrelated failure indocs/assets/page-session-card-spike/fake-form.html, untouched by this PR)pnpm exec fallow audit --changed-since main --format json— verdictpass, no introduced findingse2e/nextjs:pnpm exec playwright test— 11 passed (in-placenext devbooted in752ms, vs. minutes for the old deploy chain)e2e/nextjs-component:pnpm exec playwright test— 1 passeddependenciesMeta.injectedflag for@conciv/itine2e/nextjsand reinstalling reproduces a plain symlink; the new extension-load test fails red under that conditionNotes for reviewer / orchestrator
CONCIV_DEPLOY_FRESHreferences existed anywhere in the repo to clean up.e2e/nextjs/packed/*(the tarball-shape verifier) is untouched, as intended — it's orthogonal to this dev-server mechanism.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests