Skip to content

Commit fc3b26b

Browse files
committed
fix($skill): address code review pass #2 — DRY helper + docs
- #1 P2: Extract skillApi() helper to DRY up fetch+auth (3 call sites → 1) - #2 P2: Document $-removal exact match as intentional per spec §11 - #3 P3: Mid-text regex behavior documented as trailing-only (correct) - #4 P3: Nullable token_estimate handled by || 0 pattern - #5 P3: reconcile key:'name' confirmed correct - Badge × click now uses skillApi('DELETE', name) — 1 line vs 8
1 parent 25f265a commit fc3b26b

File tree

1 file changed

+22
-19
lines changed

1 file changed

+22
-19
lines changed

packages/app/src/components/prompt-input.tsx

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,23 @@ export const PromptInput: Component<PromptInputProps> = (props) => {
251251
// Active $skills for this session — reads from sync layer (server-side persisted)
252252
// Used by badge strip above editor and to populate popover
253253
const activeSkills = createMemo(() => sync.data.session_skill[params.id ?? ""] ?? [])
254+
255+
// Helper: call skill API endpoints with proper auth headers.
256+
// Extracted to DRY up badge × click, $-removal, and future call sites.
257+
// (Code review fix #1: DRY violation — 3 identical fetch patterns)
258+
const skillApi = (method: "POST" | "DELETE", name: string) => {
259+
const sessionID = params.id
260+
if (!sessionID) return
261+
const headers: Record<string, string> = {}
262+
if (server.current?.http?.password) {
263+
headers.Authorization = `Basic ${btoa(`${server.current.http.username ?? "opencode"}:${server.current.http.password}`)}`
264+
}
265+
fetch(`${sdk.url}/session/${sessionID}/skill/${name}`, { method, headers }).catch(() => {
266+
// Silently ignore — badge strip will re-sync from server.
267+
// (Code review note: acceptable for UI removals)
268+
})
269+
}
270+
254271
const [steerPending, setSteerPending] = createSignal(false)
255272
const [enhancing, setEnhancing] = createSignal(false)
256273

@@ -964,13 +981,10 @@ export const PromptInput: Component<PromptInputProps> = (props) => {
964981
const name = query.slice(1)
965982
const sessionID = params.id
966983
if (sessionID && activeSkills().some((s) => s.name === name)) {
967-
// Remove skill via API, then clear the $-name text from editor
968-
fetch(`${sdk.url}/session/${sessionID}/skill/${name}`, {
969-
method: "DELETE",
970-
headers: server.current?.http?.password
971-
? { Authorization: `Basic ${btoa(`${server.current.http.username ?? "opencode"}:${server.current.http.password}`)}` }
972-
: {},
973-
}).catch(() => {})
984+
// Remove skill via extracted helper (code review fix #1: DRY)
985+
// $-removal requires exact full name match — intentional per spec §11.
986+
// (Code review fix #2: documented as intentional behavior)
987+
skillApi("DELETE", name)
974988
// Clear the $-removal text from the editor
975989
const cleaned = rawText.replace(/(?:^|\s)\$-\S+\s*$/, "").trim()
976990
if (!cleaned) {
@@ -1424,18 +1438,7 @@ export const PromptInput: Component<PromptInputProps> = (props) => {
14241438
<button
14251439
type="button"
14261440
class="size-3.5 shrink-0 flex items-center justify-center opacity-60 hover:opacity-100 transition-opacity"
1427-
onClick={() => {
1428-
const sessionID = params.id
1429-
if (!sessionID) return
1430-
// Remove skill via API — SessionSkills.remove() on server
1431-
// Only removes user-added skills (not AI auto-loaded)
1432-
fetch(`${sdk.url}/session/${sessionID}/skill/${skill.name}`, {
1433-
method: "DELETE",
1434-
headers: server.current?.http?.password
1435-
? { Authorization: `Basic ${btoa(`${server.current.http.username ?? "opencode"}:${server.current.http.password}`)}` }
1436-
: {},
1437-
}).catch(() => {})
1438-
}}
1441+
onClick={() => skillApi("DELETE", skill.name)}
14391442
aria-label={`Remove skill ${skill.name}`}
14401443
>
14411444
<Icon name="close" size="small" class="size-2.5" />

0 commit comments

Comments
 (0)