fix(migrate): don't hijack user to Install mode when reconcile has work

[kaitranntt]

Summary

Repro: open Migrate dashboard, uncheck Skills, select Codex, keep Reconcile mode, click Run Migration. UI flipped to Install tab with a blank picker and a disabled CTA.

Root cause

`MigratePage.tsx:521-527` applied `suggestedMode` from the reconcile response unconditionally on first arrival. Since `suggestedMode` only arrives in response to a user-triggered reconcile, the auto-flip always hijacks the user mid-action. The Install tab then renders blank because `installCandidates` are only fetched by a separate endpoint.

Fix

Gate the auto-flip on the plan having zero actionable work:

• If `plan.summary.install + update + delete + conflict > 0` → user kicked off real reconcile work → stay on Reconcile tab
• Otherwise (empty plan, all skips, registry says "nothing to do here") → auto-flip to Install as originally intended

This preserves the original fresh-install UX where an empty registry steers the user to Install mode, while honoring an explicit Reconcile click when there's real work to review.

Test plan

• `bun test` — 4530 pass, 0 fail
• `bun run typecheck` / `lint:fix` / `ui:build` — clean
• Post-merge manual on Mac: trigger reconcile with pending actions → should stay on Reconcile
• Post-merge manual on Mac: trigger reconcile with empty plan → should auto-flip to Install

Closes #746

[claude]

Claude finished @kaitranntt's task in 1m 59s —— View job

---

Code Review — PR #747

• Gather context
• Read changed file(s)
• Review the fix logic
• Post review feedback

---

Summary

The fix is logically correct and well-targeted. The root cause analysis is accurate: the original useEffect applied suggestedMode unconditionally the moment it arrived, regardless of whether the user was in the middle of an active reconcile with real work pending. The gate on hasActionableWork is the right approach.

---

Logic Walkthrough (MigratePage.tsx:527–544)

useEffect(() => {
    if (smartDefaultAppliedRef.current) return;           // ① already applied
    if (!migration.suggestedMode) return;                  // ② no suggestion yet
    if (migration.suggestedMode === mode) {               // ③ NEW: server agrees with current tab
        smartDefaultAppliedRef.current = true;
        return;
    }
    const summary = migration.plan?.summary;
    const hasActionableWork = summary
        ? summary.install + summary.update + summary.delete + summary.conflict > 0
        : false;
    if (hasActionableWork) {                              // ④ NEW: user has pending work
        smartDefaultAppliedRef.current = true;
        return;
    }
    smartDefaultAppliedRef.current = true;
    setMode(migration.suggestedMode);                     // ⑤ only flip on empty plan
}, [migration.suggestedMode, migration.plan, mode]);

All code paths correctly set smartDefaultAppliedRef.current = true before returning, preventing re-triggering on subsequent renders. This is the critical invariant.

Guard ③ (suggestedMode === mode) is a nice defensive addition that wasn't in the original. It short-circuits when the server already agrees with the user's current tab, marking the default as consumed immediately so a later spurious re-render (e.g., from mode being added to deps) can't trigger a flip.

Guard ④ (hasActionableWork) is the core of the fix. It correctly covers all four action types that require user attention: install, update, delete, conflict. The skip count is intentionally excluded — an all-skips plan is semantically "nothing to do in Reconcile" and correctly allows the flip to Install.

---

Observations

mode dependency is correct but has a subtle side-effect. Adding mode to the dep array satisfies React's exhaustive-deps rule (guard ③ reads it). This means the effect re-fires on every manual tab switch. This is harmless because smartDefaultAppliedRef.current is true after the first run, so it always returns immediately at guard ①. No behavioral issue, but worth knowing if someone profiles this later.

summary null-check when migration.plan is null. hasActionableWork = false when summary is undefined. In practice, setPlan and setSuggestedMode are called in the same async callback in useMigrationPlan.ts:152–155 (no await between them), so React 18 batches them into the same render — plan and suggestedMode always arrive together. The null check is still correct defensive coding, since if suggestedMode somehow arrived without plan, the effect would fall through to ⑤ and flip modes. This edge case can't occur today but the guard is inexpensive.

smartDefaultAppliedRef is session-only and not reset by migration.reset(). This is pre-existing behavior, not introduced by this PR. It means if a user runs reconcile → gets auto-flipped to Install → manually switches back to Reconcile → runs reconcile again, the smart-default never fires a second time. This is intentional ("set once"), and the existing comment on line 509 already documents it.

No automated test for the new gate. The PR description relies on post-merge manual testing for both scenarios:

• Reconcile with pending actions → stay on Reconcile tab
• Reconcile with empty plan → auto-flip to Install

Given the CLAUDE.md note about migrate-command.ts requiring fixture-based integration tests, this is a UI behavior gap worth a follow-up. A simple vitest unit test mocking useMigrationPlan and asserting the mode state after the effect fires would lock this in. Not a blocker for a targeted UX fix, but worth tracking.

---

Verdict

Approve. The fix is minimal, focused, and correct. The two new guards cover the intended scenarios without breaking the original fresh-install flow. The dependency array update is required for correctness. The commit message is clear and the PR description accurately explains root cause and intent.

Optional follow-up: add a unit test for the useEffect guard logic to prevent regression.

[claude]

Claude finished @kaitranntt's task in 1m 23s —— View job

---

Code Review — PR #747

• Gather context & read changed files
• Review the fix logic
• Post review feedback

---

Summary

Verdict: Approve. The fix is minimal, correct, and well-targeted. The root cause analysis is accurate and the guard logic covers all relevant scenarios without breaking the original fresh-install UX.

---

Diff Walkthrough (MigratePage.tsx:521–544)

- // Apply smart-default once when suggestedMode arrives from the reconcile endpoint
+ // Apply smart-default once when suggestedMode arrives from the reconcile endpoint.
+ // Gated on the reconcile plan having no actionable work ...
  useEffect(() => {
    if (smartDefaultAppliedRef.current) return;          // ① already fired
    if (!migration.suggestedMode) return;                 // ② no suggestion yet
+   if (migration.suggestedMode === mode) {              // ③ NEW: server agrees with current tab
+     smartDefaultAppliedRef.current = true;
+     return;
+   }
+   const summary = migration.plan?.summary;
+   const hasActionableWork = summary
+     ? summary.install + summary.update + summary.delete + summary.conflict > 0
+     : false;
+   if (hasActionableWork) {                             // ④ NEW: stay put on real work
+     smartDefaultAppliedRef.current = true;
+     return;
+   }
    smartDefaultAppliedRef.current = true;
    setMode(migration.suggestedMode);                    // ⑤ flip only on empty plan
- }, [migration.suggestedMode]);
+ }, [migration.suggestedMode, migration.plan, mode]);

All code paths set smartDefaultAppliedRef.current = true before returning. This is the critical invariant — the ref is the sole gate preventing repeated mode flips. Every branch honours it correctly.

Guard ③ is a clean defensive addition: if the server's suggestion already matches the current tab, consume the default immediately so a later re-render (caused by mode entering the dep array) can't trigger a spurious flip.

Guard ④ is the core fix. It correctly tallies install + update + delete + conflict and excludes skip. An all-skips plan is semantically "nothing to do in Reconcile mode", so letting it flip to Install is correct. The ReconcilePlan.summary type definition in reconcile-types.ts:238–244 confirms all four fields are always present when summary is non-null.

hasActionableWork = false when migration.plan is null — defensively correct. In practice plan and suggestedMode are set in the same async callback in useMigrationPlan.ts:152–155 with no await between them, so React 18 batches them into the same render and they always arrive together. The null check is inexpensive and correct.

---

Observations

mode in the dependency array triggers re-runs on tab switches. This is required — guard ③ reads mode and without it in deps the exhaustive-deps rule would flag a stale closure. In practice, re-runs after the first are no-ops because guard ① exits immediately once smartDefaultAppliedRef.current is true. No behavioral issue, but worth knowing if someone profiles this later.

smartDefaultAppliedRef is session-only and not reset by migration.reset(). Pre-existing behavior, not introduced here. The comment at line 509 already documents it. A user who runs reconcile → gets auto-flipped to Install → manually switches back to Reconcile → runs reconcile again will never see the smart-default fire a second time. This is intentional.

No automated test for the new gate. The two scenarios from the test plan (real work → stay on Reconcile; empty plan → flip to Install) are left as post-merge manual checks. The CLAUDE.md migration test requirement targets migrate-command.ts / provider-registry.ts / reconciler.ts — this is a UI-layer change, so it's not a hard blocker. That said, a small vitest unit test mocking useMigrationPlan and asserting mode state after the effect fires would lock in the regression protection. Worth a follow-up issue.

Add test coverage →

---

The fix is correct, minimal, and ready to merge.

[claude]

Claude finished @kaitranntt's task in 2m 16s —— View job

---

Code Review — PR #747

• Gather context
• Read changed files
• Review the fix logic
• Post review feedback

---

Summary

Verdict: Approve. The fix is minimal, correct, and well-targeted. Root cause analysis is accurate and all code paths are handled correctly.

---

Diff Walkthrough (MigratePage.tsx:521–544)

useEffect(() => {
  if (smartDefaultAppliedRef.current) return;          // ① already fired — always exits after first run
  if (!migration.suggestedMode) return;                 // ② no suggestion yet
  if (migration.suggestedMode === mode) {              // ③ NEW: server agrees with current tab
    smartDefaultAppliedRef.current = true;
    return;
  }
  const summary = migration.plan?.summary;
  const hasActionableWork = summary
    ? summary.install + summary.update + summary.delete + summary.conflict > 0
    : false;
  if (hasActionableWork) {                             // ④ NEW: real work pending — stay put
    smartDefaultAppliedRef.current = true;
    return;
  }
  smartDefaultAppliedRef.current = true;
  setMode(migration.suggestedMode);                    // ⑤ flip only on empty/all-skips plan
}, [migration.suggestedMode, migration.plan, mode]);

All 5 code paths correctly set smartDefaultAppliedRef.current = true before returning. This is the critical invariant — the ref is the sole gate preventing repeated mode flips. Every branch honours it.

Guard ③ is a clean addition: if the server's suggestion already matches the current tab, consume the default immediately. This prevents a spurious flip on a later re-render caused by mode entering the dep array.

Guard ④ is the core fix. install + update + delete + conflict correctly captures all actionable work types. skip is intentionally excluded — an all-skips plan means "nothing to do in Reconcile" and correctly allows the auto-flip to Install.

Dependency array update (migration.plan, mode added): required for exhaustive-deps correctness — guard ③ reads mode, guard ④ reads migration.plan. Re-runs on tab switch are no-ops due to guard ①. No behavioral issue.

---

Type Verification

ReconcilePlan.summary (src/ui/src/types/reconcile-types.ts:120–126) confirms all five fields (install, update, skip, conflict, delete) are always present when summary is non-null. The plan?.summary optional-chain is correct defensive coding since migration.plan can be null before the first reconcile runs.

In practice, setPlan and setSuggestedMode are called back-to-back at useMigrationPlan.ts:152–155 with no await between them, so React 18 batches them into one render — plan and suggestedMode always arrive together. The null-check path (hasActionableWork = false) is an unreachable edge case under normal usage but costs nothing to keep.

---

Pre-existing Behaviour (not introduced here)

migration.reset() (useMigrationPlan.ts:311–320) clears plan but does not reset suggestedMode or smartDefaultAppliedRef. After a reset + re-reconcile, the smart-default never fires a second time. This is intentional "set once" behaviour documented at line 509. The fix neither breaks nor worsens this.

---

Non-blocking Follow-up

The two repro scenarios from the test plan are left as post-merge manual checks. The CLAUDE.md migration test requirement targets migrate-command.ts/reconciler.ts (backend), not UI layers — so this is not a hard gate. A small vitest unit test mocking useMigrationPlan and asserting mode state after the effect fires would lock in regression protection.

Add test coverage →

---

The cli-manifest.json and docs/cli-reference.md changes are auto-generated version/timestamp bumps — no concerns.

The fix is correct, minimal, and ready to merge.