Fix bundled dashboard symlinks in stack-cli#1485
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds symlink-aware dashboard asset copying: script helpers detect and copy hoisted pnpm symlink targets into the dashboard dist, and the dev runtime copy now preserves symlinks by using verbatimSymlinks when preparing the runtime. ChangesSymlink-aware dashboard copy
Sequence DiagramsequenceDiagram
participant CopyScript as copy-runtime-assets.mjs
participant BundledDist as bundled/dashboard/dist
participant PnpmLayout as bundled/dashboard/node_modules/.pnpm
participant RuntimePrep as prepareDashboardRuntime
CopyScript->>BundledDist: copy standalone dashboard (dereference existing files / filter)
CopyScript->>PnpmLayout: walk `.pnpm` node_modules and resolve hoisted symlinks
PnpmLayout->>BundledDist: copy resolved hoisted dependency targets into dist/node_modules/<dep>
RuntimePrep->>BundledDist: cpSync bundled dashboard into runtime with verbatimSymlinks:true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes the packed
Confidence Score: 4/5The core logic is sound and the author validated the end-to-end fix. The one scenario to watch is a build environment that produces absolute symlink targets, which would silently create broken symlinks on end-user machines. The approach — manifest at pack time, recreate at runtime — is the right way to survive npm's symlink-flattening. The replaceDashboardRuntimeSentinels traversal correctly skips symlink entries, so sentinel injection is unaffected. The only practical gap is that readlinkSync stores targets verbatim: pnpm workspaces consistently produce relative targets so this works today, but there's no guard if a future build configuration introduces absolute targets. Both changed files are straightforward. copy-runtime-assets.mjs is worth a second look around the readlinkSync storage of raw symlink targets. Important Files Changed
Sequence DiagramsequenceDiagram
participant B as Build (copy-runtime-assets.mjs)
participant D as dist/dashboard
participant P as npm pack
participant R as prepareDashboardRuntime (dev.ts)
participant RT as runtimeRoot
B->>D: cpSync standalone (verbatimSymlinks:true)
B->>D: collectSymlinks → write symlinks.json
D->>P: npm pack (symlinks expanded to real dirs)
P-->>R: installed package (no symlinks)
R->>RT: cpSync bundled dashboard (verbatimSymlinks:true, no-op for symlinks)
R->>RT: restoreDashboardRuntimeSymlinks (reads symlinks.json, rmSync+symlinkSync each entry)
R->>RT: replaceDashboardRuntimeSentinels (skips symlinks, rewrites .js sentinels)
R->>RT: spawn node server.js
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/stack-cli/scripts/copy-runtime-assets.mjs:51-54
Absolute symlink targets would silently break on end-user machines. `readlinkSync` returns the raw target, which could be an absolute path (e.g. `/home/ubuntu/.pnpm/next@16/…`) valid on the build machine but not at install time. Filtering those out at build time — rather than recreating them and finding them broken at runtime — keeps the manifest portable. The dangling-symlink guard (`existsSync`) already filters out targets that are broken at build time, but it won't catch targets whose absolute path simply doesn't exist on the end-user's machine.
```suggestion
const target = readlinkSync(path);
if (require("path").isAbsolute(target)) {
// Absolute symlink targets are not portable across machines; skip them.
continue;
}
symlinks.push({
path: path.slice(root.length + 1),
target,
});
```
Reviews (1): Last reviewed commit: "Fix bundled dashboard module symlinks" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| symlinks.push({ | ||
| path: path.slice(root.length + 1), | ||
| target: readlinkSync(path), | ||
| }); |
There was a problem hiding this comment.
readlinkSync returns the raw target, which could be an absolute path (e.g. /home/ubuntu/.pnpm/next@16/…) valid on the build machine but not at install time. Filtering those out at build time — rather than recreating them and finding them broken at runtime — keeps the manifest portable. The dangling-symlink guard (existsSync) already filters out targets that are broken at build time, but it won't catch targets whose absolute path simply doesn't exist on the end-user's machine.
| symlinks.push({ | |
| path: path.slice(root.length + 1), | |
| target: readlinkSync(path), | |
| }); | |
| const target = readlinkSync(path); | |
| if (require("path").isAbsolute(target)) { | |
| // Absolute symlink targets are not portable across machines; skip them. | |
| continue; | |
| } | |
| symlinks.push({ | |
| path: path.slice(root.length + 1), | |
| target, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/stack-cli/scripts/copy-runtime-assets.mjs
Line: 51-54
Comment:
Absolute symlink targets would silently break on end-user machines. `readlinkSync` returns the raw target, which could be an absolute path (e.g. `/home/ubuntu/.pnpm/next@16/…`) valid on the build machine but not at install time. Filtering those out at build time — rather than recreating them and finding them broken at runtime — keeps the manifest portable. The dangling-symlink guard (`existsSync`) already filters out targets that are broken at build time, but it won't catch targets whose absolute path simply doesn't exist on the end-user's machine.
```suggestion
const target = readlinkSync(path);
if (require("path").isAbsolute(target)) {
// Absolute symlink targets are not portable across machines; skip them.
continue;
}
symlinks.push({
path: path.slice(root.length + 1),
target,
});
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Fixed in 40524cc by failing the build if the dashboard standalone output contains an absolute symlink target. That keeps the generated manifest portable instead of recreating machine-local paths at runtime.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/stack-cli/scripts/copy-runtime-assets.mjs (2)
56-56: 💤 Low valueConsider using
path.relativefor clearer intent.The current slice-based approach works correctly, but using
path.relative(root, path)is more idiomatic and self-documenting.♻️ Optional refactor
+import { dirname, isAbsolute, join, relative, resolve } from "path";symlinks.push({ - path: path.slice(root.length + 1), + path: relative(root, path), target, });🤖 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 `@packages/stack-cli/scripts/copy-runtime-assets.mjs` at line 56, Replace the manual slice expression path.slice(root.length + 1) with the clearer idiom path.relative(root, path); locate the code that builds the object containing the path property (the line using path.slice(...)) and update the value to use path.relative(root, path) so intent is explicit and edge cases with different separators are handled by the path module.
48-50: 💤 Low valueConsider logging warnings for skipped broken symlinks.
Broken symlinks are silently skipped without any indication. While this may be intentional, logging a warning would help identify unexpected broken symlinks during the build process and aid in debugging.
📝 Optional logging enhancement
if (entry.isSymbolicLink()) { if (!existsSync(path)) { + console.warn(`Skipping broken symlink: ${path}`); continue; }🤖 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 `@packages/stack-cli/scripts/copy-runtime-assets.mjs` around lines 48 - 50, At the existsSync(path) check where broken symlinks are silently skipped, add a warning log before the continue so skipped paths are visible; update the branch handling the failed existsSync(path) to emit a clear message (e.g., with console.warn or the repo logger) that includes the path variable and a short note that the symlink/target was missing, then continue as before.
🤖 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.
Nitpick comments:
In `@packages/stack-cli/scripts/copy-runtime-assets.mjs`:
- Line 56: Replace the manual slice expression path.slice(root.length + 1) with
the clearer idiom path.relative(root, path); locate the code that builds the
object containing the path property (the line using path.slice(...)) and update
the value to use path.relative(root, path) so intent is explicit and edge cases
with different separators are handled by the path module.
- Around line 48-50: At the existsSync(path) check where broken symlinks are
silently skipped, add a warning log before the continue so skipped paths are
visible; update the branch handling the failed existsSync(path) to emit a clear
message (e.g., with console.warn or the repo logger) that includes the path
variable and a short note that the symlink/target was missing, then continue as
before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bed0301-f82d-4916-9413-1e1c6725175a
📒 Files selected for processing (1)
packages/stack-cli/scripts/copy-runtime-assets.mjs
|
CI investigation notes:
These CI failures appear preexisting/unrelated to the bundled dashboard symlink fix. |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| return undefined; | ||
| } | ||
|
|
||
| function copyDashboardHoistedDependencies(pnpmRoot, current = pnpmRoot) { |
There was a problem hiding this comment.
The copyDashboardHoistedDependencies function now properly checks if the directory exists before calling readdirSync() ## Bug Explanation (FIXED)
The copyDashboardHoistedDependencies function at line 72 was calling readdirSync(current) without first checking if the directory exists. The function is invoked on line 110 with the path join(dashboardStandaloneSrc, "node_modules/.pnpm"), which may not exist in all build scenarios.
When readdirSync() is called on a non-existent directory, it throws an ENOENT (Error: no such file or directory) error, causing the entire build process to fail. This can happen when there are no hoisted dependencies in the dashboard standalone build or the .pnpm directory wasn't created during the Next.js build process.
Fix Explanation
An early return check has been added at the beginning of the copyDashboardHoistedDependencies function:
if (!existsSync(current)) {
return;
}This check gracefully handles the case where the directory doesn't exist by simply returning without attempting to read its contents. The existsSync function is already imported from the fs module. This allows the build to continue successfully even when the .pnpm directory is absent, which is a valid and expected scenario in some build configurations.
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 `@packages/stack-cli/scripts/copy-runtime-assets.mjs`:
- Around line 52-53: The current path parsing (parts =
path.slice(pnpmRoot.length + 1).split("/") and
parts.lastIndexOf("node_modules")) is POSIX-only and the copy logic copies
non-hoisted symlinks instead of preferring hoisted ones, causing conflicts;
change parsing to normalize and split using the platform separator (use
path.normalize and split by path.sep or use path.relative(pnpmRoot,
p).split(path.sep)), locate indices by searching for the literal ".pnpm" and the
following "node_modules" segment (rather than a blind
lastIndexOf("node_modules")), and update the symlink selection logic (the block
around the current hoisted/non-hoisted selection at lines ~82-84) to prefer
entries whose source path includes ".pnpm/node_modules" (i.e., hoisted targets)
when multiple targets map to the same destination; ensure Windows paths are
handled consistently by using path utilities (path.normalize, path.relative,
path.sep) throughout.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 350d230e-1149-434c-b9a1-fcd319f01c71
📒 Files selected for processing (2)
packages/stack-cli/scripts/copy-runtime-assets.mjspackages/stack-cli/src/commands/dev.ts
1 participant
Fixes the packaged development-environment dashboard failing to resolve
nextafter Mintlify setup.Changes:
npm packships real files instead of broken pnpm symlinks.Validation:
pnpm --dir /home/ubuntu/repos/stack-auth --filter=@stackframe/stack-cli build@stackframe/stack-cli; verifieddist/dashboardcontains0symlinks.dist/dashboard/apps/dashboard/server.js; verifiedNext.js 16.1.7andReady.pnpm --dir /home/ubuntu/repos/stack-auth --filter=@stackframe/stack-cli lintpnpm --dir /home/ubuntu/repos/stack-auth --filter=@stackframe/stack-cli typecheckLink to Devin session: https://app.devin.ai/sessions/13728b5ccea44f23b2d300c79ffc2440
Requested by: @N2D4
Summary by CodeRabbit
New Features
Bug Fixes