Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/cli/commands/deploy.ts (2)
36-36: Useconsolainstead ofconsole.logper project logging guidelines.♻️ Proposed fix
+import { consola } from "consola";- console.log(`$ ${deployCommand}`); + consola.log(`$ ${deployCommand}`);As per coding guidelines: "Use
consolafor logging in build/dev code; usenitro.loggerwhen available."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/deploy.ts` at line 36, Replace the direct console.log call with the project's consola logger: import consola at top of the file (if not already imported) and use consola.info (or consola.log per style guide) to output the deployCommand instead of console.log; update the statement that currently references deployCommand in deploy.ts to call consola instead of console.log so it conforms to project logging guidelines.
18-21:ctx as anybypasses type safety when delegating tobuildCmd.run.
buildCmd.runexpects a context typed againstbuildArgs, but the deployctxcarries the additionalprebuiltfield. The cast is runtime-safe (the extra field is ignored), but it loses static type guarantees. Consider extracting the build logic into a shared helper that both commands call directly, or casting only theargsportion:♻️ Narrower cast alternative
- await buildCmd.run!(ctx as any); + await buildCmd.run!({ ...ctx, args: ctx.args } as Parameters<typeof buildCmd.run>[0]);Or — preferred — extract the core build steps into a
runBuild(args)helper inbuild.tsthat both commands call directly, eliminating the cross-commandrun!invocation entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/deploy.ts` around lines 18 - 21, The deploy command's run currently calls buildCmd.run with an unsafe `ctx as any`, losing type safety; refactor by extracting the build logic into a typed helper (e.g., export runBuild(args: buildArgs) from build.ts) and have both buildCmd.run and deploy's async run call runBuild(ctx.args) (or, if you prefer a minimal change, narrow the cast to the args only by passing `ctx.args as buildArgs` to buildCmd.run); update references to buildCmd.run and the deploy run to use the new runBuild helper (or the narrowed args cast) so types remain correct and no broad `as any` is used.src/build/rolldown/prod.ts (1)
47-55: Dead code:rOutput,rewriteRelativePaths, and therelativeimport are now no-ops.The static string
"You can deploy this build using \npx nitro deploy --prebuilt`"contains no./pathpattern matching the regex/([\s:])./(\S*)/g, making therewriteRelativePaths(...)call on line 54 a no-op.rOutputandrewriteRelativePathsare therefore unreachable/useless, and therelative` import on line 5 is only consumed by this dead code.♻️ Proposed cleanup
-import { relative } from "pathe"; +- // Show deploy and preview hints - const rOutput = relative(process.cwd(), nitro.options.output.dir); - const rewriteRelativePaths = (input: string) => { - return input.replace(/([\s:])\.\/(\S*)/g, `$1${rOutput}/$2`); - }; nitro.logger.success("You can preview this build using `npx nitro preview`"); if (buildInfo.commands!.deploy) { nitro.logger.success( - rewriteRelativePaths("You can deploy this build using `npx nitro deploy --prebuilt`") + "You can deploy this build using `npx nitro deploy --prebuilt`" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/rolldown/prod.ts` around lines 47 - 55, The variables rOutput and rewriteRelativePaths and the import of relative are dead because the deploy message string contains no "./" patterns; remove the unused relative import and delete the rOutput declaration and rewriteRelativePaths function, then replace the call nitro.logger.success(rewriteRelativePaths("You can deploy this build using `npx nitro deploy --prebuilt`")) with a direct nitro.logger.success("You can deploy this build using `npx nitro deploy --prebuilt`") so the message is logged without the no-op transformation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/build/rollup/prod.ts`:
- Around line 49-52: Update the deploy hint to match other builders by including
the --prebuilt flag: change the second nitro.logger.success call that runs when
buildInfo.commands!.deploy is true to instruct users to run "npx nitro deploy
--prebuilt" (i.e., add the --prebuilt suffix to the message) so the rollup
builder's message is consistent with rolldown/vite/prod.ts.
In `@src/build/vite/prod.ts`:
- Around line 123-127: The preview hint message in vite's build output uses the
wrong CLI; update the success log emitted by nitro.logger.success to recommend
"npx nitro preview" instead of "npx vite preview" so it matches rollup/rolldown
behavior and actually starts the Nitro server; change the string in the
nitro.logger.success call (the one guarded by !isTest && !isCI) to "You can
preview this build using `npx nitro preview`" and keep the existing deploy hint
logic (nitro.options.commands.deploy) unchanged.
---
Nitpick comments:
In `@src/build/rolldown/prod.ts`:
- Around line 47-55: The variables rOutput and rewriteRelativePaths and the
import of relative are dead because the deploy message string contains no "./"
patterns; remove the unused relative import and delete the rOutput declaration
and rewriteRelativePaths function, then replace the call
nitro.logger.success(rewriteRelativePaths("You can deploy this build using `npx
nitro deploy --prebuilt`")) with a direct nitro.logger.success("You can deploy
this build using `npx nitro deploy --prebuilt`") so the message is logged
without the no-op transformation.
In `@src/cli/commands/deploy.ts`:
- Line 36: Replace the direct console.log call with the project's consola
logger: import consola at top of the file (if not already imported) and use
consola.info (or consola.log per style guide) to output the deployCommand
instead of console.log; update the statement that currently references
deployCommand in deploy.ts to call consola instead of console.log so it conforms
to project logging guidelines.
- Around line 18-21: The deploy command's run currently calls buildCmd.run with
an unsafe `ctx as any`, losing type safety; refactor by extracting the build
logic into a typed helper (e.g., export runBuild(args: buildArgs) from build.ts)
and have both buildCmd.run and deploy's async run call runBuild(ctx.args) (or,
if you prefer a minimal change, narrow the cast to the args only by passing
`ctx.args as buildArgs` to buildCmd.run); update references to buildCmd.run and
the deploy run to use the new runBuild helper (or the narrowed args cast) so
types remain correct and no broad `as any` is used.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/presets/deno/preset.ts (1)
20-20:cd ./creates implicit coupling with deploy.ts path-rewriting regex.The
cd ./prefix is a synthetic placeholder whose only purpose is to be matched by the regex/([\s:])\.\/(\S*)/gindeploy.tsand rewritten tocd <outputDir>/. Read in isolation it looks like a no-op, and future preset authors won't know to follow this convention.A cleaner alternative is to run the deploy command with
cwd: outputDirdirectly inexecSync, removing the need for this coupling entirely:♻️ Suggested simplification (requires coordinated change in deploy.ts)
- deploy: "cd ./ && deno run -A jsr:`@deno/deployctl` deploy server/index.ts", + deploy: "deno run -A jsr:`@deno/deployctl` deploy server/index.ts",Then in
src/cli/commands/deploy.ts(see separate comment), passcwd: outputDirtoexecSync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presets/deno/preset.ts` at line 20, The preset currently embeds a synthetic "cd ./ " prefix in deploy: "cd ./ && deno run -A jsr:`@deno/deployctl` deploy server/index.ts" to satisfy the path-rewriting regex /([\s:])\.\/(\S*)/g in deploy.ts; remove this implicit coupling by deleting the "cd ./ && " prefix in src/presets/deno/preset.ts and instead update deploy.ts where execSync is called to pass cwd: outputDir (so commands run in the outputDir rather than relying on path-rewriting), ensuring any path-rewrite logic tied to /([\s:])\.\/(\S*)/g is removed or adapted accordingly.src/cli/commands/deploy.ts (1)
22-22: Non-null assertion andas anycast bypass type safety.
buildCmd.runis always defined bydefineCommand, so!is low-risk, but theas anycast silently passes theprebuiltarg — unknown to the build command — through to itsrunhandler. If the build command ever validates or iterates overctx.args, this could produce unexpected behaviour.♻️ Suggested fix
- await buildCmd.run!(ctx as any); + if (buildCmd.run) { + await buildCmd.run(ctx as any); + }Alternatively, destructure only the build-relevant args and construct a narrower context, though that requires more scaffolding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/deploy.ts` at line 22, Replace the unsafe non-null assertion and `as any` cast on buildCmd.run by calling it with a properly typed/narrow context: derive the expected context type from buildCmd.run (e.g. using Parameters<typeof buildCmd.run>[0]) and cast ctx to that type, or construct a new object (e.g. buildCtx) that copies only the properties the build command needs (not the extraneous `prebuilt` arg) and pass that to buildCmd.run; keep the call as await buildCmd.run(buildCtx) and remove both `!` and `as any`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/deploy.ts`:
- Around line 26-30: Remove the leftover commented-out throw in the deploy
command: delete the line "// throw new Error("No build info found, cannot
deploy.");" in the block that checks buildInfo (the check using buildInfo,
consola.error and process.exit(1) in deploy.ts) so the code is clean and only
logs and exits without dead commented code.
- Line 43: Wrap the execSync(deployCommand, { stdio: "inherit" }) call in a
try/catch inside the deploy command handler so failures don't throw a raw Node
stack: catch the thrown error from execSync, log a clear error via the same
logger used elsewhere (or console.error) including the error message and exit
with process.exit(1); keep the deployCommand identifier and execSync call intact
and only add error handling around it.
- Around line 38-41: The current path-rewriting using deployCommand =
buildInfo.commands.deploy.replace(/([\s:])\.\/(\S*)/g,
`$1${relative(process.cwd(), outputDir)}/$2`) is brittle (breaks when
relative(...) === "" and injects unquoted paths); remove this regex-based
rewrite and stop interpolating relative(process.cwd(), outputDir) into the shell
string. Instead, execute the original buildInfo.commands.deploy verbatim and
pass outputDir as the working directory to the process runner (use
execSync/child_process.exec with the cwd: outputDir option and appropriate stdio
handling), and optionally remove the redundant "cd ./..." placeholder from
presets (e.g., src/presets/deno/preset.ts) so commands don't rely on manual cd
behavior.
---
Nitpick comments:
In `@src/cli/commands/deploy.ts`:
- Line 22: Replace the unsafe non-null assertion and `as any` cast on
buildCmd.run by calling it with a properly typed/narrow context: derive the
expected context type from buildCmd.run (e.g. using Parameters<typeof
buildCmd.run>[0]) and cast ctx to that type, or construct a new object (e.g.
buildCtx) that copies only the properties the build command needs (not the
extraneous `prebuilt` arg) and pass that to buildCmd.run; keep the call as await
buildCmd.run(buildCtx) and remove both `!` and `as any`.
In `@src/presets/deno/preset.ts`:
- Line 20: The preset currently embeds a synthetic "cd ./ " prefix in deploy:
"cd ./ && deno run -A jsr:`@deno/deployctl` deploy server/index.ts" to satisfy the
path-rewriting regex /([\s:])\.\/(\S*)/g in deploy.ts; remove this implicit
coupling by deleting the "cd ./ && " prefix in src/presets/deno/preset.ts and
instead update deploy.ts where execSync is called to pass cwd: outputDir (so
commands run in the outputDir rather than relying on path-rewriting), ensuring
any path-rewrite logic tied to /([\s:])\.\/(\S*)/g is removed or adapted
accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.