Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Zephyr Nitro preset (preset, types, runtime), deployment utilities, CLI deploy integration, package dependency for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: |
Nsttt
force-pushed
the
feat/zephyr-preset
branch
from
695c9f9 to
2b0464d
Compare
Nsttt
force-pushed
the
feat/zephyr-preset
branch
from
2b0464d to
e3bd861
Compare
# Conflicts: # pnpm-lock.yaml
![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
🧹 Nitpick comments (2)
test/unit/zephyr-preset.test.ts (1)
11-36: Consider adding a test for thecompiledhook deployment wiring.Current coverage validates
build:before, but notcompiledbehavior (uploadNitroOutputToZephyrcall path and publicDir resolution). A focused test here would catch deploy-time regressions earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/zephyr-preset.test.ts` around lines 11 - 36, Add a unit test that invokes the zephyr preset's compiled hook to validate deployment wiring: locate the preset via zephyrPresets and its hooks (hooks["compiled"]) and call it with a nitro-like object that stubs options (including output.dir and output.publicDir scenarios), a logger, and a mocked uploadNitroOutputToZephyr function; assert that uploadNitroOutputToZephyr is called with the resolved server output path and the correct publicDir resolution, and that logger methods are used as expected to capture deploy-time messages.src/presets/zephyr/utils.ts (1)
98-109: Provider validation is duplicated and stateful; simplify to a single stateless check.
parsePulledProvideralready enforces the supported provider set, so the laterprovider !== "cloudflare"branch is redundant today. Also, module-levelpulledProvidercouples separate invocations in the same process.Refactor sketch
-let pulledProvider: ZephyrProvider | undefined; @@ -function resolveProvider(appPlatform: unknown): ZephyrProvider { - const pulled = parsePulledProvider(appPlatform); - if (!pulledProvider) { - pulledProvider = pulled; - } - if (pulledProvider !== pulled) { - throw new TypeError( - `[${LOGGER_TAG}] Zephyr PLATFORM changed from "${pulledProvider}" to "${pulled}" within the same process.` - ); - } - return pulledProvider; -} +function resolveProvider(appPlatform: unknown): ZephyrProvider { + return parsePulledProvider(appPlatform); +} @@ - const provider = resolveProvider(appConfig?.PLATFORM); - if (provider !== "cloudflare") { - throw new TypeError( - `[${LOGGER_TAG}] Zephyr PLATFORM "${provider}" is not supported yet by this Nitro preset. Supported today: cloudflare. See https://docs.zephyr-cloud.io` - ); - } + resolveProvider(appConfig?.PLATFORM);Also applies to: 185-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presets/zephyr/utils.ts` around lines 98 - 109, The resolveProvider function uses a module-level pulledProvider and duplicate validation; remove the stateful pulledProvider usage and simplify resolveProvider to be stateless by returning the result of parsePulledProvider(appPlatform) directly (i.e., call parsePulledProvider and return its value), removing the pulledProvider assignment and the subsequent equality check/TypeError; also apply the same simplification to the similar code block referenced around lines 185-189 so both sites rely solely on parsePulledProvider for validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 181: Replace the wildcard dependency for "zephyr-agent" in package.json
with a pinned semver (e.g. "^0.1.13") and update the lockfile; then verify that
the pinned release exposes the internal APIs used (readDirRecursiveWithContents,
buildAssetsMap, zeBuildDashData, ZephyrEngine.create) by checking the package
source or installed types and run a full build/test to confirm compatibility—if
any API differs, adjust calls in src/presets/zephyr/utils.ts to match the pinned
version or choose a compatible semver range.
---
Nitpick comments:
In `@src/presets/zephyr/utils.ts`:
- Around line 98-109: The resolveProvider function uses a module-level
pulledProvider and duplicate validation; remove the stateful pulledProvider
usage and simplify resolveProvider to be stateless by returning the result of
parsePulledProvider(appPlatform) directly (i.e., call parsePulledProvider and
return its value), removing the pulledProvider assignment and the subsequent
equality check/TypeError; also apply the same simplification to the similar code
block referenced around lines 185-189 so both sites rely solely on
parsePulledProvider for validation.
In `@test/unit/zephyr-preset.test.ts`:
- Around line 11-36: Add a unit test that invokes the zephyr preset's compiled
hook to validate deployment wiring: locate the preset via zephyrPresets and its
hooks (hooks["compiled"]) and call it with a nitro-like object that stubs
options (including output.dir and output.publicDir scenarios), a logger, and a
mocked uploadNitroOutputToZephyr function; assert that uploadNitroOutputToZephyr
is called with the resolved server output path and the correct publicDir
resolution, and that logger methods are used as expected to capture deploy-time
messages.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
package.jsonsrc/presets/_all.gen.tssrc/presets/_types.gen.tssrc/presets/zephyr/preset.tssrc/presets/zephyr/runtime/server.tssrc/presets/zephyr/utils.tstest/unit/zephyr-preset.test.ts
|
Update: Zephyr preset now supports both build-time and deploy-time flows, with declarative opt-out. Usage: import { defineConfig } from "nitro";
export default defineConfig({
preset: "zephyr",
zephyr: {
deployOnBuild: false, // optional; default true
},
});Behavior:
Generated contract in output dir:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/unit/zephyr-preset.test.ts (1)
4-5: Switch test path operations fromnode:pathtopathe.This test file imports
joinandresolvefromnode:pathon line 4-5 and uses them throughout. Usepatheinstead for cross-platform path handling consistency with the repository guideline.Proposed fix
-import { join, resolve } from "node:path"; +import { join, resolve } from "pathe";Affected usages: lines 159, 164, 166, 180.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/zephyr-preset.test.ts` around lines 4 - 5, Replace node:path imports with pathe: update the import statement that currently reads "import { join, resolve } from 'node:path'" to import from "pathe" instead, and ensure all usages of join and resolve in this test (e.g., where test paths are built around tmpdir and fixtures inside the zephyr-preset.test.ts file) continue to call the same function names; no other behavioral changes are needed. Keep tmpdir import from "node:os" as-is and run the unit tests to confirm cross-platform paths now use pathe.
🤖 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/presets/zephyr/preset.ts`:
- Around line 54-61: Capture the return value from uploadNitroOutputToZephyr
(e.g., const deploymentUrl = await uploadNitroOutputToZephyr(...)) and include
that URL in the success log so the deployed Zephyr URL is printed after
build-time deploy; update the nitro.logger.success call (or add a subsequent
nitro.logger.info) to reference deploymentUrl and provide a clear message like
"Zephyr deployment succeeded: <deploymentUrl>" using the existing LOGGER_TAG.
In `@src/presets/zephyr/utils.ts`:
- Around line 84-87: The current meta loading uses .catch(() => ({})) which
hides real read/parse errors; replace that with an explicit try/catch around
reading and JSON.parse of metaPath (references: metaPath, meta, outputDir) so
that if readFile throws ENOENT you return {} (treat missing file as
recoverable/warn), but for any other error (including JSON.parse errors) rethrow
or propagate the error so invalid states fail loudly; optionally log a warning
when returning {} for a missing file.
- Around line 1-4: Replace the node:path import with pathe's resolve to keep
path operations cross-platform: remove "resolve" import from "node:path" and
import { resolve } from "pathe" (alongside the existing normalize import), then
ensure every call site that uses resolve in this module (the preset utility
functions that build file paths) uses the pathe resolve import. Keep all other
logic unchanged so normalize and resolve come from pathe for consistent behavior
across platforms.
---
Nitpick comments:
In `@test/unit/zephyr-preset.test.ts`:
- Around line 4-5: Replace node:path imports with pathe: update the import
statement that currently reads "import { join, resolve } from 'node:path'" to
import from "pathe" instead, and ensure all usages of join and resolve in this
test (e.g., where test paths are built around tmpdir and fixtures inside the
zephyr-preset.test.ts file) continue to call the same function names; no other
behavioral changes are needed. Keep tmpdir import from "node:os" as-is and run
the unit tests to confirm cross-platform paths now use pathe.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/presets/_types.gen.tssrc/presets/zephyr/preset.tssrc/presets/zephyr/types.tssrc/presets/zephyr/utils.tstest/unit/zephyr-preset.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/presets/_types.gen.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/presets/zephyr/utils.ts (1)
180-183: Remove unreachable fallback.
opts.rootDiris a required parameter in the function signature, so|| process.cwd()is unreachable dead code.♻️ Suggested fix
const zephyrEngine = await zephyrAgent.ZephyrEngine.create({ builder: "unknown", - context: opts.rootDir || process.cwd(), + context: opts.rootDir, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presets/zephyr/utils.ts` around lines 180 - 183, The ZephyrEngine creation contains an unreachable fallback using process.cwd(); update the call to zephyrAgent.ZephyrEngine.create to pass opts.rootDir directly for the context parameter (remove the "|| process.cwd()" fallback) since opts.rootDir is required; adjust any related doc/comments or assumptions around context if needed.
🤖 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 42-53: The Zephyr deploy branch in the CLI omits publicDir and
baseURL when calling uploadNitroOutputToZephyr, causing inconsistent asset path
resolution compared to the preset hook; update the call inside the
buildInfo.preset === "zephyr" branch to pass the same publicDir and baseURL
values used at build time (alongside rootDir and outputDir) so
uploadNitroOutputToZephyr (and resolveAssetPath) receives the correct
parameters.
---
Nitpick comments:
In `@src/presets/zephyr/utils.ts`:
- Around line 180-183: The ZephyrEngine creation contains an unreachable
fallback using process.cwd(); update the call to zephyrAgent.ZephyrEngine.create
to pass opts.rootDir directly for the context parameter (remove the "||
process.cwd()" fallback) since opts.rootDir is required; adjust any related
doc/comments or assumptions around context if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cli/commands/deploy.tssrc/presets/zephyr/preset.tssrc/presets/zephyr/utils.tstest/unit/zephyr-preset.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/presets/zephyr/preset.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/presets/zephyr/utils.ts (1)
185-186: Consider capturing or logging the resolved provider.
resolveProvider(appConfig?.PLATFORM)is called purely for validation (to throw if unsupported), but the resolved provider value is discarded. This is fine for validation, but capturing it could be useful for future logging or conditional logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presets/zephyr/utils.ts` around lines 185 - 186, The call to resolveProvider(appConfig?.PLATFORM) only validates the platform but discards the returned provider; update the code where zephyrEngine.application_configuration is awaited to capture the resolved value (e.g., assign to a local like resolvedProvider from resolveProvider(appConfig?.PLATFORM)) and then use or log that variable for future conditional logic or debugging (refer to resolveProvider and the appConfig variable retrieved from zephyrEngine.application_configuration); ensure any existing behavior (throwing on unsupported platforms) remains unchanged.test/unit/zephyr-preset.test.ts (1)
11-185: Consider adding error scenario tests.The test suite covers happy paths well. Consider adding tests for error conditions:
- When
uploadNitroOutputToZephyrthrows (e.g., no deployable assets, missing entrypoint)- When deployment succeeds but
deploymentUrlisnullThis would improve resilience against regressions in error handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/zephyr-preset.test.ts` around lines 11 - 185, Add negative-path unit tests for the zephyr preset: mock uploadNitroOutputToZephyr to throw and assert hooks.compiled calls result in nitro.logger.error being called with a descriptive failure message (and nitro.logger.success not called), and mock uploadNitroOutputToZephyr to resolve with deploymentUrl: null and assert hooks.compiled results in nitro.logger.error or nitro.logger.info being called appropriately (and success not called); use the same vi.doMock pattern as existing tests to replace uploadNitroOutputToZephyr and invoke preset.hooks!.compiled with a nitro object (including logger stubs) to assert the error-handling behavior of the compiled hook.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/presets/zephyr/utils.ts`:
- Around line 185-186: The call to resolveProvider(appConfig?.PLATFORM) only
validates the platform but discards the returned provider; update the code where
zephyrEngine.application_configuration is awaited to capture the resolved value
(e.g., assign to a local like resolvedProvider from
resolveProvider(appConfig?.PLATFORM)) and then use or log that variable for
future conditional logic or debugging (refer to resolveProvider and the
appConfig variable retrieved from zephyrEngine.application_configuration);
ensure any existing behavior (throwing on unsupported platforms) remains
unchanged.
In `@test/unit/zephyr-preset.test.ts`:
- Around line 11-185: Add negative-path unit tests for the zephyr preset: mock
uploadNitroOutputToZephyr to throw and assert hooks.compiled calls result in
nitro.logger.error being called with a descriptive failure message (and
nitro.logger.success not called), and mock uploadNitroOutputToZephyr to resolve
with deploymentUrl: null and assert hooks.compiled results in nitro.logger.error
or nitro.logger.info being called appropriately (and success not called); use
the same vi.doMock pattern as existing tests to replace
uploadNitroOutputToZephyr and invoke preset.hooks!.compiled with a nitro object
(including logger stubs) to assert the error-handling behavior of the compiled
hook.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.jsonsrc/cli/commands/deploy.tssrc/presets/zephyr/preset.tssrc/presets/zephyr/utils.tstest/unit/zephyr-preset.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/presets/zephyr/preset.ts
- package.json
- src/cli/commands/deploy.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/commands/deploy.ts (1)
42-53: Consider wrapping the Zephyr upload in try/catch for consistent error handling.The
uploadNitroOutputToZephyrfunction can throw (e.g., no assets to deploy, SSR entrypoint not detected). Other error paths in this file (lines 39-40, 55-58) useconsola.error+process.exit(1)for cleaner output. Currently, a thrown error will bubble up and terminate with a stack trace.♻️ Proposed refactor for consistent error handling
if (buildInfo.preset === "zephyr") { + try { const { deploymentUrl } = await uploadNitroOutputToZephyr({ rootDir, outputDir, }); if (deploymentUrl) { consola.success(`[${LOGGER_TAG}] Zephyr deployment succeeded: ${deploymentUrl}`); } else { consola.success(`[${LOGGER_TAG}] Zephyr deployment succeeded.`); } return; + } catch (error) { + consola.error(`[${LOGGER_TAG}] Zephyr deployment failed:`, error instanceof Error ? error.message : error); + process.exit(1); + } }🤖 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 42 - 53, The Zephyr deployment call in the buildInfo.preset === "zephyr" branch should be wrapped in a try/catch so thrown errors are handled consistently; inside the try call uploadNitroOutputToZephyr({ rootDir, outputDir }) as currently done and on success log via consola.success, and in the catch call consola.error(`[${LOGGER_TAG}] Zephyr deployment failed: ${error.message || error}`) followed by process.exit(1) to match other error paths (refer to the uploadNitroOutputToZephyr invocation and the surrounding branch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/commands/deploy.ts`:
- Around line 42-53: The Zephyr deployment call in the buildInfo.preset ===
"zephyr" branch should be wrapped in a try/catch so thrown errors are handled
consistently; inside the try call uploadNitroOutputToZephyr({ rootDir, outputDir
}) as currently done and on success log via consola.success, and in the catch
call consola.error(`[${LOGGER_TAG}] Zephyr deployment failed: ${error.message ||
error}`) followed by process.exit(1) to match other error paths (refer to the
uploadNitroOutputToZephyr invocation and the surrounding branch).
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
2 participants
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.
This PR adds a built-in preset for Zephyr Cloud
(co-authored with @Nsttt based on ZephyrCloudIO/zephyr-packages#358)
Checklist:
zephyr.providerconfigzephyr-agentSDK with prompt to install