Adding nitro server layer support for vite apps#3251
Merged
Conversation
Collaborator
|
@BugBot run |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
github-actions
Bot
added
the
needs-human:review-issue
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (6 items)
🚫 Dropped False Positives (4 items)
Generated by Dyadbot multi-agent code review |
Collaborator
|
@BugBot run |
Contributor
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (6 items)
🚫 Dropped False Positives (6 items)
Generated by Dyadbot multi-agent code review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Collaborator
|
@BugBot run |
Collaborator
Author
Claude Code Review SummaryPR Confidence: 4/5All unresolved review threads from trusted authors have been addressed with code changes and resolved; tests, lint, and type-check pass locally. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions Generated by Claude Code |
Contributor
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (0 new items)All LOW-severity findings were either already covered by existing review comments or were dropped as false positives during validation. 🚫 Dropped False Positives (12 items)
Generated by Dyadbot multi-agent code review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Collaborator
|
@BugBot run |
azizmejri1
removed
the
needs-human:review-issue
github-actions
Bot
added
the
needs-human:review-issue
Collaborator
|
@BugBot run |
Collaborator
|
@BugBot run |
azizmejri1
removed
the
needs-human:review-issue
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
|
@BugBot run |
Contributor
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (0 items)No low-priority issues found after deduplication with existing comments. 🚫 Dropped False Positives (13 items)
Generated by Dyadbot multi-agent code review |
| */ | ||
| export function getNeonClientCode( | ||
| frameworkType: "nextjs" | "vite" | "other" | null, | ||
| frameworkType: AppFrameworkType | null, |
Contributor
There was a problem hiding this comment.
🟡 MEDIUM | missing-branch
getNeonClientCode missing vite-nitro branch — server-side warning absent
Widening to AppFrameworkType means "vite-nitro" now silently falls through to the generic fallback (line 408+) which lacks the // IMPORTANT: Only use this in server-side code warning that "nextjs" gets.
For a Neon-connected Vite+Nitro app, the AI will get Neon client code that uses process.env.DATABASE_URL without guidance that it must go in a Nitro server route (server/routes/api/), potentially leading the model to place database imports in client-side React components where process.env.DATABASE_URL is unavailable.
Note: The PR plan defers Neon-specific integration to a follow-up, so this could be addressed there. But the type widening here opens the gap now.
💡 Suggestion: At minimum, update the comment on line 408 to mention "vite-nitro". Ideally, add "vite-nitro" to the "nextjs" branch so it gets the server-side-only warning too.
Collaborator
|
@BugBot run |
Contributor
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (4 items)
🚫 Dropped False Positives (12 items)
Generated by Dyadbot multi-agent code review |
| <DyadStateIndicator state={state} abortedLabel="Did not finish" /> | ||
| )} | ||
| </DyadCardHeader> | ||
| {!isPending && !isAborted && ( |
Contributor
There was a problem hiding this comment.
🟡 MEDIUM | error-states
No body copy on aborted state — user has no actionable next step
When the Nitro setup is aborted mid-run, the card shows only the headline “Nitro server layer setup aborted” and a “Did not finish” indicator. The user has no idea whether partial changes were made, whether they should retry, or what went wrong.
Since Nitro setup touches the filesystem (writes a config, installs a package), a partial failure could leave the project in an inconsistent state — the rollback logic handles this internally, but the user has no visibility.
💡 Suggestion: Add a short aborted-state body with guidance like “Setup did not complete. Re-send your request to try again.”
github-actions
Bot
added
the
needs-human:review-issue
Contributor
🎭 Playwright Test Results❌ Some tests failed
Summary: 264 passed, 27 failed, 5 flaky, 6 skipped Failed Tests🍎 macOSShow all 27 failures
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/capacitor.spec.ts \
e2e-tests/chat_tabs.spec.ts \
e2e-tests/copy_app.spec.ts \
e2e-tests/custom_apps_folder.spec.ts \
e2e-tests/engine.spec.ts \
e2e-tests/local_agent_advanced.spec.ts \
e2e-tests/local_agent_auto.spec.ts \
e2e-tests/local_agent_basic.spec.ts \
e2e-tests/mention_app.spec.ts \
e2e-tests/per_chat_input.spec.ts \
e2e-tests/select_component.spec.ts \
e2e-tests/setup_flow.spec.ts \
e2e-tests/smart_context_balanced.spec.ts \
e2e-tests/smart_context_deep.spec.ts \
e2e-tests/thinking_budget.spec.ts \
e2e-tests/turbo_edits_v2.spec.ts
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
|
@BugBot run |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54ccc12f45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+107
to
+110
| const result = await installPackages({ | ||
| packages: ["nitro"], | ||
| appPath: ctx.appPath, | ||
| }); |
There was a problem hiding this comment.
enable_nitro currently installs nitro without dev: true, so package managers persist it under dependencies instead of devDependencies. In this repo, Vite/tooling packages are treated as dev-time tooling, and adding Nitro to production deps can unnecessarily inflate packaged app size and production install footprint for every project that enables this tool. Since this commit added explicit dev plumbing in installPackages/buildAddDependencyCommand, this call site should use it to avoid persisting Nitro in runtime dependencies.
Useful? React with 👍 / 👎.
Contributor
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (1 item)
🚫 Dropped False Positives (16 items)
Generated by Dyadbot multi-agent code review |
Contributor
🎭 Playwright Test Results❌ Some tests failed
Summary: 286 passed, 5 failed, 8 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/chat_tabs.spec.ts \
e2e-tests/custom_apps_folder.spec.ts \
e2e-tests/per_chat_input.spec.ts \
e2e-tests/setup_flow.spec.ts
|
Contributor
🎭 Playwright Test Results❌ Some tests failed
Summary: 869 passed, 14 failed, 11 flaky, 280 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/queued_message.spec.ts \
e2e-tests/setup_flow.spec.ts \
e2e-tests/visual_editing.spec.ts
|
Collaborator
Author
HI @wwwillchen , should we still show supabase in add_integration when a nitro server layer was already added to the app or maybe just warn the user that choosing supabase may confuse the AI whether to use supabase edge functions vs. nitro? |
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.
No description provided.