From cf5dfd712717ba23583ace52009ce3aba8b0c6e6 Mon Sep 17 00:00:00 2001
From: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
Date: Tue, 26 May 2026 18:44:57 -0700
Subject: [PATCH] fix: address review findings (#2868)

---
 .../settings/AgentSkillSetupPanel.test.tsx    | 68 +++++++++++++++
 .../settings/AgentSkillSetupPanel.tsx         | 83 +++++++++----------
 2 files changed, 105 insertions(+), 46 deletions(-)
 create mode 100644 src/renderer/src/components/settings/AgentSkillSetupPanel.test.tsx

diff --git a/src/renderer/src/components/settings/AgentSkillSetupPanel.test.tsx b/src/renderer/src/components/settings/AgentSkillSetupPanel.test.tsx
new file mode 100644
index 00000000000..1e1b4428fd4
--- /dev/null
+++ b/src/renderer/src/components/settings/AgentSkillSetupPanel.test.tsx
@@ -0,0 +1,68 @@
+import { renderToStaticMarkup } from 'react-dom/server'
+import type { ComponentProps } from 'react'
+import { describe, expect, it, vi } from 'vitest'
+import { AgentSkillSetupPanel } from './AgentSkillSetupPanel'
+
+function renderPanel(overrides: Partial<ComponentProps<typeof AgentSkillSetupPanel>> = {}): string {
+  return renderToStaticMarkup(
+    <AgentSkillSetupPanel
+      title="CLI skill"
+      description="Enables agents to use Orca workflows."
+      command="npx skills add https://github.com/stablyai/orca --skill orca-cli --global"
+      terminalTitle="CLI skill setup"
+      terminalAriaLabel="CLI skill install terminal"
+      terminalWorktreeId="settings-cli-skill-terminal"
+      installed={false}
+      loading={false}
+      error={null}
+      onRecheck={vi.fn()}
+      {...overrides}
+    />
+  )
+}
+
+function buttonLabels(html: string): string[] {
+  return Array.from(html.matchAll(/<button\b[^>]*>([\s\S]*?)<\/button>/g), ([, content]) =>
+    content
+      .replace(/<[^>]*>/g, '')
+      .replace(/\s+/g, ' ')
+      .trim()
+  )
+}
+
+function buttonMarkupByLabel(html: string, label: string): string | undefined {
+  return Array.from(html.matchAll(/<button\b[^>]*>[\s\S]*?<\/button>/g), ([button]) => button).find(
+    (button) => buttonLabels(button).includes(label)
+  )
+}
+
+describe('AgentSkillSetupPanel', () => {
+  it('keeps the install action visible after the skill is detected', () => {
+    const html = renderPanel({ installed: true })
+
+    expect(html).toContain('Installed')
+    expect(buttonLabels(html)).toContain('Install')
+    expect(buttonLabels(html)).toContain('Re-check')
+  })
+
+  it('hides only re-check when installed re-checks are disabled', () => {
+    const html = renderPanel({ installed: true, showRecheckWhenInstalled: false })
+
+    expect(html).toContain('Installed')
+    expect(buttonLabels(html)).toContain('Install')
+    expect(buttonLabels(html)).not.toContain('Re-check')
+  })
+
+  it('keeps re-check visible before install when installed re-checks are disabled', () => {
+    const html = renderPanel({ installed: false, showRecheckWhenInstalled: false })
+
+    expect(buttonLabels(html)).toContain('Install')
+    expect(buttonLabels(html)).toContain('Re-check')
+  })
+
+  it('keeps install visible but disabled when parent setup is disabled', () => {
+    const html = renderPanel({ installDisabled: true })
+
+    expect(buttonMarkupByLabel(html, 'Install')).toContain('disabled=""')
+  })
+})
diff --git a/src/renderer/src/components/settings/AgentSkillSetupPanel.tsx b/src/renderer/src/components/settings/AgentSkillSetupPanel.tsx
index be3de086e88..41ce7ab62e4 100644
--- a/src/renderer/src/components/settings/AgentSkillSetupPanel.tsx
+++ b/src/renderer/src/components/settings/AgentSkillSetupPanel.tsx
@@ -54,12 +54,6 @@ export function AgentSkillSetupPanel({
   const [terminalOpen, setTerminalOpen] = useState(false)
   const [preInstallNoticeVisible, setPreInstallNoticeVisible] = useState(Boolean(preInstallNotice))
 
-  useEffect(() => {
-    if (installed) {
-      setTerminalOpen(false)
-    }
-  }, [installed])
-
   useEffect(() => {
     if (!preInstallNotice) {
       setPreInstallNoticeVisible(false)
@@ -99,45 +93,42 @@ export function AgentSkillSetupPanel({
       setPreInstallNoticeVisible(true)
     }
   }
-  const actionRow =
-    !installed || showRecheckWhenInstalled ? (
-      <div className="mt-3 flex flex-wrap items-center gap-2">
-        {!installed ? (
-          <Button
-            type="button"
-            variant="outline"
-            size="sm"
-            onClick={() => {
-              void (async () => {
-                try {
-                  await onBeforeOpenTerminal?.()
-                  await refreshPreInstallNotice()
-                } finally {
-                  setTerminalOpen(true)
-                }
-              })()
-            }}
-            disabled={terminalOpen || installDisabled}
-          >
-            <Terminal className="size-3.5" />
-            Install
-          </Button>
-        ) : null}
-        {!installed || showRecheckWhenInstalled ? (
-          <Button
-            type="button"
-            variant="ghost"
-            size="sm"
-            className="gap-1.5"
-            onClick={() => void onRecheck()}
-            disabled={loading}
-          >
-            <RefreshCw className={cn('size-3.5', loading && 'animate-spin')} />
-            Re-check
-          </Button>
-        ) : null}
-      </div>
-    ) : null
+  const actionRow = (
+    <div className="mt-3 flex flex-wrap items-center gap-2">
+      <Button
+        type="button"
+        variant="outline"
+        size="sm"
+        onClick={() => {
+          void (async () => {
+            try {
+              await onBeforeOpenTerminal?.()
+              await refreshPreInstallNotice()
+            } finally {
+              setTerminalOpen(true)
+            }
+          })()
+        }}
+        disabled={terminalOpen || installDisabled}
+      >
+        <Terminal className="size-3.5" />
+        Install
+      </Button>
+      {!installed || showRecheckWhenInstalled ? (
+        <Button
+          type="button"
+          variant="ghost"
+          size="sm"
+          className="gap-1.5"
+          onClick={() => void onRecheck()}
+          disabled={loading}
+        >
+          <RefreshCw className={cn('size-3.5', loading && 'animate-spin')} />
+          Re-check
+        </Button>
+      ) : null}
+    </div>
+  )
 
   return (
     <div
@@ -180,7 +171,7 @@ export function AgentSkillSetupPanel({
           ) : null}
         </div>
       </div>
-      {!installed && terminalOpen ? (
+      {terminalOpen ? (
         <div className={cn(variant === 'card' ? 'px-5 pb-5' : 'mt-2')}>
           <OnboardingInlineCommandTerminal
             worktreeId={terminalWorktreeId}