Skip to content

add automatique upgrade in app import#3489

Open
nourzakhama2003 wants to merge 15 commits into
dyad-sh:mainfrom
nourzakhama2003:fix/auto-apply-select-component-upgrade
Open

add automatique upgrade in app import#3489
nourzakhama2003 wants to merge 15 commits into
dyad-sh:mainfrom
nourzakhama2003:fix/auto-apply-select-component-upgrade

Conversation

@nourzakhama2003

@nourzakhama2003 nourzakhama2003 commented May 23, 2026

Copy link
Copy Markdown
Collaborator

closes #3451
This PR adds automatic component tagger support during GitHub imports for Vite React apps, so imported projects can be optimized for Dyad features with one click. It also updates the import UI to localize the optimize label, adds a warning toast when the optional upgrade fails, and expands test coverage for the unchecked opt-out path.

test
npm test -- app_upgrade_utils.test.ts
npm run e2e -- github-import.spec.ts --grep "should skip component tagger upgrade when optimize for Dyad is unchecked"
npm run e2e -- github-import.spec.ts --grep "should auto-apply component tagger upgrade on GitHub import"

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements the automatic application of the component tagger upgrade during GitHub imports and repository cloning. It refactors the upgrade logic into a new utility module, updates the UI to handle imported apps more deterministically, and improves the mock GitHub server for testing. Review feedback highlights a high-severity command injection risk in shell command interpolation and suggests more robust parsing for configuration file modifications. Additionally, the reviewer recommends using targeted git staging instead of gitAddAll to avoid committing unrelated files and improving error message accuracy by referencing actual filenames.

Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
Comment thread src/ipc/utils/app_upgrade_utils.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 issues found across 7 files

Confidence score: 2/5

  • There is a high-confidence security risk in src/ipc/utils/app_upgrade_utils.ts: interpolating appName into a shell: true command can enable shell injection during npx cap init, so this is not a low-risk merge.
  • src/ipc/handlers/github_handlers.ts appears to swallow upgrade failures after clone, which can report success while leaving apps partially upgraded and user-facingly broken.
  • src/ipc/utils/app_upgrade_utils.ts also stages with gitAddAll, which can unintentionally commit unrelated local changes; scoping staged files to the upgrade outputs would reduce regression risk.
  • Pay close attention to src/ipc/utils/app_upgrade_utils.ts and src/ipc/handlers/github_handlers.ts - command execution safety, failure handling, and commit staging behavior need fixes before merging confidently.

Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.

Re-trigger cubic

Comment thread src/ipc/handlers/github_handlers.ts
Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 6 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/ipc/handlers/app_upgrade_handlers.ts
Comment thread src/ipc/handlers/github_handlers.ts Outdated
Comment thread src/ipc/handlers/app_upgrade_handlers.ts
@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 87aba001b1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 πŸ‘.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/utils/app_upgrade_utils.ts

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 3 inline finding(s).

Comment thread src/ipc/utils/app_upgrade_utils.ts
Comment thread src/ipc/utils/app_upgrade_utils.ts
Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
@dyad-assistant

dyad-assistant Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: βœ… YES - Ready to merge
Recommendation: ready

This PR cleanly extracts the component tagger logic into a shared utility, adds a well-designed opt-out UX, and handles failure gracefully without blocking import. The refactoring reduces duplication, adds a proper installDependencies: false path for the auto-import case, and includes good rollback logic. Test coverage is solid with both E2E and unit tests.

No HIGH severity issues found. Several MEDIUM notes below for consideration (none block merge):

Issues Summary

Severity File Issue
🟑 MEDIUM src/ipc/utils/app_upgrade_utils.ts:36 New plugin-react filter narrows scope for existing upgrade panel
🟑 MEDIUM src/ipc/utils/app_upgrade_utils.ts:164 shouldCommitChanges variable is always true (dead logic)
🟑 MEDIUM src/ipc/utils/app_upgrade_utils.ts:94 String.replace only modifies first plugins array occurrence

New plugin-react filter narrows scope for existing upgrade panel β€” The shared isComponentTaggerUpgradeNeeded now requires "plugin-react" in the vite config. This changes behavior for the existing get-app-upgrades panel (not just the new auto-import path). However, since the tool is @dyad-sh/react-vite-component-tagger and all standard React Vite setups use @vitejs/plugin-react or @vitejs/plugin-react-swc (both containing the substring "plugin-react"), this is arguably a correctness improvement β€” the old code would incorrectly offer a React tool to Vue/Svelte apps.

shouldCommitChanges is always true β€” Both the if (installDependencies) and else branches unconditionally set shouldCommitChanges = true. The variable never remains false at the commit check. Consider removing it and always running the commit block (which already has its own try/catch).

First plugins: [ replaced β€” content.replace("plugins: [", ...) only affects the first match. If a vite config has multiple plugins: [ occurrences (e.g., nested Vitest config), the tagger could be inserted into the wrong array. This is pre-existing behavior but now applies automatically to more users.

🟒 Low Priority Notes (4 items)
  • Warning toast message is technical - "component tagger upgrade failed" may confuse users unfamiliar with the term. Consider user-friendlier language like "optional enhancement could not be added." (src/i18n/locales/en/home.json)
  • Shared optimizeForDyad state across tabs - Unchecking in one import tab carries over to the other tab. Low risk since the dialog resets on open and the checkbox defaults to true. (src/components/ImportAppDialog.tsx)
  • Module-level mockReposRoot - The temp directory is shared across the fake server lifetime, coupling tests to execution order. Safe for current tests since the bare repo is read-only. (testing/fake-llm-server/githubHandler.ts)
  • Minor typo in comment - "Auto-upgrade Failures" has a double space. (src/ipc/handlers/github_handlers.ts:1363)

Generated by Dyadbot persona-based code review

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 14 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

🎭 Playwright Test Results

❌ Some tests failed

OS Passed Failed Flaky Skipped
🍎 macOS 520 1 3 159
πŸͺŸ Windows 518 1 1 159

Summary: 1038 passed, 2 failed, 4 flaky, 318 skipped

Failed Tests

🍎 macOS

  • local_agent_advanced.spec.ts > local-agent - mcp tool search
    • Error: expect(locator).toMatchAriaSnapshot(expected) failed

πŸͺŸ Windows

  • concurrent_chat.spec.ts > concurrent chat
    • Error: expect(locator).toBeVisible() failed

πŸ“‹ Re-run Failing Tests (macOS)

Copy and paste to re-run all failing spec files locally:

npm run e2e \
  e2e-tests/local_agent_advanced.spec.ts

⚠️ Flaky Tests

🍎 macOS

  • context_limit_banner.spec.ts > context limit banner shows 'running out' when near context limit (passed after 1 retry)
  • local_agent_advanced.spec.ts > local-agent - mcp tool call (passed after 1 retry)
  • mention_files.spec.ts > mention file (passed after 2 retries)

πŸͺŸ Windows

  • copy_chat.spec.ts > copy message content - basic functionality (passed after 1 retry)

πŸ“Š View full report

@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f9b3899bdf

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 πŸ‘.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/handlers/github_handlers.ts

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 4 inline finding(s).

Comment thread src/ipc/utils/app_upgrade_utils.ts
Comment thread src/components/ImportAppDialog.tsx Outdated
Comment thread src/__tests__/app_upgrade_utils.test.ts
Comment thread e2e-tests/github-import.spec.ts Outdated
@dyad-assistant

dyad-assistant Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: βœ… YES - Ready to merge
Recommendation: ready

Well-structured PR that cleanly extracts app_upgrade_utils.ts, adds a sensible opt-out checkbox, and includes both unit and E2E test coverage. The IPC contract changes follow existing Zod validation patterns, the UI correctly uses Base UI Checkbox, and errors are handled with DyadError. No HIGH severity issues found.

Issues Summary

Severity File Issue
🟑 MEDIUM src/ipc/utils/app_upgrade_utils.ts:38 Behavioral narrowing of isComponentTaggerUpgradeNeeded changes existing manual upgrade flow
🟑 MEDIUM src/components/ImportAppDialog.tsx:121 refreshApps() failure could mask successful import as error
🟑 MEDIUM src/__tests__/app_upgrade_utils.test.ts:90 No unit test for installDependencies: false path used during import
🟑 MEDIUM e2e-tests/github-import.spec.ts:276 Checkbox locator uses input# selector that may be fragile with Base UI
🟒 Low Priority Notes (5 items)
  • Comment typo: double space - // Auto-upgrade Failures are logged... should read // Auto-upgrade failures are logged.... (src/ipc/handlers/github_handlers.ts:1363)

  • Unnecessary defaultValue in i18n call - t("home:autoUpgradeFailed", { defaultValue: "..." }) provides a fallback, but the key already exists in all locale files. Inconsistent with other t() calls in the same file. (src/components/ImportAppDialog.tsx:125)

  • Hardcoded COMPONENT_TAGGER_VERSION - When installDependencies: false, version ^0.9.0 is written to package.json. When installDependencies: true, the version is whatever pnpm add/npm install resolves. These could diverge over time. (src/ipc/utils/app_upgrade_utils.ts:11)

  • Shared optimizeForDyad state across tabs - A single state is shared across the GitHub Repos and URL tabs. Unchecking on one tab carries over to the other. Functionally correct but could be surprising to users. (src/components/ImportAppDialog.tsx:60)

  • mockReposRoot now module-level - Was per-request before. Necessary for the existing-vite-app fixture to persist across clone requests, but creates a test isolation risk if multiple tests use the same repo name. (testing/fake-llm-server/githubHandler.ts:31)


Generated by Dyadbot persona-based code review

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f36939bef9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 πŸ‘.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 2 inline finding(s).

Comment thread src/__tests__/app_upgrade_utils.test.ts Outdated
Comment thread src/components/ImportAppDialog.tsx
@dyad-assistant

dyad-assistant Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: β›” NO - Do NOT merge
Recommendation: auto-fix

Issues Summary

Severity File Issue
πŸ”΄ HIGH src/__tests__/app_upgrade_utils.test.ts:168 Test passes wrong argument type to applyComponentTagger
🟑 MEDIUM src/components/ImportAppDialog.tsx:126 Success toast suppressed when auto-upgrade warning is shown

πŸ”΄ HIGH: Test passes wrong argument type to applyComponentTagger

In the test "skips dependency installation when installDependencies is false" (line 168):

await applyComponentTagger(mockPath, false);

The function signature expects an options object:

export async function applyComponentTagger(
  appPath: string,
  options: ApplyComponentTaggerOptions = {},  // { installDependencies?: boolean }
)

Passing false (a bare boolean) instead of { installDependencies: false } is a type error that vitest/esbuild won't catch at runtime. When JavaScript destructures false, const { installDependencies = true } = false yields installDependencies = true (the default), so the test enters the wrong code path β€” it calls simpleSpawn instead of skipping it, and the assertion expect(simpleSpawn).not.toHaveBeenCalled() will fail.

Compare with the correct call in github_handlers.ts:1357:

await applyComponentTagger(appPath, { installDependencies: false });

Fix: Change line 168 to await applyComponentTagger(mockPath, { installDependencies: false });

🟑 MEDIUM: Success toast suppressed when auto-upgrade warning is shown

In processCloneResult (line 126), the success and warning toasts are mutually exclusive:

if (result.autoUpgradeWarning) {
  showWarning(t("home:autoUpgradeFailed", { ... }));
} else {
  showSuccess(t("home:successfullyImported", { name: result.app.name }));
}

When the auto-upgrade fails, users see only a yellow warning toast instead of the green success toast they'd normally see. While the warning text mentions the import succeeded, the UX signal (yellow vs. green) may mislead users into thinking the import itself had a problem. Consider showing the success toast unconditionally and adding the warning separately.

🟒 Low Priority Notes (3 items)
  • Double space in comment - // Auto-upgrade Failures are logged... has a stray double space. (src/ipc/handlers/github_handlers.ts:1363)
  • simpleSpawn.ts env type widening is correct - Widening from Record<string, string> to NodeJS.ProcessEnv aligns with Node.js spawn API and getPackageManagerCommandEnv() return type. No behavior change. (src/ipc/utils/simpleSpawn.ts:17)
  • isComponentTaggerUpgradeNeeded now filters non-React Vite apps - The new plugin-react heuristic also changes the existing manual upgrade path in app_upgrade_handlers.ts, but this is correct since the tagger is React-specific. (src/ipc/utils/app_upgrade_utils.ts:35)

Generated by Dyadbot persona-based code review

@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9e5b0319d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 πŸ‘.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread e2e-tests/github-import.spec.ts Outdated
@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 4 inline finding(s).

Comment thread src/ipc/utils/app_upgrade_utils.ts
Comment thread src/ipc/utils/app_upgrade_utils.ts
Comment thread e2e-tests/github-import.spec.ts
Comment thread src/components/ImportAppDialog.tsx Outdated
@dyad-assistant

dyad-assistant Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: βœ… YES - Ready to merge
Recommendation: ready

This PR cleanly extracts the component tagger logic into a shared utility, adds auto-upgrade on GitHub import with a user opt-out checkbox, and includes solid test coverage (both unit and E2E). The refactoring removes code duplication in ImportAppDialog.tsx via processCloneResult, the IPC contract changes are properly validated with Zod schemas, errors use DyadError appropriately, and upgrade failures are non-blocking with a warning toast. The mock infrastructure changes are well-considered (shared mockReposRoot, idempotent repo creation, pre-seeded fixture).

Issues Summary

Severity File Issue
🟑 MEDIUM src/ipc/utils/app_upgrade_utils.ts:111 plugins replacement drops original whitespace formatting
🟑 MEDIUM src/ipc/utils/app_upgrade_utils.ts:36 React heuristic narrows existing manual upgrade path
🟑 MEDIUM e2e-tests/github-import.spec.ts:307 Hardcoded waitForTimeout(2000) in negative assertion test is fragile
🟑 MEDIUM src/components/ImportAppDialog.tsx:132 Inline defaultValue in showWarning is redundant with i18n keys
🟒 Low Priority Notes (5 items)
  • Exported logger should be private - logger in app_upgrade_utils.ts is exported but only used within the file. Make it const logger instead of export const logger. (src/ipc/utils/app_upgrade_utils.ts)

  • Comment typo: double space - // Auto-upgrade Failures are logged... has a double space between "Auto-upgrade" and "Failures". (src/ipc/handlers/github_handlers.ts)

  • refreshApps() is a new behavioral addition - processCloneResult adds a refreshApps() call that was not in the original import handlers. This is likely a bugfix (ensuring the app list updates), but it's a behavioral change beyond the stated scope. (src/components/ImportAppDialog.tsx)

  • Checkbox label concatenation is not i18n-friendly - The label {t("home:autoUpgradeAnnotator")} ({t("common:recommended")}) concatenates two translated strings with literal parentheses. Some languages may need different punctuation or ordering. A single interpolated key would be more translation-friendly. (src/components/ImportAppDialog.tsx)

  • simpleSpawn env type fix is a good incidental change - Changing env?: Record<string, string> to env?: NodeJS.ProcessEnv correctly matches what getPackageManagerCommandEnv() returns and what child_process.spawn accepts. (src/ipc/utils/simpleSpawn.ts)


Generated by Dyadbot persona-based code review

@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 4 inline finding(s).

Comment thread src/ipc/utils/app_upgrade_utils.ts Outdated
Comment thread src/ipc/utils/app_upgrade_utils.ts
Comment thread src/components/ImportAppDialog.tsx
Comment thread testing/fake-llm-server/githubHandler.ts
@dyad-assistant

dyad-assistant Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: βœ… YES - Ready to merge
Recommendation: ready

Well-structured PR that extracts and improves the component tagger logic into a reusable utility, adds a user-facing opt-out checkbox, and includes both unit and E2E test coverage. The refactoring correctly consolidates duplicated code, adds proper rollback on failure, uses DyadError with appropriate DyadErrorKind, and validates the new IPC param via Zod schema. Error handling is improved throughout (replacing (error as any).toString() with proper type narrowing). No HIGH severity issues found.

Issues Summary

Severity File Issue
🟑 MEDIUM src/ipc/utils/app_upgrade_utils.ts:78 Import insertion uses startsWith which misses indented imports
🟑 MEDIUM src/ipc/utils/app_upgrade_utils.ts:36 Behavior change: upgrade panel no longer offers tagger for non-React Vite apps
🟑 MEDIUM src/components/ImportAppDialog.tsx:664 Checkbox placement differs between GitHub Repos and URL tabs
🟑 MEDIUM testing/fake-llm-server/githubHandler.ts:28 Shared mockReposRoot across requests risks E2E test isolation

MEDIUM-1: Import insertion uses startsWith which misses indented imports (src/ipc/utils/app_upgrade_utils.ts:78)

The import-finding loop uses lines[i].startsWith("import ") to locate the last import statement. If a vite config has indented imports (e.g., from template literal formatting or unusual project structure), lastImportIndex stays at -1 and the new import is inserted at line 0. Standard vite configs have unindented imports so this is unlikely to cause issues in practice, but using lines[i].trimStart().startsWith("import ") would be more robust.

MEDIUM-2: Behavior change in existing upgrade panel (src/ipc/utils/app_upgrade_utils.ts:36)

The new isComponentTaggerUpgradeNeeded adds a plugin-react heuristic guard that the old inline version in app_upgrade_handlers.ts did not have. Since the upgrade handler at line 281 of app_upgrade_handlers.ts also calls this shared function, the App Settings > Upgrades panel will no longer offer the component tagger for non-React Vite apps (Vue, Svelte, etc.). This is arguably correct since the tagger is React-specific (@dyad-sh/react-vite-component-tagger), but it is an undocumented behavior change to an existing feature surface.

MEDIUM-3: Checkbox placement differs between tabs (src/components/ImportAppDialog.tsx:664)

In the "Your GitHub Repos" tab, the "optimize for Dyad" checkbox is nested inside the "Advanced options" accordion and not visible by default. In the "GitHub URL" tab, the same checkbox is placed directly in the form, always visible. Since the checkbox defaults to true, users importing from the repos tab may not realize auto-upgrade is happening unless they expand advanced options. Consider consistent placement across both tabs.

MEDIUM-4: Shared mockReposRoot across requests (testing/fake-llm-server/githubHandler.ts:28)

mockReposRoot is now created once at module load time and shared across all requests. The if (!fs.existsSync(bareRepoPath)) guard means bare repos are initialized only on first access. This is correct for the existing-vite-app fixture (needs to persist across info-refs and pack requests), but if other tests reuse repo names across test files in the same server process, stale state could leak between tests.

🟒 Low Priority Notes (2 items)
  • Hard-coded waitForTimeout(2000) in skip test - The "should skip component tagger upgrade" E2E test uses a fixed 2-second sleep before asserting the tagger was NOT applied. This is a common flakiness source. Since the test already polls for the app to appear in the list, the sleep is likely redundant. (e2e-tests/github-import.spec.ts:307)

  • Silent pnpm-to-npm fallback - When installDependencies: true, the code silently falls back from pnpm to npm on failure. For projects using yarn or bun, both attempts would succeed but produce a node_modules inconsistent with the project's lockfile. This pre-existed in the old code and is not a regression. (src/ipc/utils/app_upgrade_utils.ts:163)


Generated by Dyadbot persona-based code review

@wwwillchen

Copy link
Copy Markdown
Collaborator

@BugBot run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’‘ Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b61dd2e412

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 πŸ‘.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ipc/utils/app_upgrade_utils.ts

@dyad-assistant dyad-assistant Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review: 3 inline finding(s).

Comment thread src/ipc/utils/app_upgrade_utils.ts
Comment thread src/components/ImportAppDialog.tsx
Comment thread src/components/ImportAppDialog.tsx
@dyad-assistant

dyad-assistant Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

πŸ” Dyadbot Code Review Summary

Verdict: βœ… YES - Ready to merge
Recommendation: ready

This PR cleanly extracts the component tagger logic into a reusable utility module, adds an opt-in/opt-out UI for automatic upgrades during GitHub imports, and includes both unit and E2E test coverage for the new feature. The refactoring deduplicates the import result handling via processCloneResult, improves error formatting (replacing (error as any).toString() with proper instanceof checks), and uses DyadError with appropriate DyadErrorKind throughout. The IPC contract changes are properly validated via Zod schemas, and the auto-upgrade failure is correctly non-blocking with user notification.

Issues Summary

Severity File Issue
🟑 MEDIUM src/ipc/utils/app_upgrade_utils.ts:103 Plugin insertion fallback can modify wrong plugins array
🟑 MEDIUM src/components/ImportAppDialog.tsx:131 Back-to-back success and warning toasts may confuse users
🟑 MEDIUM src/components/ImportAppDialog.tsx:593 Optimize checkbox shown for all project types with no feedback when inapplicable
🟒 Low Priority Notes (6 items)
  • isViteApp duplicates findViteConfigPath - app_upgrade_handlers.ts still has a local isViteApp() that does exactly what findViteConfigPath(p) !== null does in the new utils module. Could reuse the shared helper. (src/ipc/handlers/app_upgrade_handlers.ts)

  • waitForTimeout(2000) in skip-upgrade E2E test - The "should skip component tagger upgrade" test uses a fixed 2-second wait before asserting absence. Since optimizeForDyad is false and the upgrade code never runs, the wait is unnecessary β€” the assertions can run immediately after the app is confirmed imported. (e2e-tests/github-import.spec.ts)

  • Unnecessary "Advanced options" click in skip-upgrade test - The test clicks the "Advanced options" accordion trigger before interacting with #optimize-for-dyad-repos, but the checkbox is rendered outside the accordion. The click is harmless but misleading β€” it implies the checkbox is an advanced option when it is not. (e2e-tests/github-import.spec.ts)

  • Version divergence between install paths - COMPONENT_TAGGER_VERSION = "^0.9.0" is written to package.json when installDependencies: false, but the installDependencies: true path runs pnpm add which resolves to @latest. The two paths may install different effective versions. (src/ipc/utils/app_upgrade_utils.ts)

  • Import insertion at file top when no imports exist - When lastImportIndex stays at -1 (no import lines found), the tagger import is spliced at index 0, before any existing code. In practice this is unlikely since isComponentTaggerUpgradeNeeded filters on plugin-react presence, but no test covers this edge case. (src/ipc/utils/app_upgrade_utils.ts)

  • Minor label whitespace inconsistency - The repos tab splits the (Recommended) parenthetical across JSX lines while the URL tab keeps it inline. React normalizes both to the same output, but the source formatting is inconsistent. (src/components/ImportAppDialog.tsx)


Generated by Dyadbot persona-based code review

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

🎭 Playwright Test Results

❌ Some tests failed

OS Passed Failed Flaky Skipped
🍎 macOS 521 1 2 159
πŸͺŸ Windows 520 0 1 159

Summary: 1041 passed, 1 failed, 3 flaky, 318 skipped

Failed Tests

🍎 macOS

  • local_agent_advanced.spec.ts > local-agent - mcp tool search
    • 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/local_agent_advanced.spec.ts

⚠️ Flaky Tests

🍎 macOS

  • chat_image_generation.spec.ts > generate image from chat - full flow (passed after 1 retry)
  • local_agent_consent.spec.ts > local-agent - add_dependency consent: always allow (passed after 1 retry)

πŸͺŸ Windows

  • app_screenshot.spec.ts > captures an app screenshot after the first generated commit (passed after 1 retry)

πŸ“Š View full report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human:review-issue ai agent flagged an issue that requires human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants