fix: support current app deploy frameworks#48
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughThis PR migrates the ChangesApp Deploy Framework Migration and Bun Support
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
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/cli/tests/app-controller.test.ts`:
- Around line 1197-1201: Add a new test case to the existing test matrix in
packages/cli/tests/app-controller.test.ts that exercises framework-only Bun
inference (i.e., a case where framework: "bun" is provided but no entrypoint
property is set). Copy the existing object structure used for the entries (the
objects that include name, framework, entrypoint, expectedBuildType) and add a
variant with name: "Bun from --framework bun (no entry)", framework: "bun", omit
the entrypoint field entirely, and expectedBuildType: "bun" so the test asserts
the CLI infers Bun without an explicit entry.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4464c840-ecf3-4e16-ae7a-a5af575d4a3c
📒 Files selected for processing (6)
docs/product/command-spec.mdpackages/cli/src/commands/app/index.tspackages/cli/src/controllers/app.tspackages/cli/src/shell/command-meta.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/app-env-vars.test.ts
|
Actionable comments posted: 0 |
luanvdw
left a comment
There was a problem hiding this comment.
@AmanVarshney01 thanks for pushing this through. I pulled the branch locally and the implementation checks out: pnpm --filter @prisma/cli test and pnpm --filter @prisma/cli build both pass for me.
Two small things before we merge:
- Could you rebase against current main to resolve the current conflicts (let me know if I can help you with any of these) I think these conflicts are from the changes I made to explicit project setup. So I'm also happy to resolve these, if helpful. Just let me know
- small nit:
examples/next-smoke/README.mdstill documents app deploy --build-type nextjs, but deploy now rejects --build-type. Can you switch that example and the related bullet to--framework nextjs?
Approving so long, so you're unblocked from merging afterwards
7a87637 to
4181761
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/controllers/app.ts (1)
1207-1213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor explicit
--projectprecedence inapp domainresolution.With a stale
.prisma/local.json,app domain ... --project <id-or-name>can fail before explicit project resolution, because local-pin validation is only skipped forPRISMA_PROJECT_ID. Explicit--projectshould bypass stale-pin reads here too.Suggested fix
- const skipLocalPin = Boolean(envProjectId); + const skipLocalPin = Boolean(envProjectId || options?.projectRef);🤖 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/cli/src/controllers/app.ts` around lines 1207 - 1213, The code only skips local-pin validation when envProjectId is present, causing explicit CLI --project to be ignored; update the skip logic so skipLocalPin is true when either envProjectId OR the explicit CLI project flag is provided (check the variable that holds the --project input, e.g., context.flags.project or the existing CLI input variable your controller uses), then keep the rest (localPin read/validation and throwing localResolutionPinStaleError) unchanged so providing --project bypasses reading/validating .prisma/local.json.
🤖 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/cli/tests/app-controller.test.ts`:
- Around line 1204-1209: The test case for "Bun from --framework bun with
package.json module" is missing the expectedEntrypoint check: update the test
object to set expectedEntrypoint to "index.ts" so resolveDeployEntrypoint will
be asserted to read packageJson.module and the controller will pass entrypoint:
"index.ts" into deployApp; ensure the assertion comparing deployApp's call uses
the expectedEntrypoint value (entrypoint) so the test validates
resolveDeployEntrypoint, deployApp, expectedEntrypoint behavior.
---
Outside diff comments:
In `@packages/cli/src/controllers/app.ts`:
- Around line 1207-1213: The code only skips local-pin validation when
envProjectId is present, causing explicit CLI --project to be ignored; update
the skip logic so skipLocalPin is true when either envProjectId OR the explicit
CLI project flag is provided (check the variable that holds the --project input,
e.g., context.flags.project or the existing CLI input variable your controller
uses), then keep the rest (localPin read/validation and throwing
localResolutionPinStaleError) unchanged so providing --project bypasses
reading/validating .prisma/local.json.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2173b112-2ade-4c45-bd89-b72ad00f716b
📒 Files selected for processing (7)
docs/product/command-spec.mdexamples/next-smoke/README.mdpackages/cli/src/commands/app/index.tspackages/cli/src/controllers/app.tspackages/cli/src/shell/command-meta.tspackages/cli/tests/app-controller.test.tspackages/cli/tests/app-env-vars.test.ts
Summary
next.config.mts--framework bunor--entry--entryby defaulting tosrc/index.ts--build-typepath so deploy uses--frameworkChecked
pnpm --filter @prisma/cli testpnpm --filter @prisma/cli buildcreate-next-appdeploy returned HTTP 200 withHello world!