Merge pull request #159 from laststance/fix/cleanup-agent-dialog-hooks

Fix cleanup dialog hook-order crash

Author: ryota-murakami

src/renderer/src/components/sidebar/CleanupAgentDialog.browser.test.tsx

@@ -0,0 +1,118 @@
+import { configureStore } from '@reduxjs/toolkit'
+import { Provider } from 'react-redux'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { render } from 'vitest-browser-react'
+
+import type { AgentId, SyncPreviewResult } from '@/shared/types'
+
+const mockSyncPreview = vi.fn()
+const mockSyncExecute = vi.fn()
+
+const SCOPED_PREVIEW: SyncPreviewResult = {
+  totalSkills: 4,
+  totalAgents: 1,
+  toCreate: 2,
+  alreadySynced: 2,
+  conflicts: [],
+  forAgent: 'claude-code',
+}
+
+beforeEach(() => {
+  mockSyncPreview.mockReset()
+  mockSyncPreview.mockResolvedValue(SCOPED_PREVIEW)
+  mockSyncExecute.mockReset()
+  mockSyncExecute.mockResolvedValue({ details: [] })
+  // Browser mode replaces Electron's preload bridge, so install the sync IPC
+  // surface that CleanupAgentDialog reaches through the Redux thunks.
+  vi.stubGlobal('electron', {
+    sync: {
+      preview: mockSyncPreview,
+      execute: mockSyncExecute,
+    },
+  })
+})
+
+afterEach(() => {
+  vi.unstubAllGlobals()
+})
+
+/**
+ * Build the smallest Redux store CleanupAgentDialog needs.
+ * @returns Store with the ui slice wired for preview target dispatches.
+ * @example
+ * const store = await createStore()
+ * store.dispatch(setCleanupAgentTarget('claude-code'))
+ */
+async function createStore() {
+  const { default: uiReducer } =
+    await import('@/renderer/src/redux/slices/uiSlice')
+  return configureStore({
+    reducer: { ui: uiReducer },
+  })
+}
+
+/**
+ * Opens the dialog through Redux after the component's first closed render.
+ * This mirrors the production path where AgentItem sets cleanupAgentTarget
+ * from a context-menu click after CleanupAgentDialog has already mounted.
+ * @param agentId - Agent receiving the scoped cleanup preview.
+ * @returns The rendered browser screen and backing store.
+ * @example
+ * const { screen } = await renderClosedThenOpen('claude-code')
+ */
+async function renderClosedThenOpen(agentId: AgentId) {
+  const store = await createStore()
+  const { CleanupAgentDialog } = await import('./CleanupAgentDialog')
+  const { setCleanupAgentTarget } =
+    await import('@/renderer/src/redux/slices/uiSlice')
+
+  const screen = await render(
+    <Provider store={store}>
+      <CleanupAgentDialog />
+    </Provider>,
+  )
+
+  // The crash video opened the dialog from an already-mounted, closed state.
+  store.dispatch(setCleanupAgentTarget(agentId))
+
+  return { screen, store }
+}
+
+describe('CleanupAgentDialog', () => {
+  it('stays closed and skips preview when no agent target is active', async () => {
+    const store = await createStore()
+    const { CleanupAgentDialog } = await import('./CleanupAgentDialog')
+
+    const screen = await render(
+      <Provider store={store}>
+        <CleanupAgentDialog />
+      </Provider>,
+    )
+
+    await new Promise((resolve) => setTimeout(resolve, 0))
+    expect(mockSyncPreview).toHaveBeenCalledTimes(0)
+    expect(screen.getByText(/Cleanup missing skills/i).query()).toBeNull()
+    expect(
+      screen.getByRole('button', { name: /Cleanup \d+ skills/ }).query(),
+    ).toBeNull()
+  })
+
+  it('opens from the closed state without changing the hook order', async () => {
+    const { screen } = await renderClosedThenOpen('claude-code')
+
+    await expect
+      .poll(() =>
+        mockSyncPreview.mock.calls.some(
+          ([options]) => options?.agentId === 'claude-code',
+        ),
+      )
+      .toBe(true)
+    await expect
+      .element(screen.getByText(/Cleanup missing skills.*Claude Code/))
+      .toBeInTheDocument()
+    await expect.element(screen.getByText('Symlinks to create')).toBeVisible()
+    await expect
+      .element(screen.getByRole('button', { name: 'Cleanup 2 skills' }))
+      .toBeVisible()
+  })
+})

src/renderer/src/components/sidebar/CleanupAgentDialog.tsx

@@ -82,31 +82,17 @@ export const CleanupAgentDialog = React.memo(
       )
     }, [cleanupAgentTarget, dispatch])
 
-    if (!cleanupAgentTarget) return null
-
-    // Stale-preview guard: only trust the preview when its `forAgent`
-    // matches the currently-targeted agent. Defends against the race
-    // where a global preview lands while a scoped fetch is in flight.
-    const previewMatchesTarget =
-      syncPreview?.forAgent === cleanupAgentTarget ? syncPreview : null
-
-    const agentName: AgentName | undefined = AGENT_DEFINITIONS.find(
-      (a) => a.id === cleanupAgentTarget,
-    )?.name
-
-    const conflictCount = previewMatchesTarget?.conflicts.length ?? 0
-    const missingCount = previewMatchesTarget?.toCreate ?? 0
-    const alreadySyncedCount = previewMatchesTarget?.alreadySynced ?? 0
-    const hasWork = missingCount > 0
-    const isLoadingPreview = !previewMatchesTarget
-
     const handleClose = useCallback((): void => {
       if (!isExecuting) {
         dispatch(clearCleanupAgentTarget())
       }
     }, [dispatch, isExecuting])
 
     const handleCleanup = useCallback(async (): Promise<void> => {
+      // When the dialog is closed this handler can still exist from the
+      // stable hook order; only execute once a concrete agent owns the flow.
+      if (!cleanupAgentTarget) return
+
       const succeeded = await executeCleanup({
         replaceConflicts: [],
         agentId: cleanupAgentTarget,
@@ -119,6 +105,24 @@ export const CleanupAgentDialog = React.memo(
       }
     }, [cleanupAgentTarget, dispatch, executeCleanup])
 
+    if (!cleanupAgentTarget) return null
+
+    // Stale-preview guard: only trust the preview when its `forAgent`
+    // matches the currently-targeted agent. Defends against the race
+    // where a global preview lands while a scoped fetch is in flight.
+    const previewMatchesTarget =
+      syncPreview?.forAgent === cleanupAgentTarget ? syncPreview : null
+
+    const agentName: AgentName | undefined = AGENT_DEFINITIONS.find(
+      (a) => a.id === cleanupAgentTarget,
+    )?.name
+
+    const conflictCount = previewMatchesTarget?.conflicts.length ?? 0
+    const missingCount = previewMatchesTarget?.toCreate ?? 0
+    const alreadySyncedCount = previewMatchesTarget?.alreadySynced ?? 0
+    const hasWork = missingCount > 0
+    const isLoadingPreview = !previewMatchesTarget
+
     return (
       <Dialog open onOpenChange={handleClose}>
         <DialogContent className="max-w-md">