Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@BugBot run |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![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.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO — Do not merge as-is Reviewed by 3 independent agents (Correctness Expert, Code Health Expert, UX Wizard), then cross-validated against the code. One confirmed CI-breaking issue plus several user-facing and maintainability concerns; the PR author has already called out several deferrals in the description (destructive-change warnings, manual branch creation, test coverage), and I've honored those where applicable. Issues Summary
Additional Observations (not posted inline to keep noise down)
🚫 Dropped False Positives (1 item)
🟢 Low-priority / Intentionally Deferred (condensed)
Generated by Dyadbot multi-agent code review |
![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.
github-actions
bot
added
the
needs-human:review-issue
- Resolve hardcoded "neondb" by dynamically fetching database name via Neon API - Replace fragile regex SQL injection with explicit query constants - Fix errorDetails showing same value as errorSummary (use error.stack) - Add confirmation dialog for "Migrate to Production" button - Add cleanup/rollback in setAppProject if env var injection fails - Remove dead NEON_NOT_AVAILABLE_SYSTEM_PROMPT export - Extract UnavailableIntegrationCard to deduplicate 4 card instances - Extract buildNeonPromptForApp to deduplicate prompt block across handlers - Add roving tabindex + arrow-key navigation for radio group accessibility - Drop OAuth spinner timeout from 60s to 20s - Update stale E2E snapshots with current Neon feature strings
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All review comments have been addressed with code changes; confidence is high but some changes (dynamic DB name resolution, prompt refactoring) would benefit from integration testing. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — principles were clear enough for all decisions. Principle #4 (Transparent Over Magical) directly guided the migration confirmation dialog addition. 🤖 Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO — Do NOT merge as-is Reviewed by 3 independent agents (Correctness Expert, Code Health Expert, UX Wizard), then cross-validated against the source and deduplicated against existing review comments. New Issues Summary (11 inline comments posted)
Top merge-blockers
🟢 Lower priority notes (14 items, not posted inline to reduce noise)
🚫 Dropped / already addressed (7 items)
Generated by Dyadbot multi-agent code review (Correctness · Code Health · UX) |
- run drizzle-kit in an Electron utility process and guard unsupported test commands - keep active branch updates atomic and fix the migration dialog trigger plus missing locale copy - remove the unused Neon context framework parameter
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5Formatting, lint, type-checking, and the existing unit test suite all passed locally; the main remaining risk is that the migration flow still has no dedicated E2E coverage in this patch. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO — Do NOT merge as-is Reviewed by 3 independent agents (Correctness Expert, Code Health Expert, UX Wizard), then cross-validated against the 29 existing PR comments to avoid duplicate flags. The Issues Summary
🟢 Low-priority notes (not posted inline)
🚫 Dropped / de-duplicated findings
RecommendationThe HIGH issue in The follow-up items listed in the PR description (destructive-diff warnings, dev/prod sync, more tests, manual branch creation) are fine to defer — none of the findings above belong in that bucket. Generated by Dyadbot multi-agent code review |
- harden Neon env var and branch handling - align framework detection and improve Neon UX/error states - add unit and E2E coverage for the migration flow
|
@BugBot run |
🤖 Codex Review SummaryPR Confidence: 5/5All addressed review items are now on Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 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 (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.
github-actions
bot
added
the
needs-human:review-issue
🎭 Playwright Test Results
|
| OS | Passed | Failed | Flaky | Skipped |
|---|---|---|---|---|
| 🍎 macOS | 141 | 2 | 3 | 0 |
Summary: 141 passed, 2 failed, 3 flaky
Failed Tests
🍎 macOS
neon_branch.spec.ts > neon branch selection updates env vars- TimeoutError: locator.click: Timeout 30000ms exceeded.
supabase_stale_ui.spec.ts > supabase - stale ui- Error: expect(locator).toMatchAriaSnapshot(expected) failed
📋 Re-run Failing Tests (macOS)
Copy and paste to re-run all failing spec files locally:
npm run e2e \
e2e-tests/neon_branch.spec.ts \
e2e-tests/supabase_stale_ui.spec.ts⚠️ Flaky Tests
🍎 macOS
local_agent_summarize.spec.ts > local-agent - summarize to new chat works(passed after 1 retry)per_chat_input.spec.ts > closing a chat tab clears its stored input(passed after 1 retry)setup_flow.spec.ts > Setup Flow > setup banner shows correct state when node.js is installed(passed after 1 retry)
- Restore active→development branch fallback for Neon SQL execution - Keep DATABASE_URL in sync with POSTGRES_URL during version switching - Fix E2E test selecting filtered-out preview branch (use main instead) - Add explicit error when Neon project set but active branch is null - Fix isProductionBranchActive branch resolution to match backend - Snapshot env file before modification in createProject for safe rollback - Fix nested interactive elements (link inside button) in DyadAddIntegration - Fix invalid JSX comment syntax in add-authentication guide - Preserve email query param in callbackURL for email verification guide - Translate hardcoded "Guide" badge label (en/pt-BR/zh-CN)
|
@BugBot run |
azizmejri1
removed
the
needs-human:review-issue
🤖 Claude Code Review SummaryPR Confidence: 4/5All actionable code issues have been addressed; 5 threads remain open for human review on larger refactoring/UX decisions. Unresolved Threads
Resolved Threads
Product Principle SuggestionsThe following suggestions could improve
🤖 Generated by Claude Code |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 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 (11 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
| logger.warn( | ||
| `Neon Auth schema conflict (409) on branch ${branchId}, and retry fetch also failed: ${message}`, | ||
| ); | ||
| return undefined; |
There was a problem hiding this comment.
🟡 MEDIUM | error-handling
ensureNeonAuth silently returns undefined on 409 retry failure
When Neon Auth creation gets a 409 (schema already exists) and the retry getNeonAuth also fails, this silently returns undefined. The caller (autoInjectNeonEnvVars) then sets preserveExistingAuth: true, which preserves stale auth env vars from a different branch. After a branch switch the .env.local may point to the old branch's auth endpoint.
💡 Suggestion: Consider throwing here so the caller can surface the conflict to the user, or at minimum include the branch context in the warning message so users understand why auth may not work.
| } catch (error: any) { | ||
| if (error instanceof DyadError) throw error; | ||
| const errorMessage = getNeonErrorMessage(error); | ||
| const message = `Failed to create Neon project for app ${appId}: ${errorMessage}`; |
There was a problem hiding this comment.
🟡 MEDIUM | error-handling
Outer catch double-wraps non-DyadError messages
The inner try block (post-create setup) already throws postCreateError which may itself be a wrapped error with context. The outer catch at this line re-wraps it as "Failed to create Neon project for app ${appId}: ${errorMessage}", losing the original context and producing misleading messages like "Failed to create Neon project: Failed to set active branch: ..." when project creation actually succeeded but a post-creation step failed.
💡 Suggestion: Preserve the original error's message or rethrow directly when the error already has adequate context.
| setShowCreateForm(false); | ||
| setNewProjectName(""); | ||
| setCreateProjectError(null); | ||
| queryClient.removeQueries({ queryKey: queryKeys.neon.all }); |
There was a problem hiding this comment.
🟡 MEDIUM | logic
handleDisconnectAccount doesn't invalidate appEnvVars query
Compare with handleDisconnectProject (line ~213) which correctly calls queryClient.invalidateQueries({ queryKey: queryKeys.appEnvVars.byApp({ appId }) }). This handler calls unsetAppProject (which removes env vars from disk) but never invalidates the cached env vars query, so the UI may show stale Neon env vars after account disconnect until the user navigates away and back.
💡 Suggestion: Add queryClient.invalidateQueries({ queryKey: queryKeys.appEnvVars.byApp({ appId }) }) after the unsetAppProject call.
| {t("integrations.neon.activeBranch")} | ||
| </Label> | ||
| {branchesError ? ( | ||
| <div className="text-sm text-red-500"> |
There was a problem hiding this comment.
🟡 MEDIUM | accessibility
Error state divs missing role="alert" for screen readers
The error messages at lines 402 and 640 use plain <div className="text-red-500"> without role="alert". Screen readers won't announce these errors when they appear, so users relying on assistive technology won't know something went wrong.
💡 Suggestion: Add role="alert" to error container divs:
<div className="text-sm text-red-500" role="alert">|
|
||
| <AlertDialog> | ||
| <AlertDialogTrigger | ||
| disabled={pushMutation.isPending || isProductionBranchActive} |
There was a problem hiding this comment.
🟡 MEDIUM | accessibility
Disabled migrate button states are indistinguishable
The button is disabled when either pushMutation.isPending or isProductionBranchActive, but the label only changes for the pending state. When disabled because the production branch is active, the button still shows "Migrate to Production" with no explanation of why it's disabled. Users (especially screen reader users) have no indication of what action to take.
💡 Suggestion: Add a tooltip or helper text explaining why the button is disabled, and add aria-describedby pointing to it. For example: "Switch to a development branch to push migrations."`
github-actions
bot
added
the
needs-human:review-issue
🎭 Playwright Test Results❌ Some tests failed
Summary: 266 passed, 15 failed, 4 flaky, 6 skipped Failed Tests🍎 macOSShow all 15 failures
📋 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/context_manage.spec.ts \
e2e-tests/local_agent_ask.spec.ts \
e2e-tests/local_agent_auto.spec.ts \
e2e-tests/local_agent_basic.spec.ts \
e2e-tests/per_chat_input.spec.ts \
e2e-tests/supabase_stale_ui.spec.ts
|
- Move env file snapshot read inside try/catch to prevent orphan Neon projects on failure - Stop inferring development branch from arbitrary non-default branches - Enable email-config query with active-or-development branch fallback - Initialize selectedProvider from requestedProvider prop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 3/5Four code issues were fixed but five refactoring/UX threads remain open for human review, and the PR lacks test coverage as noted by the author. Unresolved Threads
Resolved Threads
Product Principle SuggestionsThe following suggestions could improve
🤖 Generated by Claude Code |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/ipc/handlers/neon_handlers.ts">
<violation number="1" location="src/ipc/handlers/neon_handlers.ts:111">
P2: Using `null` as the initial env snapshot can delete an existing `.env.local` if snapshot reading fails, because cleanup interprets `null` as “remove file.”</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const authWarnings: string[] = []; | ||
|
|
||
| // Snapshot env file before modification so we can restore on failure | ||
| let envFileSnapshot: string | null = null; |
There was a problem hiding this comment.
P2: Using null as the initial env snapshot can delete an existing .env.local if snapshot reading fails, because cleanup interprets null as “remove file.”
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/handlers/neon_handlers.ts, line 111:
<comment>Using `null` as the initial env snapshot can delete an existing `.env.local` if snapshot reading fails, because cleanup interprets `null` as “remove file.”</comment>
<file context>
@@ -108,10 +108,11 @@ export function registerNeonHandlers() {
// Snapshot env file before modification so we can restore on failure
- const envFileSnapshot = await readEnvFileIfExists({ appPath });
+ let envFileSnapshot: string | null = null;
// Post-creation steps: if any fail, best-effort delete the orphan project
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32e8e0ee90
ℹ️ 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".
|
|
||
| ## Auth (detailed guide available) | ||
|
|
||
| When the task involves authentication, login, sign-up, user sessions, or auth UI, you MUST call the \`read_guide\` tool with guide="add-authentication" BEFORE writing any auth code. Do NOT implement auth without reading the guide first. |
There was a problem hiding this comment.
This prompt text makes read_guide mandatory for auth work in all Neon chats, but the same Neon prompt is injected outside local-agent mode where there is no executable read_guide tool path. In build/ask flows, that can cause the model to emit a non-actionable guide step instead of implementing auth changes, stalling the task despite valid user input. Make the hard requirement conditional on local-agent mode (or provide a non-tool equivalent for non-local-agent chats).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I agree on making the hard requirement conditional on local-agent.In build mode, we should inline the full guide content from add-authentication.md and add-email-verification.md if enabled.
| queryClient.invalidateQueries({ | ||
| queryKey: queryKeys.neon.emailPasswordConfig({ | ||
| appId, | ||
| branchId: app?.neonActiveBranchId ?? null, |
There was a problem hiding this comment.
The invalidation key uses only neonActiveBranchId, while the actual email-config query key is built from neonActiveBranchId ?? neonDevelopmentBranchId. For upgraded apps where neonActiveBranchId is still null, toggling email verification updates the backend on the development branch but misses the cached query entry, so the UI can keep showing stale verification state until a full reload.
Useful? React with 👍 / 👎.
|
|
||
| // Post-creation steps: if any fail, best-effort delete the orphan project | ||
| try { | ||
| envFileSnapshot = await readEnvFileIfExists({ appPath }); |
There was a problem hiding this comment.
🔴 HIGH | data-loss
createProject cleanup can delete user's .env.local when initial snapshot read fails
envFileSnapshot is initialized to null at line 111, and readEnvFileIfExists() is called inside the try block at line 115. If the read throws for a non-ENOENT reason (e.g. EACCES, EPERM, EISDIR, or a file-locking error on Windows), control jumps to the catch at line 232 with envFileSnapshot still null. The catch then calls restoreEnvFileSnapshot({ appPath, snapshot: null }), which executes fs.rm(envFilePath, { force: true }) (see restoreEnvFileSnapshot L44-46) — deleting the user's existing .env.local even though we never successfully read it.
💡 Suggestion: Distinguish "snapshot not taken yet" from "snapshot was absent". Use undefined (or a distinct sentinel) for "never read" and skip the fs.rm branch in that case, or call readEnvFileIfExists() before entering the block whose errors trigger cleanup so a read failure here aborts before any env-file mutation is possible.
| if (NEON_ONLY_ENV_VAR_KEYS.includes(envVar.key)) return false; | ||
| if ( | ||
| GENERIC_DB_ENV_VAR_KEYS.includes(envVar.key) && | ||
| envVar.value.includes(".neon.tech") |
There was a problem hiding this comment.
🟡 MEDIUM | data-integrity
removeNeonEnvVars uses .neon.tech substring match to decide whether to delete DATABASE_URL/POSTGRES_URL
On disconnect, DATABASE_URL and POSTGRES_URL are only removed if the value includes the substring ".neon.tech". Two risks:
- Stale vars left behind: If a user's connection string uses a non-
.neon.techhost (custom pooler, VPC endpoint, private link, etc.), the env vars survive disconnect and leave an invalid/stale URL pointing at an unlinked database. - Wrong var deleted: If a user legitimately has a separate
DATABASE_URLwhose value happens to include.neon.tech(e.g. a different unrelated Neon project used by another tool), we'd remove it on disconnect.
💡 Suggestion: Track injected env-var keys explicitly (e.g. persist the keys that updateNeonEnvVars wrote), or remove the generic DB keys unconditionally when the handler knows the app had a Neon project linked (which the unsetAppProject call site already knows).
| reject(timeoutError); | ||
| return; | ||
| } | ||
| resolve({ stdout, stderr, exitCode: code }); |
There was a problem hiding this comment.
🟡 MEDIUM | type-safety
spawnDrizzleKit resolves exitCode as number | null but the Promise is typed as number
Electron's utilityProcess emits 'exit' with code: number | null — null when the process is terminated by a signal (e.g. proc.kill() from the timeout path, SIGKILL from OOM, external SIGTERM). The resolve({ stdout, stderr, exitCode: code }) at line 204 passes null straight through, violating the declared return type Promise<{ stdout: string; stderr: string; exitCode: number }>.
Current callers (migration_handlers.ts:115, :162) only check exitCode !== 0, so null happens to behave as "non-zero / failed". But the type is lying, and any future caller that does arithmetic or strict comparison will silently break.
💡 Suggestion: Coerce null to a sentinel: resolve({ stdout, stderr, exitCode: code ?? -1 }), or widen the return type to number | null and update callers.
| const result = await ipc.neon.setActiveBranch({ appId, branchId }); | ||
| const branch = branches.find((b) => b.branchId === branchId); | ||
| toast.success( | ||
| `${t("integrations.neon.branchSwitched")}: ${branch?.branchName ?? branchId}. ${t("integrations.neon.envUpdated")}`, |
There was a problem hiding this comment.
🟡 MEDIUM | i18n
Toast message concatenates two translation keys with hard-coded punctuation
`${t("integrations.neon.branchSwitched")}: ${branch?.branchName ?? branchId}. ${t("integrations.neon.envUpdated")}` hard-codes ":" and "." — both wrong for CJK typography (zh-CN uses full-width ： / 。), and the fixed sentence order forces pt-BR into awkward phrasing ("Mudou para a branch: X. DATABASE_URL atualizado…"). Translators cannot reorder the pieces.
💡 Suggestion: Replace with a single interpolated key, e.g. integrations.neon.branchSwitchedWithEnv: "Switched to branch {{branchName}}. DATABASE_URL updated in .env.local." — each locale can then format the full sentence naturally.
| setIsOpeningOauth(false); | ||
| oauthTimeoutRef.current = null; | ||
| toast.warning(t("integrations.neon.signInTimedOut")); | ||
| }, 20_000); |
There was a problem hiding this comment.
🟡 MEDIUM | error-states
OAuth sign-in timeout is hard-coded 20s with no recovery path
20 seconds is too short in realistic conditions: 2FA prompts, password-manager autofill, slow mobile networks, or simply a user reading the consent screen can all exceed 20s. When the timer fires the UI shows a warning toast and resets isOpeningOauth, but the OAuth handshake may still complete moments later — the neon-oauth-return deep link handler (lines 92-108) will then fire on top of the already-shown warning, producing conflicting UI.
💡 Suggestion: Extend the timeout significantly (e.g. 2+ minutes), downgrade the message to an info/"Still waiting…" affordance, and make the deep-link handler idempotent with a follow-up success toast that supersedes any earlier warning.
| aria-label={`Visit ${option.name} website`} | ||
| > | ||
| <ExternalLink size={12} /> | ||
| </span> |
There was a problem hiding this comment.
🟡 MEDIUM | accessibility
<span role="link"> with onClick nested inside <button role="radio">
Lines 245-255 render an ExternalLink affordance as <span role="link" onClick=…> inside a <button role="radio">. This is invalid ARIA (interactive inside interactive) and inaccessible:
- The span has no
tabIndex, so it is not keyboard-focusable;Space/Enteron the focused radio selects the radio, not the link. - Screen readers announce conflicting roles for the nested interactives.
aria-label={Visit ${option.name} website}is also not translated.
💡 Suggestion: Move the external link outside the radio button (e.g., as a sibling in the card header), or at minimum use a real <a href target="_blank" rel="noopener noreferrer"> with tabIndex={0}, keyboard handlers, e.stopPropagation(), and a translated aria-label.
| id={errorDetailsId} | ||
| className="max-h-64 overflow-auto whitespace-pre-wrap rounded bg-red-100 p-2 font-mono text-xs dark:bg-red-900/40" | ||
| > | ||
| {errorDetails} |
There was a problem hiding this comment.
🟡 MEDIUM | error-states
Error details reveal raw stack traces and are not localized
When pushMutation.isError, the errorDetails expression at lines 84-89 renders pushMutation.error.stack ?? pushMutation.error.message, and the <pre> block at line 205-211 shows it to the user. For typical migration failures (introspection errors, DyadError(Precondition) from lines 44-91 of migration_handlers.ts, drizzle-kit failures) the stack includes internal file paths and node_modules frames — developer-centric output in a user-facing dialog. errorSummary itself is getErrorMessage(err), which is typically English and not localized.
💡 Suggestion: Map known precondition failures (no tables, active branch == production, etc.) to localized, user-friendly messages, and hide the stack behind an explicit "Copy technical details" action rather than inline-rendering it.
| <p className="text-sm text-amber-700 dark:text-amber-300"> | ||
| {t("integrations.migration.switchBranchHint")} | ||
| </p> | ||
| )} |
There was a problem hiding this comment.
🟡 MEDIUM | interaction-design
Disabled "Migrate to production" button has no adjacent explanation
When isProductionBranchActive is true the button at line 108-112 is disabled, and the "Switch to a non-production branch" hint is rendered after the button at lines 151-155. Users will hover/click the disabled button first and not realize why it's disabled until they scroll past it. There's also no title/tooltip on the disabled button and no aria-describedby linking it to the hint, so assistive-tech users have no explanation at all.
💡 Suggestion: Move the hint above the button (context before the action), add aria-describedby on the disabled button referencing the hint's id, and/or wrap the button in a Tooltip that surfaces the hint on hover/focus.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues to address Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues validated via reasoned analysis (not vote counting). Acknowledged follow-up items from the PR description (direct-to-prod SQL, missing destructive-change warnings, default branch as active when no dev branch exists, test coverage) were not flagged. Issues Summary
🟢 Low-priority notes (25 items)Correctness / robustness
Code health
UX polish
🚫 Dropped false positives (2 items)
Generated by Dyadbot multi-agent code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 264 passed, 17 failed, 2 flaky, 6 skipped Failed Tests🍎 macOSShow all 17 failures
📋 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/context_manage.spec.ts \
e2e-tests/local_agent_ask.spec.ts \
e2e-tests/local_agent_auto.spec.ts \
e2e-tests/local_agent_basic.spec.ts \
e2e-tests/per_chat_input.spec.ts \
e2e-tests/supabase_stale_ui.spec.ts
|
wwwillchen
left a comment
wwwillchen
left a comment
There was a problem hiding this comment.
awesome job! feel free to merge when you think it's ready.
the main thing to watch out for is the inline comment i left about proxy.ts - this could be quite risky if we get wrong because it'll silently not work if the user is on an old next.js version (like the one we use in the next.js template)
There was a problem hiding this comment.
as mentioned in meeting, i think this could be a read_file tool call, but i think this might be nice in the future as it serves as a table of contents for guides, so let's just keep this as-is, thanks.
|
|
||
| ### Request-Boundary File | ||
|
|
||
| Protect routes with \`auth.middleware(...)\`. Reuse the project's existing request-boundary file — current Neon quickstarts use \`proxy.ts\`, older Next.js apps may use \`middleware.ts\`. Reuse whichever exists. Do NOT create both. |
There was a problem hiding this comment.
we need to not use proxy.ts if user is on Next.js v15 or earlier. proxy.ts was only added in next.js v16
| (app.neonActiveBranchId || app.neonDevelopmentBranchId) && ( | ||
| <MigrationPanel appId={selectedAppId} /> | ||
| )} | ||
|
|
There was a problem hiding this comment.
i think this shoudl be structured so that it guarantees only one migration panel is shown.
i'd first do the check for the regular MigrationPanel and then check for Portal since MigrationPanel should be the main flow going forward.
There was a problem hiding this comment.
i don't think the "Migrate to Production" button should be red - it's not necessarily destructive. i think it should be primary color, but there should be a warning banner above the buttons that says: Make sure you have database backups enabled otherwise you cannot undo this.
I think it would also be nice to show the SQL that's being applied in a collapsed accordion so that users can inspect the SQL if they want to.
2 participants
Context for review bots :
1.The agent can execute sql commands directly on production , this is fine because they user may choose to start with a simple setup first and use the production database directly (in a follow up PR we should give the users the ability to sync dev branch to match production branch).The user also has the possiblity to revert database changes .
2.Right now there’s no warning for destructive changes before migration , this is fine for now as we're gonna add a follow up PR for this
3.When linking an existing neon project that doesnt have a developement database , we're setting the default branch as the active branch . This is fine because the user may prefer to start a simple setup at the beginning and work directly on the production database , in addition since the project was created outside dyad it may have a developement branch named differently so it wouldnt be wise to auto create one here . However, in a follow up PR we should allow the user to manually create branches and sync dev branch to match production .
4.neon is only available for next.js now
5.We dont have enough tests for now, i am gonna add a follow-up PR to cover this